Andy Fleming <afleming at freescale.com> :
> This patch contains the PHY layer itself, no phy drivers
[...]
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/phy/Kconfig
[...]
> +config MARVELL_PHY
> + bool "Drivers for Marvell PHYs"
> + depends on PHYLIB
> + ---help---
> + Currently has a driver for the 88E1011S
> +
> +config DAVICOM_PHY
> + bool "Drivers for Davicom PHYs"
> + depends on PHYLIB
> + ---help---
> + Currently supports dm9161e and dm9131
[snip]
They try to escape...
[...]
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/phy/Makefile
> @@ -0,0 +1,9 @@
> +# Makefile for Linux PHY drivers
> +
> +obj-$(CONFIG_PHYLIB) += phy.o phy_device.o mdio_bus.o
> +
> +obj-$(CONFIG_MARVELL_PHY) += marvell.o
> +obj-$(CONFIG_DAVICOM_PHY) += davicom.o
> +obj-$(CONFIG_CICADA_PHY) += cicada.o
> +obj-$(CONFIG_LXT_PHY) += lxt.o
> +obj-$(CONFIG_QSEMI_PHY) += qsemi.o
...and they are not alone. :o)
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -0,0 +1,175 @@
[...]
> +int mdiobus_register(struct mii_bus *bus)
> +{
> + int i;
> + int err = 0;
> +
> + spin_lock_init(&bus->mdio_lock);
> +
> + if (NULL == bus || NULL == bus->name ||
> + NULL == bus->read ||
> + NULL == bus->write)
Be spartan:
if (!bus || !bus->name || !bus->read || !bus->write)
> + return -EINVAL;
> +
> + if (bus->reset)
> + bus->reset(bus);
> +
> + for (i=0; i < PHY_MAX_ADDR; i++) {
for (i = 0; ...
> + struct phy_device *phydev;
> +
> + phydev = get_phy_device(bus, i);
> +
> + /* There's a PHY at this address
> + * We need to set:
> + * 1) IRQ
> + * 2) bus_id
> + * 3) parent
> + * 4) bus
> + * 5) mii_bus
> + * And, we need to register it */
> + if (phydev) {
> + phydev->irq = bus->irq[i];
> +
> + phydev->dev.parent = bus->dev;
> +
> + phydev->dev.bus = &mdio_bus_type;
> +
> + phydev->bus = bus;
> +
> + sprintf(phydev->dev.bus_id, "phy%d:%d", bus->id,
Imho you are going a bit too far with empty lines. Not a real issue.
> i);
Something decided to wrap the lines after you did the patch. It appears
several times in the patch (+ extra tabs/spaces at the end of the lines:
there are plenty of nice colorised editors around).
> +
> + err = device_register(&phydev->dev);
> +
> + if (err)
> + printk("phy %d did not register (%d)\n",
> + i, err);
Missing KERN_SOMETHING in the printk.
> +
> + /* If get_phy_device returned NULL, it may be
> + * because an error occurred. If so, we return
> + * that error */
> + } else if (errno)
> + return errno;
I'd rather use ERR_PTR/PTR_ERR/IS_ERR + goto in the first place
(then the previous printk will fit on a single line).
> +
> + bus->phy_map[i] = phydev;
> + }
> +
> + pr_info("%s: probed\n", bus->name);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(mdiobus_register);
> +
> +void mdiobus_unregister(struct mii_bus *bus)
> +{
> + int i;
> +
> + for (i=0; i < PHY_MAX_ADDR; i++)
for( i = 0, ... + missing brace.
[...]
> +static int mdio_bus_suspend(struct device * dev, u32 state)
> +{
> + int ret = 0;
> +
> + if (dev->driver && dev->driver->suspend) {
> + ret = dev->driver->suspend(dev, state, SUSPEND_DISABLE);
> + if (ret == 0)
> + ret = dev->driver->suspend(dev, state,
> SUSPEND_SAVE_STATE);
> + if (ret == 0)
> + ret = dev->driver->suspend(dev, state,
> SUSPEND_POWER_DOWN);
> + }
Copy/paste abuse:
struct device_driver *drv = dev->driver;
(appears in several functions)
[...]
> +struct bus_type mdio_bus_type = {
> + .name = "mdio_bus",
> + .match = mdio_bus_match,
> + .suspend= mdio_bus_suspend,
^^
[...]
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/phy/phy.c
[...]
> +int phy_read(struct phy_device *phydev, u16 regnum);
> +int phy_write(struct phy_device *phydev, u16 regnum, u16 val);
> +void phy_change(void *data);
> +void phy_timer(unsigned long data);
phy_read and phy_write do not need to be forward-declared.
Not sure if it is possible for phy_{change/timer} as well.
> +
> +/* Convenience function to print out the current phy status
> + */
> +void phy_print_status(struct phy_device *phydev)
> +{
> + pr_info("%s: Link is %s", phydev->dev.bus_id,
> + phydev->link ? "Up" : "Down");
> + if (phydev->link)
> + printk(" - %d/%s", phydev->speed,
Missing KERN_SOMETHING in the printk.
[...]
> +static inline int phy_aneg_done(struct phy_device *phydev)
> +{
> + int retval;
> +
> + retval = phy_read(phydev, MII_BMSR);
> +
> + if (retval < 0)
> + return retval;
> +
> + return retval & BMSR_ANEGCOMPLETE;
Please use a ternary operator.
> +}
> +
> +/* phy_start_aneg
> + *
> + * description: Calls the PHY driver's config_aneg, and then
> + * sets the PHY state to PHY_AN if auto-negotiation is enabled,
> + * and to PHY_FORCING if auto-negotiation is disabled. Unless
> + * the PHY is currently HALTED.
> + */
> +int phy_start_aneg(struct phy_device *phydev)
> +{
> + int err = 0;
Unneeded initialization.
> +
> + spin_lock(&phydev->lock);
> +
> + if (AUTONEG_DISABLE == phydev->autoneg)
> + phy_sanitize_settings(phydev);
> +
> + err = phydev->drv->config_aneg(phydev);
> +
> + if (err < 0)
> + return err;
The lock should be released. Add a 'goto out_unlock;' ?
> +
> + if (phydev->state != PHY_HALTED) {
> + if (AUTONEG_ENABLE == phydev->autoneg) {
> + phydev->state = PHY_AN;
> + phydev->link_timeout = PHY_AN_TIMEOUT;
> + } else {
> + phydev->state = PHY_FORCING;
> + phydev->link_timeout = PHY_FORCE_TIMEOUT;
> + }
> + }
> +
out_unlock:
> + spin_unlock(&phydev->lock);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(phy_start_aneg);
> +
> +
[...]
> +/* A mapping of all SUPPORTED settings to speed/duplex */
> +static struct phy_setting settings[] = {
> + { .speed = 10000, .duplex = DUPLEX_FULL,
> + .setting = SUPPORTED_10000baseT_Full,
> + },
> + { .speed = SPEED_1000, .duplex = DUPLEX_FULL,
> + .setting = SUPPORTED_1000baseT_Full,
> + },
> + { .speed = SPEED_1000, .duplex = DUPLEX_HALF,
> + .setting = SUPPORTED_1000baseT_Half,
> + },
> + { .speed = SPEED_100, .duplex = DUPLEX_FULL,
> + .setting = SUPPORTED_100baseT_Full,
> + },
> + { .speed = SPEED_100, .duplex = DUPLEX_HALF,
> + .setting = SUPPORTED_100baseT_Half,
> + },
> + { .speed = SPEED_10, .duplex = DUPLEX_FULL,
> + .setting = SUPPORTED_10baseT_Full,
> + },
> + { .speed = SPEED_10, .duplex = DUPLEX_HALF,
> + .setting = SUPPORTED_10baseT_Half,
> + },
> +};
Would you veto some macro to initialise this array ?
> +
> +#define MAX_NUM_SETTINGS (sizeof(settings)/sizeof(struct phy_setting))
The kernel provides ARRAY_SIZE
[...]
> +static inline int phy_find_setting(int speed, int duplex)
> +{
> + int idx = 0;
> +
> + while (idx < MAX_NUM_SETTINGS &&
> + (settings[idx].speed != speed ||
> + settings[idx].duplex != duplex))
> + idx++;
"for" loop in disguise ?
> +
> + return idx < MAX_NUM_SETTINGS ? idx : MAX_NUM_SETTINGS - 1;
Ok (dunno if "idx % MAX_NUM_SETTINGS" is more idiomatic or not).
[...]
> +int phy_start_interrupts(struct phy_device *phydev)
> +{
> + int err = 0;
> +
> + INIT_WORK(&phydev->phy_queue, phy_change, phydev);
> +
> + if (request_irq(phydev->irq, phy_interrupt,
> + SA_SHIRQ,
> + "phy_interrupt",
> + phydev) < 0) {
Please, don't do that :o(
err = request_irq(phydev->irq, phy_interrupt, SA_SHIRQ,
"phy_interrupt", phydev);
if (err < 0)
...
> + printk(KERN_ERR "%s: Can't get IRQ %d (PHY)\n",
> + phydev->bus->name,
> + phydev->irq);
> + phydev->irq = PHY_POLL;
> + return 0;
The description of the function says "Returns 0 on success".
[...]
> +/* Bring down the PHY link, and stop checking the status. */
> +void phy_stop(struct phy_device *phydev)
> +{
> + spin_lock(&phydev->lock);
> +
> + if (PHY_HALTED == phydev->state) {
> + spin_unlock(&phydev->lock);
> + return;
> + }
"goto out_unlock;"
> +
> + if (phydev->irq != PHY_POLL) {
> + /* Clear any pending interrupts */
> + phy_clear_interrupt(phydev);
> +
> + /* Disable PHY Interrupts */
> + phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
> + }
> +
> + phydev->state = PHY_HALTED;
> +
> + spin_unlock(&phydev->lock);
> +}
[...]
> +struct phy_device * get_phy_device(struct mii_bus *bus, uint addr)
> +{
uint ?
> + int phy_reg;
> + u32 phy_id;
> + struct phy_device *dev = NULL;
> +
> + errno = 0;
A bit ugly.
> +
> + /* Grab the bits from PHYIR1, and put them
> + * in the upper half */
> + phy_reg = bus->read(bus, addr, MII_PHYSID1);
> +
> + if (phy_reg < 0) {
> + errno = phy_reg;
> + return NULL;
dev = ERR_PTR(phy_reg);
goto out;
...
> + }
> +
> + phy_id = (phy_reg & 0xffff) << 16;
> +
> + /* Grab the bits from PHYIR2, and put them in the lower half */
> + phy_reg = bus->read(bus, addr, MII_PHYSID2);
> +
> + if (phy_reg < 0) {
> + errno = phy_reg;
> + return NULL;
> + }
> +
> + phy_id |= (phy_reg & 0xffff);
> +
> + /* If the phy_id is all Fs, there is no device there */
> + if (0xffffffff == phy_id)
> + return NULL;
> +
> + /* Otherwise, we allocate the device, and initialize the
> + * default values */
> + dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> +
> + if (NULL == dev) {
> + errno = -ENOMEM;
> + return NULL;
> + }
> +
> + memset(dev, 0, sizeof(*dev));
The kernel provides kcalloc.
[...]
> +static int phy_compare_id(struct device *dev, void *data)
> +{
> + const char *name = data;
> +
> + if (strcmp(name, dev->bus_id) == 0)
> + return 1;
> + return 0;
Ternary operator ?
> +}
> +
> +struct phy_device *phy_attach(struct net_device *dev,
> + const char *phy_id, u32 flags)
> +{
> + struct phy_device *phydev = NULL;
Useless initialization.
> + struct bus_type *bus = &mdio_bus_type;
> + struct device *d;
> +
> + /* Search the list of PHY devices on the mdio bus for the
> + * PHY with the requested name */
> + d = bus_find_device(bus, NULL, (void *)phy_id, phy_compare_id);
> +
> + if (d) {
> + phydev = to_phy_device(d);
> + } else {
> + printk(KERN_ERR "%s not found\n", phy_id);
> + errno = -ENODEV;
> + return NULL;
> + }
> +
> + /* Assume that if there is no driver, that it doesn't
> + * exist, and we should use the genphy driver. */
> + if (NULL == phydev->dev.driver) {
> + int err;
> + down_write(&phydev->dev.bus->subsys.rwsem);
> + phydev->dev.driver = &genphy_driver.driver;
> +
> + err = phydev->dev.driver->probe(&phydev->dev);
Would it be possible to s/phydev->dev/d/ ?
--
Ueimor
