[U-Boot] [PATCH] net: usb: r8152: Add DM support

[Simon Glass]

Hi Stefan,

On 28 June 2016 at 08:28, Stefan Roese <sr at denx.de> wrote:
> Hi Simon,
>
> On 26.06.2016 04:53, Simon Glass wrote:
>> On 22 June 2016 at 01:18, Stefan Roese <sr at denx.de> wrote:
>>> Add support for driver model, so that CONFIG_DM_ETH can be defined and
>>> used with this driver.
>>>
>>> This patch also adds the read_rom_hwaddr() callback so that the ROM MAC
>>> address will be used to the DM part of this driver.
>>>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>> Cc: Stephen Warren <swarren at nvidia.com>
>>> Cc: Ted Chen <tedchen at realtek.com>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>>> ---
>>>   drivers/usb/eth/r8152.c    | 235 ++++++++++++++++++++++++++++++++++++++++-----
>>>   drivers/usb/eth/r8152.h    |   4 +
>>>   drivers/usb/eth/r8152_fw.c |   2 +
>>>   3 files changed, 219 insertions(+), 22 deletions(-)
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> With a few comments below.

[snip]

>>> +int r8152_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>>> +{
>>> +       struct r8152 *tp = dev_get_priv(dev);
>>> +       struct ueth_data *ueth = &tp->ueth;
>>> +       uint8_t *ptr;
>>> +       int ret, len;
>>> +       struct rx_desc *rx_desc;
>>> +       u16 packet_len;
>>> +
>>> +       len = usb_ether_get_rx_bytes(ueth, &ptr);
>>> +       debug("%s: first try, len=%d\n", __func__, len);
>>> +       if (!len) {
>>> +               if (!(flags & ETH_RECV_CHECK_DEVICE))
>>> +                       return -EAGAIN;
>>> +               ret = usb_ether_receive(ueth, RTL8152_AGG_BUF_SZ);
>>> +               if (ret == -EAGAIN)
>>> +                       return ret;
>>
>> What about if ret returns some other error?
>
> Good catch. Will fix in v2.
>
> BTW: I've cloned this part from other drivers (e.g. smsc95xx.c
> and asix.c). So they would need some additional error handling here
> as well.

OK

>
>>> +
>>> +               len = usb_ether_get_rx_bytes(ueth, &ptr);
>>> +               debug("%s: second try, len=%d\n", __func__, len);
>>> +       }
>>> +
>>> +       rx_desc = (struct rx_desc *)ptr;
>>> +       packet_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
>>> +       packet_len -= CRC_SIZE;
>>> +
>>> +       if (packet_len > len - (sizeof(struct rx_desc) + CRC_SIZE)) {
>>> +               debug("Rx: too large packet: %d\n", packet_len);
>>> +               goto err;
>>> +       }
>>> +
>>> +       *packetp = ptr + sizeof(struct rx_desc);
>>> +       return packet_len;
>>> +
>>> +err:
>>> +       usb_ether_advance_rxbuf(ueth, -1);
>>> +       return -EINVAL;
>>
>> -E2BIG?
>
> Hmmm. This does not seem appropriate here:
>
> #define E2BIG           7               /* Argument list too long */
>
> Or is it commonly used also for non argument list return failure?

Not sure...:-)

I was probably thinking of -ENOSPC.

[snip]

Regards,
Simon