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

[Armin Wolf <W_Armin@xxxxxx>]

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@xxxxxx>
> > ---
> >   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@xxxxxxxxx>

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

Thanks,
Armin Wolf

> > <snip>