[U-Boot] [PATCH v3 1/3] net: fec_mxc: Convert into driver model

[Simon Glass]

Hi,

On 2 October 2016 at 06:34, Jagan Teki <jteki at openedev.com> wrote:
> From: Jagan Teki <jagan at amarulasolutions.com>
>
> This patch add driver model support for fec_mxc driver.
>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Peng Fan <peng.fan at nxp.com>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Michael Trimarchi <michael at amarulasolutions.com>
> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> ---
>  drivers/net/fec_mxc.c | 273 +++++++++++++++++++++++++++++++++++++++++++++-----
>  drivers/net/fec_mxc.h |  11 ++
>  2 files changed, 258 insertions(+), 26 deletions(-)

I think you would have an easier time if you move all the common code
into common functions, and just have stubs for the legacy and new DM
path. For example fecmxc_probe() is repeating code. There really
should be almost no duplicated code in the driver. It just makes it
hard to maintain, and understand what is happening.

I think it is best to avoid renaming the functions. So for example:

#ifdef CONFIG_DM_ETH
static int fecmxc_recv(struct udevice *dev, int flags, uchar **packetp)
#else
static int fec_recv(struct eth_device *dev)
#endif
{

You may as well keep the name the same - fec_recv().

In one case you have not put the DM_ETH case first. It should be:

#ifdef CONFIG_DM_ETH
...
#else
#endif

rather than:

#ifndef CONFIG_DM_ETH
...
#else
#endif

I'm not sure you are dealing with all the cases. Unfortunately the
driver already has #idefs. For example if CONFIG_PHYLIB is not
defined. With CONFIG_DM_ETH, struct eth_device will not be available,
so you need to make sure no code builds with that.

Also fecmxc_recv() does not appear to work. It needs to set packetp
and return a packet length. Also, do you need a free_pkt()?

Regards,
Simon