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