On Wed, 2017-06-07 at 10:37 -0700, Guenter Roeck wrote: > 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 > > > > > > > > <msba...@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 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.
Yes, I've received loud and clear that I made the wrong choice :) Apologies. Thanks again for your feedback. Andrew
signature.asc
Description: This is a digitally signed message part
