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

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

[Даниил Булгарь]

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@gmail.com>
---
 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

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 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@gmail.com>
> ---
>  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.

Mark

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@gmail.com>
>> ---
>>  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

[PATCH v2] platform/x86: thinkpad_acpi: remove obsolete TODO comment

[Daniil Bulgar]

This patch removes the obsolete TODO comment regarding fan speed
presets in fan_write_cmd_speed. After discussion with the
maintainers, it was decided that fixed presets (low/medium/high)
are not suitable due to platform-specific variations.

Signed-off-by: Daniil Bulgar <bulgardaniil18@gmail.com>
---
v2:
 - Removed the preset implementation as requested by Mark Pearson
   and only removed the TODO comment.
v1: 
 - Initial attempt to implement low/medium/high presets.
 
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index 8982d92df..75e141304 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -9249,9 +9249,6 @@ 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)
 		return 0;
 
-- 
2.53.0

Re: [PATCH v2] platform/x86: thinkpad_acpi: remove obsolete TODO comment

[Mark Pearson]

On Tue, Apr 7, 2026, at 3:05 PM, Daniil Bulgar wrote:
> This patch removes the obsolete TODO comment regarding fan speed
> presets in fan_write_cmd_speed. After discussion with the
> maintainers, it was decided that fixed presets (low/medium/high)
> are not suitable due to platform-specific variations.
>
> Signed-off-by: Daniil Bulgar <bulgardaniil18@gmail.com>
> ---
> v2:
>  - Removed the preset implementation as requested by Mark Pearson
>    and only removed the TODO comment.
> v1: 
>  - Initial attempt to implement low/medium/high presets.
> 
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c 
> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index 8982d92df..75e141304 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -9249,9 +9249,6 @@ 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)
>  		return 0;
> 
> -- 
> 2.53.0

Looks good - thanks :)
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

Re: [PATCH v2] platform/x86: thinkpad_acpi: remove obsolete TODO comment

[Ilpo Järvinen]

On Tue, 07 Apr 2026 21:05:46 +0200, Daniil Bulgar wrote:

> This patch removes the obsolete TODO comment regarding fan speed
> presets in fan_write_cmd_speed. After discussion with the
> maintainers, it was decided that fixed presets (low/medium/high)
> are not suitable due to platform-specific variations.
> 
> 


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: thinkpad_acpi: remove obsolete TODO comment
      commit: a29b5cd42f5bc6ba1be6422f61f3f05bab707ce8

--
 i.