[net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[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

Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Heiner Kallweit]

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,

Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Niklas Söderlund]

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

Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Paolo Abeni]

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

Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Dimitri Fedrau]

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

Re: [net-next] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Niklas Söderlund]

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