Re: [PATCH v2 1/2] platform/x86: hp-wmi: add manual fan control for Victus S models

[Krishna Chomal <krishna.chomal108@gmail.com>]

On Mon, Jan 12, 2026 at 05:13:05PM +0200, Ilpo Järvinen wrote:
>On Tue, 30 Dec 2025, Krishna Chomal wrote:
>
[snip]
>>  #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>
>>
[snip]
>> +
>> +struct victus_s_fan_table_header {
>> +	u8 unknown;
>> +	u8 num_entries;
>> +} __packed;
>
>Please add #include for __packed.
>

__packed is defined in compiler_attributes.h, which is included in this
patch. Please let me know if there are any other headers that should be
included.

>> +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.
>

Apologies for the bad alignment of multi-line function calls and
if-statements in this patch. I have noted them and will fix them all in
v3.

[snip]
>> -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);
>	}
>

Understood. I will introduce a new "gpu_speed" variable and follow this
style in v3.

[snip]
>> @@ -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.
>

Will do.

>> +			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.
>

Understood. I will directly use current_rpm then.

[snip]
>>  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).
>

Corrected.

>> +			if (priv->mode != PWM_MODE_MANUAL)
>> +				return -EINVAL;
>
>ADd empty line here.
>

Added.

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

Fixed.

>> +			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.
>

Agreed, this is too much nesting. I will split it into separate statements
for each function call.

>> +						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 ?
>

Yes, will do in v3.

>> +			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).
>

Ok, will fix the alignment and ensure to keep the > in a single line.