* [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
@ 2025-03-16 11:20 Niklas Söderlund
2025-03-16 11:47 ` Heiner Kallweit
2025-03-16 15:23 ` Dimitri Fedrau
0 siblings, 2 replies; 6+ messages in thread
From: Niklas Söderlund @ 2025-03-16 11:20 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dimitri Fedrau, netdev
Cc: linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
2025-03-16 11:20 [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x Niklas Söderlund
@ 2025-03-16 11:47 ` Heiner Kallweit
2025-03-16 12:02 ` Niklas Söderlund
2025-03-16 15:23 ` Dimitri Fedrau
1 sibling, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2025-03-16 11:47 UTC (permalink / raw)
To: Niklas Söderlund, Andrew Lunn, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dimitri Fedrau, netdev
Cc: linux-renesas-soc
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
2025-03-16 11:47 ` Heiner Kallweit
@ 2025-03-16 12:02 ` Niklas Söderlund
2025-03-21 20:37 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Niklas Söderlund @ 2025-03-16 12:02 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Dimitri Fedrau, netdev,
linux-renesas-soc
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,
>
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
2025-03-16 11:20 [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x Niklas Söderlund
2025-03-16 11:47 ` Heiner Kallweit
@ 2025-03-16 15:23 ` Dimitri Fedrau
2025-03-17 0:32 ` Niklas Söderlund
1 sibling, 1 reply; 6+ messages in thread
From: Dimitri Fedrau @ 2025-03-16 15:23 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev,
linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
2025-03-16 15:23 ` Dimitri Fedrau
@ 2025-03-17 0:32 ` Niklas Söderlund
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Söderlund @ 2025-03-17 0:32 UTC (permalink / raw)
To: Dimitri Fedrau
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev,
linux-renesas-soc
Hi Dimitri,
On 2025-03-16 16:23:24 +0100, Dimitri Fedrau wrote:
> 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:
The board I have to test on is remote, so I have no way to inspect the
LED. I did check the datasheet and schematics, and as you say it looks
the same for both devices.
>
> Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com>
>
> Best regards,
> Dimitri Fedrau
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
2025-03-16 12:02 ` Niklas Söderlund
@ 2025-03-21 20:37 ` Paolo Abeni
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2025-03-21 20:37 UTC (permalink / raw)
To: Niklas Söderlund, Heiner Kallweit
Cc: Andrew Lunn, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Dimitri Fedrau, netdev, linux-renesas-soc
Hi,
On 3/16/25 1:02 PM, Niklas Söderlund wrote:
> 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/
My take is that you should at least clarify in the commit message the
state required for a correct reading - e.g. link up vs power-up.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-21 20:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-16 11:20 [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x Niklas Söderlund
2025-03-16 11:47 ` Heiner Kallweit
2025-03-16 12:02 ` Niklas Söderlund
2025-03-21 20:37 ` Paolo Abeni
2025-03-16 15:23 ` Dimitri Fedrau
2025-03-17 0:32 ` Niklas Söderlund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).