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

[Guenter Roeck]

On Wed, Jun 07, 2017 at 04:15:06PM +0930, Andrew Jeffery wrote:
> On Wed, 2017-06-07 at 12:18 +0930, Joel Stanley wrote:
> > On Wed, Jun 7, 2017 at 1:50 AM, Matthew Barth
> > > <msbarth@xxxxxxxxxxxxxxxxxx> 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 are many other examples with one writeable and multiple readable
attributes. Temperature offset attributes are an excellent example.
Next question would be what exactly would be writable. pwm attributes are
commonly completely independent of fan attributes. pwm1 output doesn't
mean that fan1 is the matching input; in fact, most of the time it isn't.
The only question would be numbering (is the pair numbered fan1/2 or
fan1/fan(1+X) ?) which is just a matter of personal preference. However,
everything is better than coming up with non-standard attributes which
can not be used with any standard application beyond the application of the
person submitting the driver. It is bad enough if a non-standard attribute
describes something really driver specific. But a non-standard attribute
for a fan speed reading ? Please no. We don't use outX_output instead of
inX_input for voltage outputs either.

Guenter

> 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