1. Patchew
  2. linux
  3. [PATCH v2 0/4] platform/x86: acer-wmi: Add fan control support
  4. View patch

[PATCH v2 2/4] platform/x86: acer-wmi: Add fan control support

Armin Wolf posted 4 patches 4 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 2/4] platform/x86: acer-wmi: Add fan control support
Posted by Armin Wolf 4 months, 2 weeks ago 
Add support for controlling the fan speed using the
SetGamingFanSpeed() and GetGamingFanSpeed() WMI methods.

This feature is only enabled if the machine has ACER_CAP_PWM enabled
and depend on ACER_CAP_HWMON for detecting the number of available
fans.

Reviewed-by: Kurt Borja <kuurtb@gmail.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 221 +++++++++++++++++++++++++++++++-
 1 file changed, 219 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index a41555ee8589..345b6e24ae31 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -12,10 +12,12 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kernel.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/dmi.h>
+#include <linux/fixp-arith.h>
 #include <linux/backlight.h>
 #include <linux/leds.h>
 #include <linux/platform_device.h>
@@ -69,11 +71,16 @@ MODULE_LICENSE("GPL");
 #define ACER_WMID_GET_GAMING_LED_METHODID 4
 #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
 #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
+#define ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID 15
+#define ACER_WMID_SET_GAMING_FAN_SPEED_METHODID 16
+#define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
 #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
 #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
 
+#define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
 #define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
 #define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
+#define ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK GENMASK_ULL(23, 8)
 
 #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
 #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
@@ -81,6 +88,10 @@ MODULE_LICENSE("GPL");
 #define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
 #define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
 
+#define ACER_GAMING_FAN_SPEED_STATUS_MASK GENMASK_ULL(7, 0)
+#define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
+#define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
+
 #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
 #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
 #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
@@ -130,6 +141,11 @@ enum acer_wmi_predator_v4_sensor_id {
 	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
 };
 
+enum acer_wmi_gaming_fan_id {
+	ACER_WMID_CPU_FAN	= 0x01,
+	ACER_WMID_GPU_FAN	= 0x04,
+};
+
 enum acer_wmi_gaming_fan_mode {
 	ACER_WMID_FAN_MODE_AUTO		= 0x01,
 	ACER_WMID_FAN_MODE_TURBO	= 0x02,
@@ -292,6 +308,7 @@ struct hotkey_function_type_aa {
 #define ACER_CAP_TURBO_FAN		BIT(9)
 #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
 #define ACER_CAP_HWMON			BIT(11)
+#define ACER_CAP_PWM			BIT(12)
 
 /*
  * Interface type flags
@@ -386,6 +403,7 @@ struct quirk_entry {
 	u8 cpu_fans;
 	u8 gpu_fans;
 	u8 predator_v4;
+	u8 pwm;
 };
 
 static struct quirk_entry *quirks;
@@ -405,6 +423,9 @@ static void __init set_quirks(void)
 	if (quirks->predator_v4)
 		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
 					 ACER_CAP_HWMON;
+
+	if (quirks->pwm)
+		interface->capability |= ACER_CAP_PWM;
 }
 
 static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -1651,6 +1672,27 @@ static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
 	return 0;
 }
 
+static int WMID_gaming_get_fan_behavior(u16 fan_bitmap, u8 *mode_bitmap)
+{
+	acpi_status status;
+	u32 input = 0;
+	u64 result;
+
+	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
+	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID, input,
+					    &result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* The return status must be zero for the operation to have succeeded */
+	if (FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK, result))
+		return -EIO;
+
+	*mode_bitmap = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK, result);
+
+	return 0;
+}
+
 static void WMID_gaming_set_fan_mode(u8 fan_mode)
 {
 	u16 mode_bitmap = 0;
@@ -1669,6 +1711,55 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
 	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
 }
 
+static int WMID_gaming_set_gaming_fan_speed(u8 fan, u8 speed)
+{
+	acpi_status status;
+	u64 input = 0;
+	u64 result;
+
+	if (speed > 100)
+		return -EINVAL;
+
+	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
+	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_VALUE_MASK, speed);
+
+	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_SPEED_METHODID, input, &result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	switch (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result)) {
+	case 0x00:
+		return 0;
+	case 0x01:
+		return -ENODEV;
+	case 0x02:
+		return -EINVAL;
+	default:
+		return -ENXIO;
+	}
+}
+
+static int WMID_gaming_get_gaming_fan_speed(u8 fan, u8 *speed)
+{
+	acpi_status status;
+	u32 input = 0;
+	u64 result;
+
+	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
+
+	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_SPEED_METHODID, input,
+					    &result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	if (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result))
+		return -ENODEV;
+
+	*speed = FIELD_GET(ACER_GAMING_FAN_SPEED_VALUE_MASK, result);
+
+	return 0;
+}
+
 static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
 {
 	acpi_status status;
@@ -2840,6 +2931,16 @@ static const enum acer_wmi_predator_v4_sensor_id acer_wmi_fan_channel_to_sensor_
 	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
 };
 
+static const enum acer_wmi_gaming_fan_id acer_wmi_fan_channel_to_fan_id[] = {
+	[0] = ACER_WMID_CPU_FAN,
+	[1] = ACER_WMID_GPU_FAN,
+};
+
+static const u16 acer_wmi_fan_channel_to_fan_bitmap[] = {
+	[0] = ACER_GAMING_FAN_BEHAVIOR_CPU,
+	[1] = ACER_GAMING_FAN_BEHAVIOR_GPU,
+};
+
 static umode_t acer_wmi_hwmon_is_visible(const void *data,
 					 enum hwmon_sensor_types type, u32 attr,
 					 int channel)
@@ -2851,6 +2952,11 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
 	case hwmon_temp:
 		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
 		break;
+	case hwmon_pwm:
+		if (!has_cap(ACER_CAP_PWM))
+			return 0;
+
+		fallthrough;
 	case hwmon_fan:
 		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
 		break;
@@ -2858,8 +2964,12 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
 		return 0;
 	}
 
-	if (*supported_sensors & BIT(sensor_id - 1))
+	if (*supported_sensors & BIT(sensor_id - 1)) {
+		if (type == hwmon_pwm)
+			return 0644;
+
 		return 0444;
+	}
 
 	return 0;
 }
@@ -2868,8 +2978,10 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			       u32 attr, int channel, long *val)
 {
 	u64 command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING;
+	u8 fan, speed, mode_bitmap;
+	u16 fan_bitmap;
+	int mode, ret;
 	u64 result;
-	int ret;
 
 	switch (type) {
 	case hwmon_temp:
@@ -2893,6 +3005,106 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 
 		*val = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
 		return 0;
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			fan = acer_wmi_fan_channel_to_fan_id[channel];
+			ret = WMID_gaming_get_gaming_fan_speed(fan, &speed);
+			if (ret < 0)
+				return ret;
+
+			*val = fixp_linear_interpolate(0, 0, 100, U8_MAX, speed);
+			return 0;
+		case hwmon_pwm_enable:
+			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
+			ret = WMID_gaming_get_fan_behavior(fan_bitmap, &mode_bitmap);
+			if (ret < 0)
+				return ret;
+
+			switch (channel) {
+			case 0:
+				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
+						 mode_bitmap);
+				break;
+			case 1:
+				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
+						 mode_bitmap);
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			switch (mode) {
+			case ACER_WMID_FAN_MODE_AUTO:
+				*val = 2;
+				return 0;
+			case ACER_WMID_FAN_MODE_TURBO:
+				*val = 0;
+				return 0;
+			case ACER_WMID_FAN_MODE_CUSTOM:
+				*val = 1;
+				return 0;
+			default:
+				return -ENXIO;
+			}
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int acer_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+				u32 attr, int channel, long val)
+{
+	u8 fan, speed, mode_bitmap;
+	u16 fan_bitmap;
+	int mode;
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			fan = acer_wmi_fan_channel_to_fan_id[channel];
+			speed = fixp_linear_interpolate(0, 0, U8_MAX, 100,
+							clamp_val(val, 0, U8_MAX));
+
+			return WMID_gaming_set_gaming_fan_speed(fan, speed);
+		case hwmon_pwm_enable:
+			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
+
+			switch (val) {
+			case 0:
+				mode = ACER_WMID_FAN_MODE_TURBO;
+				break;
+			case 1:
+				mode = ACER_WMID_FAN_MODE_CUSTOM;
+				break;
+			case 2:
+				mode = ACER_WMID_FAN_MODE_AUTO;
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			switch (channel) {
+			case 0:
+				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
+							 mode);
+				break;
+			case 1:
+				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
+							 mode);
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			return WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
+		default:
+			return -EOPNOTSUPP;
+		}
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -2908,11 +3120,16 @@ static const struct hwmon_channel_info *const acer_wmi_hwmon_info[] = {
 			   HWMON_F_INPUT,
 			   HWMON_F_INPUT
 			   ),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
+			   ),
 	NULL
 };
 
 static const struct hwmon_ops acer_wmi_hwmon_ops = {
 	.read = acer_wmi_hwmon_read,
+	.write = acer_wmi_hwmon_write,
 	.is_visible = acer_wmi_hwmon_is_visible,
 };
 
-- 
2.39.5
Re: [PATCH v2 2/4] platform/x86: acer-wmi: Add fan control support
Posted by Ilpo Järvinen 4 months, 2 weeks ago 
On Tue, 23 Sep 2025, Armin Wolf wrote:

> Add support for controlling the fan speed using the
> SetGamingFanSpeed() and GetGamingFanSpeed() WMI methods.
> 
> This feature is only enabled if the machine has ACER_CAP_PWM enabled
> and depend on ACER_CAP_HWMON for detecting the number of available
> fans.
> 
> Reviewed-by: Kurt Borja <kuurtb@gmail.com>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/acer-wmi.c | 221 +++++++++++++++++++++++++++++++-
>  1 file changed, 219 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index a41555ee8589..345b6e24ae31 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -12,10 +12,12 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/kernel.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/dmi.h>
> +#include <linux/fixp-arith.h>
>  #include <linux/backlight.h>
>  #include <linux/leds.h>
>  #include <linux/platform_device.h>
> @@ -69,11 +71,16 @@ MODULE_LICENSE("GPL");
>  #define ACER_WMID_GET_GAMING_LED_METHODID 4
>  #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>  #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
> +#define ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID 15
> +#define ACER_WMID_SET_GAMING_FAN_SPEED_METHODID 16
> +#define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
>  #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>  #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>  
> +#define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
>  #define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
> +#define ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK GENMASK_ULL(23, 8)
>  
>  #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
>  #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> @@ -81,6 +88,10 @@ MODULE_LICENSE("GPL");
>  #define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
>  #define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
>  
> +#define ACER_GAMING_FAN_SPEED_STATUS_MASK GENMASK_ULL(7, 0)
> +#define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
> +#define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
> +
>  #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
> @@ -130,6 +141,11 @@ enum acer_wmi_predator_v4_sensor_id {
>  	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
>  };
>  
> +enum acer_wmi_gaming_fan_id {
> +	ACER_WMID_CPU_FAN	= 0x01,
> +	ACER_WMID_GPU_FAN	= 0x04,
> +};

In patch 1 you added these:

+#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
...
+#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
+#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)

Are these numbers related (you even called the field "ID mask")? Could we 
do with less defines for the same thing if they're related?

>  enum acer_wmi_gaming_fan_mode {
>  	ACER_WMID_FAN_MODE_AUTO		= 0x01,
>  	ACER_WMID_FAN_MODE_TURBO	= 0x02,
> @@ -292,6 +308,7 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_TURBO_FAN		BIT(9)
>  #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
>  #define ACER_CAP_HWMON			BIT(11)
> +#define ACER_CAP_PWM			BIT(12)
>  
>  /*
>   * Interface type flags
> @@ -386,6 +403,7 @@ struct quirk_entry {
>  	u8 cpu_fans;
>  	u8 gpu_fans;
>  	u8 predator_v4;
> +	u8 pwm;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -405,6 +423,9 @@ static void __init set_quirks(void)
>  	if (quirks->predator_v4)
>  		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
>  					 ACER_CAP_HWMON;
> +
> +	if (quirks->pwm)
> +		interface->capability |= ACER_CAP_PWM;
>  }
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -1651,6 +1672,27 @@ static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
>  	return 0;
>  }
>  
> +static int WMID_gaming_get_fan_behavior(u16 fan_bitmap, u8 *mode_bitmap)
> +{
> +	acpi_status status;
> +	u32 input = 0;
> +	u64 result;
> +
> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
> +	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID, input,
> +					    &result);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	/* The return status must be zero for the operation to have succeeded */
> +	if (FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK, result))
> +		return -EIO;
> +
> +	*mode_bitmap = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK, result);
> +
> +	return 0;
> +}
> +
>  static void WMID_gaming_set_fan_mode(u8 fan_mode)
>  {
>  	u16 mode_bitmap = 0;
> @@ -1669,6 +1711,55 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
>  	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
>  }
>  
> +static int WMID_gaming_set_gaming_fan_speed(u8 fan, u8 speed)
> +{
> +	acpi_status status;
> +	u64 input = 0;
> +	u64 result;
> +
> +	if (speed > 100)
> +		return -EINVAL;
> +
> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_VALUE_MASK, speed);
> +
> +	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_SPEED_METHODID, input, &result);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	switch (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result)) {
> +	case 0x00:
> +		return 0;
> +	case 0x01:
> +		return -ENODEV;
> +	case 0x02:
> +		return -EINVAL;
> +	default:
> +		return -ENXIO;
> +	}
> +}
> +
> +static int WMID_gaming_get_gaming_fan_speed(u8 fan, u8 *speed)
> +{
> +	acpi_status status;
> +	u32 input = 0;
> +	u64 result;
> +
> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> +
> +	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_SPEED_METHODID, input,
> +					    &result);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	if (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result))
> +		return -ENODEV;
> +
> +	*speed = FIELD_GET(ACER_GAMING_FAN_SPEED_VALUE_MASK, result);
> +
> +	return 0;
> +}
> +
>  static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
>  {
>  	acpi_status status;
> @@ -2840,6 +2931,16 @@ static const enum acer_wmi_predator_v4_sensor_id acer_wmi_fan_channel_to_sensor_
>  	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
>  };
>  
> +static const enum acer_wmi_gaming_fan_id acer_wmi_fan_channel_to_fan_id[] = {
> +	[0] = ACER_WMID_CPU_FAN,
> +	[1] = ACER_WMID_GPU_FAN,
> +};
> +
> +static const u16 acer_wmi_fan_channel_to_fan_bitmap[] = {
> +	[0] = ACER_GAMING_FAN_BEHAVIOR_CPU,
> +	[1] = ACER_GAMING_FAN_BEHAVIOR_GPU,
> +};
> +
>  static umode_t acer_wmi_hwmon_is_visible(const void *data,
>  					 enum hwmon_sensor_types type, u32 attr,
>  					 int channel)
> @@ -2851,6 +2952,11 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
>  	case hwmon_temp:
>  		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
>  		break;
> +	case hwmon_pwm:
> +		if (!has_cap(ACER_CAP_PWM))
> +			return 0;
> +
> +		fallthrough;
>  	case hwmon_fan:
>  		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
>  		break;
> @@ -2858,8 +2964,12 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
>  		return 0;
>  	}
>  
> -	if (*supported_sensors & BIT(sensor_id - 1))
> +	if (*supported_sensors & BIT(sensor_id - 1)) {
> +		if (type == hwmon_pwm)
> +			return 0644;
> +
>  		return 0444;
> +	}
>  
>  	return 0;
>  }
> @@ -2868,8 +2978,10 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			       u32 attr, int channel, long *val)
>  {
>  	u64 command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING;
> +	u8 fan, speed, mode_bitmap;
> +	u16 fan_bitmap;
> +	int mode, ret;
>  	u64 result;
> -	int ret;
>  
>  	switch (type) {
>  	case hwmon_temp:
> @@ -2893,6 +3005,106 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  
>  		*val = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
>  		return 0;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			fan = acer_wmi_fan_channel_to_fan_id[channel];
> +			ret = WMID_gaming_get_gaming_fan_speed(fan, &speed);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = fixp_linear_interpolate(0, 0, 100, U8_MAX, speed);
> +			return 0;
> +		case hwmon_pwm_enable:
> +			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
> +			ret = WMID_gaming_get_fan_behavior(fan_bitmap, &mode_bitmap);
> +			if (ret < 0)
> +				return ret;
> +
> +			switch (channel) {
> +			case 0:
> +				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> +						 mode_bitmap);
> +				break;
> +			case 1:
> +				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> +						 mode_bitmap);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}

You've managed to create an internal API that resembles the WMI method 
input by working in terms of fan_bitmap and mode_bitmap which makes things 
complicated as then you have to build logic on top of it to do all the 
mapping in the caller...

I suggest changing this so you make WMID_gaming_get_fan_behavior() to 
input ACER_GAMING_FAN_BEHAVIOR_CPU / GPU (or ACER_WMID_CPU_FAN / GPU_FAN 
depending on the question I asked above) and perform the output mapping 
within that function. I think it would be much cleaner.

I didn't exactly like the internal API in patch 1 either and I'd prefer 
the field GENMASK()s to directly point to the correct bits instead of the 
nested FIELD_GET/PREP()s.

> +			switch (mode) {
> +			case ACER_WMID_FAN_MODE_AUTO:
> +				*val = 2;
> +				return 0;
> +			case ACER_WMID_FAN_MODE_TURBO:
> +				*val = 0;
> +				return 0;
> +			case ACER_WMID_FAN_MODE_CUSTOM:
> +				*val = 1;
> +				return 0;
> +			default:
> +				return -ENXIO;
> +			}
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int acer_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +				u32 attr, int channel, long val)
> +{
> +	u8 fan, speed, mode_bitmap;
> +	u16 fan_bitmap;
> +	int mode;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			fan = acer_wmi_fan_channel_to_fan_id[channel];
> +			speed = fixp_linear_interpolate(0, 0, U8_MAX, 100,
> +							clamp_val(val, 0, U8_MAX));
> +
> +			return WMID_gaming_set_gaming_fan_speed(fan, speed);
> +		case hwmon_pwm_enable:
> +			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
> +
> +			switch (val) {
> +			case 0:
> +				mode = ACER_WMID_FAN_MODE_TURBO;
> +				break;
> +			case 1:
> +				mode = ACER_WMID_FAN_MODE_CUSTOM;
> +				break;
> +			case 2:
> +				mode = ACER_WMID_FAN_MODE_AUTO;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +
> +			switch (channel) {
> +			case 0:
> +				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> +							 mode);
> +				break;
> +			case 1:
> +				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> +							 mode);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}

These too look they could be mapped inside WMID_gaming_set_fan_behavior().

> +
> +			return WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -2908,11 +3120,16 @@ static const struct hwmon_channel_info *const acer_wmi_hwmon_info[] = {
>  			   HWMON_F_INPUT,
>  			   HWMON_F_INPUT
>  			   ),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
> +			   ),
>  	NULL
>  };
>  
>  static const struct hwmon_ops acer_wmi_hwmon_ops = {
>  	.read = acer_wmi_hwmon_read,
> +	.write = acer_wmi_hwmon_write,
>  	.is_visible = acer_wmi_hwmon_is_visible,
>  };
>  
> 

-- 
 i.
Re: [PATCH v2 2/4] platform/x86: acer-wmi: Add fan control support
Posted by Armin Wolf 4 months, 1 week ago 
Am 24.09.25 um 12:52 schrieb Ilpo Järvinen:

> On Tue, 23 Sep 2025, Armin Wolf wrote:
>
>> Add support for controlling the fan speed using the
>> SetGamingFanSpeed() and GetGamingFanSpeed() WMI methods.
>>
>> This feature is only enabled if the machine has ACER_CAP_PWM enabled
>> and depend on ACER_CAP_HWMON for detecting the number of available
>> fans.
>>
>> Reviewed-by: Kurt Borja <kuurtb@gmail.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/acer-wmi.c | 221 +++++++++++++++++++++++++++++++-
>>   1 file changed, 219 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index a41555ee8589..345b6e24ae31 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -12,10 +12,12 @@
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>   
>>   #include <linux/kernel.h>
>> +#include <linux/minmax.h>
>>   #include <linux/module.h>
>>   #include <linux/init.h>
>>   #include <linux/types.h>
>>   #include <linux/dmi.h>
>> +#include <linux/fixp-arith.h>
>>   #include <linux/backlight.h>
>>   #include <linux/leds.h>
>>   #include <linux/platform_device.h>
>> @@ -69,11 +71,16 @@ MODULE_LICENSE("GPL");
>>   #define ACER_WMID_GET_GAMING_LED_METHODID 4
>>   #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>>   #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
>> +#define ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID 15
>> +#define ACER_WMID_SET_GAMING_FAN_SPEED_METHODID 16
>> +#define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
>>   #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>>   #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>>   
>> +#define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
>>   #define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
>> +#define ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK GENMASK_ULL(23, 8)
>>   
>>   #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
>>   #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>> @@ -81,6 +88,10 @@ MODULE_LICENSE("GPL");
>>   #define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
>>   #define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
>>   
>> +#define ACER_GAMING_FAN_SPEED_STATUS_MASK GENMASK_ULL(7, 0)
>> +#define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
>> +#define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
>> +
>>   #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
>> @@ -130,6 +141,11 @@ enum acer_wmi_predator_v4_sensor_id {
>>   	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
>>   };
>>   
>> +enum acer_wmi_gaming_fan_id {
>> +	ACER_WMID_CPU_FAN	= 0x01,
>> +	ACER_WMID_GPU_FAN	= 0x04,
>> +};
> In patch 1 you added these:
>
> +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
> ...
> +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>
> Are these numbers related (you even called the field "ID mask")? Could we
> do with less defines for the same thing if they're related?

I honestly don't know if those IDs are related. I would like to play it safe
and keep them separate for now.

>>   enum acer_wmi_gaming_fan_mode {
>>   	ACER_WMID_FAN_MODE_AUTO		= 0x01,
>>   	ACER_WMID_FAN_MODE_TURBO	= 0x02,
>> @@ -292,6 +308,7 @@ struct hotkey_function_type_aa {
>>   #define ACER_CAP_TURBO_FAN		BIT(9)
>>   #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
>>   #define ACER_CAP_HWMON			BIT(11)
>> +#define ACER_CAP_PWM			BIT(12)
>>   
>>   /*
>>    * Interface type flags
>> @@ -386,6 +403,7 @@ struct quirk_entry {
>>   	u8 cpu_fans;
>>   	u8 gpu_fans;
>>   	u8 predator_v4;
>> +	u8 pwm;
>>   };
>>   
>>   static struct quirk_entry *quirks;
>> @@ -405,6 +423,9 @@ static void __init set_quirks(void)
>>   	if (quirks->predator_v4)
>>   		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
>>   					 ACER_CAP_HWMON;
>> +
>> +	if (quirks->pwm)
>> +		interface->capability |= ACER_CAP_PWM;
>>   }
>>   
>>   static int __init dmi_matched(const struct dmi_system_id *dmi)
>> @@ -1651,6 +1672,27 @@ static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
>>   	return 0;
>>   }
>>   
>> +static int WMID_gaming_get_fan_behavior(u16 fan_bitmap, u8 *mode_bitmap)
>> +{
>> +	acpi_status status;
>> +	u32 input = 0;
>> +	u64 result;
>> +
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
>> +	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID, input,
>> +					    &result);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	/* The return status must be zero for the operation to have succeeded */
>> +	if (FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK, result))
>> +		return -EIO;
>> +
>> +	*mode_bitmap = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK, result);
>> +
>> +	return 0;
>> +}
>> +
>>   static void WMID_gaming_set_fan_mode(u8 fan_mode)
>>   {
>>   	u16 mode_bitmap = 0;
>> @@ -1669,6 +1711,55 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
>>   	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
>>   }
>>   
>> +static int WMID_gaming_set_gaming_fan_speed(u8 fan, u8 speed)
>> +{
>> +	acpi_status status;
>> +	u64 input = 0;
>> +	u64 result;
>> +
>> +	if (speed > 100)
>> +		return -EINVAL;
>> +
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_VALUE_MASK, speed);
>> +
>> +	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_SPEED_METHODID, input, &result);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	switch (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result)) {
>> +	case 0x00:
>> +		return 0;
>> +	case 0x01:
>> +		return -ENODEV;
>> +	case 0x02:
>> +		return -EINVAL;
>> +	default:
>> +		return -ENXIO;
>> +	}
>> +}
>> +
>> +static int WMID_gaming_get_gaming_fan_speed(u8 fan, u8 *speed)
>> +{
>> +	acpi_status status;
>> +	u32 input = 0;
>> +	u64 result;
>> +
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
>> +
>> +	status = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_SPEED_METHODID, input,
>> +					    &result);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	if (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result))
>> +		return -ENODEV;
>> +
>> +	*speed = FIELD_GET(ACER_GAMING_FAN_SPEED_VALUE_MASK, result);
>> +
>> +	return 0;
>> +}
>> +
>>   static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
>>   {
>>   	acpi_status status;
>> @@ -2840,6 +2931,16 @@ static const enum acer_wmi_predator_v4_sensor_id acer_wmi_fan_channel_to_sensor_
>>   	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
>>   };
>>   
>> +static const enum acer_wmi_gaming_fan_id acer_wmi_fan_channel_to_fan_id[] = {
>> +	[0] = ACER_WMID_CPU_FAN,
>> +	[1] = ACER_WMID_GPU_FAN,
>> +};
>> +
>> +static const u16 acer_wmi_fan_channel_to_fan_bitmap[] = {
>> +	[0] = ACER_GAMING_FAN_BEHAVIOR_CPU,
>> +	[1] = ACER_GAMING_FAN_BEHAVIOR_GPU,
>> +};
>> +
>>   static umode_t acer_wmi_hwmon_is_visible(const void *data,
>>   					 enum hwmon_sensor_types type, u32 attr,
>>   					 int channel)
>> @@ -2851,6 +2952,11 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
>>   	case hwmon_temp:
>>   		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
>>   		break;
>> +	case hwmon_pwm:
>> +		if (!has_cap(ACER_CAP_PWM))
>> +			return 0;
>> +
>> +		fallthrough;
>>   	case hwmon_fan:
>>   		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
>>   		break;
>> @@ -2858,8 +2964,12 @@ static umode_t acer_wmi_hwmon_is_visible(const void *data,
>>   		return 0;
>>   	}
>>   
>> -	if (*supported_sensors & BIT(sensor_id - 1))
>> +	if (*supported_sensors & BIT(sensor_id - 1)) {
>> +		if (type == hwmon_pwm)
>> +			return 0644;
>> +
>>   		return 0444;
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -2868,8 +2978,10 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>>   			       u32 attr, int channel, long *val)
>>   {
>>   	u64 command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING;
>> +	u8 fan, speed, mode_bitmap;
>> +	u16 fan_bitmap;
>> +	int mode, ret;
>>   	u64 result;
>> -	int ret;
>>   
>>   	switch (type) {
>>   	case hwmon_temp:
>> @@ -2893,6 +3005,106 @@ static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>>   
>>   		*val = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
>>   		return 0;
>> +	case hwmon_pwm:
>> +		switch (attr) {
>> +		case hwmon_pwm_input:
>> +			fan = acer_wmi_fan_channel_to_fan_id[channel];
>> +			ret = WMID_gaming_get_gaming_fan_speed(fan, &speed);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			*val = fixp_linear_interpolate(0, 0, 100, U8_MAX, speed);
>> +			return 0;
>> +		case hwmon_pwm_enable:
>> +			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
>> +			ret = WMID_gaming_get_fan_behavior(fan_bitmap, &mode_bitmap);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			switch (channel) {
>> +			case 0:
>> +				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
>> +						 mode_bitmap);
>> +				break;
>> +			case 1:
>> +				mode = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
>> +						 mode_bitmap);
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
> You've managed to create an internal API that resembles the WMI method
> input by working in terms of fan_bitmap and mode_bitmap which makes things
> complicated as then you have to build logic on top of it to do all the
> mapping in the caller...
>
> I suggest changing this so you make WMID_gaming_get_fan_behavior() to
> input ACER_GAMING_FAN_BEHAVIOR_CPU / GPU (or ACER_WMID_CPU_FAN / GPU_FAN
> depending on the question I asked above) and perform the output mapping
> within that function. I think it would be much cleaner.
>
> I didn't exactly like the internal API in patch 1 either and I'd prefer
> the field GENMASK()s to directly point to the correct bits instead of the
> nested FIELD_GET/PREP()s.

Ok, will do.

>> +			switch (mode) {
>> +			case ACER_WMID_FAN_MODE_AUTO:
>> +				*val = 2;
>> +				return 0;
>> +			case ACER_WMID_FAN_MODE_TURBO:
>> +				*val = 0;
>> +				return 0;
>> +			case ACER_WMID_FAN_MODE_CUSTOM:
>> +				*val = 1;
>> +				return 0;
>> +			default:
>> +				return -ENXIO;
>> +			}
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int acer_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>> +				u32 attr, int channel, long val)
>> +{
>> +	u8 fan, speed, mode_bitmap;
>> +	u16 fan_bitmap;
>> +	int mode;
>> +
>> +	switch (type) {
>> +	case hwmon_pwm:
>> +		switch (attr) {
>> +		case hwmon_pwm_input:
>> +			fan = acer_wmi_fan_channel_to_fan_id[channel];
>> +			speed = fixp_linear_interpolate(0, 0, U8_MAX, 100,
>> +							clamp_val(val, 0, U8_MAX));
>> +
>> +			return WMID_gaming_set_gaming_fan_speed(fan, speed);
>> +		case hwmon_pwm_enable:
>> +			fan_bitmap = acer_wmi_fan_channel_to_fan_bitmap[channel];
>> +
>> +			switch (val) {
>> +			case 0:
>> +				mode = ACER_WMID_FAN_MODE_TURBO;
>> +				break;
>> +			case 1:
>> +				mode = ACER_WMID_FAN_MODE_CUSTOM;
>> +				break;
>> +			case 2:
>> +				mode = ACER_WMID_FAN_MODE_AUTO;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +
>> +			switch (channel) {
>> +			case 0:
>> +				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
>> +							 mode);
>> +				break;
>> +			case 1:
>> +				mode_bitmap = FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
>> +							 mode);
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
> These too look they could be mapped inside WMID_gaming_set_fan_behavior().

I don't think so, as the function also needs to handle the setting of multiple fans
at once.

Thanks,
Armin Wolf

>> +
>> +			return WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>>   	default:
>>   		return -EOPNOTSUPP;
>>   	}
>> @@ -2908,11 +3120,16 @@ static const struct hwmon_channel_info *const acer_wmi_hwmon_info[] = {
>>   			   HWMON_F_INPUT,
>>   			   HWMON_F_INPUT
>>   			   ),
>> +	HWMON_CHANNEL_INFO(pwm,
>> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
>> +			   ),
>>   	NULL
>>   };
>>   
>>   static const struct hwmon_ops acer_wmi_hwmon_ops = {
>>   	.read = acer_wmi_hwmon_read,
>> +	.write = acer_wmi_hwmon_write,
>>   	.is_visible = acer_wmi_hwmon_is_visible,
>>   };
>>   
>>
Re: [PATCH v2 2/4] platform/x86: acer-wmi: Add fan control support
Posted by Ilpo Järvinen 4 months ago 
On Thu, 2 Oct 2025, Armin Wolf wrote:

> Am 24.09.25 um 12:52 schrieb Ilpo Järvinen:
> 
> > On Tue, 23 Sep 2025, Armin Wolf wrote:
> > 
> > > Add support for controlling the fan speed using the
> > > SetGamingFanSpeed() and GetGamingFanSpeed() WMI methods.
> > > 
> > > This feature is only enabled if the machine has ACER_CAP_PWM enabled
> > > and depend on ACER_CAP_HWMON for detecting the number of available
> > > fans.
> > > 
> > > Reviewed-by: Kurt Borja <kuurtb@gmail.com>
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   drivers/platform/x86/acer-wmi.c | 221 +++++++++++++++++++++++++++++++-
> > >   1 file changed, 219 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/acer-wmi.c
> > > b/drivers/platform/x86/acer-wmi.c
> > > index a41555ee8589..345b6e24ae31 100644
> > > --- a/drivers/platform/x86/acer-wmi.c
> > > +++ b/drivers/platform/x86/acer-wmi.c
> > > @@ -12,10 +12,12 @@
> > >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >     #include <linux/kernel.h>
> > > +#include <linux/minmax.h>
> > >   #include <linux/module.h>
> > >   #include <linux/init.h>
> > >   #include <linux/types.h>
> > >   #include <linux/dmi.h>
> > > +#include <linux/fixp-arith.h>
> > >   #include <linux/backlight.h>
> > >   #include <linux/leds.h>
> > >   #include <linux/platform_device.h>
> > > @@ -69,11 +71,16 @@ MODULE_LICENSE("GPL");
> > >   #define ACER_WMID_GET_GAMING_LED_METHODID 4
> > >   #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
> > >   #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
> > > +#define ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID 15
> > > +#define ACER_WMID_SET_GAMING_FAN_SPEED_METHODID 16
> > > +#define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
> > >   #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> > >   #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
> > >   +#define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
> > >   #define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
> > >   #define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
> > > +#define ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK GENMASK_ULL(23, 8)
> > >     #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> > >   #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> > > @@ -81,6 +88,10 @@ MODULE_LICENSE("GPL");
> > >   #define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
> > >   #define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
> > >   +#define ACER_GAMING_FAN_SPEED_STATUS_MASK GENMASK_ULL(7, 0)
> > > +#define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
> > > +#define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
> > > +
> > >   #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
> > >   #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
> > >   #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
> > > @@ -130,6 +141,11 @@ enum acer_wmi_predator_v4_sensor_id {
> > >   	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
> > >   };
> > >   +enum acer_wmi_gaming_fan_id {
> > > +	ACER_WMID_CPU_FAN	= 0x01,
> > > +	ACER_WMID_GPU_FAN	= 0x04,
> > > +};
> > In patch 1 you added these:
> > 
> > +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
> > ...
> > +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> > +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> > 
> > Are these numbers related (you even called the field "ID mask")? Could we
> > do with less defines for the same thing if they're related?
> 
> I honestly don't know if those IDs are related. I would like to play it safe
> and keep them separate for now.
> 
> > >   enum acer_wmi_gaming_fan_mode {
> > >   	ACER_WMID_FAN_MODE_AUTO		= 0x01,
> > >   	ACER_WMID_FAN_MODE_TURBO	= 0x02,
> > > @@ -292,6 +308,7 @@ struct hotkey_function_type_aa {
> > >   #define ACER_CAP_TURBO_FAN		BIT(9)
> > >   #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
> > >   #define ACER_CAP_HWMON			BIT(11)
> > > +#define ACER_CAP_PWM			BIT(12)
> > >     /*
> > >    * Interface type flags
> > > @@ -386,6 +403,7 @@ struct quirk_entry {
> > >   	u8 cpu_fans;
> > >   	u8 gpu_fans;
> > >   	u8 predator_v4;
> > > +	u8 pwm;
> > >   };
> > >     static struct quirk_entry *quirks;
> > > @@ -405,6 +423,9 @@ static void __init set_quirks(void)
> > >   	if (quirks->predator_v4)
> > >   		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
> > >   					 ACER_CAP_HWMON;
> > > +
> > > +	if (quirks->pwm)
> > > +		interface->capability |= ACER_CAP_PWM;
> > >   }
> > >     static int __init dmi_matched(const struct dmi_system_id *dmi)
> > > @@ -1651,6 +1672,27 @@ static int WMID_gaming_set_fan_behavior(u16
> > > fan_bitmap, u8 mode_bitmap)
> > >   	return 0;
> > >   }
> > >   +static int WMID_gaming_get_fan_behavior(u16 fan_bitmap, u8
> > > *mode_bitmap)
> > > +{
> > > +	acpi_status status;
> > > +	u32 input = 0;
> > > +	u64 result;
> > > +
> > > +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
> > > +	status =
> > > WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID,
> > > input,
> > > +					    &result);
> > > +	if (ACPI_FAILURE(status))
> > > +		return -EIO;
> > > +
> > > +	/* The return status must be zero for the operation to have succeeded
> > > */
> > > +	if (FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK, result))
> > > +		return -EIO;
> > > +
> > > +	*mode_bitmap = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK,
> > > result);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static void WMID_gaming_set_fan_mode(u8 fan_mode)
> > >   {
> > >   	u16 mode_bitmap = 0;
> > > @@ -1669,6 +1711,55 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
> > >   	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
> > >   }
> > >   +static int WMID_gaming_set_gaming_fan_speed(u8 fan, u8 speed)
> > > +{
> > > +	acpi_status status;
> > > +	u64 input = 0;
> > > +	u64 result;
> > > +
> > > +	if (speed > 100)
> > > +		return -EINVAL;
> > > +
> > > +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> > > +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_VALUE_MASK, speed);
> > > +
> > > +	status =
> > > WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_SPEED_METHODID, input,
> > > &result);
> > > +	if (ACPI_FAILURE(status))
> > > +		return -EIO;
> > > +
> > > +	switch (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result)) {
> > > +	case 0x00:
> > > +		return 0;
> > > +	case 0x01:
> > > +		return -ENODEV;
> > > +	case 0x02:
> > > +		return -EINVAL;
> > > +	default:
> > > +		return -ENXIO;
> > > +	}
> > > +}
> > > +
> > > +static int WMID_gaming_get_gaming_fan_speed(u8 fan, u8 *speed)
> > > +{
> > > +	acpi_status status;
> > > +	u32 input = 0;
> > > +	u64 result;
> > > +
> > > +	input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> > > +
> > > +	status =
> > > WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_SPEED_METHODID, input,
> > > +					    &result);
> > > +	if (ACPI_FAILURE(status))
> > > +		return -EIO;
> > > +
> > > +	if (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result))
> > > +		return -ENODEV;
> > > +
> > > +	*speed = FIELD_GET(ACER_GAMING_FAN_SPEED_VALUE_MASK, result);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int WMID_gaming_set_misc_setting(enum
> > > acer_wmi_gaming_misc_setting setting, u8 value)
> > >   {
> > >   	acpi_status status;
> > > @@ -2840,6 +2931,16 @@ static const enum acer_wmi_predator_v4_sensor_id
> > > acer_wmi_fan_channel_to_sensor_
> > >   	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
> > >   };
> > >   +static const enum acer_wmi_gaming_fan_id
> > > acer_wmi_fan_channel_to_fan_id[] = {
> > > +	[0] = ACER_WMID_CPU_FAN,
> > > +	[1] = ACER_WMID_GPU_FAN,
> > > +};
> > > +
> > > +static const u16 acer_wmi_fan_channel_to_fan_bitmap[] = {
> > > +	[0] = ACER_GAMING_FAN_BEHAVIOR_CPU,
> > > +	[1] = ACER_GAMING_FAN_BEHAVIOR_GPU,
> > > +};
> > > +
> > >   static umode_t acer_wmi_hwmon_is_visible(const void *data,
> > >   					 enum hwmon_sensor_types type, u32
> > > attr,
> > >   					 int channel)
> > > @@ -2851,6 +2952,11 @@ static umode_t acer_wmi_hwmon_is_visible(const void
> > > *data,
> > >   	case hwmon_temp:
> > >   		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
> > >   		break;
> > > +	case hwmon_pwm:
> > > +		if (!has_cap(ACER_CAP_PWM))
> > > +			return 0;
> > > +
> > > +		fallthrough;
> > >   	case hwmon_fan:
> > >   		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
> > >   		break;
> > > @@ -2858,8 +2964,12 @@ static umode_t acer_wmi_hwmon_is_visible(const void
> > > *data,
> > >   		return 0;
> > >   	}
> > >   -	if (*supported_sensors & BIT(sensor_id - 1))
> > > +	if (*supported_sensors & BIT(sensor_id - 1)) {
> > > +		if (type == hwmon_pwm)
> > > +			return 0644;
> > > +
> > >   		return 0444;
> > > +	}
> > >     	return 0;
> > >   }
> > > @@ -2868,8 +2978,10 @@ static int acer_wmi_hwmon_read(struct device *dev,
> > > enum hwmon_sensor_types type,
> > >   			       u32 attr, int channel, long *val)
> > >   {
> > >   	u64 command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING;
> > > +	u8 fan, speed, mode_bitmap;
> > > +	u16 fan_bitmap;
> > > +	int mode, ret;
> > >   	u64 result;
> > > -	int ret;
> > >     	switch (type) {
> > >   	case hwmon_temp:
> > > @@ -2893,6 +3005,106 @@ static int acer_wmi_hwmon_read(struct device *dev,
> > > enum hwmon_sensor_types type,
> > >     		*val =
> > > FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
> > >   		return 0;
> > > +	case hwmon_pwm:
> > > +		switch (attr) {
> > > +		case hwmon_pwm_input:
> > > +			fan = acer_wmi_fan_channel_to_fan_id[channel];
> > > +			ret = WMID_gaming_get_gaming_fan_speed(fan, &speed);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			*val = fixp_linear_interpolate(0, 0, 100, U8_MAX,
> > > speed);
> > > +			return 0;
> > > +		case hwmon_pwm_enable:
> > > +			fan_bitmap =
> > > acer_wmi_fan_channel_to_fan_bitmap[channel];
> > > +			ret = WMID_gaming_get_fan_behavior(fan_bitmap,
> > > &mode_bitmap);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			switch (channel) {
> > > +			case 0:
> > > +				mode =
> > > FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> > > +						 mode_bitmap);
> > > +				break;
> > > +			case 1:
> > > +				mode =
> > > FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> > > +						 mode_bitmap);
> > > +				break;
> > > +			default:
> > > +				return -EINVAL;
> > > +			}
> > You've managed to create an internal API that resembles the WMI method
> > input by working in terms of fan_bitmap and mode_bitmap which makes things
> > complicated as then you have to build logic on top of it to do all the
> > mapping in the caller...
> > 
> > I suggest changing this so you make WMID_gaming_get_fan_behavior() to
> > input ACER_GAMING_FAN_BEHAVIOR_CPU / GPU (or ACER_WMID_CPU_FAN / GPU_FAN
> > depending on the question I asked above) and perform the output mapping
> > within that function. I think it would be much cleaner.
> > 
> > I didn't exactly like the internal API in patch 1 either and I'd prefer
> > the field GENMASK()s to directly point to the correct bits instead of the
> > nested FIELD_GET/PREP()s.
> 
> Ok, will do.
> 
> > > +			switch (mode) {
> > > +			case ACER_WMID_FAN_MODE_AUTO:
> > > +				*val = 2;
> > > +				return 0;
> > > +			case ACER_WMID_FAN_MODE_TURBO:
> > > +				*val = 0;
> > > +				return 0;
> > > +			case ACER_WMID_FAN_MODE_CUSTOM:
> > > +				*val = 1;
> > > +				return 0;
> > > +			default:
> > > +				return -ENXIO;
> > > +			}
> > > +		default:
> > > +			return -EOPNOTSUPP;
> > > +		}
> > > +	default:
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +}
> > > +
> > > +static int acer_wmi_hwmon_write(struct device *dev, enum
> > > hwmon_sensor_types type,
> > > +				u32 attr, int channel, long val)
> > > +{
> > > +	u8 fan, speed, mode_bitmap;
> > > +	u16 fan_bitmap;
> > > +	int mode;
> > > +
> > > +	switch (type) {
> > > +	case hwmon_pwm:
> > > +		switch (attr) {
> > > +		case hwmon_pwm_input:
> > > +			fan = acer_wmi_fan_channel_to_fan_id[channel];
> > > +			speed = fixp_linear_interpolate(0, 0, U8_MAX, 100,
> > > +							clamp_val(val, 0,
> > > U8_MAX));
> > > +
> > > +			return WMID_gaming_set_gaming_fan_speed(fan, speed);
> > > +		case hwmon_pwm_enable:
> > > +			fan_bitmap =
> > > acer_wmi_fan_channel_to_fan_bitmap[channel];
> > > +
> > > +			switch (val) {
> > > +			case 0:
> > > +				mode = ACER_WMID_FAN_MODE_TURBO;
> > > +				break;
> > > +			case 1:
> > > +				mode = ACER_WMID_FAN_MODE_CUSTOM;
> > > +				break;
> > > +			case 2:
> > > +				mode = ACER_WMID_FAN_MODE_AUTO;
> > > +				break;
> > > +			default:
> > > +				return -EINVAL;
> > > +			}
> > > +
> > > +			switch (channel) {
> > > +			case 0:
> > > +				mode_bitmap =
> > > FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> > > +							 mode);
> > > +				break;
> > > +			case 1:
> > > +				mode_bitmap =
> > > FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> > > +							 mode);
> > > +				break;
> > > +			default:
> > > +				return -EINVAL;
> > > +			}
> > These too look they could be mapped inside WMID_gaming_set_fan_behavior().
> 
> I don't think so, as the function also needs to handle the setting of multiple
> fans
> at once.

Oh, right. How about making it two functions then, one which is called 
from here to set one fan and which then internally calls into the 
function that can set any fans?


-- 
 i.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.