| Message ID | 20250316112033.1097152-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x | expand |
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? > # 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@ragnatech.se> > --- > 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,
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@gmail.com/ > > > # 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@ragnatech.se> > > --- > > 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, >
Hi Niklas, Am Sun, Mar 16, 2025 at 12:20:33PM +0100 schrieb Niklas Söderlund: > 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. > > # 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@ragnatech.se> > --- > 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, > -- > 2.48.1 > thanks for your patch. Looks good to me. Did you have the chance to test the LED support as well ? I'm asking because mv88q2xxx_leds_probe gets called in mv88q2xxx_probe. LED handling should be equal for all 88Q2XXX devices, adding the support should be easy. Anyway: Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com> Best regards, Dimitri Fedrau
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,
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. # 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@ragnatech.se> --- drivers/net/phy/marvell-88q2xxx.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)