On Thu, Aug 4, 2016 at 8:51 PM, Joe Hershberger
<joe.hershber...@gmail.com> wrote:
> On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov <jcmvb...@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 <jcmvb...@gmail.com>
>
> A few small nits below, but otherwise,
>
> Acked-by: Joe Hershberger <joe.hershber...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
