Message-ID: <ce48f7b8-7d88-266f-ca8d-6af3b01815db@linux.intel.com>
Date: Mon, 12 Jan 2026 17:13:05 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Krishna Chomal <krishna.chomal108@...il.com>
cc: Hans de Goede <hansg@...nel.org>, linux@...ck-us.net, 
    platform-driver-x86@...r.kernel.org, linux-hwmon@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] platform/x86: hp-wmi: add manual fan control for
 Victus S models

On Tue, 30 Dec 2025, Krishna Chomal wrote:

> Add manual fan speed control and PWM reporting for HP Victus S-series
> laptops.
> 
> While HPWMI_FAN_SPEED_SET_QUERY was previously added to reset max fan
> mode, it is actually capable of individual fan control. This patch
> implements hp_wmi_fan_speed_set() to allow manual control and hides
> PWM inputs for non-Victus devices as the query is Victus specific.
> 
> The existing hp_wmi_fan_speed_max_get() query is unreliable on Victus S
> firmware, often incorrectly reporting "Auto" mode even when "Max" is
> active. To resolve this synchronization issue, move state tracking to
> a per-device private context and apply "Auto" mode during driver
> initialization to ensure a consistent starting point.
> 
> Refactor hp_wmi_apply_fan_settings() to use an intermediate ret
> variable. This prepares the switch block for keep-alive logic being
> added in a later patch, avoiding the need for duplicated mode check.
> 
> Tested on: HP Omen 16-wf1xxx (board ID 8C78)
> 
> Signed-off-by: Krishna Chomal <krishna.chomal108@...il.com>
> ---
> Changes in v2:
> - Added limits.h and minmax.h headers
> - Re-written pwm-to-rpm (and vice-versa) conversion from macros to
>   static inline functions
> - Removed redundant NULL checking of drvdata
> - Made integer promotion explicit by casting intermediate result to
>   unsigned int when calculating the GPU fan speed
> - Renamed "enforce_ctx" to "apply_fan_settings" for clarity
> - Removed unnecessary comments
> - Renamed "ctx" to "priv" as it is more consistent for drvdata
> - Added new struct victus_s_fan_table to avoid complex type casting
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 263 +++++++++++++++++++++++++------
>  1 file changed, 218 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index f4ea1ea05997..ef419575174c 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -30,6 +30,10 @@
>  #include <linux/rfkill.h>
>  #include <linux/string.h>
>  #include <linux/dmi.h>
> +#include <linux/fixp-arith.h>
> +#include <linux/limits.h>
> +#include <linux/minmax.h>
> +#include <linux/compiler_attributes.h>
>  
>  MODULE_AUTHOR("Matthew Garrett <mjg59@...f.ucam.org>");
>  MODULE_DESCRIPTION("HP laptop WMI driver");
> @@ -190,7 +194,8 @@ enum hp_wmi_gm_commandtype {
>  	HPWMI_SET_GPU_THERMAL_MODES_QUERY	= 0x22,
>  	HPWMI_SET_POWER_LIMITS_QUERY		= 0x29,
>  	HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY	= 0x2D,
> -	HPWMI_FAN_SPEED_SET_QUERY		= 0x2E,
> +	HPWMI_VICTUS_S_FAN_SPEED_SET_QUERY	= 0x2E,
> +	HPWMI_VICTUS_S_GET_FAN_TABLE_QUERY	= 0x2F,
>  };
>  
>  enum hp_wmi_command {
> @@ -348,6 +353,51 @@ static const char * const tablet_chassis_types[] = {
>  
>  #define DEVICE_MODE_TABLET	0x06
>  
> +#define CPU_FAN 0
> +#define GPU_FAN 1
> +
> +enum pwm_modes {
> +	PWM_MODE_MAX		= 0,
> +	PWM_MODE_MANUAL		= 1,
> +	PWM_MODE_AUTO		= 2,
> +};
> +
> +struct hp_wmi_hwmon_priv {
> +	u8 min_rpm;
> +	u8 max_rpm;
> +	u8 gpu_delta;
> +	u8 mode;
> +	u8 pwm;
> +};
> +
> +struct victus_s_fan_table_header {
> +	u8 unknown;
> +	u8 num_entries;
> +} __packed;

Please add #include for __packed.

> +struct victus_s_fan_table_entry {
> +	u8 cpu_rpm;
> +	u8 gpu_rpm;
> +	u8 unknown;
> +} __packed;
> +
> +struct victus_s_fan_table {
> +	struct victus_s_fan_table_header header;
> +	struct victus_s_fan_table_entry entries[];
> +} __packed;
> +
> +static inline u8 rpm_to_pwm(u8 rpm, struct hp_wmi_hwmon_priv *priv)
> +{
> +	return fixp_linear_interpolate(0, 0, priv->max_rpm, U8_MAX,
> +					clamp_val(rpm, 0, priv->max_rpm));

Please align the correctly.

> +}
> +
> +static inline u8 pwm_to_rpm(u8 pwm, struct hp_wmi_hwmon_priv *priv)
> +{
> +	return fixp_linear_interpolate(0, 0, U8_MAX, priv->max_rpm,
> +					clamp_val(pwm, 0, U8_MAX));
> +}
> +
>  /* map output size to the corresponding WMI method id */
>  static inline int encode_outsize_for_pvsz(int outsize)
>  {
> @@ -637,41 +687,53 @@ static int hp_wmi_fan_speed_max_set(int enabled)
>  	return enabled;
>  }
>  
> -static int hp_wmi_fan_speed_reset(void)
> +static int hp_wmi_fan_speed_set(struct hp_wmi_hwmon_priv *priv, u8 speed)
>  {
> -	u8 fan_speed[2] = { HP_FAN_SPEED_AUTOMATIC, HP_FAN_SPEED_AUTOMATIC };
> +	u8 fan_speed[2];
>  	int ret;
>  
> -	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_SET_QUERY, HPWMI_GM,
> -				   &fan_speed, sizeof(fan_speed), 0);
> -
> -	return ret;
> -}
> +	fan_speed[CPU_FAN] = speed;
> +	fan_speed[GPU_FAN] = speed;
>  
> -static int hp_wmi_fan_speed_max_reset(void)
> -{
> -	int ret;
> +	/*
> +	 * GPU fan speed is always a little higher than CPU fan speed, we fetch
> +	 * this delta value from the fan table during hwmon init.
> +	 * Exception: Speed is set to HP_FAN_SPEED_AUTOMATIC, to revert to
> +	 * automatic mode.
> +	 */
> +	if (speed != HP_FAN_SPEED_AUTOMATIC)
> +		fan_speed[GPU_FAN] = clamp_val((unsigned int)speed +
> +						(unsigned int)priv->gpu_delta,
> +						0, U8_MAX);

Add braces is it's multiline if.

If you use unsigned int, clamp to 0 makes no sense as those values have 
already underflowed.

You also have an alignment problem here, but this seems a cleaner way 
which doesn't have underflow issues:

	if (...) {
		int new_speed = speed + priv->gpu_delta;

		fan_speed[GPU_FAN] = clamp_val(new_speed, 0, U8_MAX);
	}

>  
> +	ret = hp_wmi_get_fan_count_userdefine_trigger();
> +	if (ret < 0)
> +		return ret;
> +	/* Max fans need to be explicitly disabled */
>  	ret = hp_wmi_fan_speed_max_set(0);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
> +	ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_SET_QUERY, HPWMI_GM,
> +				   &fan_speed, sizeof(fan_speed), 0);
>  
> -	/* Disabling max fan speed on Victus s1xxx laptops needs a 2nd step: */
> -	ret = hp_wmi_fan_speed_reset();
>  	return ret;
>  }
>  
> -static int hp_wmi_fan_speed_max_get(void)
> +static int hp_wmi_fan_speed_reset(struct hp_wmi_hwmon_priv *priv)
>  {
> -	int val = 0, ret;
> +	return hp_wmi_fan_speed_set(priv, HP_FAN_SPEED_AUTOMATIC);
> +}
>  
> -	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
> -				   &val, zero_if_sup(val), sizeof(val));
> +static int hp_wmi_fan_speed_max_reset(struct hp_wmi_hwmon_priv *priv)
> +{
> +	int ret;
>  
> +	ret = hp_wmi_fan_speed_max_set(0);
>  	if (ret)
> -		return ret < 0 ? ret : -EINVAL;
> +		return ret;
>  
> -	return val;
> +	/* Disabling max fan speed on Victus s1xxx laptops needs a 2nd step: */
> +	return hp_wmi_fan_speed_reset(priv);
>  }
>  
>  static int __init hp_wmi_bios_2008_later(void)
> @@ -2108,12 +2170,45 @@ static struct platform_driver hp_wmi_driver __refdata = {
>  	.remove = __exit_p(hp_wmi_bios_remove),
>  };
>  
> +static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
> +{
> +	int ret;
> +
> +	switch (priv->mode) {
> +	case PWM_MODE_MAX:
> +		if (is_victus_s_thermal_profile())
> +			hp_wmi_get_fan_count_userdefine_trigger();
> +		ret = hp_wmi_fan_speed_max_set(1);
> +		return ret;
> +	case PWM_MODE_MANUAL:
> +		if (!is_victus_s_thermal_profile())
> +			return -EOPNOTSUPP;
> +		ret = hp_wmi_fan_speed_set(priv, pwm_to_rpm(priv->pwm, priv));
> +		return ret;
> +	case PWM_MODE_AUTO:
> +		if (is_victus_s_thermal_profile()) {
> +			hp_wmi_get_fan_count_userdefine_trigger();
> +			ret = hp_wmi_fan_speed_max_reset(priv);
> +		} else {
> +			ret = hp_wmi_fan_speed_max_set(0);
> +		}
> +		return ret;
> +	default:
> +		/* shouldn't happen */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static umode_t hp_wmi_hwmon_is_visible(const void *data,
>  				       enum hwmon_sensor_types type,
>  				       u32 attr, int channel)
>  {
>  	switch (type) {
>  	case hwmon_pwm:
> +		if (attr == hwmon_pwm_input && !is_victus_s_thermal_profile())
> +			return 0;
>  		return 0644;
>  	case hwmon_fan:
>  		if (is_victus_s_thermal_profile()) {
> @@ -2134,8 +2229,10 @@ static umode_t hp_wmi_hwmon_is_visible(const void *data,
>  static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			     u32 attr, int channel, long *val)
>  {
> -	int ret;
> +	struct hp_wmi_hwmon_priv *priv;
> +	int current_rpm, ret;
>  
> +	priv = dev_get_drvdata(dev);
>  	switch (type) {
>  	case hwmon_fan:
>  		if (is_victus_s_thermal_profile())
> @@ -2147,16 +2244,21 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  		*val = ret;
>  		return 0;
>  	case hwmon_pwm:
> -		switch (hp_wmi_fan_speed_max_get()) {
> -		case 0:
> -			/* 0 is automatic fan, which is 2 for hwmon */
> -			*val = 2;
> +		if (attr == hwmon_pwm_input) {
> +			if (!is_victus_s_thermal_profile())
> +				return -EOPNOTSUPP;

Add an empty line here.

> +			ret = hp_wmi_get_fan_speed_victus_s(channel);
> +			if (ret < 0)
> +				return ret;
> +			current_rpm = ret;

I'm not sure if using ret here makes things better or not, I'd prefer just 
assigning directly to current_rpm without ret as intermediate var.

> +			*val = rpm_to_pwm(current_rpm / 100, priv);
>  			return 0;
> -		case 1:
> -			/* 1 is max fan, which is 0
> -			 * (no fan speed control) for hwmon
> -			 */
> -			*val = 0;
> +		}
> +		switch (priv->mode) {
> +		case PWM_MODE_MAX:
> +		case PWM_MODE_MANUAL:
> +		case PWM_MODE_AUTO:
> +			*val = priv->mode;
>  			return 0;
>  		default:
>  			/* shouldn't happen */
> @@ -2170,23 +2272,47 @@ 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)
>  {
> +	struct hp_wmi_hwmon_priv *priv;
> +	int current_rpm, ret;
> +
> +	priv = dev_get_drvdata(dev);
>  	switch (type) {
>  	case hwmon_pwm:
> +		if (attr == hwmon_pwm_input) {
> +			if (!is_victus_s_thermal_profile())
> +				return -EOPNOTSUPP;
> +			/* pwm input is invalid when not in manual mode */

PWM (capitalize textual/comment "pwm"s correctly please).

> +			if (priv->mode != PWM_MODE_MANUAL)
> +				return -EINVAL;

ADd empty line here.

> +			/* ensure pwm input is within valid fan speeds */

PWM

> +			priv->pwm = rpm_to_pwm(clamp_val(pwm_to_rpm(val, priv),
> +							priv->min_rpm,
> +							priv->max_rpm),

These look misaligned.

I suggest you split this to multiple lines though, it will likely be 
easier to read that way.

> +						priv);
> +			return hp_wmi_apply_fan_settings(priv);
> +		}
>  		switch (val) {
> -		case 0:
> -			if (is_victus_s_thermal_profile())
> -				hp_wmi_get_fan_count_userdefine_trigger();
> -			/* 0 is no fan speed control (max), which is 1 for us */
> -			return hp_wmi_fan_speed_max_set(1);
> -		case 2:
> -			/* 2 is automatic speed control, which is 0 for us */
> -			if (is_victus_s_thermal_profile()) {
> -				hp_wmi_get_fan_count_userdefine_trigger();
> -				return hp_wmi_fan_speed_max_reset();
> -			} else
> -				return hp_wmi_fan_speed_max_set(0);
> +		case PWM_MODE_MAX:
> +			priv->mode = PWM_MODE_MAX;
> +			return hp_wmi_apply_fan_settings(priv);
> +		case PWM_MODE_MANUAL:
> +			if (!is_victus_s_thermal_profile())
> +				return -EOPNOTSUPP;
> +			/*
> +			 * When switching to manual mode, set fan speed to
> +			 * current RPM values to ensure a smooth transition.
> +			 */
> +			ret = hp_wmi_get_fan_speed_victus_s(channel);

Assign directly to current_rpm ?

> +			if (ret < 0)
> +				return ret;
> +			current_rpm = ret;
> +			priv->pwm = rpm_to_pwm(current_rpm / 100, priv);
> +			priv->mode = PWM_MODE_MANUAL;
> +			return hp_wmi_apply_fan_settings(priv);
> +		case PWM_MODE_AUTO:
> +			priv->mode = PWM_MODE_AUTO;
> +			return hp_wmi_apply_fan_settings(priv);
>  		default:
> -			/* we don't support manual fan speed control */
>  			return -EINVAL;
>  		}
>  	default:
> @@ -2196,7 +2322,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_ENABLE | HWMON_PWM_INPUT),
>  	NULL
>  };
>  
> @@ -2211,12 +2337,57 @@ static const struct hwmon_chip_info chip_info = {
>  	.info = info,
>  };
>  
> +static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
> +{
> +	u8 fan_data[128] = { 0 };
> +	struct victus_s_fan_table *fan_table;
> +	u8 min_rpm, max_rpm, gpu_delta;
> +	int ret;
> +
> +	/* Default behaviour on hwmon init is automatic mode */
> +	priv->mode = PWM_MODE_AUTO;
> +
> +	/* Bypass all non-Victus S devices */
> +	if (!is_victus_s_thermal_profile())
> +		return 0;
> +
> +	ret = hp_wmi_perform_query(HPWMI_VICTUS_S_GET_FAN_TABLE_QUERY,
> +				   HPWMI_GM, &fan_data, 4, sizeof(fan_data));
> +	if (ret)
> +		return ret;
> +
> +	fan_table = (struct victus_s_fan_table *)fan_data;
> +	if (fan_table->header.num_entries == 0 ||
> +		sizeof(struct victus_s_fan_table_header) +
> +		sizeof(struct victus_s_fan_table_entry) * fan_table->header.num_entries >
> +		sizeof(fan_data))

Badly misaligned.

Splitting at > is somewhat misleading so I'd prefer to avoid it (you can 
use up to 100 chars per line if needed).

> +		return -EINVAL;
> +
> +	min_rpm = fan_table->entries[0].cpu_rpm;
> +	max_rpm = fan_table->entries[fan_table->header.num_entries - 1].cpu_rpm;
> +	gpu_delta = fan_table->entries[0].gpu_rpm - fan_table->entries[0].cpu_rpm;
> +	priv->min_rpm = min_rpm;
> +	priv->max_rpm = max_rpm;
> +	priv->gpu_delta = gpu_delta;
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void)
>  {
>  	struct device *dev = &hp_wmi_platform_dev->dev;
> +	struct hp_wmi_hwmon_priv *priv;
>  	struct device *hwmon;
> +	int ret;
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, "hp", &hp_wmi_driver,
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = hp_wmi_setup_fan_settings(priv);
> +	if (ret)
> +		return ret;
> +	hwmon = devm_hwmon_device_register_with_info(dev, "hp", priv,
>  						     &chip_info, NULL);
>  
>  	if (IS_ERR(hwmon)) {
> @@ -2224,6 +2395,8 @@ static int hp_wmi_hwmon_init(void)
>  		return PTR_ERR(hwmon);
>  	}
>  
> +	hp_wmi_apply_fan_settings(priv);
> +
>  	return 0;
>  }
>  
> 

-- 
 i.

Powered by blists - more mailing lists