[PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
Add a basic driver for the MAX31785, focusing on the fan control features but ignoring the temperature and voltage monitoring features of the device. This driver supports all fan control modes and tachometer / PWM readback where applicable. Signed-off-by: Timothy Pearson Signed-off-by: Andrew Jeffery --- Hello, This is a rework of Timothy Pearson's original patch: https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html I've labelled it as v3 to differentiate from Timothy's postings. The original thread had some discussion about the MAX31785 being a PMBus device and that it should thus be a PMBus driver. The implementation still makes use of features not available in the pmbus core, so I've taken up the earlier suggestion and ported it to the devm_hwmon_device_register_with_info() interface. This gave a modest reduction in lines-of-code and at least to me is more aesthetically pleasing. 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). This feature is not documented in the public datasheet[1]. [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf The need to read a 4-byte value drives the addition of a helper that is a cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions aren't a defined transaction type in the PMBus spec. This seemed more tasteful than hacking the PMBus core to support the quirks of a single device. Also changed from Timothy's original posting is I've massaged the locking a bit and removed what seemed to be a copy/paste bug around max31785_fan_set_pulses() setting the fan_command member. Tested on an IBM Witherspoon machine. Cheers, Andrew Documentation/hwmon/max31785 | 44 +++ drivers/hwmon/Kconfig| 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/max31785.c | 824 +++ 4 files changed, 879 insertions(+) create mode 100644 Documentation/hwmon/max31785 create mode 100644 drivers/hwmon/max31785.c diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785 new file mode 100644 index ..dd891c06401e --- /dev/null +++ b/Documentation/hwmon/max31785 @@ -0,0 +1,44 @@ +Kernel driver max31785 +== + +Supported chips: + * Maxim MAX31785 +Prefix: 'max31785' +Addresses scanned: 0x52 0x53 0x54 0x55 +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf + +Author: Timothy Pearson + + +Description +--- + +This driver implements support for the Maxim MAX31785 chip. + +The MAX31785 controls the speeds of up to six fans using six independent +PWM outputs. The desired fan speeds (or PWM duty cycles) are written +through the I2C interface. The outputs drive "4-wire" fans directly, +or can be used to modulate the fan's power terminals using an external +pass transistor. + +Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) +monitoring and control of fan RPM as well as detection of fan failure. + + +Sysfs entries +- + +fan[1-6]_input RO fan tachometer speed in RPM +fan[1-6]_fault RO fan experienced fault +fan[1-6]_pulses RW tachometer pulses per fan revolution +fan[1-6]_target RW desired fan speed in RPM +pwm[1-6]_enable RW pwm mode, 0=disabled, 1=pwm, 2=rpm, 3=automatic +pwm[1-6] RW fan target duty cycle (0-255) + +Dynamic sysfs entries + + +Whether these entries are present depends on the firmware features detected on +the device during probe. + +fan[1-6]_input_fast RO fan tachometer speed in RPM (fast rotor measurement) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index e80ca81577f4..c75d6072c823 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -886,6 +886,16 @@ config SENSORS_MAX6697 This driver can also be built as a module. If so, the module will be called max6697. +config SENSORS_MAX31785 + tristate "Maxim MAX31785 sensor chip" + depends on I2C + help + If you say yes here you get support for 6-Channel PWM-Output + Fan RPM Controller. + + This driver can also be built as a module. If so, the module + will be called max31785. + config SENSORS_MAX31790 tristate "Maxim MAX31790 sensor chip" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index f03dd0a15933..dc55722bee88 100644 --- a/drivers/hwmon/Makefile +++ b/dr
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On 06/06/2017 12:02 AM, Andrew Jeffery wrote: Add a basic driver for the MAX31785, focusing on the fan control features but ignoring the temperature and voltage monitoring features of the device. This driver supports all fan control modes and tachometer / PWM readback where applicable. Signed-off-by: Timothy Pearson Signed-off-by: Andrew Jeffery --- Hello, This is a rework of Timothy Pearson's original patch: https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html I've labelled it as v3 to differentiate from Timothy's postings. The original thread had some discussion about the MAX31785 being a PMBus device and that it should thus be a PMBus driver. The implementation still makes use of features not available in the pmbus core, so I've taken up the earlier suggestion and ported it to the devm_hwmon_device_register_with_info() interface. This gave a modest reduction in lines-of-code and at least to me is more aesthetically pleasing. 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. This feature is not documented in the public datasheet[1]. [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf The need to read a 4-byte value drives the addition of a helper that is a cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions aren't a defined transaction type in the PMBus spec. This seemed more tasteful than hacking the PMBus core to support the quirks of a single device. That is why we have PMBus helper drivers. Guenter Also changed from Timothy's original posting is I've massaged the locking a bit and removed what seemed to be a copy/paste bug around max31785_fan_set_pulses() setting the fan_command member. Tested on an IBM Witherspoon machine. Cheers, Andrew Documentation/hwmon/max31785 | 44 +++ drivers/hwmon/Kconfig| 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/max31785.c | 824 +++ 4 files changed, 879 insertions(+) create mode 100644 Documentation/hwmon/max31785 create mode 100644 drivers/hwmon/max31785.c diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785 new file mode 100644 index ..dd891c06401e --- /dev/null +++ b/Documentation/hwmon/max31785 @@ -0,0 +1,44 @@ +Kernel driver max31785 +== + +Supported chips: + * Maxim MAX31785 +Prefix: 'max31785' +Addresses scanned: 0x52 0x53 0x54 0x55 +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf + +Author: Timothy Pearson + + +Description +--- + +This driver implements support for the Maxim MAX31785 chip. + +The MAX31785 controls the speeds of up to six fans using six independent +PWM outputs. The desired fan speeds (or PWM duty cycles) are written +through the I2C interface. The outputs drive "4-wire" fans directly, +or can be used to modulate the fan's power terminals using an external +pass transistor. + +Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) +monitoring and control of fan RPM as well as detection of fan failure. + + +Sysfs entries +- + +fan[1-6]_input RO fan tachometer speed in RPM +fan[1-6]_fault RO fan experienced fault +fan[1-6]_pulses RW tachometer pulses per fan revolution +fan[1-6]_target RW desired fan speed in RPM +pwm[1-6]_enable RW pwm mode, 0=disabled, 1=pwm, 2=rpm, 3=automatic +pwm[1-6] RW fan target duty cycle (0-255) + +Dynamic sysfs entries + + +Whether these entries are present depends on the firmware features detected on +the device during probe. + +fan[1-6]_input_fast RO fan tachometer speed in RPM (fast rotor measurement) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index e80ca81577f4..c75d6072c823 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -886,6 +886,16 @@ config SENSORS_MAX6697 This driver can also be built as a module. If so, the module will be called max6697. +config SENSORS_MAX31785 + tristate "Maxim MAX31785 sensor chip
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On 06/06/17 8:33 AM, Guenter Roeck wrote: On 06/06/2017 12:02 AM, Andrew Jeffery wrote: Add a basic driver for the MAX31785, focusing on the fan control features but ignoring the temperature and voltage monitoring features of the device. This driver supports all fan control modes and tachometer / PWM readback where applicable. Signed-off-by: Timothy Pearson Signed-off-by: Andrew Jeffery --- Hello, This is a rework of Timothy Pearson's original patch: https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html I've labelled it as v3 to differentiate from Timothy's postings. The original thread had some discussion about the MAX31785 being a PMBus device and that it should thus be a PMBus driver. The implementation still makes use of features not available in the pmbus core, so I've taken up the earlier suggestion and ported it to the devm_hwmon_device_register_with_info() interface. This gave a modest reduction in lines-of-code and at least to me is more aesthetically pleasing. 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. This feature is not documented in the public datasheet[1]. [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf The need to read a 4-byte value drives the addition of a helper that is a cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions aren't a defined transaction type in the PMBus spec. This seemed more tasteful than hacking the PMBus core to support the quirks of a single device. That is why we have PMBus helper drivers. Guenter Also changed from Timothy's original posting is I've massaged the locking a bit and removed what seemed to be a copy/paste bug around max31785_fan_set_pulses() setting the fan_command member. Tested on an IBM Witherspoon machine. Cheers, Andrew Documentation/hwmon/max31785 | 44 +++ drivers/hwmon/Kconfig| 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/max31785.c | 824 +++ 4 files changed, 879 insertions(+) create mode 100644 Documentation/hwmon/max31785 create mode 100644 drivers/hwmon/max31785.c diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785 new file mode 100644 index ..dd891c06401e --- /dev/null +++ b/Documentation/hwmon/max31785 @@ -0,0 +1,44 @@ +Kernel driver max31785 +== + +Supported chips: + * Maxim MAX31785 +Prefix: 'max31785' +Addresses scanned: 0x52 0x53 0x54 0x55 +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf + +Author: Timothy Pearson + + +Description +--- + +This driver implements support for the Maxim MAX31785 chip. + +The MAX31785 controls the speeds of up to six fans using six independent +PWM outputs. The desired fan speeds (or PWM duty cycles) are written +through the I2C interface. The outputs drive "4-wire" fans directly, +or can be used to modulate the fan's power terminals using an external +pass transistor. + +Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) +monitoring and control of fan RPM as well as detection of fan failure. + + +Sysfs entries +- + +fan[1-6]_input RO fan tachometer speed in RPM +fan[1-6]_fault RO fan experienced fault +fan[1-6]_pulses RW tachometer pulses per fan revolution +fan[1-6]_target RW desired fan speed in RPM +pwm[1-6]_enable RW
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
Hi Andrew, [auto build test ERROR on hwmon/hwmon-next] [also build test ERROR on v4.12-rc4 next-20170606] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Andrew-Jeffery/hwmon-Add-support-for-MAX31785-intelligent-fan-controller/20170607-020015 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 6.2.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=ia64 All error/warnings (new ones prefixed by >>): In file included from include/linux/kobject.h:21:0, from include/linux/device.h:17, from include/linux/hwmon-sysfs.h:23, from drivers/hwmon/max31785.c:20: >> drivers/hwmon/max31785.c:727:50: error: initialization from incompatible >> pointer type [-Werror=incompatible-pointer-types] static SENSOR_DEVICE_ATTR(fan1_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/linux/hwmon-sysfs.h:38:4: note: in expansion of macro 'SENSOR_ATTR' = SENSOR_ATTR(_name, _mode, _show, _store, _index) ^~~ >> drivers/hwmon/max31785.c:727:8: note: in expansion of macro >> 'SENSOR_DEVICE_ATTR' static SENSOR_DEVICE_ATTR(fan1_input_fast, 0444, max31785_fan_get_fast, ^~ drivers/hwmon/max31785.c:727:50: note: (near initialization for 'sensor_dev_attr_fan1_input_fast.dev_attr.show') static SENSOR_DEVICE_ATTR(fan1_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/linux/hwmon-sysfs.h:38:4: note: in expansion of macro 'SENSOR_ATTR' = SENSOR_ATTR(_name, _mode, _show, _store, _index) ^~~ >> drivers/hwmon/max31785.c:727:8: note: in expansion of macro >> 'SENSOR_DEVICE_ATTR' static SENSOR_DEVICE_ATTR(fan1_input_fast, 0444, max31785_fan_get_fast, ^~ drivers/hwmon/max31785.c:729:50: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] static SENSOR_DEVICE_ATTR(fan2_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/linux/hwmon-sysfs.h:38:4: note: in expansion of macro 'SENSOR_ATTR' = SENSOR_ATTR(_name, _mode, _show, _store, _index) ^~~ drivers/hwmon/max31785.c:729:8: note: in expansion of macro 'SENSOR_DEVICE_ATTR' static SENSOR_DEVICE_ATTR(fan2_input_fast, 0444, max31785_fan_get_fast, ^~ drivers/hwmon/max31785.c:729:50: note: (near initialization for 'sensor_dev_attr_fan2_input_fast.dev_attr.show') static SENSOR_DEVICE_ATTR(fan2_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/linux/hwmon-sysfs.h:38:4: note: in expansion of macro 'SENSOR_ATTR' = SENSOR_ATTR(_name, _mode, _show, _store, _index) ^~~ drivers/hwmon/max31785.c:729:8: note: in expansion of macro 'SENSOR_DEVICE_ATTR' static SENSOR_DEVICE_ATTR(fan2_input_fast, 0444, max31785_fan_get_fast, ^~ drivers/hwmon/max31785.c:731:50: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] static SENSOR_DEVICE_ATTR(fan3_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/linux/hwmon-sysfs.h:38:4: note: in expansion of macro 'SENSOR_ATTR' = SENSOR_ATTR(_name, _mode, _show, _store, _index) ^~~ drivers/hwmon/max31785.c:731:8: note: in expansion of macro 'SENSOR_DEVICE_ATTR' static SENSOR_DEVICE_ATTR(fan3_input_fast, 0444, max31785_fan_get_fast, ^~ drivers/hwmon/max31785.c:731:50: note: (near initialization for 'sensor_dev_attr_fan3_input_fast.dev_attr.show') static SENSOR_DEVICE_ATTR(fan3_input_fast, 0444, max31785_fan_get_fast, ^ include/linux/sysfs.h:103:10: note: in definition of macro '__ATTR' .show = _show, \ ^ >> include/lin
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Wed, Jun 7, 2017 at 1:50 AM, Matthew Barth 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. Guenter, would this be acceptable to you? Cheers, Joel > >> >>> This feature is not documented in the public datasheet[1]. >>> >>> [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf >>> >>> The need to read a 4-byte value drives the addition of a helper that is a >>> cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions >>> aren't a >>> defined transaction type in the PMBus spec. This seemed more tasteful >>> than >>> hacking the PMBus core to support the quirks of a single device. >>> >> >> That is why we have PMBus helper drivers. >> >> Guenter >> >>> Also changed from Timothy's original posting is I've massaged the locking >>> a bit >>> and removed what seemed to be a copy/paste bug around >>> max31785_fan_set_pulses() >>> setting the fan_command member. >>> >>> Tested on an IBM Witherspoon machine. >>> >>> Cheers, >>> >>> Andrew >>> >>> Documentation/hwmon/max31785 | 44 +++ >>> drivers/hwmon/Kconfig| 10 + >>> drivers/hwmon/Makefile | 1 + >>> drivers/hwmon/max31785.c | 824 >>> +++ >>> 4 files changed, 879 insertions(+) >>> create mode 100644 Documentation/hwmon/max31785 >>> create mode 100644 drivers/hwmon/max31785.c >>> >>> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785 >>> new file mode 100644 >>> index ..dd891c06401e >>> --- /dev/null >>> +++ b/Documentation/hwmon/max31785 >>> @@ -0,0 +1,44 @@ >>> +Kernel driver max31785 >>> +== >>> + >>> +Supported chips: >>> + * Maxim MAX31785 >>> +Prefix: 'max31785' >>> +Addresses scanned: 0x52 0x53 0x54 0x55 >>> +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf >>> + >>> +Author: Timothy Pearson >>> + >>> + >>> +Description >>> +--- >>> + >>> +This driver implements support for the Maxim MAX31785 chip. >>> + >>> +The MAX31785 controls the speeds of up to six fans using six independent >>> +PWM outputs. The desired fan speeds (or PWM duty cycles) are written >>> +through the I2C interface. The outputs drive "4-wire" fans directly, >>> +or can be used to modulate the fan's power terminals using an external >>> +pass transistor. >>> + >>> +Tachometer inputs monitor fan tachometer logic outputs for precise >>> (+/-1%) >>> +monitoring and control of fan RPM as well as detection of fan failure. >>> + >>> + >>> +Sysfs entries >>> +- >>> + >>> +fan[1-6]_input RO fan tachometer speed in RPM >>> +fan[1-6]_fault RO fan experienced fault >>> +fan[1-6]_pulses RW tachometer pulses per fan revolution >>> +fan[1-6]_target RW desired fan speed in RPM >>> +pwm[1-6]_enable RW pwm mode, 0=disabled, 1=pwm, 2=rpm, >>> 3=automatic >>> +pwm[1-6] RW fan target duty cycle (0-255) >>> + >>> +Dynamic sysfs entries >>> + >>> + >>> +Whether these entries are present depends on the firmware features >>> detected on >>> +
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Wed, 2017-06-07 at 12:18 +0930, Joel Stanley wrote: > On Wed, Jun 7, 2017 at 1:50 AM, Matthew Barth > > 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Tue, 2017-06-06 at 06:33 -0700, Guenter Roeck wrote: > On 06/06/2017 12:02 AM, Andrew Jeffery wrote: > > Add a basic driver for the MAX31785, focusing on the fan control > > features but ignoring the temperature and voltage monitoring > > features of the device. > > > > This driver supports all fan control modes and tachometer / PWM > > readback where applicable. > > > > > > Signed-off-by: Timothy Pearson > > > > Signed-off-by: Andrew Jeffery > > --- > > Hello, > > > > This is a rework of Timothy Pearson's original patch: > > > > https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html > > > > I've labelled it as v3 to differentiate from Timothy's postings. > > > > The original thread had some discussion about the MAX31785 being a PMBus > > device > > and that it should thus be a PMBus driver. The implementation still makes > > use > > of features not available in the pmbus core, so I've taken up the earlier > > suggestion and ported it to the devm_hwmon_device_register_with_info() > > interface. This gave a modest reduction in lines-of-code and at least to me > > is > > more aesthetically pleasing. > > > > 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. At the least I'll update the documentation in line with Matt Barth's comment, as it was vague. Whether or not we keep the questionable attribute I hope will be resolved down further in the thread. > > > This feature is not documented in the public datasheet[1]. > > > > [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf > > > > The need to read a 4-byte value drives the addition of a helper that is a > > cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions > > aren't a > > defined transaction type in the PMBus spec. This seemed more tasteful than > > hacking the PMBus core to support the quirks of a single device. > > > > That is why we have PMBus helper drivers. Right - I meant to say SMBus in the last sentence, not PMBus, as it was with respect to hacking i2c_smbus_xfer_emulated(). So I understand that the device-specific PMBus drivers exist to help with quirks, just that it's not (yet) a PMBus driver. Thanks for the feedback, Andrew > > Guenter > > > Also changed from Timothy's original posting is I've massaged the locking a > > bit > > and removed what seemed to be a copy/paste bug around > > max31785_fan_set_pulses() > > setting the fan_command member. > > > > Tested on an IBM Witherspoon machine. > > > > Cheers, > > > > Andrew > > > > Documentation/hwmon/max31785 | 44 +++ > > drivers/hwmon/Kconfig| 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/max31785.c | 824 > > +++ > > 4 files changed, 879 insertions(+) > > create mode 100644 Documentation/hwmon/max31785 > > create mode 100644 drivers/hwmon/max31785.c > > > > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785 > > new file mode 100644 > > index ..dd891c06401e > > --- /dev/null > > +++ b/Documentation/hwmon/max31785 > > @@ -0,0 +1,44 @@ > > +Kernel driver max31785 > > +== > > + > > +Supported chips: > > + * Maxim MAX31785 > > +Prefix: 'max31785' > > +Addresses scanned: 0x52 0x53 0x54 0x55 > > +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf > > + > > > > +Author: Timothy Pearson > > + > > + > > +Description > > +--- > > + > > +This driver implements support for the Maxim MAX31785 chip. > > + > > +The MAX31785 controls the speeds of up to six fans using six independent > > +PWM outputs. The desired fan speeds (or PWM duty cycles) are written > > +through the I2C interface. The outputs drive "4-wire" fans directly, > > +or can be used to modulate the fan's power terminals using an external > > +pass transistor. > > + > > +Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) > > +monitoring and control of fan RPM as well as detection of fan failure. > > + > > + > > +Sysfs entries > > +
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Tue, Jun 06, 2017 at 04:32:30PM +0930, Andrew Jeffery wrote: > Add a basic driver for the MAX31785, focusing on the fan control > features but ignoring the temperature and voltage monitoring > features of the device. > > This driver supports all fan control modes and tachometer / PWM > readback where applicable. > > Signed-off-by: Timothy Pearson > Signed-off-by: Andrew Jeffery > --- > Hello, > > This is a rework of Timothy Pearson's original patch: > > https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html > > I've labelled it as v3 to differentiate from Timothy's postings. > > The original thread had some discussion about the MAX31785 being a PMBus > device > and that it should thus be a PMBus driver. The implementation still makes use After thinking about it, that is what it should be. If I accept it as non-PMBus driver, it will be all but impossible to convert it to a PMBus driver later on, and that just doesn't make any sense. With no one interested in writing that driver, I'll try to give it some more priority myself. I do have an evaluation board somewhere, which should help. Note that the second fan reading should be implemented as just that, not with a non-standard attribute. Guenter
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
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 > > > 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
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
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 > > > > > > > > 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
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Wed, 2017-06-07 at 08:55 -0700, Guenter Roeck wrote: > On Tue, Jun 06, 2017 at 04:32:30PM +0930, Andrew Jeffery wrote: > > Add a basic driver for the MAX31785, focusing on the fan control > > features but ignoring the temperature and voltage monitoring > > features of the device. > > > > This driver supports all fan control modes and tachometer / PWM > > readback where applicable. > > > > > > Signed-off-by: Timothy Pearson > > > > Signed-off-by: Andrew Jeffery > > --- > > Hello, > > > > This is a rework of Timothy Pearson's original patch: > > > > https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html > > > > I've labelled it as v3 to differentiate from Timothy's postings. > > > > The original thread had some discussion about the MAX31785 being a PMBus > > device > > and that it should thus be a PMBus driver. The implementation still makes > > use > > After thinking about it, that is what it should be. If I accept it as > non-PMBus > driver, it will be all but impossible to convert it to a PMBus driver later > on, > and that just doesn't make any sense. Hopefully not being too ignorant here, but can you expand on why it would be all but impossible to convert? > > With no one interested in writing that driver, I'll try to give it some more > priority myself. I do have an evaluation board somewhere, which should help. > > Note that the second fan reading should be implemented as just that, not with > a non-standard attribute. Agreed. Andrew signature.asc Description: This is a digitally signed message part
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On 06/08/2017 12:53 AM, Andrew Jeffery wrote: On Wed, 2017-06-07 at 08:55 -0700, Guenter Roeck wrote: On Tue, Jun 06, 2017 at 04:32:30PM +0930, Andrew Jeffery wrote: Add a basic driver for the MAX31785, focusing on the fan control features but ignoring the temperature and voltage monitoring features of the device. This driver supports all fan control modes and tachometer / PWM readback where applicable. Signed-off-by: Timothy Pearson Signed-off-by: Andrew Jeffery --- Hello, This is a rework of Timothy Pearson's original patch: https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html I've labelled it as v3 to differentiate from Timothy's postings. The original thread had some discussion about the MAX31785 being a PMBus device and that it should thus be a PMBus driver. The implementation still makes use After thinking about it, that is what it should be. If I accept it as non-PMBus driver, it will be all but impossible to convert it to a PMBus driver later on, and that just doesn't make any sense. Hopefully not being too ignorant here, but can you expand on why it would be all but impossible to convert? I've got a lot of noise recently just for converting a driver from the old to the new API (which changes the attribute location). Changing the driver from non-PMBus to PMBus would very quite likely change some attributes as well. Besides that, I think it is a bad idea to bypass an infrastructure just because it may require a few tweaks. That generates a bad precedent, and people _would_ use that to argue that the next PMBus chip driver should not use the infrastructure either. Guenter With no one interested in writing that driver, I'll try to give it some more priority myself. I do have an evaluation board somewhere, which should help. Note that the second fan reading should be implemented as just that, not with a non-standard attribute. Agreed. Andrew
Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller
On Thu, 2017-06-08 at 05:37 -0700, Guenter Roeck wrote: > On 06/08/2017 12:53 AM, Andrew Jeffery wrote: > > On Wed, 2017-06-07 at 08:55 -0700, Guenter Roeck wrote: > > > On Tue, Jun 06, 2017 at 04:32:30PM +0930, Andrew Jeffery wrote: > > > > Add a basic driver for the MAX31785, focusing on the fan control > > > > features but ignoring the temperature and voltage monitoring > > > > features of the device. > > > > > > > > This driver supports all fan control modes and tachometer / PWM > > > > readback where applicable. > > > > > > > > > > Signed-off-by: Timothy Pearson > > > > > > Signed-off-by: Andrew Jeffery > > > > > > > > --- > > > > Hello, > > > > > > > > This is a rework of Timothy Pearson's original patch: > > > > > > > > > > > > https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html > > > > > > > > I've labelled it as v3 to differentiate from Timothy's postings. > > > > > > > > The original thread had some discussion about the MAX31785 being a > > > > PMBus device > > > > and that it should thus be a PMBus driver. The implementation still > > > > makes use > > > > > > After thinking about it, that is what it should be. If I accept it as > > > non-PMBus > > > driver, it will be all but impossible to convert it to a PMBus driver > > > later on, > > > and that just doesn't make any sense. > > > > Hopefully not being too ignorant here, but can you expand on why it > > would be all but impossible to convert? > > > > I've got a lot of noise recently just for converting a driver from the old to > the > new API (which changes the attribute location). Changing the driver from > non-PMBus > to PMBus would very quite likely change some attributes as well. Okay. > > Besides that, I think it is a bad idea to bypass an infrastructure just > because > it may require a few tweaks. That generates a bad precedent, and people > _would_ > use that to argue that the next PMBus chip driver should not use the > infrastructure > either. I understand not wanting to set a precedent. Thanks for your response. Andrew > > Guenter > > > > > > > With no one interested in writing that driver, I'll try to give it some > > > more > > > priority myself. I do have an evaluation board somewhere, which should > > > help. > > > > > > Note that the second fan reading should be implemented as just that, not > > > with > > > a non-standard attribute. > > > > Agreed. > > > > Andrew > > > > signature.asc Description: This is a digitally signed message part