Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

On Wed, 2017-06-07 at 12:18 +0930, Joel Stanley wrote:
> On Wed, Jun 7, 2017 at 1:50 AM, Matthew Barth
> > <msbarth@linux.vnet.ibm.com> wrote:
> > 
> > On 06/06/17 8:33 AM, Guenter Roeck wrote:
> > > 
> > > On 06/06/2017 12:02 AM, Andrew Jeffery wrote:
> > > > Over and above the features of the original patch is support for a
> > > > secondary
> > > > rotor measurement value that is provided by MAX31785 chips with a revised
> > > > firmware. The feature(s) of the firmware are determined at probe time and
> > > > extra
> > > > attributes exposed accordingly. Specifically, the MFR_REVISION 0x3040 of
> > > > the
> > > > firmware supports 'slow' and 'fast' rotor reads. The feature is
> > > > implemented by
> > > > command 0x90 (READ_FAN_SPEED_1) providing a 4-byte response (with the
> > > > 'fast'
> > > > measurement in the second word) rather than the 2-bytes response in the
> > > > original firmware (MFR_REVISION 0x3030).
> > > > 
> > > 
> > > Taking the pmbus driver question out, why would this warrant another
> > > non-standard
> > > attribute outside the ABI ? I could see the desire to replace the 'slow'
> > > access
> > > with the 'fast' one, but provide two attributes ? No, I don't see the
> > > point, sorry,
> > > even more so without detailed explanation why the second attribute in
> > > addition
> > > to the first one would add any value.
> > 
> > In the case of counter-rotating(CR) fans which contain two rotors to provide
> > more airflow there are then two tach feedbacks. These CR fans take a single
> > target speed and provide individual feedbacks for each rotor contained
> > within the fan enclosure. Providing these individual feedbacks assists in
> > fan fault driven speed changes, improved thermal characterization among
> > other things.
> > 
> > Maxim provided this as a 'slow' / 'fast' set of bytes to be user
> > compatible(so the 'slow' rotor speed, regardless of which rotor, is in the
> > first 2 bytes with the 'slow' version of firmware as well). In some cases,
> > mfg systems could have a mix of these revisions.
> 
> Andrew, instead of creating the _fast sysfs nodes, I think you could
> expose the extra rotors are separate fan devices in sysfs. This would
> not introduce new ABI.

I considered this approach: I debated whether to consider the fan unit
as a single entity with two inputs, or just separate fans, and ended up
leaning towards the former. To go the latter path we need to consider
whether or not to expose the writeable properties for the second input
(given they also affect the first) and how to sensibly arrange the
pairs given the functionality is determined at probe-time. Not rocket
science but decisions we need to make.

There's also the issue that the physical fan that each element of an
input pair represents will change as the fan speeds vary, based on the
behaviour that Matt outlined. I don't feel this maps well onto the
expectations of the sysfs interface, but then I'm not sure there's much
we can do to alleviate it either other than designating one of the
input attributes of a fan pair as the fastest input.

Regardless, I'll look into it in the anticipation that this is a viable
way forward.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]