Than you Stefano.
> Date: Wed, 2 Feb 2011 19:02:11 +0100
> From: sba...@denx.de
> To: lg...@hotmail.com
> CC: u-boot@lists.denx.de; w...@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 <lg...@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(®->pcr);
> > + tmp |= PCR_PHY0_RESET_CLEAR;
> > +
> > + writel(tmp, ®->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(®->pcr);
> > + writel((tmp | PCR_LOOP_BACK), ®->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: off...@denx.de
> =====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
