Re: [PATCH v2 1/2] platform/x86: hp-wmi: add manual fan control for Victus S models
From: Krishna Chomal
Date: Mon Jan 12 2026 - 13:04:02 EST
On Mon, Jan 12, 2026 at 05:13:05PM +0200, Ilpo Järvinen wrote:
On Tue, 30 Dec 2025, Krishna Chomal wrote:[snip]
[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>
+
+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.