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

[Stefano Babic]

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.

Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I
added him now.

> --- /dev/null
> +++ b/drivers/net/ax88783.c
> @@ -0,0 +1,297 @@
> +/*
> + *

You should drop this line

> +
> +static int ax88183_phy_initial(struct eth_device *dev)

You forget to replace the name of the function. It has still ax88183_

> +     /* 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.

> +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 ?

> +
> +     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.

> +
> +     res = ax88183_phy_initial(dev);

Name must be changed.

> 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.

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