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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
