Re: [RFC PATCH 2/3] platform/x86: acer-wmi: Add fan control support

[Armin Wolf <W_Armin@gmx.de>]

Am 15.02.25 um 02:33 schrieb Kurt Borja:

> On Fri Feb 14, 2025 at 5:13 PM -05, 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.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/acer-wmi.c | 222 +++++++++++++++++++++++++++++++-
>>   1 file changed, 220 insertions(+), 2 deletions(-)
>>
>> --
>> 2.39.5
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index f20a882e3650..e24f5a323f95 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>
> I didn't know about this, thanks!
>
>>   #include <linux/backlight.h>
>>   #include <linux/leds.h>
>>   #include <linux/platform_device.h>
>> @@ -30,6 +32,7 @@
>>   #include <linux/input/sparse-keymap.h>
>>   #include <acpi/video.h>
>>   #include <linux/hwmon.h>
>> +#include <linux/unaligned.h>
> Duplicated include.
>
>>   #include <linux/units.h>
>>   #include <linux/unaligned.h>
>>   #include <linux/bitfield.h>
> ...
>> @@ -2867,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:
>> @@ -2892,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_mode:
> hwmon_pwm_enable?
>
> Other than that:
>
> Reviewed-by: Kurt Borja <kuurtb@gmail.com>

Thanks for spotting this mistakes, i send an updated series without those flaws.

Thanks,
Armin Wolf

>> <snip>