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 <and...@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
