Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn <andrew@lunn.ch>]

> +/* RGMII Rx Clock delay value change with board lay-out */
> +static u8 rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different
layouts? You should be getting this value from the device tree.

> +	phydev->supported = (SUPPORTED_1000baseT_Full |
> +			     SUPPORTED_1000baseT_Half |
> +			     SUPPORTED_100baseT_Full  |
> +			     SUPPORTED_100baseT_Half  |
> +			     SUPPORTED_10baseT_Full   |
> +			     SUPPORTED_10baseT_Half   |
> +			     SUPPORTED_Autoneg        |
> +			     SUPPORTED_Pause          |
> +			     SUPPORTED_Asym_Pause     |
> +			     SUPPORTED_TP);
> +
> +	phydev->speed = SPEED_1000;
> +	phydev->duplex = DUPLEX_FULL;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +	phydev->interface = PHY_INTERFACE_MODE_RGMII;
> +	phydev->mdix = ETH_TP_MDI_AUTO;

Why are you setting all these? This is not normal, if you look at
other drivers.

> +
> +	mutex_lock(&phydev->lock);

What are you locking against?

> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> +	if (rc != 0) {
> +		rc = -EINVAL;

Why do you overwrite the error code vsc85xx_phy_page_set gives you?

> +		goto out_unlock;
> +	}
> +	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
> +	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
> +	reg_val |= (rgmii_rx_clk_delay << RGMII_RX_CLK_DELAY_POS);
> +	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +	if (rc != 0)
> +		rc = -EINVAL;

Same here.

> +
> +out_unlock:
> +	mutex_unlock(&phydev->lock);
> +
> +	return rc;
> +}
> +
> +static int vsc85xx_config_init(struct phy_device *phydev)
> +{
> +	int rc = 0;

No need to initialise rc.

> +	rc = vsc85xx_default_config(phydev);

  	if (rc)
    		return rc;

> +	rc = genphy_config_init(phydev);
> +
> +	return rc;

Or just 
	return genphy_config_init(phydev);

> +}
> +
> +static int vsc85xx_ack_interrupt(struct phy_device *phydev)
> +{
> +	int rc = 0;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> +
> +	return (rc < 0) ? rc : 0;
> +}
> +
> +static int vsc85xx_config_intr(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
> +			       MII_VSC85XX_INT_MASK_MASK);
> +	} else {
> +		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> +		if (rc < 0)
> +			return rc;

And the purpose of this read is? I assume it clears an outstanding
interrupt? If so, shouldn't you do it after disabling interrupts, not
before? Otherwise you have a race condition.

> +		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
> +	}
> +
> +	return rc;

  Andrew