Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[Armin Wolf <W_Armin@gmx.de>]

Am 03.09.21 um 16:59 schrieb Guenter Roeck:
> On 9/3/21 7:49 AM, Pali Rohár wrote:
>> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> For allowing tools like i8kutils to query the fan state
>>> without having to rely on the deprecated /proc/i8k interface,
>>> they need to scale the pwm values (0 - 255) back to the
>>> real hardware values (0 - 2/3).
>>> Show fan_max in sysfs to allow for such a scenario.
>>
>> Guenter, I think that this is general problem and not specific to
>> dell-smm-hwmon.c driver and i8kutils.
>>
>> All other hwmon tools should have similar problem. If e.g. GUI tool has
>> slider for controlling pwm then such tool needs to know number of steps.
>> Otherwise usage of such slider would be not very user friendly...
>>
>> Currently in hwmon sysfs API there is not attribute which could export
>> this kind of information.
>>
>> What about e.g. introducing 'pwm_steps' attribute which would provide
>> this information?
>>
>
> It isn't really a problem. The problem is that the tool writers insist
> not following the ABI. All they would have to do is to use a scale of
> 0..255,
> read back any written pwm value, and adjust the displayed value based
> on the returned value. I do not see why this would be "not user
> friendly".
>
> An attribute such as pwm_steps would not work because the step size
> can be
> non-linear.
>
> Guenter
>
i8kutils uses values from 0 to 2/3 for historic reasons, as the
deprecated /proc/i8k interface
provided such values. In order to not break backwards compatibility,
they cannot simply
switch to standard pwm values (ithink).
I believe that showing fan_max via sysfs would allow i8kutils to finally
use the standard interface,
since when this happens, we maybe can remove the /proc/i8k interface.
>>> Tested on a Dell Latitude C600.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>> b/drivers/hwmon/dell-smm-hwmon.c
>>> index 774c1b0715d9..6d3fd4f0f99d 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
>>>   MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with
>>> (default: autodetect)");
>>>
>>>   static uint fan_max;
>>> -module_param(fan_max, uint, 0);
>>> +module_param(fan_max, uint, 0444);
>>>   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed
>>> (default: autodetect)");
>>>
>>>   struct smm_regs {
>>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct
>>> platform_device *pdev)
>>>               fan_max = conf->fan_max;
>>>       }
>>>
>>> -    data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;    /* Must not be
>>> 0 */
>>> +    if (!fan_max)    /* Must not be 0*/
>>> +        fan_max = I8K_FAN_HIGH;
>>> +
>>> +    data->i8k_fan_max = fan_max;
>>>       data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>>
>>>       fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>> --
>>> 2.20.1
>>>
>