Hi Stefano,
Thank you for your advice.It has been changed according to you suggestion and
passed the checking of checkpatch.pl (two warnings , one is asm/io.h one is
struct eth_device's member function send has a volatile parameter).
Although AX88183 has the similar name with AX88180, they are very different.
It works well on my board.I have submitted it again.
Best wishes.
Joe
> Date: Fri, 28 Jan 2011 09:02:38 +0100
> From: sba...@denx.de
> To: lg...@hotmail.com
> CC: u-boot@lists.denx.de
> Subject: Re: [U-Boot] [PATCH] Driver for AX88183 ethernet chip
>
> On 01/27/2011 07:55 PM, root wrote:
> > From: Joe Xue <lg...@hotmail.com>
>
> You have to set you Signe-off-by in your patch. Please add also
> a commit message to explain what your patch is doing, and on which board
> you tested this driver. And set the net custodian as CC, this helps to
> get faster review.
>
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
> > COBJS-$(CONFIG_ULI526X) += uli526x.o
> > COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
> > COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
> > +COBJS-$(CONFIG_DRIVER_AX88183) += ax88183.o
>
> Please maintain the list sorted.
>
> > @@ -0,0 +1,265 @@
> > +
> > +/*
> > + * (C) Copyright 2010 Joe Xue <lg...@hotmail.com>
> > + *
> > + * This file is released under the terms of GPL v2 and any later version.
> > + * See the file COPYING in the root directory of the source tree for
> > details.
> > +*/
>
> The header is not enough. Please add coherent header with license, as
> you can see for other drivers. This must be fixed globally.
>
> > +
> > +/*
> > + * This driver works on 32bit mode
> > + * AX88783 has two ethernet ports, we only use port 0 in u-boot
> > + */
>
> I suggest you add this comment to the README file, too,
>
> > +#include <common.h>
> > +#include <command.h>
> > +#include <net.h>
> > +#include <linux/mii.h>
> > +#include <miiphy.h>
> > +#include <config.h>
> > +#include "ax88183.h"
> > +
> > +static int ax88183_init(bd_t *bd)
> > +{
> > + volatile unsigned int tmp;
> > + int i;
> > +
> > + /* reset chip */
> > + tmp = CR;
> > + CR = (tmp & ~CR_CHIP_RESET);
>
> u-boot does not not accept anymore to write/read into registers using
> offsets and volatile variables. Please define an
> appropriate structure for your chip, and use read/write accessors to
> work with registers. This must be fixed globally in all your code.
>
> I do not know this chip. However, u-boot has already a ax88180 driver.
> Would be not possible to modify this driver to make it suitable for your
> chip, too ?
>
> Other remarks: it seems to me your driver does not support CONFIG_MULTI,
> as it should do. And it must call eth_register() to be inserted as
> device at runtime. Please check with other network drivers.
>
> > + udelay(1000);
> > + CR = (tmp | CR_CHIP_RESET);
> > +
> > + /* disable interrupt */
> > + IMSR = IMSR_MASK_ALL;
> > +
> > + /* set mac address*/
> > + unsigned char mactmp[4];
> > + unsigned int * mac = (unsigned int *)mactmp;
> > + mactmp[0] = bd->bi_enetaddr[5];
> > + mactmp[1] = bd->bi_enetaddr[4];
> > + mactmp[2] = bd->bi_enetaddr[3];
> > + mactmp[3] = bd->bi_enetaddr[2];
>
> This is not correct anymore. You do not have to set the mac from the bd.
> Instead of that, you have to provide in your initialization function a
> struct eth_device that should be passed to eth_register().
> > +
> > +int eth_init (bd_t * bd)
> > +{
> > +
> > + char *s, *e;
> > + int i;
> > +
> > + s = getenv ("ethaddr");
>
> The driver must not call getenv explicitely.
>
> > +#if 0
> > + printf ("\tHW MAC address: "
> > + "%02X:%02X:%02X:%02X:%02X:%02X\n",
> > + bd->bi_enetaddr[0], bd->bi_enetaddr[1],
> > + bd->bi_enetaddr[2], bd->bi_enetaddr[3],
> > + bd->bi_enetaddr[4], bd->bi_enetaddr[5] );
> > +#endif
>
> do not add dead code. Instead of that, you can use the debug() function
> to trace.
>
>
> > +/*
> > + *-------------------------------------------------------------------------
> > + * Registers
> > + *-------------------------------------------------------------------------
> > + */
> > +
> > +#define PCR (*(volatile unsigned int *)(CONFIG_AX88183_BASE + 0x0004))
>
> Replace all defines with a C structure describing your chip.
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
> =====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
