[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6c5d7946-0776-4129-89db-2602e1874615@gmail.com>
Date: Wed, 7 Feb 2024 20:51:29 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Marek Behún <kabel@...nel.org>,
Russell King - ARM Linux <linux@...linux.org.uk>,
Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO
constants
Hi Andrew,
On 04.02.2024 17:35, Heiner Kallweit wrote:
> 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@...nel.org>
>>>>
>>>> Drop the ad-hoc MDIO constants used in the driver and use generic
>>>> constants instead.
>>>>
>>>> Signed-off-by: Marek Behún <kabel@...nel.org>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.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.
>>
Is it fine with you to do this in a follow-up patch?
The series is marked "under review", so Jakub seems to wait for an outcome
of our discussion.
>>> 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
>
Heiner
Powered by blists - more mailing lists