On Mon, Sep 10, 2012 at 05:45:49PM +0200, Christophe Leroy wrote: > This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding > precautions linked to ERRATA Item 4:
I have a few nit picking remarks, below... > Item 4: MDIO Interface and Repeated Polling > Problem: Repeated polling of odd-numbered registers via the MDIO interface > randomly returns the contents of the previous even register. > Implication: Managed applications may not obtain the correct register contents > when a particular register is monitored for device status. > Workaround: None. > Status: This erratum has been previously fixed (in rev A3) This text looks like it has been copied verbatim from a errata sheet. If so, that document is probably under someone else's copyright. If am right, then you should paraphrase the erratum instead, both here and in the code comment. > > Signed-off-by: Christophe Leroy <christophe.le...@c-s.fr> > > diff -u linux-3.5-vanilla/drivers/net/phy/lxt.c > linux-3.5/drivers/net/phy/lxt.c > --- linux-3.5-vanilla/drivers/net/phy/lxt.c 2012-07-21 22:58:29.000000000 > +0200 > +++ linux-3.5/drivers/net/phy/lxt.c 2012-09-07 18:08:54.000000000 +0200 > @@ -7,6 +7,10 @@ > * > * Copyright (c) 2004 Freescale Semiconductor, Inc. > * > + * Copyright (c) 2010 CSSI > + * > + * Added support for buggy LXT973 rev A2 We don't do change logs in the code. That is what git is for. Also, I think it is a bit of a stretch to add your copyright here. > + * > * This program is free software; you can redistribute it and/or modify it > * under the terms of the GNU General Public License as published by the > * Free Software Foundation; either version 2 of the License, or (at your > @@ -54,8 +58,12 @@ > #define MII_LXT971_ISR 19 /* Interrupt Status Register */ > > /* register definitions for the 973 */ > -#define MII_LXT973_PCR 16 /* Port Configuration Register */ > +#define MII_LXT973_PCR 16 /* Port Configuration Register */ > #define PCR_FIBER_SELECT 1 > +#define MII_LXT973_SFR 27 /* Special Function Register */ > + > +#define PHYDEV_PRIV_FIBER 1 > +#define PHYDEV_PRIV_REVA2 2 > > MODULE_DESCRIPTION("Intel LXT PHY driver"); > MODULE_AUTHOR("Andy Fleming"); > @@ -99,6 +107,9 @@ > return err; > } > > +/* register definitions for the 973 */ > +#define MII_LXT973_PCR 16 /* Port Configuration Register */ > +#define PCR_FIBER_SELECT 1 > > static int lxt971_ack_interrupt(struct phy_device *phydev) > { > @@ -122,9 +133,141 @@ > return err; > } > > +/* > + * ERRATA on LXT973 > + * > + * Item 4: MDIO Interface and Repeated Polling > + * Problem: Repeated polling of odd-numbered registers via the MDIO > interface randomly returns the > + * contents of the previous even register. > + * Implication: Managed applications may not obtain the correct register > contents when a particular > + * register is monitored for device status. > + * Workaround: None. > + * Status: This erratum has been previously fixed (in rev A3) > + * > + */ Please make sure that the above text is your own words. > +static int lxt973a2_update_link(struct phy_device *phydev) > +{ > + int status; > + int control; > + int retry = 8; /* we try 8 times */ > + > + /* Do a fake read */ > + status = phy_read(phydev, MII_BMSR); > + > + if (status < 0) > + return status; > + > + control = phy_read(phydev, MII_BMCR); > + if (control < 0) > + return control; > + > + do { > + /* Read link and autonegotiation status */ > + status = phy_read(phydev, MII_BMSR); > + } while (status>=0 && retry-- && status == control); spacing: status >= 0 > + > + if (status < 0) > + return status; > + > + if ((status & BMSR_LSTATUS) == 0) > + phydev->link = 0; > + else > + phydev->link = 1; > + > + return 0; > +} > + > +int lxt973a2_read_status(struct phy_device *phydev) > +{ > + int adv; > + int err; > + int lpa; > + int lpagb = 0; > + > + /* Update the link, but return if there was an error */ > + err = lxt973a2_update_link(phydev); > + if (err) > + return err; > + > + if (AUTONEG_ENABLE == phydev->autoneg) { > + int retry = 1; > + > + adv = phy_read(phydev, MII_ADVERTISE); > + > + if (adv < 0) > + return adv; > + > + do { > + lpa = phy_read(phydev, MII_LPA); > + > + if (lpa < 0) > + return lpa; > + > + /* If both registers are equal, it is suspect but not > impossible, hence a new try */ Trailing white space. Line is way too long. > + } while (lpa == adv && retry--); > + > + lpa &= adv; > + > + phydev->speed = SPEED_10; > + phydev->duplex = DUPLEX_HALF; > + phydev->pause = phydev->asym_pause = 0; > + > + if (lpagb & (LPA_1000FULL | LPA_1000HALF)) { > + phydev->speed = SPEED_1000; > + > + if (lpagb & LPA_1000FULL) > + phydev->duplex = DUPLEX_FULL; > + } else if (lpa & (LPA_100FULL | LPA_100HALF)) { > + phydev->speed = SPEED_100; > + > + if (lpa & LPA_100FULL) > + phydev->duplex = DUPLEX_FULL; > + } else I would either put this 'else' with the 'if' on the same line, or add braces like in the other cases. > + if (lpa & LPA_10FULL) > + phydev->duplex = DUPLEX_FULL; > + > + if (phydev->duplex == DUPLEX_FULL){ > + phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0; > + phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0; > + } > + } else { > + int bmcr = phy_read(phydev, MII_BMCR); > + > + if (bmcr < 0) > + return bmcr; > + > + if (bmcr & BMCR_FULLDPLX) > + phydev->duplex = DUPLEX_FULL; > + else > + phydev->duplex = DUPLEX_HALF; > + > + if (bmcr & BMCR_SPEED1000) > + phydev->speed = SPEED_1000; > + else if (bmcr & BMCR_SPEED100) > + phydev->speed = SPEED_100; > + else > + phydev->speed = SPEED_10; > + > + phydev->pause = phydev->asym_pause = 0; > + } > + > + return 0; > +} > + > +static int lxt973_read_status(struct phy_device *phydev) > +{ > + return (int)phydev->priv&PHYDEV_PRIV_REVA2 ? > lxt973a2_read_status(phydev) : genphy_read_status(phydev); spacing, and way too long line. return (int) phydev->priv & PHYDEV_PRIV_REVA2 ? lxt973a2_read_status(phydev) : genphy_read_status(phydev); > +} > + > static int lxt973_probe(struct phy_device *phydev) > { > int val = phy_read(phydev, MII_LXT973_PCR); > + int priv = 0; > + > + phydev->priv = NULL; > + > + if (val<0) return val; spacing > > if (val & PCR_FIBER_SELECT) { > /* > @@ -136,17 +279,24 @@ > val &= ~BMCR_ANENABLE; > phy_write(phydev, MII_BMCR, val); > /* Remember that the port is in fiber mode. */ > - phydev->priv = lxt973_probe; > - } else { > - phydev->priv = NULL; > + priv |= PHYDEV_PRIV_FIBER; > } > + val = phy_read(phydev, MII_PHYSID2); > + > + if (val<0) return val; spacing > + > + if ((val & 0xf) == 0) { /* rev A2 */ > + dev_info(&phydev->dev, " LXT973 revision A2 has bugs\n"); > + priv |= PHYDEV_PRIV_REVA2; > + } > + phydev->priv = (void*)priv; spacing > return 0; > } > > static int lxt973_config_aneg(struct phy_device *phydev) > { > /* Do nothing if port is in fiber mode. */ > - return phydev->priv ? 0 : genphy_config_aneg(phydev); > + return (int)phydev->priv&PHYDEV_PRIV_FIBER ? 0 : > genphy_config_aneg(phydev); spacing, and a long line. You might want to try checkpatch next time. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/