Re: [PATCH 1/2] platform/x86: hp-wmi: add manual fan control for Victus S models
From: Krishna Chomal
Date: Tue Dec 30 2025 - 09:22:23 EST
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.