On 7/1/24 7:21 PM, Guenter Roeck wrote:
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.
OK so the issue is that the current fan speed in RPM could be 0 because
it's disconnected, or because it exceeds 6M tach pulses.
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.
Agreed, but I'm also wondering if there really exist fans at 6M+ RPMs?
Maybe we're discussing a scenario that just doesn't exist (yet) and that
we don't need to handle?
[...]
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.
I'll forage in our shelves tomorrow if I don't forget, trying to find
one... if we have one.
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.
Works for me, we can always revisit later on if needed/desired.
Quentin
