Temperature sensor gets enabled for 88Q222X devices in
mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
all 88Q2XXX devices support the temperature sensor.
Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
drivers/net/phy/marvell-88q2xxx.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 7b0913968bb404df1d271b040e698a4c8c391705..1859db10b3914f54486c7e6132b10b0353fa49e6 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -513,6 +513,15 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
return ret;
}
+ /* Enable temperature sense */
+ if (priv->enable_temp) {
+ ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+ MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+ if (ret < 0)
+ return ret;
+ }
+
return 0;
}
@@ -903,18 +912,6 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
static int mv88q222x_config_init(struct phy_device *phydev)
{
- struct mv88q2xxx_priv *priv = phydev->priv;
- int ret;
-
- /* Enable temperature sense */
- if (priv->enable_temp) {
- ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
- MDIO_MMD_PCS_MV_TEMP_SENSOR2,
- MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
- if (ret < 0)
- return ret;
- }
-
if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
return mv88q222x_revb0_config_init(phydev);
else
--
2.39.5On Fri, Feb 14, 2025 at 05:32:05PM +0100, Dimitri Fedrau wrote:
> Temperature sensor gets enabled for 88Q222X devices in
> mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> all 88Q2XXX devices support the temperature sensor.
>
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
The change itself looks fine:
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
but is the sensor actually usable if the PHY has not yet been
configured?
Architecturally, it seems odd to register the HWMON in probe, and then
enable it in config_init.
Andrew
Am Fri, Feb 14, 2025 at 07:06:22PM +0100 schrieb Andrew Lunn: > On Fri, Feb 14, 2025 at 05:32:05PM +0100, Dimitri Fedrau wrote: > > Temperature sensor gets enabled for 88Q222X devices in > > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because > > all 88Q2XXX devices support the temperature sensor. > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > The change itself looks fine: > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > but is the sensor actually usable if the PHY has not yet been > configured? > No. > Architecturally, it seems odd to register the HWMON in probe, and then > enable it in config_init. I think it makes sense to enable it in probe as well. I just moved it to config_init because of the PHYs hard reset. Before https://lore.kernel.org/netdev/20250118-marvell-88q2xxx-fix-hwmon-v2-1-402e62ba2dcb@gmail.com/ it was already done in mv88q2xxx_hwmon_probe. I will come up with a patch. Best regards, Dimitri Fedrau
Hi Dimitir,
Thanks for your work.
On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote:
> Temperature sensor gets enabled for 88Q222X devices in
> mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> all 88Q2XXX devices support the temperature sensor.
Is this true for mv88q2110 devices too? The current implementation only
enables it for mv88q222x devices. The private structure is not even
initialized for mv88q2110, and currently crashes. I have fixed that [1],
but I'm not sure if that should be extended to also enable temperature
sensor for mv88q2110?
>
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
In either case with [1] for an unrelated fix this is tested on
mv88q2110.
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
1. https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/
> ---
> drivers/net/phy/marvell-88q2xxx.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 7b0913968bb404df1d271b040e698a4c8c391705..1859db10b3914f54486c7e6132b10b0353fa49e6 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -513,6 +513,15 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
> return ret;
> }
>
> + /* Enable temperature sense */
> + if (priv->enable_temp) {
> + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> + MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> + if (ret < 0)
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -903,18 +912,6 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
>
> static int mv88q222x_config_init(struct phy_device *phydev)
> {
> - struct mv88q2xxx_priv *priv = phydev->priv;
> - int ret;
> -
> - /* Enable temperature sense */
> - if (priv->enable_temp) {
> - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> - MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> - MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> - if (ret < 0)
> - return ret;
> - }
> -
> if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
> return mv88q222x_revb0_config_init(phydev);
> else
>
> --
> 2.39.5
>
--
Kind Regards,
Niklas Söderlund
Hi Niklas, Am Fri, Feb 14, 2025 at 06:59:38PM +0100 schrieb Niklas Söderlund: > Hi Dimitir, > > Thanks for your work. > > On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote: > > Temperature sensor gets enabled for 88Q222X devices in > > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because > > all 88Q2XXX devices support the temperature sensor. > > Is this true for mv88q2110 devices too? The current implementation only > enables it for mv88q222x devices. The private structure is not even > initialized for mv88q2110, and currently crashes. I have fixed that [1], > but I'm not sure if that should be extended to also enable temperature > sensor for mv88q2110? > Yes, according to the datasheet. I don't have a mv88q2110 device, so I can't test it. I would like to see it enabled. So if you can test it and it works why not enabling it. Thanks for finding this. > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > In either case with [1] for an unrelated fix this is tested on > mv88q2110. > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > 1. https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/ > [...] Best regards, Dimitri Fedrau
Hi, On 2025-02-14 20:45:26 +0100, Dimitri Fedrau wrote: > Hi Niklas, > > Am Fri, Feb 14, 2025 at 06:59:38PM +0100 schrieb Niklas Söderlund: > > Hi Dimitir, > > > > Thanks for your work. > > > > On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote: > > > Temperature sensor gets enabled for 88Q222X devices in > > > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because > > > all 88Q2XXX devices support the temperature sensor. > > > > Is this true for mv88q2110 devices too? The current implementation only > > enables it for mv88q222x devices. The private structure is not even > > initialized for mv88q2110, and currently crashes. I have fixed that [1], > > but I'm not sure if that should be extended to also enable temperature > > sensor for mv88q2110? > > > Yes, according to the datasheet. I don't have a mv88q2110 device, so I > can't test it. I would like to see it enabled. So if you can test it and > it works why not enabling it. Thanks for finding this. Thanks for confirming. I will attempt to test and enable this once as soon as I can. > > > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > > In either case with [1] for an unrelated fix this is tested on > > mv88q2110. > > > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > 1. https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/ > > > [...] > > Best regards, > Dimitri Fedrau -- Kind Regards, Niklas Söderlund
© 2016 - 2025 Red Hat, Inc.