[U-Boot] [PATCH] Add support for ASIX's AX88783 ethernet chip

[Joe XUE]

Than you Stefano.

> Date: Wed, 2 Feb 2011 19:02:11 +0100
> From: sbabic at denx.de
> To: lgxue at hotmail.com
> CC: u-boot at lists.denx.de; wd at denx.de
> Subject: Re: [U-Boot] [PATCH] Add support for ASIX's AX88783 ethernet chip
> 
> On 01/31/2011 06:42 PM, Joe Xue wrote:
> > for more information about this chip, please check:
> > http://www.asix.com.tw/products.php?op=pItemdetail&PItemID=98;65;86&PLine=65
> > 
> > Signed-off-by: Joe Xue <lgxue at hotmail.com>
> > 
> 
> Please add a version number to your patch to make easier tracking which
> is your last version.
> 
Will add it.
> Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I
> added him now.
> 
Not exactly understand your meaning. You mean I should add wd as maintainer to my code or just add him in mail.
> > --- /dev/null
> > +++ b/drivers/net/ax88783.c
> > @@ -0,0 +1,297 @@
> > +/*
> > + *
> 
> You should drop this line
> 
will drop it.
> > +
> > +static int ax88183_phy_initial(struct eth_device *dev)
> 
> You forget to replace the name of the function. It has still ax88183_
> 
will change, sorry for it.
> > +	/* phy init */
> > +	tmp = readl(&reg->pcr);
> > +	tmp |= PCR_PHY0_RESET_CLEAR;
> > +
> > +	writel(tmp, &reg->pcr);
> > +	udelay(100000);
> 
> you already explained why you need such a long delay. It is not bad to
> add your explanation as comment here, so everyone knows your answer.
> 
will do.
> > +static void ax88783_halt(struct eth_device *dev)
> > +{
> > +	unsigned int tmp;
> > +	struct ax88783_reg *reg = (struct ax88783_reg *)dev->iobase;
> > +	tmp = readl(&reg->pcr);
> > +	writel((tmp | PCR_LOOP_BACK), &reg->pcr);
> > +}
> 
> From the name it seems you set the controller in loopback, instead of
> disabling it. Is it correct ?
> 
mmn. I just make it can't receive the data outside.The other way is make it into sleep mode.
> > +
> > +	res = eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> > +	if (!res) {
> > +		puts("Please set your MAC address!");
> > +		free(dev);
> > +		return 0;
> > +	}
> 
> This is wrong. A network driver should not call directly
> eth_getenv_enetaddr, and you do not need. If you want to check the mac
> addrsss, use is_valid_ether_addr() inside ax88783_init() before copying
> the mac to the hardware.
> 
I refer to the newest net driver patch ftgmac100.c, it uses this function to getmac address from environmental setting. and I checked the code eth_getenv_enetaddralso call the is_valid_ether_addr(). 
Anyway, will change it according to your advice.
> > +
> > +	res = ax88183_phy_initial(dev);
> 
> Name must be changed.
> 
will change.
> > diff --git a/drivers/net/ax88783.h b/drivers/net/ax88783.h
> > new file mode 100644
> > index 0000000..09ac9ed
> > --- /dev/null
> > +++ b/drivers/net/ax88783.h
> > @@ -0,0 +1,100 @@
> > +/*
> > + *
> 
> As explained, all headers start with copyright on the second line. Drop
> this line.
> 
Yes, thank you for your patience :-) 

> Best regards,
> Stefano Babic
Best wishes,
Joe
> 
> -- 
> =====================================================================
> 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: office at denx.de
> =====================================================================