* [PATCH 0/7] hwmon: (max31790) Fixes and improvements
@ 2021-05-26 15:40 Guenter Roeck
2021-05-26 15:40 ` [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12 Guenter Roeck
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
The following series provides various fixes and improvements for the
max31790 driver.
----------------------------------------------------------------
Guenter Roeck (7):
hwmon: (max31790) Fix fan speed reporting for fan7..12
hwmon: (max31790) Report correct current pwm duty cycles
hwmon: (max31790) Fix pwmX_enable attributes
hwmon: (max31790) Add support for fanX_enable attributes
hwmon: (max31790) Clear fan fault after reporting it
hwmon: (max31790) Detect and report zero fan speed
hwmon: (max31790) Add support for fanX_min attributes
Documentation/hwmon/max31790.rst | 17 +++-
drivers/hwmon/max31790.c | 171 ++++++++++++++++++++++++++++++---------
2 files changed, 147 insertions(+), 41 deletions(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-02 12:27 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles Guenter Roeck
` (6 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
Fans 7..12 do not have their own set of configuration registers.
So far the code ignored that and read beyond the end of the configuration
register range to get the tachometer period. This resulted in more or less
random fan speed values for those fans.
The datasheet is quite vague when it comes to defining the tachometer
period for fans 7..12. Experiments confirm that the period is the same
for both fans associated with a given set of configuration registers.
Fixes: 54187ff9d766 ("hwmon: (max31790) Convert to use new hwmon registration API")
Fixes: 195a4b4298a7 ("hwmon: Driver for Maxim MAX31790")
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>
---
drivers/hwmon/max31790.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 86e6c71db685..f6d4fc0a2f13 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -170,7 +170,7 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
switch (attr) {
case hwmon_fan_input:
- sr = get_tach_period(data->fan_dynamics[channel]);
+ sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]);
rpm = RPM_FROM_REG(data->tach[channel], sr);
*val = rpm;
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
2021-05-26 15:40 ` [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12 Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 7:51 ` Václav Kubernát
2021-06-02 12:36 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes Guenter Roeck
` (5 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
The MAX31790 has two sets of registers for pwm duty cycles, one to request
a duty cycle and one to read the actual current duty cycle. Both do not
have to be the same.
When reporting the pwm duty cycle to the user, the actual pwm duty cycle
from pwm duty cycle registers needs to be reported. When setting it, the
pwm target duty cycle needs to be written. Since we don't know the actual
pwm duty cycle after a target pwm duty cycle has been written, set the
valid flag to false to indicate that actual pwm duty cycle should be read
from the chip instead of using cached values.
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 | 3 ++-
drivers/hwmon/max31790.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index f301385d8cef..54ff0f49e28f 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -39,5 +39,6 @@ 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] RW fan target duty cycle (0-255)
+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 f6d4fc0a2f13..693497e09ac0 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -104,7 +104,7 @@ static struct max31790_data *max31790_update_device(struct device *dev)
data->tach[NR_CHANNEL + i] = rv;
} else {
rv = i2c_smbus_read_word_swapped(client,
- MAX31790_REG_PWMOUT(i));
+ MAX31790_REG_PWM_DUTY_CYCLE(i));
if (rv < 0)
goto abort;
data->pwm[i] = rv;
@@ -299,10 +299,10 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
err = -EINVAL;
break;
}
- data->pwm[channel] = val << 8;
+ data->valid = false;
err = i2c_smbus_write_word_swapped(client,
MAX31790_REG_PWMOUT(channel),
- data->pwm[channel]);
+ val << 8);
break;
case hwmon_pwm_enable:
fan_config = data->fan_config[channel];
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
2021-05-26 15:40 ` [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12 Guenter Roeck
2021-05-26 15:40 ` [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 8:01 ` Václav Kubernát
2021-06-02 12:44 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes Guenter Roeck
` (4 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
` (2 preceding siblings ...)
2021-05-26 15:40 ` [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 8:02 ` Václav Kubernát
2021-06-02 13:04 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it Guenter Roeck
` (3 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
Since pwmX_enable is now fixed and only handles pwm support instead
of also enabling/disabling fan tachometers, we need an explicit means
to do that.
For fan channels 7..12, display the enable status if the channel
is configured for fan speed reporting. The displayed status matches
the value of the companion channel but is read-only.
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 | 3 ++
drivers/hwmon/max31790.c | 55 ++++++++++++++++++++++++--------
2 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index 7b097c3b9b90..b43aa32813fd 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -35,6 +35,9 @@ Sysfs entries
-------------
================== === =======================================================
+fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monitoring
+ The value is RO for companion channels (7-12). For those
+ channels, the value matches the value of the primary channel.
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
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 67677c437768..19651feb40fb 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -170,6 +170,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
return PTR_ERR(data);
switch (attr) {
+ case hwmon_fan_enable:
+ *val = !!(data->fan_config[channel % NR_CHANNEL] & MAX31790_FAN_CFG_TACH_INPUT_EN);
+ return 0;
case hwmon_fan_input:
sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]);
rpm = RPM_FROM_REG(data->tach[channel], sr);
@@ -195,12 +198,32 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
struct i2c_client *client = data->client;
int target_count;
int err = 0;
- u8 bits;
+ u8 bits, config;
int sr;
mutex_lock(&data->update_lock);
switch (attr) {
+ case hwmon_fan_enable:
+ config = data->fan_config[channel];
+ if (val == 0) {
+ /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */
+ if (!(config & MAX31790_FAN_CFG_RPM_MODE))
+ config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
+ } else if (val == 1) {
+ config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
+ } else {
+ err = -EINVAL;
+ break;
+ }
+ if (config != data->fan_config[channel]) {
+ err = i2c_smbus_write_byte_data(client,
+ MAX31790_REG_FAN_CONFIG(channel),
+ config);
+ if (!err)
+ data->fan_config[channel] = config;
+ }
+ break;
case hwmon_fan_target:
val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
bits = bits_for_tach_period(val);
@@ -240,6 +263,12 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
u8 fan_config = data->fan_config[channel % NR_CHANNEL];
switch (attr) {
+ case hwmon_fan_enable:
+ if (channel < NR_CHANNEL)
+ return 0644;
+ if (fan_config & MAX31790_FAN_CFG_TACH_INPUT)
+ return 0444;
+ return 0;
case hwmon_fan_input:
case hwmon_fan_fault:
if (channel < NR_CHANNEL ||
@@ -404,18 +433,18 @@ static umode_t max31790_is_visible(const void *data,
static const struct hwmon_channel_info *max31790_info[] = {
HWMON_CHANNEL_INFO(fan,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_INPUT | HWMON_F_FAULT),
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
HWMON_CHANNEL_INFO(pwm,
HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
` (3 preceding siblings ...)
2021-05-26 15:40 ` [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 8:08 ` Václav Kubernát
2021-05-26 15:40 ` [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed Guenter Roeck
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
Fault bits in MAX31790 are sticky and have to be cleared explicitly.
A write operation into either the 'Target Duty Cycle' register or the
'Target Count' register is necessary to clear a fault.
At the same time, we can never clear cached fault status values before
reading them because the companion fault status for any given fan is
cleared as well when clearing a fault.
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>
---
drivers/hwmon/max31790.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 19651feb40fb..8e4fd9b7c889 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -80,7 +80,7 @@ static struct max31790_data *max31790_update_device(struct device *dev)
MAX31790_REG_FAN_FAULT_STATUS1);
if (rv < 0)
goto abort;
- data->fault_status = rv & 0x3F;
+ data->fault_status |= rv & 0x3F;
rv = i2c_smbus_read_byte_data(client,
MAX31790_REG_FAN_FAULT_STATUS2);
@@ -184,7 +184,21 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
*val = rpm;
return 0;
case hwmon_fan_fault:
+ mutex_lock(&data->update_lock);
*val = !!(data->fault_status & (1 << channel));
+ data->fault_status &= ~(1 << channel);
+ /*
+ * If a fault bit is set, we need to write into one of the fan
+ * configuration registers to clear it. Note that this also
+ * clears the fault for the companion channel if enabled.
+ */
+ if (*val) {
+ int reg = MAX31790_REG_TARGET_COUNT(channel % NR_CHANNEL);
+
+ i2c_smbus_write_byte_data(data->client, reg,
+ data->target_count[channel % NR_CHANNEL] >> 8);
+ }
+ mutex_unlock(&data->update_lock);
return 0;
default:
return -EOPNOTSUPP;
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
` (4 preceding siblings ...)
2021-05-26 15:40 ` [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 8:11 ` Václav Kubernát
2021-05-26 15:40 ` [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes Guenter Roeck
2021-06-01 8:29 ` [PATCH 0/7] hwmon: (max31790) Fixes and improvements Václav Kubernát
7 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
If a fan is not running or not connected, of if fan monitoring is disabled,
the fan count register returns a fixed value of 0xffe0. So far this is then
translated to a RPM value larger than 0. Since this is misleading and does
not really make much sense, report a fan RPM of 0 in this situation.
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>
---
drivers/hwmon/max31790.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 8e4fd9b7c889..91fe419b596c 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -40,6 +40,8 @@
#define FAN_RPM_MIN 120
#define FAN_RPM_MAX 7864320
+#define FAN_COUNT_REG_MAX 0xffe0
+
#define RPM_FROM_REG(reg, sr) (((reg) >> 4) ? \
((60 * (sr) * 8192) / ((reg) >> 4)) : \
FAN_RPM_MAX)
@@ -175,7 +177,10 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
return 0;
case hwmon_fan_input:
sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]);
- rpm = RPM_FROM_REG(data->tach[channel], sr);
+ if (data->tach[channel] == FAN_COUNT_REG_MAX)
+ rpm = 0;
+ else
+ rpm = RPM_FROM_REG(data->tach[channel], sr);
*val = rpm;
return 0;
case hwmon_fan_target:
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
` (5 preceding siblings ...)
2021-05-26 15:40 ` [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed Guenter Roeck
@ 2021-05-26 15:40 ` Guenter Roeck
2021-06-01 8:19 ` Václav Kubernát
2021-06-01 8:29 ` [PATCH 0/7] hwmon: (max31790) Fixes and improvements Václav Kubernát
7 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2021-05-26 15:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: Jean Delvare, jan.kundrat, kubernat, Guenter Roeck
The minimum fan speed is closely correlated to the target fan speed.
The correlation depends on the pwm mode. It is essential to be able
to read and to set it to be able to control when a channel will report
a fan fault.
Since the minimum fan speed also applies to channels 7..12, make it
visible but read-only for those channels as well.
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 | 9 ++++
drivers/hwmon/max31790.c | 70 ++++++++++++++++++++++++--------
2 files changed, 61 insertions(+), 18 deletions(-)
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index b43aa32813fd..9225c2a78b68 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -40,6 +40,15 @@ fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monito
channels, the value matches the value of the primary channel.
fan[1-12]_input RO fan tachometer speed in RPM
fan[1-12]_fault RO fan experienced fault
+fan[1-12]_min RW minimum fan speed in RPM. Equivalent to target fan speed
+ in PWM mode and if PWM support is disabled for a channel.
+ Equivalent to half the target fan speed in RPM mode.
+ The value is RO for companion channels (7-12). For those
+ channels, the value matches the value of the primary channel.
+ Note: In RPM mode, if the pwm duty cycle is 100%, the
+ minimum fan speed is equivalent to the target fan speed,
+ and the chip will report a fault condition if the fan
+ speed is below the target fan speed.
fan[1-6]_target RW desired fan speed in RPM
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,
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 91fe419b596c..db97ee99515a 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -111,13 +111,11 @@ static struct max31790_data *max31790_update_device(struct device *dev)
if (rv < 0)
goto abort;
data->pwm[i] = rv;
-
- rv = i2c_smbus_read_word_swapped(client,
- MAX31790_REG_TARGET_COUNT(i));
- if (rv < 0)
- goto abort;
- data->target_count[i] = rv;
}
+ rv = i2c_smbus_read_word_swapped(client, MAX31790_REG_TARGET_COUNT(i));
+ if (rv < 0)
+ goto abort;
+ data->target_count[i] = rv;
}
data->last_updated = jiffies;
@@ -183,6 +181,21 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
rpm = RPM_FROM_REG(data->tach[channel], sr);
*val = rpm;
return 0;
+ case hwmon_fan_min:
+ channel %= NR_CHANNEL;
+ sr = get_tach_period(data->fan_dynamics[channel]);
+ if (!(data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE) ||
+ (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT)) {
+ /* pwm mode: target == min */
+ rpm = RPM_FROM_REG(data->target_count[channel], sr);
+ } else {
+ /* rpm mode: min tach count is twice target count */
+ u16 tach = min(data->target_count[channel] * 2, FAN_COUNT_REG_MAX);
+
+ rpm = RPM_FROM_REG(tach, sr);
+ }
+ *val = rpm;
+ return 0;
case hwmon_fan_target:
sr = get_tach_period(data->fan_dynamics[channel]);
rpm = RPM_FROM_REG(data->target_count[channel], sr);
@@ -243,6 +256,20 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
data->fan_config[channel] = config;
}
break;
+ case hwmon_fan_min:
+ /*
+ * The minimum fan speed is the same as the target fan speed in
+ * PWM mode and if a PWM channel is disabled, or it is half the
+ * target fan speed in RPM mode.
+ */
+ if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT) &&
+ (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)) {
+ /* partial clamp to avoid overflow */
+ if (val > FAN_RPM_MAX / 2)
+ val = FAN_RPM_MAX / 2;
+ val *= 2;
+ }
+ fallthrough;
case hwmon_fan_target:
val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
bits = bits_for_tach_period(val);
@@ -282,6 +309,7 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
u8 fan_config = data->fan_config[channel % NR_CHANNEL];
switch (attr) {
+ case hwmon_fan_min:
case hwmon_fan_enable:
if (channel < NR_CHANNEL)
return 0644;
@@ -452,18 +480,24 @@ static umode_t max31790_is_visible(const void *data,
static const struct hwmon_channel_info *max31790_info[] = {
HWMON_CHANNEL_INFO(fan,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
- HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
+ HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT),
HWMON_CHANNEL_INFO(pwm,
HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles
2021-05-26 15:40 ` [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles Guenter Roeck
@ 2021-06-01 7:51 ` Václav Kubernát
2021-06-02 12:36 ` Jan Kundrát
1 sibling, 0 replies; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 7:51 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
PWM duty reading works.
Tested-by: Václav Kubernát <kubernat@ceesnet.cz>
st 26. 5. 2021 v 17:40 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> The MAX31790 has two sets of registers for pwm duty cycles, one to request
> a duty cycle and one to read the actual current duty cycle. Both do not
> have to be the same.
>
> When reporting the pwm duty cycle to the user, the actual pwm duty cycle
> from pwm duty cycle registers needs to be reported. When setting it, the
> pwm target duty cycle needs to be written. Since we don't know the actual
> pwm duty cycle after a target pwm duty cycle has been written, set the
> valid flag to false to indicate that actual pwm duty cycle should be read
> from the chip instead of using cached values.
>
> 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 | 3 ++-
> drivers/hwmon/max31790.c | 6 +++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index f301385d8cef..54ff0f49e28f 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -39,5 +39,6 @@ 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] RW fan target duty cycle (0-255)
> +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 f6d4fc0a2f13..693497e09ac0 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -104,7 +104,7 @@ static struct max31790_data *max31790_update_device(struct device *dev)
> data->tach[NR_CHANNEL + i] = rv;
> } else {
> rv = i2c_smbus_read_word_swapped(client,
> - MAX31790_REG_PWMOUT(i));
> + MAX31790_REG_PWM_DUTY_CYCLE(i));
> if (rv < 0)
> goto abort;
> data->pwm[i] = rv;
> @@ -299,10 +299,10 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> err = -EINVAL;
> break;
> }
> - data->pwm[channel] = val << 8;
> + data->valid = false;
> err = i2c_smbus_write_word_swapped(client,
> MAX31790_REG_PWMOUT(channel),
> - data->pwm[channel]);
> + val << 8);
> break;
> case hwmon_pwm_enable:
> fan_config = data->fan_config[channel];
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes
2021-05-26 15:40 ` [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes Guenter Roeck
@ 2021-06-01 8:01 ` Václav Kubernát
2021-06-02 10:47 ` Guenter Roeck
2021-06-02 12:44 ` Jan Kundrát
1 sibling, 1 reply; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:01 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes
2021-05-26 15:40 ` [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes Guenter Roeck
@ 2021-06-01 8:02 ` Václav Kubernát
2021-06-02 13:04 ` Jan Kundrát
1 sibling, 0 replies; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:02 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
fanX_enable works fine.
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:
>
> Since pwmX_enable is now fixed and only handles pwm support instead
> of also enabling/disabling fan tachometers, we need an explicit means
> to do that.
>
> For fan channels 7..12, display the enable status if the channel
> is configured for fan speed reporting. The displayed status matches
> the value of the companion channel but is read-only.
>
> 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 | 3 ++
> drivers/hwmon/max31790.c | 55 ++++++++++++++++++++++++--------
> 2 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index 7b097c3b9b90..b43aa32813fd 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -35,6 +35,9 @@ Sysfs entries
> -------------
>
> ================== === =======================================================
> +fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monitoring
> + The value is RO for companion channels (7-12). For those
> + channels, the value matches the value of the primary channel.
> 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
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 67677c437768..19651feb40fb 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -170,6 +170,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> return PTR_ERR(data);
>
> switch (attr) {
> + case hwmon_fan_enable:
> + *val = !!(data->fan_config[channel % NR_CHANNEL] & MAX31790_FAN_CFG_TACH_INPUT_EN);
> + return 0;
> case hwmon_fan_input:
> sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]);
> rpm = RPM_FROM_REG(data->tach[channel], sr);
> @@ -195,12 +198,32 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
> struct i2c_client *client = data->client;
> int target_count;
> int err = 0;
> - u8 bits;
> + u8 bits, config;
> int sr;
>
> mutex_lock(&data->update_lock);
>
> switch (attr) {
> + case hwmon_fan_enable:
> + config = data->fan_config[channel];
> + if (val == 0) {
> + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */
> + if (!(config & MAX31790_FAN_CFG_RPM_MODE))
> + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
> + } else if (val == 1) {
> + config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
> + } else {
> + err = -EINVAL;
> + break;
> + }
> + if (config != data->fan_config[channel]) {
> + err = i2c_smbus_write_byte_data(client,
> + MAX31790_REG_FAN_CONFIG(channel),
> + config);
> + if (!err)
> + data->fan_config[channel] = config;
> + }
> + break;
> case hwmon_fan_target:
> val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
> bits = bits_for_tach_period(val);
> @@ -240,6 +263,12 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> u8 fan_config = data->fan_config[channel % NR_CHANNEL];
>
> switch (attr) {
> + case hwmon_fan_enable:
> + if (channel < NR_CHANNEL)
> + return 0644;
> + if (fan_config & MAX31790_FAN_CFG_TACH_INPUT)
> + return 0444;
> + return 0;
> case hwmon_fan_input:
> case hwmon_fan_fault:
> if (channel < NR_CHANNEL ||
> @@ -404,18 +433,18 @@ static umode_t max31790_is_visible(const void *data,
>
> static const struct hwmon_channel_info *max31790_info[] = {
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT),
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it
2021-05-26 15:40 ` [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it Guenter Roeck
@ 2021-06-01 8:08 ` Václav Kubernát
0 siblings, 0 replies; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:08 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
Fan fault reading/clearing works fine.
Tested-by: Václav Kubernát <kubernat@cesnet.cz>
st 26. 5. 2021 v 17:41 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> Fault bits in MAX31790 are sticky and have to be cleared explicitly.
> A write operation into either the 'Target Duty Cycle' register or the
> 'Target Count' register is necessary to clear a fault.
>
> At the same time, we can never clear cached fault status values before
> reading them because the companion fault status for any given fan is
> cleared as well when clearing a fault.
>
> 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>
> ---
> drivers/hwmon/max31790.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 19651feb40fb..8e4fd9b7c889 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -80,7 +80,7 @@ static struct max31790_data *max31790_update_device(struct device *dev)
> MAX31790_REG_FAN_FAULT_STATUS1);
> if (rv < 0)
> goto abort;
> - data->fault_status = rv & 0x3F;
> + data->fault_status |= rv & 0x3F;
>
> rv = i2c_smbus_read_byte_data(client,
> MAX31790_REG_FAN_FAULT_STATUS2);
> @@ -184,7 +184,21 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> *val = rpm;
> return 0;
> case hwmon_fan_fault:
> + mutex_lock(&data->update_lock);
> *val = !!(data->fault_status & (1 << channel));
> + data->fault_status &= ~(1 << channel);
> + /*
> + * If a fault bit is set, we need to write into one of the fan
> + * configuration registers to clear it. Note that this also
> + * clears the fault for the companion channel if enabled.
> + */
> + if (*val) {
> + int reg = MAX31790_REG_TARGET_COUNT(channel % NR_CHANNEL);
> +
> + i2c_smbus_write_byte_data(data->client, reg,
> + data->target_count[channel % NR_CHANNEL] >> 8);
> + }
> + mutex_unlock(&data->update_lock);
> return 0;
> default:
> return -EOPNOTSUPP;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed
2021-05-26 15:40 ` [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed Guenter Roeck
@ 2021-06-01 8:11 ` Václav Kubernát
0 siblings, 0 replies; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:11 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
Zero fan speed is reported correctly.
Tested-by: Václav Kubernát <kubernat@cesnet.cz>
st 26. 5. 2021 v 17:41 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> If a fan is not running or not connected, of if fan monitoring is disabled,
> the fan count register returns a fixed value of 0xffe0. So far this is then
> translated to a RPM value larger than 0. Since this is misleading and does
> not really make much sense, report a fan RPM of 0 in this situation.
>
> 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>
> ---
> drivers/hwmon/max31790.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 8e4fd9b7c889..91fe419b596c 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -40,6 +40,8 @@
> #define FAN_RPM_MIN 120
> #define FAN_RPM_MAX 7864320
>
> +#define FAN_COUNT_REG_MAX 0xffe0
> +
> #define RPM_FROM_REG(reg, sr) (((reg) >> 4) ? \
> ((60 * (sr) * 8192) / ((reg) >> 4)) : \
> FAN_RPM_MAX)
> @@ -175,7 +177,10 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> return 0;
> case hwmon_fan_input:
> sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]);
> - rpm = RPM_FROM_REG(data->tach[channel], sr);
> + if (data->tach[channel] == FAN_COUNT_REG_MAX)
> + rpm = 0;
> + else
> + rpm = RPM_FROM_REG(data->tach[channel], sr);
> *val = rpm;
> return 0;
> case hwmon_fan_target:
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes
2021-05-26 15:40 ` [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes Guenter Roeck
@ 2021-06-01 8:19 ` Václav Kubernát
0 siblings, 0 replies; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:19 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
fanX_min works fine.
Tested-by: Václav Kubernát <kubernat@cesnet.cz>
st 26. 5. 2021 v 17:41 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> The minimum fan speed is closely correlated to the target fan speed.
> The correlation depends on the pwm mode. It is essential to be able
> to read and to set it to be able to control when a channel will report
> a fan fault.
>
> Since the minimum fan speed also applies to channels 7..12, make it
> visible but read-only for those channels as well.
>
> 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 | 9 ++++
> drivers/hwmon/max31790.c | 70 ++++++++++++++++++++++++--------
> 2 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index b43aa32813fd..9225c2a78b68 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -40,6 +40,15 @@ fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monito
> channels, the value matches the value of the primary channel.
> fan[1-12]_input RO fan tachometer speed in RPM
> fan[1-12]_fault RO fan experienced fault
> +fan[1-12]_min RW minimum fan speed in RPM. Equivalent to target fan speed
> + in PWM mode and if PWM support is disabled for a channel.
> + Equivalent to half the target fan speed in RPM mode.
> + The value is RO for companion channels (7-12). For those
> + channels, the value matches the value of the primary channel.
> + Note: In RPM mode, if the pwm duty cycle is 100%, the
> + minimum fan speed is equivalent to the target fan speed,
> + and the chip will report a fault condition if the fan
> + speed is below the target fan speed.
> fan[1-6]_target RW desired fan speed in RPM
> 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,
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 91fe419b596c..db97ee99515a 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -111,13 +111,11 @@ static struct max31790_data *max31790_update_device(struct device *dev)
> if (rv < 0)
> goto abort;
> data->pwm[i] = rv;
> -
> - rv = i2c_smbus_read_word_swapped(client,
> - MAX31790_REG_TARGET_COUNT(i));
> - if (rv < 0)
> - goto abort;
> - data->target_count[i] = rv;
> }
> + rv = i2c_smbus_read_word_swapped(client, MAX31790_REG_TARGET_COUNT(i));
> + if (rv < 0)
> + goto abort;
> + data->target_count[i] = rv;
> }
>
> data->last_updated = jiffies;
> @@ -183,6 +181,21 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> rpm = RPM_FROM_REG(data->tach[channel], sr);
> *val = rpm;
> return 0;
> + case hwmon_fan_min:
> + channel %= NR_CHANNEL;
> + sr = get_tach_period(data->fan_dynamics[channel]);
> + if (!(data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE) ||
> + (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT)) {
> + /* pwm mode: target == min */
> + rpm = RPM_FROM_REG(data->target_count[channel], sr);
> + } else {
> + /* rpm mode: min tach count is twice target count */
> + u16 tach = min(data->target_count[channel] * 2, FAN_COUNT_REG_MAX);
> +
> + rpm = RPM_FROM_REG(tach, sr);
> + }
> + *val = rpm;
> + return 0;
> case hwmon_fan_target:
> sr = get_tach_period(data->fan_dynamics[channel]);
> rpm = RPM_FROM_REG(data->target_count[channel], sr);
> @@ -243,6 +256,20 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
> data->fan_config[channel] = config;
> }
> break;
> + case hwmon_fan_min:
> + /*
> + * The minimum fan speed is the same as the target fan speed in
> + * PWM mode and if a PWM channel is disabled, or it is half the
> + * target fan speed in RPM mode.
> + */
> + if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT) &&
> + (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)) {
> + /* partial clamp to avoid overflow */
> + if (val > FAN_RPM_MAX / 2)
> + val = FAN_RPM_MAX / 2;
> + val *= 2;
> + }
> + fallthrough;
> case hwmon_fan_target:
> val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
> bits = bits_for_tach_period(val);
> @@ -282,6 +309,7 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> u8 fan_config = data->fan_config[channel % NR_CHANNEL];
>
> switch (attr) {
> + case hwmon_fan_min:
> case hwmon_fan_enable:
> if (channel < NR_CHANNEL)
> return 0644;
> @@ -452,18 +480,24 @@ static umode_t max31790_is_visible(const void *data,
>
> static const struct hwmon_channel_info *max31790_info[] = {
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_TARGET |
> + HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
> + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/7] hwmon: (max31790) Fixes and improvements
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
` (6 preceding siblings ...)
2021-05-26 15:40 ` [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes Guenter Roeck
@ 2021-06-01 8:29 ` Václav Kubernát
2021-06-01 12:08 ` Guenter Roeck
7 siblings, 1 reply; 22+ messages in thread
From: Václav Kubernát @ 2021-06-01 8:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
Hello,
I have tested your patches. It seems like the driver is now a lot more
usable, thanks. What seems to be still unusable for me, is RPM mode.
The chip creates an effort to set the RPM, but it almost never
stabilizes. I think the reason is mainly because the driver doesn't
work with registers like "window" and "pwm rate of change". PWM mode
seems to work fine.
I think some of the patches in my patch series on the chip are now
obsolete. However, I do think at least fanX_div should be implemented.
When testing with Sunon PF36281BX-000U-S99 (its max RPM is 23000), the
default divisor is not enough (one bit of change equals to about 500
RPM). The only way to change the divisor right now, is to set the
target RPM or min RPM.
There is also the regmap patch, but I've implemented that one mainly
because it made the driver a bit easier to debug.
Václav
st 26. 5. 2021 v 17:40 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> The following series provides various fixes and improvements for the
> max31790 driver.
>
> ----------------------------------------------------------------
> Guenter Roeck (7):
> hwmon: (max31790) Fix fan speed reporting for fan7..12
> hwmon: (max31790) Report correct current pwm duty cycles
> hwmon: (max31790) Fix pwmX_enable attributes
> hwmon: (max31790) Add support for fanX_enable attributes
> hwmon: (max31790) Clear fan fault after reporting it
> hwmon: (max31790) Detect and report zero fan speed
> hwmon: (max31790) Add support for fanX_min attributes
>
> Documentation/hwmon/max31790.rst | 17 +++-
> drivers/hwmon/max31790.c | 171 ++++++++++++++++++++++++++++++---------
> 2 files changed, 147 insertions(+), 41 deletions(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/7] hwmon: (max31790) Fixes and improvements
2021-06-01 8:29 ` [PATCH 0/7] hwmon: (max31790) Fixes and improvements Václav Kubernát
@ 2021-06-01 12:08 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-06-01 12:08 UTC (permalink / raw)
To: Václav Kubernát; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
On Tue, Jun 01, 2021 at 10:29:48AM +0200, Václav Kubernát wrote:
> Hello,
>
[ short reply - I am on the road. I'll look into your replies in
more detail later ]
> I have tested your patches. It seems like the driver is now a lot more
> usable, thanks. What seems to be still unusable for me, is RPM mode.
> The chip creates an effort to set the RPM, but it almost never
> stabilizes. I think the reason is mainly because the driver doesn't
> work with registers like "window" and "pwm rate of change". PWM mode
> seems to work fine.
>
Agreed; I found the same problem. I actually tried to implement support
for an attribute to support the window registers, but could not find
working settings. I only played with window registers, though, so we'll
probably need attributes for both window mode and pwm rate of change.
> I think some of the patches in my patch series on the chip are now
> obsolete. However, I do think at least fanX_div should be implemented.
> When testing with Sunon PF36281BX-000U-S99 (its max RPM is 23000), the
> default divisor is not enough (one bit of change equals to about 500
> RPM). The only way to change the divisor right now, is to set the
> target RPM or min RPM.
>
Agreed.
> There is also the regmap patch, but I've implemented that one mainly
> because it made the driver a bit easier to debug.
>
Only reason for me to omit that was because it was buggy (FWIW, I didn't
know either that the defalt caching mode is "do not cache", and that all
the 'volatile' declarations were useless).
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes
2021-06-01 8:01 ` Václav Kubernát
@ 2021-06-02 10:47 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-06-02 10:47 UTC (permalink / raw)
To: Václav Kubernát; +Cc: linux-hwmon, Jean Delvare, Jan Kundrát
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
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12
2021-05-26 15:40 ` [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12 Guenter Roeck
@ 2021-06-02 12:27 ` Jan Kundrát
0 siblings, 0 replies; 22+ messages in thread
From: Jan Kundrát @ 2021-06-02 12:27 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, kubernat
On středa 26. května 2021 17:40:16 CEST, Guenter Roeck wrote:
> Fans 7..12 do not have their own set of configuration registers.
> So far the code ignored that and read beyond the end of the configuration
> register range to get the tachometer period. This resulted in more or less
> random fan speed values for those fans.
>
> The datasheet is quite vague when it comes to defining the tachometer
> period for fans 7..12. Experiments confirm that the period is the same
> for both fans associated with a given set of configuration registers.
>
> Fixes: 54187ff9d766 ("hwmon: (max31790) Convert to use new
> hwmon registration API")
> Fixes: 195a4b4298a7 ("hwmon: Driver for Maxim MAX31790")
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles
2021-05-26 15:40 ` [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles Guenter Roeck
2021-06-01 7:51 ` Václav Kubernát
@ 2021-06-02 12:36 ` Jan Kundrát
1 sibling, 0 replies; 22+ messages in thread
From: Jan Kundrát @ 2021-06-02 12:36 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, kubernat
On středa 26. května 2021 17:40:17 CEST, Guenter Roeck wrote:
> The MAX31790 has two sets of registers for pwm duty cycles, one to request
> a duty cycle and one to read the actual current duty cycle. Both do not
> have to be the same.
>
> When reporting the pwm duty cycle to the user, the actual pwm duty cycle
> from pwm duty cycle registers needs to be reported. When setting it, the
> pwm target duty cycle needs to be written. Since we don't know the actual
> pwm duty cycle after a target pwm duty cycle has been written, set the
> valid flag to false to indicate that actual pwm duty cycle should be read
> from the chip instead of using cached values.
>
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes
2021-05-26 15:40 ` [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes Guenter Roeck
2021-06-01 8:01 ` Václav Kubernát
@ 2021-06-02 12:44 ` Jan Kundrát
1 sibling, 0 replies; 22+ messages in thread
From: Jan Kundrát @ 2021-06-02 12:44 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, kubernat
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes
2021-05-26 15:40 ` [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes Guenter Roeck
2021-06-01 8:02 ` Václav Kubernát
@ 2021-06-02 13:04 ` Jan Kundrát
2021-06-02 16:43 ` Guenter Roeck
1 sibling, 1 reply; 22+ messages in thread
From: Jan Kundrát @ 2021-06-02 13:04 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, Jean Delvare, kubernat
On středa 26. května 2021 17:40:19 CEST, Guenter Roeck wrote:
> Since pwmX_enable is now fixed and only handles pwm support instead
> of also enabling/disabling fan tachometers, we need an explicit means
> to do that.
>
> For fan channels 7..12, display the enable status if the channel
> is configured for fan speed reporting. The displayed status matches
> the value of the companion channel but is read-only.
This phrasing is confusing to me. That's once again that "configured to"
which in this context doesn't refer to the kernel, but to an initial config
of the chip. I suggest the following:
Fan channels 7..12 are only available when the chip has been configured
with PWM output N-6 disabled. This configuration is outside of scope of the
kernel. The displayed status matches the value of the companion channel but
is read-only.
> +fan[1-12]_enable RW 0=disable fan speed monitoring,
> 1=enable fan speed monitoring
> + The value is RO for companion channels
> (7-12). For those
> + channels, the value matches the value
> of the primary channel.
I realize that it probably doesn't belong to this patch because it affects
the other fan_* files, but the docs would be improved by something like:
Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
monitoring and control of fan RPM as well as detection of fan failure.
Six pins are dedicated tachometer inputs. Any of the six PWM outputs can
-also be configured to serve as tachometer inputs.
+also be reconfigured to serve as tachometer inputs by the firmware. The
+kernel will respect the initial configuration of the chip.
Want an extra patch on top of this series?
> + case hwmon_fan_enable:
> + config = data->fan_config[channel];
> + if (val == 0) {
> + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */
> + if (!(config & MAX31790_FAN_CFG_RPM_MODE))
> + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
This means that a "nonsensical" write from userspace will be silently
ignored, doesn't it? I think it should return an error instead.
Cheers,
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes
2021-06-02 13:04 ` Jan Kundrát
@ 2021-06-02 16:43 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2021-06-02 16:43 UTC (permalink / raw)
To: Jan Kundrát; +Cc: linux-hwmon, Jean Delvare, kubernat
On Wed, Jun 02, 2021 at 03:04:35PM +0200, Jan Kundrát wrote:
> On středa 26. května 2021 17:40:19 CEST, Guenter Roeck wrote:
> > Since pwmX_enable is now fixed and only handles pwm support instead
> > of also enabling/disabling fan tachometers, we need an explicit means
> > to do that.
> >
> > For fan channels 7..12, display the enable status if the channel
> > is configured for fan speed reporting. The displayed status matches
> > the value of the companion channel but is read-only.
>
> This phrasing is confusing to me. That's once again that "configured to"
> which in this context doesn't refer to the kernel, but to an initial config
> of the chip. I suggest the following:
>
> Fan channels 7..12 are only available when the chip has been configured with
> PWM output N-6 disabled. This configuration is outside of scope of the
> kernel. The displayed status matches the value of the companion channel but
> is read-only.
>
> > +fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan
> > speed monitoring
> > + The value is RO for companion channels (7-12).
> > For those
> > + channels, the value matches the value of the
> > primary channel.
>
> I realize that it probably doesn't belong to this patch because it affects
> the other fan_* files, but the docs would be improved by something like:
>
> Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
> monitoring and control of fan RPM as well as detection of fan failure.
> Six pins are dedicated tachometer inputs. Any of the six PWM outputs can
> -also be configured to serve as tachometer inputs.
> +also be reconfigured to serve as tachometer inputs by the firmware. The
> +kernel will respect the initial configuration of the chip.
>
"Precise (+/-1%)" sounds like chip advertising, which I'd rather avoid.
> Want an extra patch on top of this series?
>
> > + case hwmon_fan_enable:
> > + config = data->fan_config[channel];
> > + if (val == 0) {
> > + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */
> > + if (!(config & MAX31790_FAN_CFG_RPM_MODE))
> > + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
>
> This means that a "nonsensical" write from userspace will be silently
> ignored, doesn't it? I think it should return an error instead.
>
Trade-off between confusing users and trying to match the ABI with somewhat
odd chip capabilities. The above isn't exactly specified; it is the result
of trial and error (and a reason why the configuration register must be
treated as volatile when using regmap).
One can argue one way or another. For now I'd rather keep the code as is
because I am away from the evaluation board for the next few weeks and
won't be able to test any functional changes until I am back.
Given the complexity of the chip and its sometimes odd behavior I'll want
to be able to do that kind of testing. Is this important for you ? If so,
we can move forward with patches 1-3 of the series and leave this and
subsequent patches for later.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-06-02 16:44 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 15:40 [PATCH 0/7] hwmon: (max31790) Fixes and improvements Guenter Roeck
2021-05-26 15:40 ` [PATCH 1/7] hwmon: (max31790) Fix fan speed reporting for fan7..12 Guenter Roeck
2021-06-02 12:27 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 2/7] hwmon: (max31790) Report correct current pwm duty cycles Guenter Roeck
2021-06-01 7:51 ` Václav Kubernát
2021-06-02 12:36 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 3/7] hwmon: (max31790) Fix pwmX_enable attributes Guenter Roeck
2021-06-01 8:01 ` Václav Kubernát
2021-06-02 10:47 ` Guenter Roeck
2021-06-02 12:44 ` Jan Kundrát
2021-05-26 15:40 ` [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes Guenter Roeck
2021-06-01 8:02 ` Václav Kubernát
2021-06-02 13:04 ` Jan Kundrát
2021-06-02 16:43 ` Guenter Roeck
2021-05-26 15:40 ` [PATCH 5/7] hwmon: (max31790) Clear fan fault after reporting it Guenter Roeck
2021-06-01 8:08 ` Václav Kubernát
2021-05-26 15:40 ` [PATCH 6/7] hwmon: (max31790) Detect and report zero fan speed Guenter Roeck
2021-06-01 8:11 ` Václav Kubernát
2021-05-26 15:40 ` [PATCH 7/7] hwmon: (max31790) Add support for fanX_min attributes Guenter Roeck
2021-06-01 8:19 ` Václav Kubernát
2021-06-01 8:29 ` [PATCH 0/7] hwmon: (max31790) Fixes and improvements Václav Kubernát
2021-06-01 12:08 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.