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

[net-next,v2] 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 link 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>
Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
* Changes since v1
- Clarify in commit message that it's the link that must be up for the
  reporting to work, not just power up. Hopefully this oddity can be
  solved in the future by patch [1].

  This change just aligns the mv88q211x with the current mv88q222x
  behavior.

1. https://lore.kernel.org/all/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@gmail.com/
---
 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 23e1f0521f54..5c687164b8e0 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -828,6 +828,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)
@@ -835,17 +836,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;
@@ -1118,7 +1108,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.49.0

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

[Andrew Lunn]

On Fri, Apr 18, 2025 at 04:58:00PM +0200, 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 link 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>
> Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Heiner Kallweit]

On 18.04.2025 16:58, 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 link 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>
> Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
> * Changes since v1
> - Clarify in commit message that it's the link that must be up for the
>   reporting to work, not just power up. Hopefully this oddity can be
>   solved in the future by patch [1].
> 
Following this link I don't see anything which would deal with incorrect
values being shown if link is down. What are you referring to in detail?
I think you have to modify mv88q2xxx_hwmon_is_visible(), or return an
appropriate error in mv88q2xxx_hwmon_read() if link is down.

>   This change just aligns the mv88q211x with the current mv88q222x
>   behavior.
> 
> 1. https://lore.kernel.org/all/20250220-marvell-88q2xxx-hwmon-enable-at-probe-v2-0-78b2838a62da@gmail.com/
> ---
>  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 23e1f0521f54..5c687164b8e0 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -828,6 +828,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)
> @@ -835,17 +836,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;
> @@ -1118,7 +1108,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,v2] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x

[Paolo Abeni]

On 4/18/25 6:10 PM, Heiner Kallweit wrote:
> On 18.04.2025 16:58, 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 link 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>
>> Reviewed-by: Dimitri Fedrau <dima.fedrau@gmail.com>
>> ---
>> * Changes since v1
>> - Clarify in commit message that it's the link that must be up for the
>>   reporting to work, not just power up. Hopefully this oddity can be
>>   solved in the future by patch [1].
>>
> Following this link I don't see anything which would deal with incorrect
> values being shown if link is down. What are you referring to in detail?
> I think you have to modify mv88q2xxx_hwmon_is_visible(), or return an
> appropriate error in mv88q2xxx_hwmon_read() if link is down.

@Heiner: my take is that the temp reading oddity should/will be handled
in a separate (net) patch, let's not block this patch due to that.

Thanks,

Paolo

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 18 Apr 2025 16:58:00 +0200 you 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 link must be up
> for a correct temperature reading to be reported.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: phy: marvell-88q2xxx: Enable temperature sensor for mv88q211x
    https://git.kernel.org/netdev/net-next/c/bef4f1156b74

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html