Hi,
On 2 October 2016 at 06:34, Jagan Teki <jt...@openedev.com> wrote:
> From: Jagan Teki <ja...@amarulasolutions.com>
>
> This patch add driver model support for fec_mxc driver.
>
> Cc: Simon Glass <s...@chromium.org>
> Cc: Joe Hershberger <joe.hershber...@ni.com>
> Cc: Peng Fan <peng....@nxp.com>
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Michael Trimarchi <mich...@amarulasolutions.com>
> Signed-off-by: Jagan Teki <ja...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
