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.
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!
