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

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:43:40AM +0000, Russell King (Oracle) wrote:
> 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@xxxxxxxxx>
> > ---
> >  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.

For some AT803x ID it might be O.K. but here we are mixing all kind of
thing and you already noticing the state of this driver with the priv
changes. Again it's all to facilitate the last 2 patch of this series.

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

-- 
	Ansuel