Re: [PATCH net-next] net: phy: marvell: mv88e6390 temperature sensor reading

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Jan 09, 2018 at 04:14:44PM -0800, Florian Fainelli wrote:
> On 01/09/2018 01:42 PM, Andrew Lunn wrote:
> > The internal PHYs in the mv88e6390 switch have a temperature sensor.
> > It uses a different register layout to other PHY currently supported.
> > It also has an errata, in that some reads of the sensor result in bad
> > values. So a number of reads need to be made, and the average taken.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Just a few nits below:
> 
> [snip]
> 
> > +static int m88e6390_hwmon_read(struct device *dev,
> > +			       enum hwmon_sensor_types type,
> > +			       u32 attr, int channel, long *temp)
> > +{
> > +	struct phy_device *phydev = dev_get_drvdata(dev);
> 
> Why not to_phy_device()?

Hi Florian

The marvell driver already supports two other temperature
sensors. While implementing this third, i just copy/pasted the
m88e1121 code, and then applied changes as needed for the 6390.
dev_get_drvdata() is what the other two use.

> [snip]
> 
> > +
> > +static int m88e6390_hwmon_probe(struct phy_device *phydev)
> > +{
> > +	return marvell_hwmon_probe(phydev, &m88e6390_hwmon_chip_info);
> > +}
> >  #else
> >  static int m88e1121_hwmon_probe(struct phy_device *phydev)
> >  {
> > @@ -1794,6 +1927,11 @@ static int m88e1510_hwmon_probe(struct phy_device *phydev)
> >  {
> >  	return 0;
> >  }
> > +
> > +static int m88e6390_hwmon_probe(struct phy_device *phydev)
> > +{
> > +	return 0;
> > +}
> 
> Instead of having to define m88e6390_hwmon_probe() twice, I would just
> make marvell_hwmon_probe() a stub when CONFIG_HWMON=n?

Yes, i could do that. But again, i'm just following the pattern from
the other two sensors.

So in order to make these change requests, i will add another two
patches to the series, which first changes the existing two temperature
sensors. Then add the 6390.

	 Andrew