Hi Heiner, Thanks for your feedback. On 2025-03-16 12:47:55 +0100, Heiner Kallweit wrote: > On 16.03.2025 12:20, Niklas Söderlund wrote: > > The temperature sensor enabled for mv88q222x devices also functions for > > mv88q211x based devices. Unify the two devices probe functions to enable > > the sensors for all devices supported by this driver. > > > > The same oddity as for mv88q222x devices exists, the PHY must be up for > > a correct temperature reading to be reported. > > > In this case, wouldn't it make sense to extend mv88q2xxx_hwmon_is_visible() > and hide the temp_input attribute if PHY is down? > Whatever down here means in detail: Link down? In power-down mode? These are good suggestions, this issue is being worked on [1]. I just wanted to highlight that this entablement behaves the same as the current models that support the temperature sensor and log how this was tested on mv88q211x. 1. https://lore.kernel.org/all/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@xxxxxxxxx/ > > > # cat /sys/class/hwmon/hwmon9/temp1_input > > -75000 > > > > # ifconfig end5 up > > > > # cat /sys/class/hwmon/hwmon9/temp1_input > > 59000 > > > > Worth noting is that while the temperature register offsets and layout > > are the same between mv88q211x and mv88q222x devices their names in the > > datasheets are different. This change keeps the mv88q222x names for the > > mv88q211x support. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > > --- > > drivers/net/phy/marvell-88q2xxx.c | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > index 15c0f8adc2f5..cdd40b613ce8 100644 > > --- a/drivers/net/phy/marvell-88q2xxx.c > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > @@ -834,6 +834,7 @@ static int mv88q2xxx_leds_probe(struct phy_device *phydev) > > static int mv88q2xxx_probe(struct phy_device *phydev) > > { > > struct mv88q2xxx_priv *priv; > > + int ret; > > > > priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); > > if (!priv) > > @@ -841,17 +842,6 @@ static int mv88q2xxx_probe(struct phy_device *phydev) > > > > phydev->priv = priv; > > > > - return 0; > > -} > > - > > -static int mv88q222x_probe(struct phy_device *phydev) > > -{ > > - int ret; > > - > > - ret = mv88q2xxx_probe(phydev); > > - if (ret) > > - return ret; > > - > > ret = mv88q2xxx_leds_probe(phydev); > > if (ret) > > return ret; > > @@ -1124,7 +1114,7 @@ static struct phy_driver mv88q2xxx_driver[] = { > > .phy_id_mask = MARVELL_PHY_ID_MASK, > > .name = "mv88q2220", > > .flags = PHY_POLL_CABLE_TEST, > > - .probe = mv88q222x_probe, > > + .probe = mv88q2xxx_probe, > > .get_features = mv88q2xxx_get_features, > > .config_aneg = mv88q2xxx_config_aneg, > > .aneg_done = genphy_c45_aneg_done, > -- Kind Regards, Niklas Söderlund
