Message-ID: <aVPe3jntoH7EqHcy@archlinux>
Date: Tue, 30 Dec 2025 19:52:15 +0530
From: Krishna Chomal <krishna.chomal108@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: hansg@...nel.org, linux@...ck-us.net, 
	platform-driver-x86@...r.kernel.org, linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] platform/x86: hp-wmi: add manual fan control for
 Victus S models

On Mon, Dec 29, 2025 at 03:08:16PM +0200, Ilpo Järvinen wrote:
[snip]
>> +
>> +#define RPM_TO_PWM(rpm, ctx) fixp_linear_interpolate(0, 0, \
>> +						(ctx)->max_rpm, U8_MAX, \
>
>+ limits.h
>
>> +						clamp_val((rpm), \
>
>+ minmax.h
>

Added in v2.

>> +						0, (ctx)->max_rpm))
>> +#define PWM_TO_RPM(pwm, ctx) fixp_linear_interpolate(0, 0, \
>> +						U8_MAX, (ctx)->max_rpm, \
>> +						clamp_val((pwm), \
>> +						0, U8_MAX))
>
>These look not needing to be macros at all but could be written as static
>functions which makes typing explicit.

Fixed. I have converted both of them into static inlines.

>> +
>>  /* map output size to the corresponding WMI method id */
>>  static inline int encode_outsize_for_pvsz(int outsize)
>>  {
>> @@ -637,41 +665,55 @@ 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 *ctx, 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);
>> +	if (!ctx)
>> +		return -ENODEV;
>
>Can this be NULL? Is it a bug in the driver/kernel if it is?
>

No, even I think this is redundant check. Removed in v2.

>> -	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(speed + ctx->gpu_delta, 0, U8_MAX);
>
>So this only works correctly due to C's integer promotion when doing
>arithmetic on them?
>

Yes, it relies on promotion, but for clarity I have added explicit cast
to unsigned int.

[snip]
>> +static int hp_wmi_fan_speed_max_reset(struct hp_wmi_hwmon_priv *ctx)
>> +{
>> +	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: */
>> +	ret = hp_wmi_fan_speed_reset(ctx);
>> +	return ret;
>
>Return can be done directly on the line with the call.
>

Fixed.

>>  }
>>
>>  static int __init hp_wmi_bios_2008_later(void)
>> @@ -2108,12 +2150,43 @@ static struct platform_driver hp_wmi_driver __refdata = {
>>  	.remove = __exit_p(hp_wmi_bios_remove),
>>  };
>>
>> +static int hp_wmi_hwmon_enforce_ctx(struct hp_wmi_hwmon_priv *ctx)
>
>I don't understand why this function is named as "enforce context".
>Perhaps change function's name to something that better describes what it
>does.
>

I have renamed "enforce_ctx" to "apply_fan_settings" in v2. That seems
like a more descriptive and self-explanatory name.

>> +{
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	switch (ctx->mode) {
>> +	case PWM_MODE_MAX:
>> +		if (is_victus_s_thermal_profile())
>> +			hp_wmi_get_fan_count_userdefine_trigger();
>> +		return hp_wmi_fan_speed_max_set(1);
>> +	case PWM_MODE_MANUAL:
>> +		if (!is_victus_s_thermal_profile())
>> +			return -EOPNOTSUPP;
>> +		return hp_wmi_fan_speed_set(ctx, PWM_TO_RPM(ctx->pwm, ctx));
>> +	case PWM_MODE_AUTO:
>> +		if (is_victus_s_thermal_profile()) {
>> +			hp_wmi_get_fan_count_userdefine_trigger();
>> +			return hp_wmi_fan_speed_max_reset(ctx);
>> +		} else {
>
>Unnecessary else.
>

Actually, this is needed to store the intermediate ret variable, to
prepare for the keep-alive rescheduling in patch 2/2.

>> +			return hp_wmi_fan_speed_max_set(0);
>> +		}
>> +	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; /* Hide PWM input if not Victus S */
>
>The comment add no information beyond what the code already tells us.
>

Agreed, this is also removed in v2.

[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 *ctx;
>> +	int current_rpm, ret;
>> +
>> +	ctx = dev_get_drvdata(dev);
>> +	if (!ctx)
>
>Don't you register it with a non-NULL drvdata always?
>

Yes this is also redundant. Removed.

[snip]
>>
>> +static int hp_wmi_hwmon_setup_ctx(struct hp_wmi_hwmon_priv *ctx)
>
>I suggest you change "ctx" in the name to something more meaningful.
>

Renamed "ctx" to "priv" as it is more consistent with drvdata.

>> +{
>> +	u8 fan_data[128];
>> +	u8 (*fan_table)[3];
>> +	u8 rows, min_rpm, max_rpm, gpu_delta;
>> +	int ret;
>> +
>> +	/* Default behaviour on hwmon init is automatic mode */
>> +	ctx->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));
>
>Does this end up coping from uninitialized fan_data (insize = 4)?
>

Yes, that was a mistake. Fixed in v2 by explicitly initialising
fan_data[] to zeros.

>> +	if (ret != 0)
>
>if (ret)
>

Fixed.

>> +		return ret;
>> +
>> +	rows = fan_data[1];
>> +	if (2 + rows * 3 >= sizeof(fan_data))
>> +		return -EINVAL;
>> +
>> +	fan_table = (u8 (*)[3]) & fan_data[2];
>
>Heh, a cast disguished as a bitwise and (due to misleading spacing).
>
>Can you make a real struct out of this so you could access the members
>properly without these literal offsets and casting madness? You might need
>to use DECLARE_FLEX_ARRAY().
>

Yes that cast does look like a bitwise AND. I was merely trying to satisfy
checkpatch --strict, but it seems like it is best to ignore the warning in
this case. Anyway, I agree that such casting creates unnecessary
complexity. I have added struct victus_s_fan_table to handle this more
gracefully in v2.

Powered by blists - more mailing lists