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 > > > Signed-off-by: Armin Wolf > > > --- > > > 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 > > > +#include > > > #include > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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.