[U-Boot] [PATCH v2 4/6] Create PHY Lib for U-Boot

[Mike Frysinger]

On Tuesday, April 05, 2011 17:59:52 Andy Fleming wrote:
> -#define debug(fmt,args...)	printf (fmt ,##args)
> +#define debug(fmt, args...)	printf(fmt, ##args)

it'd be nice if all these unrelated formatting changes werent intermingled.  
but i guess too hard for you to untangle now.  make it a pita to pick out what 
are functional changes and what is pure noise.

> +struct mii_dev *mdio_alloc(void)
> +{
> +	struct mii_dev *bus;
> +
> +	bus = malloc(sizeof(*bus));
> +	if (!bus)
> +		return bus;
> +
> +	memset(bus, 0, sizeof(*bus));
> +
> +	bus->name = malloc(MDIO_NAME_LEN);

considering the name len is hardcoded at build time, it'd be nice to inline 
that into the struct itself to avoid having to do multiple mallocs.  but i 
guess it might be hard to keep working with the legacy code ?  or maybe not if 
you just grep the tree to make sure no one using legacy code has a name longer 
than 31 bytes ...

> +struct phy_device *mdio_phydev_for_ethname(const char *ethname)
> ...
> +			if ((!bus->phymap[i]) || (!bus->phymap[i]->dev))

useless paren around both expressions here

> --- /dev/null
> +++ b/drivers/net/phy/phy.c
>
> +int phy_read(struct phy_device *phydev, int devad, int regnum)
> +{
> +	struct mii_dev *bus = phydev->bus;
> +
> +	return bus->read(bus, phydev->addr, devad, regnum);
> +}
> +
> +int phy_write(struct phy_device *phydev, int devad, int regnum, u16 val)
> +{
> +	struct mii_dev *bus = phydev->bus;
> +
> +	return bus->write(bus, phydev->addr, devad, regnum, val);
> +}

seems like it'd make sense for these to be inlines in the phy header

> +static struct phy_driver gen10g_driver = {
> +	.uid		= 0xffffffff,
> +	.mask		= 0xffffffff,
> +	.name		= "Generic 10G PHY",
> +	.features	= 0,
> +	.config		= gen10g_config,
> +	.startup	= gen10g_startup,
> +	.shutdown	= gen10g_shutdown,
> +};

this probably should be split out into a dedicated phy driver.  you might care 
about it, but i cant think of any board atm where i would use this.  i imagine 
for most people, it's simply useless bloat.

> +static struct phy_driver genphy_driver = {
> +	.uid		= 0xffffffff,
> +	.mask		= 0xffffffff,
> +	.name		= "Generic PHY",
> +	.features	= 0,
> +	.config		= genphy_config,
> +	.startup	= genphy_startup,
> +	.shutdown	= genphy_shutdown,
> +};

i think this should be split too for the board maintainers who know exactly 
why phy they're going to have in their system.

> +static struct list_head phy_drivers;
> +
> +int phy_init(void)
> +{
> +	INIT_LIST_HEAD(&phy_drivers);
> +
> +	return 0;
> +}

isnt there a macro for declaring/initializing a list structure statically ?  
then we could avoid this useless phy_init() call.

> +/* Indicates what features are supported by the interface. */
> +#define SUPPORTED_10baseT_Half		(1 << 0)
> +#define SUPPORTED_10baseT_Full		(1 << 1)
> +#define SUPPORTED_100baseT_Half		(1 << 2)

this stuff looks suspiciously like it was copy & pasted from linux/ethtool.h.  
why not just copy the file over instead of creating your own custom variant ?
-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/20110406/05ece4d6/attachment.pgp