From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mike Frysinger <vapier@gentoo.org>
Date: Wed, 26 Aug 2009 21:55:57 -0400
Subject: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation
In-Reply-To: <1251317583-27875-2-git-send-email-plagnioj@jcrosoft.com>
References: <4A938C76.9000009@monstr.eu>
	<1251317583-27875-1-git-send-email-plagnioj@jcrosoft.com>
	<1251317583-27875-2-git-send-email-plagnioj@jcrosoft.com>
Message-ID: <200908262155.58509.vapier@gentoo.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wednesday 26 August 2009 16:13:02 Jean-Christophe PLAGNIOL-VILLARD wrote:
> the current implementation will allow you to probe, instance and mananage
> one phy_device per mii_device

"instance" -> "instantiate"
"mananage" -> "manage"

> --- /dev/null
> +++ b/drivers/net/phy/phylib.c
> @@ -0,0 +1,535 @@
> +/*
> + * drivers/net/phy/phy_device.c

needs updating

> +static int genphy_config_init(struct phy_device *phydev);

why does _init() have a static def in this C file but the other genhy_config_* 
funcs have a non-static def in the H file

> +int phy_driver_register(struct phy_driver *phydrv)
> +{
> +	if(!phydrv)

missing space

> +int phy_init(void)
> +{
> +	/* Initialize the list */
> +	INIT_LIST_HEAD(&phy_drvs.list);

does it really need to be dynamic ?  arent there static initializers so this 
can be done in .data ?

> +			phydev = calloc(1, sizeof(struct phy_device));

this is odd style.  why dont we stop jerkin around in all our code and 
introduce zalloc() already.  the first patch in this series has malloc/memset 
usage too.

> +static int phy_connect(struct mii_device *miidev, int phy_addr)
> +{
> +	unsigned int phyaddr = (phy_addr + 1u) % 32u;

what's this business all about ?  why wrap the phy addr ?

> +	printf("%s found at 0x%x\n", phydev->phydrv->name, phyaddr);

"0x%x" -> "%#x"

> +		for (i = 0; i < 32; i++) {

i'd stick a comment here about mii bus being limited to 7 bits as not everyone 
knows it

> +			if(!phy_connect(miidev, phy_addr))
> +		if(phydrv->config_init)

missing space after the "if"

> +		printf("%dMbps %s duplex link detected\n", phydev->speed,
> +				phydev->duplex? "full" : "half");

need space before the "?"

> +	} else {
> +		printf("*Warning* no link detected\n");
> +	}
> +
> +	return 0;

should "no link" really return success ?

> +		if (phydev->duplex == DUPLEX_FULL){

need space before that "{"

> +	result = genphy_config_advert(phydev);
> +
> +	if (result < 0) /* error */
> +		return result;

the error comment seems kind of useless

> +	/* Only restart aneg if we are advertising something different
> +	 * than we were before.	 */

embedded tab at the end of the comment doesnt belong

> +	now = get_timer(0);
> +	while (get_timer(now) < PHY_AN_TIMEOUT) {

kind of useless to check the timer right away.  please convert to a do {...} 
while rather than a while {...}.

> +			puts("PHY remote fault detected\n");
> +			/* Restart auto-negotiation */
> +			puts("PHY restarting auto-negotiation\n");

combine the puts()

> +#define ETHTOOL_GSET		0x00000001 /* Get settings. */

i dont think we need all this ETHTOOL_xxx junk

> +/* compatibility with older code */
> +#define SPARC_ETH_GSET		ETHTOOL_GSET
> +#define SPARC_ETH_SSET		ETHTOOL_SSET

or this

> +/* Wake-On-Lan options. */
> +#define WAKE_PHY		(1 << 0)
> +#define WAKE_UCAST		(1 << 1)
> +#define WAKE_MCAST		(1 << 2)
> +#define WAKE_BCAST		(1 << 3)
> +#define WAKE_ARP		(1 << 4)
> +#define WAKE_MAGIC		(1 << 5)
> +#define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */

does WOL really make sense in u-boot ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090826/5d99c23e/attachment.pgp