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