Re: [PATCH 2/2] platform/x86: hp-wmi: Add ACPI PWM fan control for HP ENVY 13-bd0xxx

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 29 Mar 2026, Bani Brata wrote:

> On the HP ENVY x360 Convertible 13-bd0xxx (board 8824), standard WMI
> methods for fan speed control and RPM reporting do not function.
> 
> However, the DSDT exposes ACPI methods (\_SB.PC00.LPCB.EC0.FANG and FANW)
> that allow reading and writing an 8-bit PWM level (0-255) to the EC.
> 
> This patch maps these ACPI methods to standard hwmon `pwm1` and
> `pwm1_enable` nodes, allowing userspace tools to successfully control
> the fan speed.
> 
> Signed-off-by: Bani Brata <banibrata2007@gmail.com>
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 155 ++++++++++++++++++++++++++++++-
>  1 file changed, 155 insertions(+)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index XXXXXXX..XXXXXXX 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -48,6 +48,14 @@
>  #define HP_FAN_SPEED_AUTOMATIC	 0x00
>  #define HP_POWER_LIMIT_DEFAULT	 0x00
>  #define HP_POWER_LIMIT_NO_CHANGE 0xFF
> +#define HP_ENVY_8824_ACPI_FANG "\\_SB.PC00.LPCB.EC0.FANG"
> +#define HP_ENVY_8824_ACPI_FANW "\\_SB.PC00.LPCB.EC0.FANW"
> +#define HP_ENVY_8824_FAN_VALUE_ARG 33026ULL
> +#define HP_ENVY_8824_FAN_PRIME_ARG 33030ULL
> +#define HP_ENVY_8824_FAN_PRIME_MANUAL 0xFFULL
> +#define HP_ENVY_8824_FAN_PRIME_AUTO 0x8ULL
> +#define HP_ENVY_8824_PWM_MODE_MANUAL 1
> +#define HP_ENVY_8824_PWM_MODE_AUTO 2
>  
>  #define ACPI_AC_CLASS "ac_adapter"
>  
> @@ -193,6 +201,8 @@ static const struct dmi_system_id hp_wmi_rfkill_dmi_table[] __initconst = {
>  
>  static bool is_victus_s_board;
>  static bool is_envy_x360_8824_board;
> +static DEFINE_MUTEX(hp_envy_x360_8824_fan_lock);
> +static int hp_envy_x360_8824_pwm_mode = HP_ENVY_8824_PWM_MODE_AUTO;
>  
>  static const struct dmi_system_id hp_envy_x360_8824_boards[] __initconst = {
>  	{
> @@ -518,6 +528,120 @@ static int hp_wmi_get_fan_speed_victus_s(int fan)
>  	return ret;
>  }
>  
> +static int hp_envy_x360_8824_acpi_eval(const char *path,
> +				       const unsigned long long *args,
> +				       size_t arg_count,
> +				       unsigned long long *value)
> +{
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object params[2] = { };
> +	struct acpi_object_list input = { .count = arg_count, .pointer = params };
> +	union acpi_object *obj;
> +	acpi_status status;
> +	size_t i;
> +
> +	for (i = 0; i < arg_count; i++) {
> +		params[i].type = ACPI_TYPE_INTEGER;
> +		params[i].integer.value = args[i];
> +	}
> +
> +	status = acpi_evaluate_object(NULL, (acpi_string)path,
> +				      arg_count ? &input : NULL, &output);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	obj = output.pointer;

Please move variable declaration here and use __free() so you don't need 
to do manual kfree()s.

> +	if (!obj)
> +		return -ENODATA;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		kfree(obj);
> +		return -EINVAL;
> +	}
> +
> +	*value = obj->integer.value;
> +	kfree(obj);
> +
> +	return 0;
> +}
> +
> +static int hp_envy_x360_8824_get_pwm_locked(void)
> +{
> +	unsigned long long args[] = { HP_ENVY_8824_FAN_VALUE_ARG };
> +	unsigned long long value;
> +	int ret;
> +
> +	ret = hp_envy_x360_8824_acpi_eval(HP_ENVY_8824_ACPI_FANG, args,
> +					 ARRAY_SIZE(args), &value);
> +	if (ret)
> +		return ret;
> +	if (value > U8_MAX)
> +		return -EINVAL;
> +
> +	return value;
> +}
> +
> +static int hp_envy_x360_8824_set_pwm_locked(u8 pwm)
> +{
> +	unsigned long long args[] = {
> +		HP_ENVY_8824_FAN_PRIME_ARG,
> +		HP_ENVY_8824_FAN_PRIME_MANUAL,
> +	};
> +	unsigned long long value;
> +	int ret;
> +
> +	ret = hp_envy_x360_8824_acpi_eval(HP_ENVY_8824_ACPI_FANW, args,
> +					 ARRAY_SIZE(args), &value);
> +	if (ret)
> +		return ret;
> +
> +	args[0] = HP_ENVY_8824_FAN_VALUE_ARG;
> +	args[1] = pwm;
> +	ret = hp_envy_x360_8824_acpi_eval(HP_ENVY_8824_ACPI_FANW, args,
> +					 ARRAY_SIZE(args), &value);
> +	if (ret)
> +		return ret;
> +
> +	hp_envy_x360_8824_pwm_mode = HP_ENVY_8824_PWM_MODE_MANUAL;
> +	return 0;
> +}
> +
> +static int hp_envy_x360_8824_set_auto_locked(void)
> +{
> +	unsigned long long args[] = {
> +		HP_ENVY_8824_FAN_PRIME_ARG,
> +		HP_ENVY_8824_FAN_PRIME_AUTO,
> +	};
> +	unsigned long long value;
> +	int ret;
> +
> +	ret = hp_envy_x360_8824_acpi_eval(HP_ENVY_8824_ACPI_FANW, args,
> +					 ARRAY_SIZE(args), &value);
> +	if (ret)
> +		return ret;
> +
> +	args[0] = HP_ENVY_8824_FAN_VALUE_ARG;
> +	args[1] = HP_ENVY_8824_FAN_PRIME_MANUAL;
> +	ret = hp_envy_x360_8824_acpi_eval(HP_ENVY_8824_ACPI_FANW, args,
> +					 ARRAY_SIZE(args), &value);
> +	if (ret)
> +		return ret;
> +
> +	hp_envy_x360_8824_pwm_mode = HP_ENVY_8824_PWM_MODE_AUTO;
> +	return 0;
> +}
> +
> +static int hp_envy_x360_8824_set_manual_locked(void)
> +{
> +	int pwm;
> +
> +	pwm = hp_envy_x360_8824_get_pwm_locked();
> +	if (pwm < 0)
> +		return pwm;
> +
> +	return hp_envy_x360_8824_set_pwm_locked(pwm);
> +}
> +
>  /*
>   * Calling this hp_wmi_get_fan_count_userdefine_trigger function also enables
>   * and/or maintains the laptop in user defined thermal and fan states, instead
> @@ -2103,6 +2227,12 @@ static void hp_wmi_rfkill_setup(struct platform_device *device)
>  		rfkill_unregister(wifi_rfkill);
>  		rfkill_destroy(wifi_rfkill);
>  	}
> +
> +	if (is_envy_x360_8824_board) {
> +		mutex_lock(&hp_envy_x360_8824_fan_lock);
> +		hp_envy_x360_8824_set_auto_locked();
> +		mutex_unlock(&hp_envy_x360_8824_fan_lock);

Use guard().

> +	}
>  	if (bluetooth_rfkill) {
>  		rfkill_unregister(bluetooth_rfkill);
>  		rfkill_destroy(bluetooth_rfkill);
> @@ -2176,6 +2306,10 @@ static umode_t hp_wmi_hwmon_is_visible(const void *data,
>  {
>  	switch (type) {
>  	case hwmon_pwm:
> +		if (attr == hwmon_pwm_input)
> +			return is_envy_x360_8824_board ? 0644 : 0;
> +		if (attr == hwmon_pwm_enable)
> +			return is_envy_x360_8824_board ? 0644 : 0;
>  		return 0644;
>  	case hwmon_fan:
>  		if (is_envy_x360_8824_board)
> @@ -2213,6 +2347,25 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  		*val = ret;
>  		return 0;
>  	case hwmon_pwm:
> +		if (is_envy_x360_8824_board) {
> +			mutex_lock(&hp_envy_x360_8824_fan_lock);
> +			if (attr == hwmon_pwm_input) {

Why you need to do this under lock?

Also, use guard() instead of lock/unlock pair.

> +				ret = hp_envy_x360_8824_get_pwm_locked();
> +				if (!ret)
> +					*val = ret;
> +				else if (ret > 0)
> +					*val = ret;

Aren't these same as ret >= 0. However, with guard(), you can reverse the 
logic and return error immediately so I don't think you need other if()s 
at all.

> +				mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +				return ret < 0 ? ret : 0;
> +			}
> +			if (attr == hwmon_pwm_enable) {
> +				*val = hp_envy_x360_8824_pwm_mode;
> +				mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +				return 0;
> +			}
> +			mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +			return -EOPNOTSUPP;
> +		}
>  		switch (hp_wmi_fan_speed_max_get()) {
>  		case 0:
>  			/* 0 is automatic fan, which is 2 for hwmon */
> @@ -2236,8 +2389,40 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>  			      u32 attr, int channel, long val)
>  {
> +	int ret;
> +
>  	switch (type) {
>  	case hwmon_pwm:
> +		if (is_envy_x360_8824_board) {
> +			mutex_lock(&hp_envy_x360_8824_fan_lock);
> +			switch (attr) {
> +			case hwmon_pwm_input:
> +				if (val < 0 || val > U8_MAX) {
> +					mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +					return -EINVAL;
> +				}

Here too, isn't it enough to begin the critical section only here?

Again, please use guard() instead.

> +				ret = hp_envy_x360_8824_set_pwm_locked(val);
> +				mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +				return ret;
> +			case hwmon_pwm_enable:
> +				switch (val) {
> +				case HP_ENVY_8824_PWM_MODE_MANUAL:
> +					ret = hp_envy_x360_8824_set_manual_locked();
> +					break;
> +				case HP_ENVY_8824_PWM_MODE_AUTO:
> +					ret = hp_envy_x360_8824_set_auto_locked();
> +					break;
> +				default:
> +					ret = -EINVAL;
> +					break;
> +				}
> +				mutex_unlock(&hp_envy_x360_8824_fan_lock);
> +				return ret;
> +			default:
> +				mutex_unlock(&hp_envy_x360_8824_fan_lock);

This default path doesn't require taking the mutex.

> +				return -EOPNOTSUPP;
> +			}
> +		}
>  		switch (val) {
>  		case 0:
>  			if (is_victus_s_thermal_profile())
> @@ -2262,7 +2447,7 @@ static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>  
>  static const struct hwmon_channel_info * const info[] = {
>  	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
> -	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>  	NULL
>  };
>  
> 

-- 
 i.