Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit <hkallweit1@gmail.com>]

On 04.02.2024 17:26, Heiner Kallweit wrote:
> On 04.02.2024 17:00, Andrew Lunn wrote:
>> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote:
>>> From: Marek Behún <kabel@kernel.org>
>>>
>>> Drop the ad-hoc MDIO constants used in the driver and use generic
>>> constants instead.
>>>
>>> Signed-off-by: Marek Behún <kabel@kernel.org>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/net/phy/realtek.c | 30 +++++++++++++-----------------
>>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index 894172a3e..ffc13c495 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -57,14 +57,6 @@
>>>  #define RTL8366RB_POWER_SAVE			0x15
>>>  #define RTL8366RB_POWER_SAVE_ON			BIT(12)
>>>  
>>> -#define RTL_SUPPORTS_5000FULL			BIT(14)
>>> -#define RTL_SUPPORTS_2500FULL			BIT(13)
>>> -#define RTL_SUPPORTS_10000FULL			BIT(0)
>>> -#define RTL_ADV_2500FULL			BIT(7)
>>> -#define RTL_LPADV_10000FULL			BIT(11)
>>> -#define RTL_LPADV_5000FULL			BIT(6)
>>> -#define RTL_LPADV_2500FULL			BIT(5)
>>> -
>>>  #define RTL9000A_GINMR				0x14
>>>  #define RTL9000A_GINMR_LINK_STATUS		BIT(4)
>>>  
>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev)
>>>  		return val;
>>>  
>>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>> -			 phydev->supported, val & RTL_SUPPORTS_2500FULL);
>>> +			 phydev->supported, val & MDIO_PMA_SPEED_2_5G);
>>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>>> -			 phydev->supported, val & RTL_SUPPORTS_5000FULL);
>>> +			 phydev->supported, val & MDIO_PMA_SPEED_5G);
>>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>>> -			 phydev->supported, val & RTL_SUPPORTS_10000FULL);
>>> +			 phydev->supported, val & MDIO_SPEED_10G);
>>
>> Now that this only using generic constants, should it move into mdio.h
>> as a shared helper? Is this a standard register defined in 802.3, just
>> at a different address?
>>
> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific
> register. There's very few users of this register, and nothing where such
> a helper could be reused.
> 
>>>  
>>>  	return genphy_read_abilities(phydev);
>>>  }
>>> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
>>>  
>>>  		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>>  				      phydev->advertising))
>>> -			adv2500 = RTL_ADV_2500FULL;
>>> +			adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G;
>>>  

Similarly linkmode_adv_to_mii_10gbt_adv_t() can be used here.

>>>  		ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12,
>>> -					       RTL_ADV_2500FULL, adv2500);
>>> +					       MDIO_AN_10GBT_CTRL_ADV2_5G,
>>> +					       adv2500);
>>>  		if (ret < 0)
>>>  			return ret;
>>>  	}
>>> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev)
>>>  			return lpadv;
>>>  
>>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>>> -			phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL);
>>> +				 phydev->lp_advertising,
>>> +				 lpadv & MDIO_AN_10GBT_STAT_LP10G);
>>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>>> -			phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL);
>>> +				 phydev->lp_advertising,
>>> +				 lpadv & MDIO_AN_10GBT_STAT_LP5G);
>>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>> -			phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL);
>>> +				 phydev->lp_advertising,
>>> +				 lpadv & MDIO_AN_10GBT_STAT_LP2_5G);
>>
>> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ?
>>
> Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use
> of this helper as a follow-up patch. Then it's obvious that the helper is
> the same as the replaced code.
> 
>> Something i've done in the past is to do this sort of conversion to
>> standard macros, and the followed up with a patch which says that
>> function X is now clearly the same as helper Y, so delete the function
>> and use the helper...
>>
>>     Andrew
> Heiner