Re: [PATCH] platform/x86/lenovo: thinkpad_acpi: add named speed presets to fan_write_cmd_speed

[Mark Pearson]

Hi Daniil

On Mon, Mar 30, 2026, at 9:19 PM, Mark Pearson wrote:
> Hi Daniil
>
> On Mon, Mar 30, 2026, at 4:35 PM, Даниил Булгарь wrote:
>> platform/x86/lenovo: thinkpad_acpi: add named speed presets to
>> fan_write_cmd_speed
>>
>> The fan_write_cmd_speed() function had a TODO comment suggesting
>> support for named speed presets (low, medium, high). Implement
>> this by mapping the preset names to the existing fan_speed_presets
>> enum values, following the same strstarts() pattern used in
>> fan_write_cmd_level().
>> Signed-off-by: Daniil Bulgar <bulgardaniil18@xxxxxxxxx>
>> ---
>>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> index 8982d92df..f10cebe11 100644
>> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> @@ -7957,6 +7957,12 @@ enum fan_control_commands {
>>                          * and also watchdog cmd */
>>  };
>>
>> +enum fan_speed_preset {
>> +    TPACPI_FAN_SPEED_LOW    = 65535 / 4,    /* 25% speed */
>> +    TPACPI_FAN_SPEED_MEDIUM    = 65535 / 2,    /* 50% speed */
>> +    TPACPI_FAN_SPEED_HIGH    = 65535,    /* 100% speed */
>> +};
>> +
>>  static bool fan_control_allowed;
>>
>>  static enum fan_status_access_mode fan_status_access_mode;
>> @@ -9249,10 +9255,13 @@ static int fan_write_cmd_speed(const char *cmd, int *rc)
>>  {
>>      int speed;
>>
>> -    /* TODO:
>> -    * Support speed <low> <medium> <high> ? */
>> -
>> -    if (sscanf(cmd, "speed %d", &speed) != 1)
>> +    if (strstarts(cmd, "speed low"))
>> +        speed = TPACPI_FAN_SPEED_LOW;
>> +    else if (strstarts(cmd, "speed medium"))
>> +        speed = TPACPI_FAN_SPEED_MEDIUM;
>> +    else if (strstarts(cmd, "speed high"))
>> +        speed = TPACPI_FAN_SPEED_HIGH;
>> +    else if (sscanf(cmd, "speed %d", &speed) != 1)
>>          return 0;
>>
>>      *rc = fan_set_speed(speed);
>> -- 
>> 2.53.0
>
> I'd never noticed that TODO :)
>
> No issues with the code but I'd like to check with the thermal team if 
> those are better ways of determining the ranges. I've got a feeling 
> that they probably aren't. 
> Fans don't go that high (or low) during operation so I wonder if there 
> are better 'ranges' to be used (though it might be platform dependent 
> too)
>
> I'll ask if there is any guidance on it.
>

Afraid I'm going to nack this patch.

I reviewed with the thermal team and there are 7 or more different levels and the speeds and steps between speeds can vary between platforms quite significantly.
Trying to fit them into a low/medium/high framework just doesn't seem a good idea I'm afraid.

I'd be good with a patch removing the TODO comment :)

Mark