On 7/1/24 09:13, Quentin Schulz wrote:
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?
Problem with that is that disconnected fans would report that value as fan speed.
Traditionally drivers report a fan speed of 0 in that case.
On the other side I agree that reporting "0" as "maximum fan speed" doesn't
make much sense either because the real limit _is_ unlimited. But reporting
4294967295 in that case isn't really any better.
- 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?
Hmm, good point, and make sense.
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).
Did I say that ? If so, I must have meant something different. The register counts the
pulse period, so, yes, it would be 0 if rpm is above 6,000,000. But that is really not
realistic. In practice I don't know what the controller reports in the register if no
fan is connected - that would require real hardware which obviously I don't have.
Overall I think I'll stick with the minimum, at least for now: Permit writing 0
into the high limit register only, and otherwise keep the currently permitted ranges.
Thanks,
Guenter
