Re: [PATCH v2] lxt PHY: Support for the buggy LXT973 rev A2

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.
 

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.

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

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