| Message ID | 20210526154022.3223012-4-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (max31790) Fixes and improvements | expand |
pwmX_enable 0 and 1 work fine, but it seems like RPM mode is currently still unusable. I am testing with Sunon PF36281BX-000U-S99. Setting almost any kind of RPM, from low (about 2500) and high (maximum is 23000), the RPM never stabilizes. I think this is mainly because the driver currently doesn't work with the "window" and "pwm rate of change" registers. Tested-by: Václav Kubernát <kubernat@cesnet.cz> st 26. 5. 2021 v 17:40 odesílatel Guenter Roeck <linux@roeck-us.net> napsal: > > pwmX_enable supports three possible values: > > 0: Fan control disabled. Duty cycle is fixed to 0% > 1: Fan control enabled, pwm mode. Duty cycle is determined by > values written into Target Duty Cycle registers. > 2: Fan control enabled, rpm mode > Duty cycle is adjusted such that fan speed matches > the values in Target Count registers > > The current code does not do this; instead, it mixes pwm control > configuration with fan speed monitoring configuration. Worse, it > reports that pwm control would be disabled (pwmX_enable==0) when > it is in fact enabled in pwm mode. Part of the problem may be that > the chip sets the "TACH input enable" bit on its own whenever the > mode bit is set to RPM mode, but that doesn't mean that "TACH input > enable" accurately reflects the pwm mode. > > Fix it up and only handle pwm control with the pwmX_enable attributes. > In the documentation, clarify that disabling pwm control (pwmX_enable=0) > sets the pwm duty cycle to 0%. In the code, explain why TACH_INPUT_EN > is set together with RPM_MODE. > > While at it, only update the configuration register if the configuration > has changed, and only update the cached configuration if updating the > chip configuration was successful. > > Cc: Jan Kundrát <jan.kundrat@cesnet.cz> > Cc: Václav Kubernát <kubernat@cesnet.cz> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Documentation/hwmon/max31790.rst | 2 +- > drivers/hwmon/max31790.c | 41 ++++++++++++++++++++------------ > 2 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > index 54ff0f49e28f..7b097c3b9b90 100644 > --- a/Documentation/hwmon/max31790.rst > +++ b/Documentation/hwmon/max31790.rst > @@ -38,7 +38,7 @@ Sysfs entries > fan[1-12]_input RO fan tachometer speed in RPM > fan[1-12]_fault RO fan experienced fault > fan[1-6]_target RW desired fan speed in RPM > -pwm[1-6]_enable RW regulator mode, 0=disabled, 1=manual mode, 2=rpm mode > +pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > pwm[1-6] RW read: current pwm duty cycle, > write: target pwm duty cycle (0-255) > ================== === ======================================================= > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > index 693497e09ac0..67677c437768 100644 > --- a/drivers/hwmon/max31790.c > +++ b/drivers/hwmon/max31790.c > @@ -27,6 +27,7 @@ > > /* Fan Config register bits */ > #define MAX31790_FAN_CFG_RPM_MODE 0x80 > +#define MAX31790_FAN_CFG_CTRL_MON 0x10 > #define MAX31790_FAN_CFG_TACH_INPUT_EN 0x08 > #define MAX31790_FAN_CFG_TACH_INPUT 0x01 > > @@ -271,12 +272,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel, > *val = data->pwm[channel] >> 8; > return 0; > case hwmon_pwm_enable: > - if (fan_config & MAX31790_FAN_CFG_RPM_MODE) > + if (fan_config & MAX31790_FAN_CFG_CTRL_MON) > + *val = 0; > + else if (fan_config & MAX31790_FAN_CFG_RPM_MODE) > *val = 2; > - else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN) > - *val = 1; > else > - *val = 0; > + *val = 1; > return 0; > default: > return -EOPNOTSUPP; > @@ -307,23 +308,33 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel, > case hwmon_pwm_enable: > fan_config = data->fan_config[channel]; > if (val == 0) { > - fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN | > - MAX31790_FAN_CFG_RPM_MODE); > + fan_config |= MAX31790_FAN_CFG_CTRL_MON; > + /* > + * Disable RPM mode; otherwise disabling fan speed > + * monitoring is not possible. > + */ > + fan_config &= ~MAX31790_FAN_CFG_RPM_MODE; > } else if (val == 1) { > - fan_config = (fan_config | > - MAX31790_FAN_CFG_TACH_INPUT_EN) & > - ~MAX31790_FAN_CFG_RPM_MODE; > + fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON | MAX31790_FAN_CFG_RPM_MODE); > } else if (val == 2) { > - fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN | > - MAX31790_FAN_CFG_RPM_MODE; > + fan_config &= ~MAX31790_FAN_CFG_CTRL_MON; > + /* > + * The chip sets MAX31790_FAN_CFG_TACH_INPUT_EN on its > + * own if MAX31790_FAN_CFG_RPM_MODE is set. > + * Do it here as well to reflect the actual register > + * value in the cache. > + */ > + fan_config |= (MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_TACH_INPUT_EN); > } else { > err = -EINVAL; > break; > } > - data->fan_config[channel] = fan_config; > - err = i2c_smbus_write_byte_data(client, > - MAX31790_REG_FAN_CONFIG(channel), > - fan_config); > + if (fan_config != data->fan_config[channel]) { > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > + fan_config); > + if (!err) > + data->fan_config[channel] = fan_config; > + } > break; > default: > err = -EOPNOTSUPP; > -- > 2.25.1 >
On Tue, Jun 01, 2021 at 10:01:20AM +0200, Václav Kubernát wrote: > pwmX_enable 0 and 1 work fine, but it seems like RPM mode is currently > still unusable. I am testing with Sunon PF36281BX-000U-S99. Setting > almost any kind of RPM, from low (about 2500) and high (maximum is > 23000), the RPM never stabilizes. I think this is mainly because the > driver currently doesn't work with the "window" and "pwm rate of > change" registers. > Correct. I made the same observation. Please feel free to suggest attributes or some other solution to address the above problems. Thanks, Guenter > Tested-by: Václav Kubernát <kubernat@cesnet.cz> > > st 26. 5. 2021 v 17:40 odesílatel Guenter Roeck <linux@roeck-us.net> napsal: > > > > pwmX_enable supports three possible values: > > > > 0: Fan control disabled. Duty cycle is fixed to 0% > > 1: Fan control enabled, pwm mode. Duty cycle is determined by > > values written into Target Duty Cycle registers. > > 2: Fan control enabled, rpm mode > > Duty cycle is adjusted such that fan speed matches > > the values in Target Count registers > > > > The current code does not do this; instead, it mixes pwm control > > configuration with fan speed monitoring configuration. Worse, it > > reports that pwm control would be disabled (pwmX_enable==0) when > > it is in fact enabled in pwm mode. Part of the problem may be that > > the chip sets the "TACH input enable" bit on its own whenever the > > mode bit is set to RPM mode, but that doesn't mean that "TACH input > > enable" accurately reflects the pwm mode. > > > > Fix it up and only handle pwm control with the pwmX_enable attributes. > > In the documentation, clarify that disabling pwm control (pwmX_enable=0) > > sets the pwm duty cycle to 0%. In the code, explain why TACH_INPUT_EN > > is set together with RPM_MODE. > > > > While at it, only update the configuration register if the configuration > > has changed, and only update the cached configuration if updating the > > chip configuration was successful. > > > > Cc: Jan Kundrát <jan.kundrat@cesnet.cz> > > Cc: Václav Kubernát <kubernat@cesnet.cz> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > Documentation/hwmon/max31790.rst | 2 +- > > drivers/hwmon/max31790.c | 41 ++++++++++++++++++++------------ > > 2 files changed, 27 insertions(+), 16 deletions(-) > > > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > > index 54ff0f49e28f..7b097c3b9b90 100644 > > --- a/Documentation/hwmon/max31790.rst > > +++ b/Documentation/hwmon/max31790.rst > > @@ -38,7 +38,7 @@ Sysfs entries > > fan[1-12]_input RO fan tachometer speed in RPM > > fan[1-12]_fault RO fan experienced fault > > fan[1-6]_target RW desired fan speed in RPM > > -pwm[1-6]_enable RW regulator mode, 0=disabled, 1=manual mode, 2=rpm mode > > +pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > > pwm[1-6] RW read: current pwm duty cycle, > > write: target pwm duty cycle (0-255) > > ================== === ======================================================= > > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > > index 693497e09ac0..67677c437768 100644 > > --- a/drivers/hwmon/max31790.c > > +++ b/drivers/hwmon/max31790.c > > @@ -27,6 +27,7 @@ > > > > /* Fan Config register bits */ > > #define MAX31790_FAN_CFG_RPM_MODE 0x80 > > +#define MAX31790_FAN_CFG_CTRL_MON 0x10 > > #define MAX31790_FAN_CFG_TACH_INPUT_EN 0x08 > > #define MAX31790_FAN_CFG_TACH_INPUT 0x01 > > > > @@ -271,12 +272,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel, > > *val = data->pwm[channel] >> 8; > > return 0; > > case hwmon_pwm_enable: > > - if (fan_config & MAX31790_FAN_CFG_RPM_MODE) > > + if (fan_config & MAX31790_FAN_CFG_CTRL_MON) > > + *val = 0; > > + else if (fan_config & MAX31790_FAN_CFG_RPM_MODE) > > *val = 2; > > - else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN) > > - *val = 1; > > else > > - *val = 0; > > + *val = 1; > > return 0; > > default: > > return -EOPNOTSUPP; > > @@ -307,23 +308,33 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel, > > case hwmon_pwm_enable: > > fan_config = data->fan_config[channel]; > > if (val == 0) { > > - fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN | > > - MAX31790_FAN_CFG_RPM_MODE); > > + fan_config |= MAX31790_FAN_CFG_CTRL_MON; > > + /* > > + * Disable RPM mode; otherwise disabling fan speed > > + * monitoring is not possible. > > + */ > > + fan_config &= ~MAX31790_FAN_CFG_RPM_MODE; > > } else if (val == 1) { > > - fan_config = (fan_config | > > - MAX31790_FAN_CFG_TACH_INPUT_EN) & > > - ~MAX31790_FAN_CFG_RPM_MODE; > > + fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON | MAX31790_FAN_CFG_RPM_MODE); > > } else if (val == 2) { > > - fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN | > > - MAX31790_FAN_CFG_RPM_MODE; > > + fan_config &= ~MAX31790_FAN_CFG_CTRL_MON; > > + /* > > + * The chip sets MAX31790_FAN_CFG_TACH_INPUT_EN on its > > + * own if MAX31790_FAN_CFG_RPM_MODE is set. > > + * Do it here as well to reflect the actual register > > + * value in the cache. > > + */ > > + fan_config |= (MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_TACH_INPUT_EN); > > } else { > > err = -EINVAL; > > break; > > } > > - data->fan_config[channel] = fan_config; > > - err = i2c_smbus_write_byte_data(client, > > - MAX31790_REG_FAN_CONFIG(channel), > > - fan_config); > > + if (fan_config != data->fan_config[channel]) { > > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > > + fan_config); > > + if (!err) > > + data->fan_config[channel] = fan_config; > > + } > > break; > > default: > > err = -EOPNOTSUPP; > > -- > > 2.25.1 > >
On středa 26. května 2021 17:40:18 CEST, Guenter Roeck wrote: > pwmX_enable supports three possible values: > > 0: Fan control disabled. Duty cycle is fixed to 0% > 1: Fan control enabled, pwm mode. Duty cycle is determined by > values written into Target Duty Cycle registers. > 2: Fan control enabled, rpm mode > Duty cycle is adjusted such that fan speed matches > the values in Target Count registers > > The current code does not do this; instead, it mixes pwm control > configuration with fan speed monitoring configuration. Worse, it > reports that pwm control would be disabled (pwmX_enable==0) when > it is in fact enabled in pwm mode. Part of the problem may be that > the chip sets the "TACH input enable" bit on its own whenever the > mode bit is set to RPM mode, but that doesn't mean that "TACH input > enable" accurately reflects the pwm mode. > > Fix it up and only handle pwm control with the pwmX_enable attributes. > In the documentation, clarify that disabling pwm control (pwmX_enable=0) > sets the pwm duty cycle to 0%. In the code, explain why TACH_INPUT_EN > is set together with RPM_MODE. > > While at it, only update the configuration register if the configuration > has changed, and only update the cached configuration if updating the > chip configuration was successful. > > Cc: Jan Kundrát <jan.kundrat@cesnet.cz> > Cc: Václav Kubernát <kubernat@cesnet.cz> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz> Cheers, Jan
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst index 54ff0f49e28f..7b097c3b9b90 100644 --- a/Documentation/hwmon/max31790.rst +++ b/Documentation/hwmon/max31790.rst @@ -38,7 +38,7 @@ Sysfs entries fan[1-12]_input RO fan tachometer speed in RPM fan[1-12]_fault RO fan experienced fault fan[1-6]_target RW desired fan speed in RPM -pwm[1-6]_enable RW regulator mode, 0=disabled, 1=manual mode, 2=rpm mode +pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode pwm[1-6] RW read: current pwm duty cycle, write: target pwm duty cycle (0-255) ================== === ======================================================= diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c index 693497e09ac0..67677c437768 100644 --- a/drivers/hwmon/max31790.c +++ b/drivers/hwmon/max31790.c @@ -27,6 +27,7 @@ /* Fan Config register bits */ #define MAX31790_FAN_CFG_RPM_MODE 0x80 +#define MAX31790_FAN_CFG_CTRL_MON 0x10 #define MAX31790_FAN_CFG_TACH_INPUT_EN 0x08 #define MAX31790_FAN_CFG_TACH_INPUT 0x01 @@ -271,12 +272,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel, *val = data->pwm[channel] >> 8; return 0; case hwmon_pwm_enable: - if (fan_config & MAX31790_FAN_CFG_RPM_MODE) + if (fan_config & MAX31790_FAN_CFG_CTRL_MON) + *val = 0; + else if (fan_config & MAX31790_FAN_CFG_RPM_MODE) *val = 2; - else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN) - *val = 1; else - *val = 0; + *val = 1; return 0; default: return -EOPNOTSUPP; @@ -307,23 +308,33 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel, case hwmon_pwm_enable: fan_config = data->fan_config[channel]; if (val == 0) { - fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN | - MAX31790_FAN_CFG_RPM_MODE); + fan_config |= MAX31790_FAN_CFG_CTRL_MON; + /* + * Disable RPM mode; otherwise disabling fan speed + * monitoring is not possible. + */ + fan_config &= ~MAX31790_FAN_CFG_RPM_MODE; } else if (val == 1) { - fan_config = (fan_config | - MAX31790_FAN_CFG_TACH_INPUT_EN) & - ~MAX31790_FAN_CFG_RPM_MODE; + fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON | MAX31790_FAN_CFG_RPM_MODE); } else if (val == 2) { - fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN | - MAX31790_FAN_CFG_RPM_MODE; + fan_config &= ~MAX31790_FAN_CFG_CTRL_MON; + /* + * The chip sets MAX31790_FAN_CFG_TACH_INPUT_EN on its + * own if MAX31790_FAN_CFG_RPM_MODE is set. + * Do it here as well to reflect the actual register + * value in the cache. + */ + fan_config |= (MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_TACH_INPUT_EN); } else { err = -EINVAL; break; } - data->fan_config[channel] = fan_config; - err = i2c_smbus_write_byte_data(client, - MAX31790_REG_FAN_CONFIG(channel), - fan_config); + if (fan_config != data->fan_config[channel]) { + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), + fan_config); + if (!err) + data->fan_config[channel] = fan_config; + } break; default: err = -EOPNOTSUPP;
pwmX_enable supports three possible values: 0: Fan control disabled. Duty cycle is fixed to 0% 1: Fan control enabled, pwm mode. Duty cycle is determined by values written into Target Duty Cycle registers. 2: Fan control enabled, rpm mode Duty cycle is adjusted such that fan speed matches the values in Target Count registers The current code does not do this; instead, it mixes pwm control configuration with fan speed monitoring configuration. Worse, it reports that pwm control would be disabled (pwmX_enable==0) when it is in fact enabled in pwm mode. Part of the problem may be that the chip sets the "TACH input enable" bit on its own whenever the mode bit is set to RPM mode, but that doesn't mean that "TACH input enable" accurately reflects the pwm mode. Fix it up and only handle pwm control with the pwmX_enable attributes. In the documentation, clarify that disabling pwm control (pwmX_enable=0) sets the pwm duty cycle to 0%. In the code, explain why TACH_INPUT_EN is set together with RPM_MODE. While at it, only update the configuration register if the configuration has changed, and only update the cached configuration if updating the chip configuration was successful. Cc: Jan Kundrát <jan.kundrat@cesnet.cz> Cc: Václav Kubernát <kubernat@cesnet.cz> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Documentation/hwmon/max31790.rst | 2 +- drivers/hwmon/max31790.c | 41 ++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 16 deletions(-)