From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next] net: phy: marvell: mv88e6390 temperature sensor
 reading
Date: Wed, 10 Jan 2018 14:52:24 +0100
Message-ID: <20180110135224.GA32117@lunn.ch>
References: <1515534129-13399-1-git-send-email-andrew@lunn.ch>
 <4033cdd4-36ae-060d-57f3-f8def0598024@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Miller <davem@davemloft.net>, netdev <netdev@vger.kernel.org>
To: Florian Fainelli <f.fainelli@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:36377 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S965122AbeAJNwa (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 10 Jan 2018 08:52:30 -0500
Content-Disposition: inline
In-Reply-To: <4033cdd4-36ae-060d-57f3-f8def0598024@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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