Message-ID: <593b4cd5-4b03-40de-8749-341964970ca1@roeck-us.net>
Date: Mon, 1 Jul 2024 12:11:01 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed
 limits consistent

On 7/1/24 11:05, Quentin Schulz wrote:
> 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@...ck-us.net>
>>>>>>> ---
>>>>>>>   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?
> 

No, such fans don't exist. There are some industrial fans with high rpm, like
in the 30k+ RPM range, but it would not be technically possible to build one
much faster than that. The fastest (small) fan I could find is
https://www.sanyodenki.com/archive/document/product/cooling/catalog_E_pdf/San_Ace_40HVA28_E.pdf
which is rated for up to 38,000 rpm, but that is pretty much as fast as it goes.

As an exercise, you could try to calculate the edge speed of a fan running at
6M RPM. That would be several times the speed of sound even for a very small fan.

> [...]
>>>> 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.
> 
Agreed.

Thanks,
Guenter

Powered by blists - more mailing lists