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