[U-Boot] [PATCH 3/7] net/ethoc: add CONFIG_DM_ETH support

[Max Filippov]

On Thu, Aug 4, 2016 at 8:51 PM, Joe Hershberger
<joe.hershberger at gmail.com> wrote:
> On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov <jcmvbkbc at gmail.com> wrote:
>> Extract reusable parts from ethoc_init, ethoc_set_mac_address,
>> ethoc_send and ethoc_receive, move the rest under #ifdef CONFIG_DM_ETH.
>> Add U_BOOT_DRIVER, eth_ops structure and implement required methods.
>>
>> Signed-off-by: Max Filippov <jcmvbkbc at gmail.com>
>
> A few small nits below, but otherwise,
>
> Acked-by: Joe Hershberger <joe.hershberger at ni.com>
>
>> diff --git a/drivers/net/ethoc.c b/drivers/net/ethoc.c
>> index f5bd1ab..0225595 100644
>> --- a/drivers/net/ethoc.c
>> +++ b/drivers/net/ethoc.c

[...]

>> @@ -216,11 +217,8 @@ static inline void ethoc_write_bd(struct ethoc *priv, int index,
>>         ethoc_write(priv, offset + 4, bd->addr);
>>  }
>>
>> -static int ethoc_set_mac_address(struct eth_device *dev)
>> +static int ethoc_set_mac_address(struct ethoc *priv, u8 *mac)
>
> Please use ethoc_write_hwaddr_common() instead.

Ok.

[...]

>> +static int ethoc_recv_common(struct ethoc *priv)
>
> Please use a better name for this, something like ethoc_is_rx_pkt_rdy().

It's a bit more complex than that, it would only return
1 if new packets arrived since its last invocation. When
it returns 0 there still may be received packets in the
RX queue. I'll call it ethoc_is_new_packet_received.

[...]

>> +#ifndef CONFIG_DM_ETH
>
> Please use positive logic and reverse the order of these code snippets.
>
> #ifdef CONFIG_DM_ETH
> ...
> #else
> ...
> #endif

Ok.

[...]

>> +static int ethoc_recv(struct udevice *dev, int flags, uchar **packetp)
>> +{
>> +       struct ethoc *priv = dev_get_priv(dev);
>> +
>> +       if (flags & ETH_RECV_CHECK_DEVICE)
>> +               ethoc_recv_common(priv);
>
> Kinda seems like you should skip the next call (especially when this
> is renamed to be clearer). The code flow seems OK, though. Up to you.

Ok.

-- 
Thanks.
-- Max