Messages in this thread | | | Subject | Re: [PATCH net-next v7 5/9] net: phy: add genphy_c45_ethtool_get/set_eee() support | From | Alexander H Duyck <> | Date | Fri, 10 Feb 2023 07:43:51 -0800 |
| |
On Thu, 2023-02-09 at 10:51 +0100, Oleksij Rempel wrote: > Add replacement for phy_ethtool_get/set_eee() functions. > > Current phy_ethtool_get/set_eee() implementation is great and it is > possible to make it even better: > - this functionality is for devices implementing parts of IEEE 802.3 > specification beyond Clause 22. The better place for this code is > phy-c45.c > - currently it is able to do read/write operations on PHYs with > different abilities to not existing registers. It is better to > use stored supported_eee abilities to avoid false read/write > operations. > - the eee_active detection will provide wrong results on not supported > link modes. It is better to validate speed/duplex properties against > supported EEE link modes. > - it is able to support only limited amount of link modes. We have more > EEE link modes... > > By refactoring this code I address most of this point except of the last > one. Adding additional EEE link modes will need more work. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/phy-c45.c | 238 ++++++++++++++++++++++++++++++++++++++ > include/linux/mdio.h | 58 ++++++++++ > include/linux/phy.h | 7 ++ > include/uapi/linux/mdio.h | 8 ++ > 4 files changed, 311 insertions(+) > >
<...>
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h > index e75583f5d967..e3568e44efd0 100644 > --- a/include/linux/mdio.h > +++ b/include/linux/mdio.h > @@ -428,6 +428,64 @@ static inline void mii_eee_cap1_mod_linkmode_t(unsigned long *adv, u32 val) > adv, val & MDIO_EEE_10GKR); > } > > +/** > + * mii_eee_cap1_mod_linkmode_t > + * @adv: the linkmode advertisement settings > + * > + * A function that translates linkmode to value for IEEE 802.3-2018 45.2.7.13 > + * "EEE advertisement 1" register (7.60) > + */ > +static inline u32 linkmode_to_mii_eee_cap1_t(unsigned long *adv)
So the function comments don't match the name for the function. Perhaps a copy/paste error? Otherwise the rest of the function description looks fine.
> +{ > + u32 result = 0; > + > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, adv)) > + result |= MDIO_EEE_100TX; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, adv)) > + result |= MDIO_EEE_1000T; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, adv)) > + result |= MDIO_EEE_10GT; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, adv)) > + result |= MDIO_EEE_1000KX; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, adv)) > + result |= MDIO_EEE_10GKX4; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, adv)) > + result |= MDIO_EEE_10GKR; > + > + return result; > +} > +
| |