[U-Boot] [RFC PATCH 2/3] net: add phylib implementation

Mike Frysinger
2009-08-27T01:55:57Z

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