Hi Guenter,
On 7/1/24 4:37 PM, Guenter Roeck wrote:
On 7/1/24 07:11, Guenter Roeck wrote:
On 7/1/24 04:05, Quentin Schulz wrote:
Hi Guenter,
On 6/28/24 5:13 PM, Guenter Roeck wrote:
The default value of the maximum fan speed limit register is 0,
essentially translating to an unlimited fan speed. When reading
the limit, a value of 0 is reported in this case. However, writing
a value of 0 results in writing a value of 0xffff into the register,
which is inconsistent.
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
drivers/hwmon/amc6821.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 3c614a0bd192..e37257ae1a6b 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev,
struct device_attribute *devattr,
struct amc6821_data *data = amc6821_update_device(dev);
int ix = to_sensor_dev_attr(devattr)->index;
if (0 == data->fan[ix])
- return sprintf(buf, "0");
+ return sprintf(buf, "6000000");
return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
}
@@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev,
struct device_attribute *attr,
int ret = kstrtol(buf, 10, &val);
if (ret)
return ret;
- val = 1 > val ? 0xFFFF : 6000000/val;
+ val = val < 1 ? 0xFFFF : 6000000 / val;
mutex_lock(&data->update_lock);
- data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF);
+ data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF);
This is an unrelated change I believe and I would therefore have this
in its own commit with proper documentation in the commit log. Indeed:
1- Change in fan_show handles the default 0x0 register value (which
can only currently be achieved via the default value of the registers)
2- Allow (re-)setting unlimited fan speed by allowing the user to
pass 6000001+ instead of clamping it to 6000000 RPM.
Both changes are related.
The whole point of this commit is to report and permit consistent
values when
the register value is 0. But you do have a point - reading it after my
changes
returns 6000000, but writing the same value sets the register to 1. So
I think
the proper change would be to display 6000001 as speed if the register
value is
0, and provide a more detailed explanation. Would that address your
concerns ?
Ah, never mind, I'll do it differently:
- If the register value is 0, keep reporting 0.
Or...... maybe UINT_MAX?
- If the value written is 0, write 0, otherwise limit the range to
1..6000000
and write clamp_val(6000000 / val, 1, 0xffff)
Mmmm... I'm a bit worried about the implication of writing 0 in
TACH-Low-Limit, what is actually going to happen in that scenario? I
assume **every** possible RPM returned by TACH-DATA will be deemed
invalid/below the limit then? Reading `Fan Spin-Up` section, if FSPD bit
from register 0x20 (which we don't write to yet I think?) is set to 0, a
spin-up is started whenever the fan is detected to be running at too low
speed. And we would also be getting an interrupt for that too-low event.
Basically, wondering if we shouldn't gate the writing of 0 to only the
MAX setting?
This minimizes user visibility of the changes, and also ensures that
the reported fan speed is 0 if the register value is 0 when reading the fan
speed.
But didn't you say this means the fan is running at unknown 60 000 000+
RPMs? Do we really want to return 0 even if the fan is actually running?
In which case max < current (possibly) but with no event happening
(which I would expect, reading the datasheet).
Quentin
