Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Oct 17, 2024 at 02:54:07PM +1300, Paul Davey wrote:
> Add support for configuring MDI-X state of PHY.
> Add reporting of resolved MDI-X state in status information.

> [...]
> +static int aqr_set_polarity(struct phy_device *phydev, int polarity)

"polarity" is not the right term here. This is not about the polarity
of copper pairs, but rather about pairs being swapped.
Please name the function accordingly, eg. aqr_set_mdix().

> +{
> +	u16 val = 0;
> +
> +	switch (polarity) {
> +	case ETH_TP_MDI:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDI;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDIX;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +	case ETH_TP_MDI_INVALID:
> +	default:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_AUTO;
> +		break;
> +	}
> +
> +	return phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_PROV,
> +				      MDIO_AN_RESVD_VEND_PROV_MDIX_MASK, val);
> +}
> +
>  static int aqr_config_aneg(struct phy_device *phydev)
>  {
>  	bool changed = false;
>  	u16 reg;
>  	int ret;
>  
> +	ret = aqr_set_polarity(phydev, phydev->mdix_ctrl);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
>  	if (phydev->autoneg == AUTONEG_DISABLE)
>  		return genphy_c45_pma_setup_forced(phydev);
>  
> @@ -278,6 +315,14 @@ static int aqr_read_status(struct phy_device *phydev)
>  				 val & MDIO_AN_RX_LP_STAT1_1000BASET_HALF);
>  	}
>  
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_STATUS1);

According to the datasheet the MDI/MDI-X indication should only be
interpreted when autonegotiation has completed.
Hence this call should be protected by genphy_c45_aneg_done(phydev) and
phydev->mdix set to ETH_TP_MDI_INVALID in case auto-negotiation hasn't
completed.

> +	if (val < 0)
> +		return val;
> +	if (val & MDIO_AN_RESVD_VEND_STATUS1_MDIX)
> +		phydev->mdix = ETH_TP_MDI_X;
> +	else
> +		phydev->mdix = ETH_TP_MDI;
> +
>  	return genphy_c45_read_status(phydev);
>  }
>  
> -- 
> 2.47.0
> 
>