| Message ID | 20250214-marvell-88q2xxx-cleanup-v1-3-71d67c20f308@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c806720bafe3ae9c43eb2dc942a1849d5453736 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell-88q2xxx: cleanup | expand |
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 >
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? Architecturally, it seems odd to register the HWMON in probe, and then enable it in config_init. Andrew
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
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
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
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(-)