Hi,
On 2 October 2016 at 06:34, Jagan Teki jteki@openedev.com wrote:
From: Jagan Teki jagan@amarulasolutions.com
This patch add driver model support for fec_mxc driver.
Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Peng Fan peng.fan@nxp.com Cc: Stefano Babic sbabic@denx.de Cc: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Jagan Teki jagan@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