Re: [net-next PATCH 09/14] net: phy: at803x: remove specific qca808x check from at803x functions

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Nov 29, 2023 at 03:12:14AM +0100, Christian Marangi wrote:
> Remove specific qca808x check from at803x generic functions.
> 
> While this cause a bit of code duplication, this is needed in
> preparation for splitting the driver per PHY family and detaching
> qca808x specific bits from the at803x driver.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/at803x.c | 107 ++++++++++++++++++++++++++-------------
>  1 file changed, 71 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 8f5878ccb1a8..475b96165f45 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -1043,24 +1043,6 @@ static int at803x_config_aneg(struct phy_device *phydev)
>  	 */
>  	ret = 0;

Doesn't this become unnecessary?
>  
> -	if (phydev->drv->phy_id == QCA8081_PHY_ID) {
> -		int phy_ctrl = 0;
> -
> -		/* The reg MII_BMCR also needs to be configured for force mode, the
> -		 * genphy_config_aneg is also needed.
> -		 */
> -		if (phydev->autoneg == AUTONEG_DISABLE)
> -			genphy_c45_pma_setup_forced(phydev);
> -
> -		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> -			phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> -
> -		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> -				MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	return __genphy_config_aneg(phydev, ret);

... since you can just call genphy_config_aneg() here now?

> @@ -1845,6 +1815,47 @@ static int qca8327_suspend(struct phy_device *phydev)
>  	return qca83xx_suspend(phydev);
>  }
>  
> +static int qca808x_config_aneg(struct phy_device *phydev)
> +{
> +	int phy_ctrl = 0;
> +	int ret;
> +
> +	ret = at803x_config_mdix(phydev, phydev->mdix_ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Changes of the midx bits are disruptive to the normal operation;
> +	 * therefore any changes to these registers must be followed by a
> +	 * software reset to take effect.
> +	 */
> +	if (ret == 1) {
> +		ret = genphy_soft_reset(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* Do not restart auto-negotiation by setting ret to 0 defautly,
> +	 * when calling __genphy_config_aneg later.
> +	 */
> +	ret = 0;
> +
> +	/* The reg MII_BMCR also needs to be configured for force mode, the
> +	 * genphy_config_aneg is also needed.
> +	 */
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		genphy_c45_pma_setup_forced(phydev);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return __genphy_config_aneg(phydev, ret);
> +}

... but is it _really_ worth duplicating the entire function just to
deal with the QCA8081 difference? On balance, I think the original code
is better.

Overall, I'm getting the impression that you have a mental hang-up about
drivers checking the PHY ID in their method drivers... there's
absolutely nothing wrong with that. When the result of trying to
eliminate those results in bloating a driver, then the cleanup is not
a cleanup anymore, it creates bloat and makes future maintenance
harder.

Sorry, but no, I don't like this patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!