Re: [PATCH net-next v3] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Dimitri Fedrau]

Hi Paolo,

On Fri, May 02, 2025 at 12:51:47PM +0200, Paolo Abeni wrote:
> On 4/29/25 10:03 PM, Niklas Söderlund wrote:
> >> @@ -765,6 +768,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> >>  	struct mv88q2xxx_priv *priv = phydev->priv;
> >>  	struct device *dev = &phydev->mdio.dev;
> >>  	struct device *hwmon;
> >> +	int ret;
> >> +
> >> +	/* Enable temperature sense */
> >> +	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;
> > 
> > nit: I wonder if it make sens to create a helper function to enable the 
> > sensor? My worry being this procedure growing in the future and only 
> > being fixed in one location and not the other. It would also reduce code 
> > duplication and could be stubbed to be compiled out with the existing 
> > IS_ENABLED(CONFIG_HWMON) guard for other hwmon functions.
> > 
> > That being said I tested this with mv88q211x and the temp sensor and PHY 
> > keeps working as expected.
> > 
> > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> 
> Since this is net-next material +1 for the helper.
>
Agreed.

> Also AFAICS this is fixing a net-next regression, so it needs a fixes
> tag, too.
> 
We discussed it here:
https://lore.kernel.org/netdev/48c4cd14-be56-438e-9561-c85b0245178c@xxxxxxx/

Andrews opinion was the change is rather archtitecture then a fix.

Best regards,
Dimitri Fedrau