[PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Julien ROBIN]

The following patch adds support for HP Victus 16-s1000 laptop series,
by adding and fixing the following functionalities, which can be
accessed through hwmon and platform_profile sysfs:

 - Functional measured fan speed reading
 - Ability to enable and disable maximum fan speed
 - Platform profiles full setting ability for CPU and GPU

It sets appropriates CPU and GPU power settings both on AC and battery
power sources, for low-power, balanced and performance modes.

It has been thoroughly tested on a 16-s1034nf laptop based on a 8C9C DMI
board name, and behavior of the driver on previous boards is left
untouched thanks to the separated lists of DMI board names.

Signed-off-by: Julien ROBIN <julien.robin28@free.fr>
---
Changes since v1:
- More clear description of 0xFF special effect when setting power limits
- Added structs for clearer naming of power limits and GPU power modes settings
- Retrieve and keep current GPU slowdown temp threshold (instead of hard coded)
- Removed platform_profile_victus_s_get(), re-using platform_profile_omen_get()
- Changed char variable types to u8 where it was more relevant
- Moved some comments
- Minor typo / alignment corrections
---
 drivers/platform/x86/hp/hp-wmi.c | 364 ++++++++++++++++++++++++++++++-
 1 file changed, 352 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 20c55bab3b8c..af7a3d942d5b 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -83,11 +83,16 @@ static const char * const omen_timed_thermal_profile_boards[] = {
 	"8BAD", "8A42", "8A15"
 };
 
-/* DMI Board names of Victus laptops */
+/* DMI Board names of Victus 16-d1xxx laptops */
 static const char * const victus_thermal_profile_boards[] = {
 	"8A25"
 };
 
+/* DMI Board names of Victus 16-s1000 laptops */
+static const char * const victus_s_thermal_profile_boards[] = {
+	"8C9C"
+};
+
 enum hp_wmi_radio {
 	HPWMI_WIFI	= 0x0,
 	HPWMI_BLUETOOTH	= 0x1,
@@ -147,12 +152,32 @@ enum hp_wmi_commandtype {
 	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
 };
 
+struct victus_power_limits {
+	u8 pl1;
+	u8 pl2;
+	u8 pl4;
+	u8 cpu_gpu_concurrent_limit;
+};
+
+struct victus_gpu_power_modes {
+	u8 ctgp_enable;
+	u8 ppab_enable;
+	u8 dstate;
+	u8 gpu_slowdown_temp;
+};
+
 enum hp_wmi_gm_commandtype {
-	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
-	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
-	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
-	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
-	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
+	HPWMI_FAN_SPEED_GET_QUERY		= 0x11,
+	HPWMI_SET_PERFORMANCE_MODE		= 0x1A,
+	HPWMI_FAN_SPEED_MAX_GET_QUERY		= 0x26,
+	HPWMI_FAN_SPEED_MAX_SET_QUERY		= 0x27,
+	HPWMI_GET_SYSTEM_DESIGN_DATA		= 0x28,
+	HPWMI_FAN_COUNT_GET_QUERY		= 0x10,
+	HPWMI_GET_GPU_THERMAL_MODES_QUERY	= 0x21,
+	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,
 };
 
 enum hp_wmi_command {
@@ -211,6 +236,11 @@ enum hp_thermal_profile_victus {
 	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
 };
 
+enum hp_thermal_profile_victus_s {
+	HP_VICTUS_S_THERMAL_PROFILE_DEFAULT		= 0x00,
+	HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE		= 0x01,
+};
+
 enum hp_thermal_profile {
 	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
 	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
@@ -411,6 +441,26 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 	return ret;
 }
 
+/*
+ * Calling this hp_wmi_get_fan_count_userdefine_trigger function also enables
+ * and/or maintains the laptop in user defined thermal and fan states, instead
+ * of using a fallback state. After a 120 seconds timeout however, the laptop
+ * goes back to its fallback state.
+ */
+static int hp_wmi_get_fan_count_userdefine_trigger(void)
+{
+	u8 fan_data[4] = { 0 };
+
+	int ret = hp_wmi_perform_query(HPWMI_FAN_COUNT_GET_QUERY, HPWMI_GM,
+				       &fan_data, sizeof(u8),
+				       sizeof(fan_data));
+
+	if (ret != 0)
+		return -EINVAL;
+
+	return fan_data[0]; /* Others bytes aren't providing fan count */
+}
+
 static int hp_wmi_get_fan_speed(int fan)
 {
 	u8 fsh, fsl;
@@ -429,6 +479,23 @@ static int hp_wmi_get_fan_speed(int fan)
 	return (fsh << 8) | fsl;
 }
 
+static int hp_wmi_get_fan_speed_victus_s(int fan)
+{
+	u8 fan_data[128] = { 0 };
+
+	int ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY,
+				       HPWMI_GM, &fan_data, sizeof(u8),
+				       sizeof(fan_data));
+
+	if (ret != 0)
+		return -EINVAL;
+
+	if (fan >= 0 && fan < sizeof(fan_data))
+		return fan_data[fan] * 100;
+	else
+		return -EINVAL;
+}
+
 static int hp_wmi_read_int(int query)
 {
 	int val = 0, ret;
@@ -557,6 +624,29 @@ static int hp_wmi_fan_speed_max_set(int enabled)
 	return enabled;
 }
 
+static int hp_wmi_fan_speed_reset(void)
+{
+	int ret;
+	u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
+
+	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_SET_QUERY, HPWMI_GM,
+				   &fan_speed, sizeof(fan_speed), 0);
+
+	return ret;
+}
+
+static int hp_wmi_fan_speed_max_reset(void)
+{
+	int ret = hp_wmi_fan_speed_max_set(0);
+
+	if (ret)
+		return ret;
+
+	/* 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)
 {
 	int val = 0, ret;
@@ -1472,6 +1562,162 @@ static int platform_profile_victus_set_ec(enum platform_profile_option profile)
 	return 0;
 }
 
+static bool is_victus_s_thermal_profile(void)
+{
+	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
+
+	if (!board_name)
+		return false;
+
+	return match_string(victus_s_thermal_profile_boards,
+			    ARRAY_SIZE(victus_s_thermal_profile_boards),
+			    board_name) >= 0;
+}
+
+static int victus_s_gpu_thermal_profile_get(bool *ctgp_enable,
+					    bool *ppab_enable,
+					    u8 *dstate,
+					    u8 *gpu_slowdown_temp)
+{
+	int ret;
+	struct victus_gpu_power_modes gpu_power_modes;
+
+	ret = hp_wmi_perform_query(HPWMI_GET_GPU_THERMAL_MODES_QUERY, HPWMI_GM,
+				   &gpu_power_modes, sizeof(gpu_power_modes),
+				   sizeof(gpu_power_modes));
+
+	if (ret == 0) {
+		*ctgp_enable = gpu_power_modes.ctgp_enable ? true : false;
+		*ppab_enable = gpu_power_modes.ppab_enable ? true : false;
+		*dstate = gpu_power_modes.dstate;
+		*gpu_slowdown_temp = gpu_power_modes.gpu_slowdown_temp;
+	}
+
+	return ret;
+}
+
+static int victus_s_gpu_thermal_profile_set(bool ctgp_enable,
+					    bool ppab_enable,
+					    u8 dstate)
+{
+	struct victus_gpu_power_modes gpu_power_modes;
+	int ret;
+
+	bool current_ctgp_state, current_ppab_state;
+	u8 current_dstate, current_gpu_slowdown_temp;
+
+	/* Retrieving GPU slowdown temperature, in order to keep it unchanged */
+	ret = victus_s_gpu_thermal_profile_get(&current_ctgp_state,
+					       &current_ppab_state,
+					       &current_dstate,
+					       &current_gpu_slowdown_temp);
+
+	if (ret < 0) {
+		pr_warn("GPU modes not updated, unable to get slowdown temp\n");
+		return ret;
+	}
+
+	gpu_power_modes.ctgp_enable = ctgp_enable ? 0x01 : 0x00;
+	gpu_power_modes.ppab_enable = ppab_enable ? 0x01 : 0x00;
+	gpu_power_modes.dstate = dstate;
+	gpu_power_modes.gpu_slowdown_temp = current_gpu_slowdown_temp;
+
+
+	ret = hp_wmi_perform_query(HPWMI_SET_GPU_THERMAL_MODES_QUERY, HPWMI_GM,
+				   &gpu_power_modes, sizeof(gpu_power_modes), 0);
+
+	return ret;
+}
+
+/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
+static int victus_s_set_cpu_pl1_pl2(u8 pl1, u8 pl2)
+{
+	int ret;
+	struct victus_power_limits power_limits;
+
+	power_limits.pl1 = pl1;
+	power_limits.pl2 = pl2;
+	power_limits.pl4 = 0xFF; /* Keep current value */
+	power_limits.cpu_gpu_concurrent_limit = 0xFF; /* Keep current value */
+
+	/* Here, the 0xFF value has a special "ignore / don't change" meaning */
+	if (pl1 == 0xFF || pl2 == 0xFF)
+		return -EINVAL;
+
+	/* PL2 is not supposed to be lower than PL1 */
+	if (pl2 < pl1)
+		return -EINVAL;
+
+	ret = hp_wmi_perform_query(HPWMI_SET_POWER_LIMITS_QUERY, HPWMI_GM,
+				   &power_limits, sizeof(power_limits), 0);
+
+	return ret;
+}
+
+static int platform_profile_victus_s_set_ec(enum platform_profile_option profile)
+{
+	int err, tp;
+	bool gpu_ctgp_enable, gpu_ppab_enable;
+	u8 gpu_dstate; /* Test shows 1 = 100%, 2 = 50%, 3 = 25%, 4 = 12.5% */
+
+	switch (profile) {
+	case PLATFORM_PROFILE_PERFORMANCE:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE;
+		gpu_ctgp_enable = true;
+		gpu_ppab_enable = true;
+		gpu_dstate = 1;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
+		gpu_ctgp_enable = false;
+		gpu_ppab_enable = true;
+		gpu_dstate = 1;
+		break;
+	case PLATFORM_PROFILE_LOW_POWER:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
+		gpu_ctgp_enable = false;
+		gpu_ppab_enable = false;
+		gpu_dstate = 1;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	hp_wmi_get_fan_count_userdefine_trigger();
+
+	err = omen_thermal_profile_set(tp);
+	if (err < 0) {
+		pr_err("Failed to set platform profile %d: %d\n", profile, err);
+		return err;
+	}
+
+	err = victus_s_gpu_thermal_profile_set(gpu_ctgp_enable,
+					       gpu_ppab_enable,
+					       gpu_dstate);
+	if (err < 0) {
+		pr_err("Failed to set GPU profile %d: %d\n", profile, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int platform_profile_victus_s_set(struct platform_profile_handler *pprof,
+					 enum platform_profile_option profile)
+{
+	int err;
+
+	guard(mutex)(&active_platform_profile_lock);
+
+	err = platform_profile_victus_s_set_ec(profile);
+	if (err < 0)
+		return err;
+
+	active_platform_profile = profile;
+
+	return 0;
+}
+
 static int platform_profile_victus_set(struct platform_profile_handler *pprof,
 				       enum platform_profile_option profile)
 {
@@ -1545,6 +1791,38 @@ static int omen_powersource_event(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static int victus_s_powersource_event(struct notifier_block *nb,
+				      unsigned long value,
+				      void *data)
+{
+	struct acpi_bus_event *event_entry = data;
+	int err;
+
+	if (strcmp(event_entry->device_class, ACPI_AC_CLASS) != 0)
+		return NOTIFY_DONE;
+
+	pr_debug("Received power source device event\n");
+
+	/*
+	 * Switching to battery power source while Performance mode is active
+	 * needs manual triggering of CPU power limits. Same goes when switching
+	 * to AC power source while Performance mode is active. Other modes
+	 * however are automatically behaving without any manual action.
+	 * Seen on HP 16-s1034nf (board 8C9C) with F.11 and F.13 BIOS versions.
+	 */
+
+	if (active_platform_profile == PLATFORM_PROFILE_PERFORMANCE) {
+		pr_debug("Triggering CPU PL1/PL2 actualization\n");
+		err = victus_s_set_cpu_pl1_pl2(0, 0);
+		if (err)
+			pr_warn("Failed to actualize power limits: %d\n", err);
+
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 static int omen_register_powersource_event_handler(void)
 {
 	int err;
@@ -1560,11 +1838,31 @@ static int omen_register_powersource_event_handler(void)
 	return 0;
 }
 
+static int victus_s_register_powersource_event_handler(void)
+{
+	int err;
+
+	platform_power_source_nb.notifier_call = victus_s_powersource_event;
+	err = register_acpi_notifier(&platform_power_source_nb);
+
+	if (err < 0) {
+		pr_warn("Failed to install ACPI power source notify handler\n");
+		return err;
+	}
+
+	return 0;
+}
+
 static inline void omen_unregister_powersource_event_handler(void)
 {
 	unregister_acpi_notifier(&platform_power_source_nb);
 }
 
+static inline void victus_s_unregister_powersource_event_handler(void)
+{
+	unregister_acpi_notifier(&platform_power_source_nb);
+}
+
 static int thermal_profile_setup(void)
 {
 	int err, tp;
@@ -1603,6 +1901,22 @@ static int thermal_profile_setup(void)
 		platform_profile_handler.profile_set = platform_profile_victus_set;
 
 		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
+	} else if (is_victus_s_thermal_profile()) {
+		/*
+		 * Being unable to retrieve laptop's current thermal profile,
+		 * during this setup, we set it to Balanced by default.
+		 */
+		active_platform_profile = PLATFORM_PROFILE_BALANCED;
+
+		err = platform_profile_victus_s_set_ec(active_platform_profile);
+		if (err < 0)
+			return err;
+
+		platform_profile_handler.profile_get = platform_profile_omen_get;
+		platform_profile_handler.profile_set = platform_profile_victus_s_set;
+
+		/* Adding an equivalent to HP Omen software ECO mode: */
+		set_bit(PLATFORM_PROFILE_LOW_POWER, platform_profile_handler.choices);
 	} else {
 		tp = thermal_profile_get();
 
@@ -1628,9 +1942,14 @@ static int thermal_profile_setup(void)
 	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
 
 	err = platform_profile_register(&platform_profile_handler);
-	if (err)
+	if (err == -EEXIST) {
+		pr_warn("A platform profile handler is already registered\n");
 		return err;
-
+	} else if (err) {
+		pr_err("Platform profile handler registration fail: %d\n", err);
+		return err;
+	}
+	pr_info("Registered as platform profile handler\n");
 	platform_profile_support = true;
 
 	return 0;
@@ -1759,8 +2078,13 @@ static umode_t hp_wmi_hwmon_is_visible(const void *data,
 	case hwmon_pwm:
 		return 0644;
 	case hwmon_fan:
-		if (hp_wmi_get_fan_speed(channel) >= 0)
-			return 0444;
+		if (is_victus_s_thermal_profile()) {
+			if (hp_wmi_get_fan_speed_victus_s(channel) >= 0)
+				return 0444;
+		} else {
+			if (hp_wmi_get_fan_speed(channel) >= 0)
+				return 0444;
+		}
 		break;
 	default:
 		return 0;
@@ -1776,7 +2100,10 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 
 	switch (type) {
 	case hwmon_fan:
-		ret = hp_wmi_get_fan_speed(channel);
+		if (is_victus_s_thermal_profile())
+			ret = hp_wmi_get_fan_speed_victus_s(channel);
+		else
+			ret = hp_wmi_get_fan_speed(channel);
 
 		if (ret < 0)
 			return ret;
@@ -1810,11 +2137,17 @@ static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_pwm:
 		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 */
-			return hp_wmi_fan_speed_max_set(0);
+			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);
 		default:
 			/* we don't support manual fan speed control */
 			return -EINVAL;
@@ -1893,6 +2226,10 @@ static int __init hp_wmi_init(void)
 		err = omen_register_powersource_event_handler();
 		if (err)
 			goto err_unregister_device;
+	} else if (is_victus_s_thermal_profile()) {
+		err = victus_s_register_powersource_event_handler();
+		if (err)
+			goto err_unregister_device;
 	}
 
 	return 0;
@@ -1912,6 +2249,9 @@ static void __exit hp_wmi_exit(void)
 	if (is_omen_thermal_profile() || is_victus_thermal_profile())
 		omen_unregister_powersource_event_handler();
 
+	if (is_victus_s_thermal_profile())
+		victus_s_unregister_powersource_event_handler();
+
 	if (wmi_has_guid(HPWMI_EVENT_GUID))
 		hp_wmi_input_destroy();
 

Re: [PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Ilpo Järvinen]

On Tue, 14 Jan 2025, Julien ROBIN wrote:

> The following patch adds support for HP Victus 16-s1000 laptop series,
> by adding and fixing the following functionalities, which can be
> accessed through hwmon and platform_profile sysfs:
> 
>  - Functional measured fan speed reading
>  - Ability to enable and disable maximum fan speed
>  - Platform profiles full setting ability for CPU and GPU
> 
> It sets appropriates CPU and GPU power settings both on AC and battery
> power sources, for low-power, balanced and performance modes.
> 
> It has been thoroughly tested on a 16-s1034nf laptop based on a 8C9C DMI
> board name, and behavior of the driver on previous boards is left
> untouched thanks to the separated lists of DMI board names.
> 
> Signed-off-by: Julien ROBIN <julien.robin28@free.fr>
> ---
> Changes since v1:
> - More clear description of 0xFF special effect when setting power limits
> - Added structs for clearer naming of power limits and GPU power modes settings
> - Retrieve and keep current GPU slowdown temp threshold (instead of hard coded)
> - Removed platform_profile_victus_s_get(), re-using platform_profile_omen_get()
> - Changed char variable types to u8 where it was more relevant
> - Moved some comments
> - Minor typo / alignment corrections

I wrote a few comments to the v1 thread as the relevant discussion was 
there but they'll be relevant to a few places in this v2.

> ---
>  drivers/platform/x86/hp/hp-wmi.c | 364 ++++++++++++++++++++++++++++++-
>  1 file changed, 352 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 20c55bab3b8c..af7a3d942d5b 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -83,11 +83,16 @@ static const char * const omen_timed_thermal_profile_boards[] = {
>  	"8BAD", "8A42", "8A15"
>  };
>  
> -/* DMI Board names of Victus laptops */
> +/* DMI Board names of Victus 16-d1xxx laptops */
>  static const char * const victus_thermal_profile_boards[] = {
>  	"8A25"
>  };
>  
> +/* DMI Board names of Victus 16-s1000 laptops */
> +static const char * const victus_s_thermal_profile_boards[] = {
> +	"8C9C"
> +};
> +
>  enum hp_wmi_radio {
>  	HPWMI_WIFI	= 0x0,
>  	HPWMI_BLUETOOTH	= 0x1,
> @@ -147,12 +152,32 @@ enum hp_wmi_commandtype {
>  	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>  };
>  
> +struct victus_power_limits {
> +	u8 pl1;
> +	u8 pl2;
> +	u8 pl4;
> +	u8 cpu_gpu_concurrent_limit;
> +};
> +
> +struct victus_gpu_power_modes {
> +	u8 ctgp_enable;
> +	u8 ppab_enable;
> +	u8 dstate;
> +	u8 gpu_slowdown_temp;
> +};
> +
>  enum hp_wmi_gm_commandtype {
> -	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> -	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> -	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> -	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> -	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
> +	HPWMI_FAN_SPEED_GET_QUERY		= 0x11,
> +	HPWMI_SET_PERFORMANCE_MODE		= 0x1A,
> +	HPWMI_FAN_SPEED_MAX_GET_QUERY		= 0x26,
> +	HPWMI_FAN_SPEED_MAX_SET_QUERY		= 0x27,
> +	HPWMI_GET_SYSTEM_DESIGN_DATA		= 0x28,
> +	HPWMI_FAN_COUNT_GET_QUERY		= 0x10,
> +	HPWMI_GET_GPU_THERMAL_MODES_QUERY	= 0x21,
> +	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,
>  };
>  
>  enum hp_wmi_command {
> @@ -211,6 +236,11 @@ enum hp_thermal_profile_victus {
>  	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
>  };
>  
> +enum hp_thermal_profile_victus_s {
> +	HP_VICTUS_S_THERMAL_PROFILE_DEFAULT		= 0x00,
> +	HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE		= 0x01,
> +};
> +
>  enum hp_thermal_profile {
>  	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
>  	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> @@ -411,6 +441,26 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  	return ret;
>  }
>  
> +/*
> + * Calling this hp_wmi_get_fan_count_userdefine_trigger function also enables
> + * and/or maintains the laptop in user defined thermal and fan states, instead
> + * of using a fallback state. After a 120 seconds timeout however, the laptop
> + * goes back to its fallback state.
> + */
> +static int hp_wmi_get_fan_count_userdefine_trigger(void)
> +{
> +	u8 fan_data[4] = { 0 };

{} is enough to initialize the entire array.

> +
> +	int ret = hp_wmi_perform_query(HPWMI_FAN_COUNT_GET_QUERY, HPWMI_GM,
> +				       &fan_data, sizeof(u8),
> +				       sizeof(fan_data));
> +
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	return fan_data[0]; /* Others bytes aren't providing fan count */
> +}
> +
>  static int hp_wmi_get_fan_speed(int fan)
>  {
>  	u8 fsh, fsl;
> @@ -429,6 +479,23 @@ static int hp_wmi_get_fan_speed(int fan)
>  	return (fsh << 8) | fsl;
>  }
>  
> +static int hp_wmi_get_fan_speed_victus_s(int fan)
> +{
> +	u8 fan_data[128] = { 0 };

Ditto.

> +
> +	int ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY,
> +				       HPWMI_GM, &fan_data, sizeof(u8),
> +				       sizeof(fan_data));
> +
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	if (fan >= 0 && fan < sizeof(fan_data))
> +		return fan_data[fan] * 100;
> +	else
> +		return -EINVAL;
> +}
> +
>  static int hp_wmi_read_int(int query)
>  {
>  	int val = 0, ret;
> @@ -557,6 +624,29 @@ static int hp_wmi_fan_speed_max_set(int enabled)
>  	return enabled;
>  }
>  
> +static int hp_wmi_fan_speed_reset(void)
> +{
> +	int ret;
> +	u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
> +
> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_SET_QUERY, HPWMI_GM,
> +				   &fan_speed, sizeof(fan_speed), 0);
> +
> +	return ret;
> +}
> +
> +static int hp_wmi_fan_speed_max_reset(void)
> +{
> +	int ret = hp_wmi_fan_speed_max_set(0);
> +
> +	if (ret)

To keep the call and its error handling together, please use this form:

	int ret;

	ret = hp_wmi_fan_speed_max_set(0);
	if (ret)

> +		return ret;
> +
> +	/* Disabling max fan speed on Victus s1xxx laptops needs a 2nd step: */
> +	ret = hp_wmi_fan_speed_reset();
> +	return ret;
> +}


-- 
 i.



> +
>  static int hp_wmi_fan_speed_max_get(void)
>  {
>  	int val = 0, ret;
> @@ -1472,6 +1562,162 @@ static int platform_profile_victus_set_ec(enum platform_profile_option profile)
>  	return 0;
>  }
>  
> +static bool is_victus_s_thermal_profile(void)
> +{
> +	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> +	if (!board_name)
> +		return false;
> +
> +	return match_string(victus_s_thermal_profile_boards,
> +			    ARRAY_SIZE(victus_s_thermal_profile_boards),
> +			    board_name) >= 0;
> +}
> +
> +static int victus_s_gpu_thermal_profile_get(bool *ctgp_enable,
> +					    bool *ppab_enable,
> +					    u8 *dstate,
> +					    u8 *gpu_slowdown_temp)
> +{
> +	int ret;
> +	struct victus_gpu_power_modes gpu_power_modes;
> +
> +	ret = hp_wmi_perform_query(HPWMI_GET_GPU_THERMAL_MODES_QUERY, HPWMI_GM,
> +				   &gpu_power_modes, sizeof(gpu_power_modes),
> +				   sizeof(gpu_power_modes));
> +
> +	if (ret == 0) {
> +		*ctgp_enable = gpu_power_modes.ctgp_enable ? true : false;
> +		*ppab_enable = gpu_power_modes.ppab_enable ? true : false;
> +		*dstate = gpu_power_modes.dstate;
> +		*gpu_slowdown_temp = gpu_power_modes.gpu_slowdown_temp;
> +	}
> +
> +	return ret;
> +}
> +
> +static int victus_s_gpu_thermal_profile_set(bool ctgp_enable,
> +					    bool ppab_enable,
> +					    u8 dstate)
> +{
> +	struct victus_gpu_power_modes gpu_power_modes;
> +	int ret;
> +
> +	bool current_ctgp_state, current_ppab_state;
> +	u8 current_dstate, current_gpu_slowdown_temp;
> +
> +	/* Retrieving GPU slowdown temperature, in order to keep it unchanged */
> +	ret = victus_s_gpu_thermal_profile_get(&current_ctgp_state,
> +					       &current_ppab_state,
> +					       &current_dstate,
> +					       &current_gpu_slowdown_temp);
> +
> +	if (ret < 0) {
> +		pr_warn("GPU modes not updated, unable to get slowdown temp\n");
> +		return ret;
> +	}
> +
> +	gpu_power_modes.ctgp_enable = ctgp_enable ? 0x01 : 0x00;
> +	gpu_power_modes.ppab_enable = ppab_enable ? 0x01 : 0x00;
> +	gpu_power_modes.dstate = dstate;
> +	gpu_power_modes.gpu_slowdown_temp = current_gpu_slowdown_temp;
> +
> +
> +	ret = hp_wmi_perform_query(HPWMI_SET_GPU_THERMAL_MODES_QUERY, HPWMI_GM,
> +				   &gpu_power_modes, sizeof(gpu_power_modes), 0);
> +
> +	return ret;
> +}
> +
> +/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
> +static int victus_s_set_cpu_pl1_pl2(u8 pl1, u8 pl2)
> +{
> +	int ret;
> +	struct victus_power_limits power_limits;
> +
> +	power_limits.pl1 = pl1;
> +	power_limits.pl2 = pl2;
> +	power_limits.pl4 = 0xFF; /* Keep current value */
> +	power_limits.cpu_gpu_concurrent_limit = 0xFF; /* Keep current value */
> +
> +	/* Here, the 0xFF value has a special "ignore / don't change" meaning */
> +	if (pl1 == 0xFF || pl2 == 0xFF)
> +		return -EINVAL;
> +
> +	/* PL2 is not supposed to be lower than PL1 */
> +	if (pl2 < pl1)
> +		return -EINVAL;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SET_POWER_LIMITS_QUERY, HPWMI_GM,
> +				   &power_limits, sizeof(power_limits), 0);
> +
> +	return ret;
> +}
> +
> +static int platform_profile_victus_s_set_ec(enum platform_profile_option profile)
> +{
> +	int err, tp;
> +	bool gpu_ctgp_enable, gpu_ppab_enable;
> +	u8 gpu_dstate; /* Test shows 1 = 100%, 2 = 50%, 3 = 25%, 4 = 12.5% */
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE;
> +		gpu_ctgp_enable = true;
> +		gpu_ppab_enable = true;
> +		gpu_dstate = 1;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
> +		gpu_ctgp_enable = false;
> +		gpu_ppab_enable = true;
> +		gpu_dstate = 1;
> +		break;
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
> +		gpu_ctgp_enable = false;
> +		gpu_ppab_enable = false;
> +		gpu_dstate = 1;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	hp_wmi_get_fan_count_userdefine_trigger();
> +
> +	err = omen_thermal_profile_set(tp);
> +	if (err < 0) {
> +		pr_err("Failed to set platform profile %d: %d\n", profile, err);
> +		return err;
> +	}
> +
> +	err = victus_s_gpu_thermal_profile_set(gpu_ctgp_enable,
> +					       gpu_ppab_enable,
> +					       gpu_dstate);
> +	if (err < 0) {
> +		pr_err("Failed to set GPU profile %d: %d\n", profile, err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int platform_profile_victus_s_set(struct platform_profile_handler *pprof,
> +					 enum platform_profile_option profile)
> +{
> +	int err;
> +
> +	guard(mutex)(&active_platform_profile_lock);
> +
> +	err = platform_profile_victus_s_set_ec(profile);
> +	if (err < 0)
> +		return err;
> +
> +	active_platform_profile = profile;
> +
> +	return 0;
> +}
> +
>  static int platform_profile_victus_set(struct platform_profile_handler *pprof,
>  				       enum platform_profile_option profile)
>  {
> @@ -1545,6 +1791,38 @@ static int omen_powersource_event(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static int victus_s_powersource_event(struct notifier_block *nb,
> +				      unsigned long value,
> +				      void *data)
> +{
> +	struct acpi_bus_event *event_entry = data;
> +	int err;
> +
> +	if (strcmp(event_entry->device_class, ACPI_AC_CLASS) != 0)
> +		return NOTIFY_DONE;
> +
> +	pr_debug("Received power source device event\n");
> +
> +	/*
> +	 * Switching to battery power source while Performance mode is active
> +	 * needs manual triggering of CPU power limits. Same goes when switching
> +	 * to AC power source while Performance mode is active. Other modes
> +	 * however are automatically behaving without any manual action.
> +	 * Seen on HP 16-s1034nf (board 8C9C) with F.11 and F.13 BIOS versions.
> +	 */
> +
> +	if (active_platform_profile == PLATFORM_PROFILE_PERFORMANCE) {
> +		pr_debug("Triggering CPU PL1/PL2 actualization\n");
> +		err = victus_s_set_cpu_pl1_pl2(0, 0);
> +		if (err)
> +			pr_warn("Failed to actualize power limits: %d\n", err);
> +
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int omen_register_powersource_event_handler(void)
>  {
>  	int err;
> @@ -1560,11 +1838,31 @@ static int omen_register_powersource_event_handler(void)
>  	return 0;
>  }
>  
> +static int victus_s_register_powersource_event_handler(void)
> +{
> +	int err;
> +
> +	platform_power_source_nb.notifier_call = victus_s_powersource_event;
> +	err = register_acpi_notifier(&platform_power_source_nb);
> +
> +	if (err < 0) {
> +		pr_warn("Failed to install ACPI power source notify handler\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void omen_unregister_powersource_event_handler(void)
>  {
>  	unregister_acpi_notifier(&platform_power_source_nb);
>  }
>  
> +static inline void victus_s_unregister_powersource_event_handler(void)
> +{
> +	unregister_acpi_notifier(&platform_power_source_nb);
> +}
> +
>  static int thermal_profile_setup(void)
>  {
>  	int err, tp;
> @@ -1603,6 +1901,22 @@ static int thermal_profile_setup(void)
>  		platform_profile_handler.profile_set = platform_profile_victus_set;
>  
>  		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
> +	} else if (is_victus_s_thermal_profile()) {
> +		/*
> +		 * Being unable to retrieve laptop's current thermal profile,
> +		 * during this setup, we set it to Balanced by default.
> +		 */
> +		active_platform_profile = PLATFORM_PROFILE_BALANCED;
> +
> +		err = platform_profile_victus_s_set_ec(active_platform_profile);
> +		if (err < 0)
> +			return err;
> +
> +		platform_profile_handler.profile_get = platform_profile_omen_get;
> +		platform_profile_handler.profile_set = platform_profile_victus_s_set;
> +
> +		/* Adding an equivalent to HP Omen software ECO mode: */
> +		set_bit(PLATFORM_PROFILE_LOW_POWER, platform_profile_handler.choices);
>  	} else {
>  		tp = thermal_profile_get();
>  
> @@ -1628,9 +1942,14 @@ static int thermal_profile_setup(void)
>  	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
>  
>  	err = platform_profile_register(&platform_profile_handler);
> -	if (err)
> +	if (err == -EEXIST) {
> +		pr_warn("A platform profile handler is already registered\n");
>  		return err;
> -
> +	} else if (err) {
> +		pr_err("Platform profile handler registration fail: %d\n", err);
> +		return err;
> +	}
> +	pr_info("Registered as platform profile handler\n");
>  	platform_profile_support = true;
>  
>  	return 0;
> @@ -1759,8 +2078,13 @@ static umode_t hp_wmi_hwmon_is_visible(const void *data,
>  	case hwmon_pwm:
>  		return 0644;
>  	case hwmon_fan:
> -		if (hp_wmi_get_fan_speed(channel) >= 0)
> -			return 0444;
> +		if (is_victus_s_thermal_profile()) {
> +			if (hp_wmi_get_fan_speed_victus_s(channel) >= 0)
> +				return 0444;
> +		} else {
> +			if (hp_wmi_get_fan_speed(channel) >= 0)
> +				return 0444;
> +		}
>  		break;
>  	default:
>  		return 0;
> @@ -1776,7 +2100,10 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  
>  	switch (type) {
>  	case hwmon_fan:
> -		ret = hp_wmi_get_fan_speed(channel);
> +		if (is_victus_s_thermal_profile())
> +			ret = hp_wmi_get_fan_speed_victus_s(channel);
> +		else
> +			ret = hp_wmi_get_fan_speed(channel);
>  
>  		if (ret < 0)
>  			return ret;
> @@ -1810,11 +2137,17 @@ static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_pwm:
>  		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 */
> -			return hp_wmi_fan_speed_max_set(0);
> +			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);
>  		default:
>  			/* we don't support manual fan speed control */
>  			return -EINVAL;
> @@ -1893,6 +2226,10 @@ static int __init hp_wmi_init(void)
>  		err = omen_register_powersource_event_handler();
>  		if (err)
>  			goto err_unregister_device;
> +	} else if (is_victus_s_thermal_profile()) {
> +		err = victus_s_register_powersource_event_handler();
> +		if (err)
> +			goto err_unregister_device;
>  	}
>  
>  	return 0;
> @@ -1912,6 +2249,9 @@ static void __exit hp_wmi_exit(void)
>  	if (is_omen_thermal_profile() || is_victus_thermal_profile())
>  		omen_unregister_powersource_event_handler();
>  
> +	if (is_victus_s_thermal_profile())
> +		victus_s_unregister_powersource_event_handler();
> +
>  	if (wmi_has_guid(HPWMI_EVENT_GUID))
>  		hp_wmi_input_destroy();
>  
> 

Re: [PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Julien ROBIN]

Thanks for the fast answer,

On 1/14/25 5:20 PM, Ilpo Järvinen wrote:
>> Changes since v1:
>> - More clear description of 0xFF special effect when setting power limits
>> - Added structs for clearer naming of power limits and GPU power modes settings
>> - Retrieve and keep current GPU slowdown temp threshold (instead of hard coded)
>> - Removed platform_profile_victus_s_get(), re-using platform_profile_omen_get()
>> - Changed char variable types to u8 where it was more relevant
>> - Moved some comments
>> - Minor typo / alignment corrections
> 
> I wrote a few comments to the v1 thread as the relevant discussion was 
> there but they'll be relevant to a few places in this v2.

>> +static int hp_wmi_get_fan_count_userdefine_trigger(void)
>> +{
>> +	u8 fan_data[4] = { 0 };
> 
> {} is enough to initialize the entire array.

>> +static int hp_wmi_get_fan_speed_victus_s(int fan)
>> +{
>> +	u8 fan_data[128] = { 0 };
> 
> Ditto.

Understood, I just applied that.

While discussing these array initialisations to zero: I'm addressing what we finished discussing in the v1 thread by defining and using the following self explanatory named literals (instead of comments):

#define HP_FAN_SPEED_AUTOMATIC	 0x00
#define HP_POWER_LIMIT_DEFAULT	 0x00
#define HP_POWER_LIMIT_NO_CHANGE 0xFF

In this particular case, while technically the {} would work, I suppose it would still be preferable to change from:
u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
to:
u8 fan_speed[2] = { HP_FAN_SPEED_AUTOMATIC }

>> +static int hp_wmi_fan_speed_max_reset(void)
>> +{
>> +	int ret = hp_wmi_fan_speed_max_set(0);
>> +
>> +	if (ret)
> 
> To keep the call and its error handling together, please use this form:
> 
> 	int ret;
> 
> 	ret = hp_wmi_fan_speed_max_set(0);
> 	if (ret)

Understood, and applied.

Before sending the resulting v3, would you like me to apply this form change, alongside with { 0 } becoming {}, to some others already existing functions around? I prefer asking before doing so, because by default I'm not sure changing unrelated already existing lines is expected when sending a patch.
If not, I suppose this may be the matter of another future and unrelated patch submission.

Thanks again for the review work.

Best regards,
Julien ROBIN

Re: [PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

On Tue, 14 Jan 2025, Julien ROBIN wrote:

> Thanks for the fast answer,
> 
> On 1/14/25 5:20 PM, Ilpo Järvinen wrote:
> >> Changes since v1:
> >> - More clear description of 0xFF special effect when setting power limits
> >> - Added structs for clearer naming of power limits and GPU power modes settings
> >> - Retrieve and keep current GPU slowdown temp threshold (instead of hard coded)
> >> - Removed platform_profile_victus_s_get(), re-using platform_profile_omen_get()
> >> - Changed char variable types to u8 where it was more relevant
> >> - Moved some comments
> >> - Minor typo / alignment corrections
> > 
> > I wrote a few comments to the v1 thread as the relevant discussion was 
> > there but they'll be relevant to a few places in this v2.
> 
> >> +static int hp_wmi_get_fan_count_userdefine_trigger(void)
> >> +{
> >> +	u8 fan_data[4] = { 0 };
> > 
> > {} is enough to initialize the entire array.
> 
> >> +static int hp_wmi_get_fan_speed_victus_s(int fan)
> >> +{
> >> +	u8 fan_data[128] = { 0 };
> > 
> > Ditto.
> 
> Understood, I just applied that.
> 
> While discussing these array initialisations to zero: I'm addressing 
> what we finished discussing in the v1 thread by defining and using the 
> following self explanatory named literals (instead of comments):
> 
> #define HP_FAN_SPEED_AUTOMATIC	 0x00
> #define HP_POWER_LIMIT_DEFAULT	 0x00
> #define HP_POWER_LIMIT_NO_CHANGE 0xFF

Sound good.

> In this particular case, while technically the {} would work, I suppose it would still be preferable to change from:
> u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
> to:
> u8 fan_speed[2] = { HP_FAN_SPEED_AUTOMATIC }

Do you actually want this?

u8 fan_speed[2]  = { HP_FAN_SPEED_AUTOMATIC, HP_FAN_SPEED_AUTOMATIC };

That is, are those both bytes fans?

> >> +static int hp_wmi_fan_speed_max_reset(void)
> >> +{
> >> +	int ret = hp_wmi_fan_speed_max_set(0);
> >> +
> >> +	if (ret)
> > 
> > To keep the call and its error handling together, please use this form:
> > 
> > 	int ret;
> > 
> > 	ret = hp_wmi_fan_speed_max_set(0);
> > 	if (ret)
> 
> Understood, and applied.
> 
> Before sending the resulting v3, would you like me to apply this form 
> change, alongside with { 0 } becoming {}, to some others already 
> existing functions around? I prefer asking before doing so, because by 
> default I'm not sure changing unrelated already existing lines is 
> expected when sending a patch. 

Yes, please change all similar cases. I always assume/hope people look 
through their own patches for similar things to address. If I'd try to 
mark all of cases, I'd anyway miss a few because my review is not going to 
be perfect (often I only realize these mid-patch, having passed n similar 
cases already; I try to focus on finding logic problems but I catch many 
many style things too while at it).

> If not, I suppose this may be the matter of another future and unrelated 
> patch submission. 
> 
> Thanks again for the review work.

You can always improve the patches beyond what is mentioned by the review 
comments! :-)

-- 
 i.

Re: [PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Julien ROBIN]

On 15/01/2025 2:07 PM, Ilpo Järvinen wrote:

>> In this particular case, while technically the {} would work, I suppose it would still be preferable to change from:
>> u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
>> to:
>> u8 fan_speed[2] = { HP_FAN_SPEED_AUTOMATIC }
> 
> Do you actually want this?
> 
> u8 fan_speed[2]  = { HP_FAN_SPEED_AUTOMATIC, HP_FAN_SPEED_AUTOMATIC };
> 
> That is, are those both bytes fans?

Yes, I did another double check to be sure, with different non zero values, here each byte is clearly setting its own fan speed, they're independent from each other.
So the writing you suggest is more appropriate, resetting both fans to automatic speed, needs the zero value (HP_FAN_SPEED_AUTOMATIC) to be set into both bytes.

>>>> +static int hp_wmi_fan_speed_max_reset(void)
>>>> +{
>>>> +	int ret = hp_wmi_fan_speed_max_set(0);
>>>> +
>>>> +	if (ret)
>>>
>>> To keep the call and its error handling together, please use this form:
>>>
>>> 	int ret;
>>>
>>> 	ret = hp_wmi_fan_speed_max_set(0);
>>> 	if (ret)
>>
>> Understood, and applied.
>>
>> Before sending the resulting v3, would you like me to apply this form 
>> change, alongside with { 0 } becoming {}, to some others already 
>> existing functions around? I prefer asking before doing so, because by 
>> default I'm not sure changing unrelated already existing lines is 
>> expected when sending a patch. 
> 
> Yes, please change all similar cases. I always assume/hope people look 
> through their own patches for similar things to address. If I'd try to 
> mark all of cases, I'd anyway miss a few because my review is not going to 
> be perfect (often I only realize these mid-patch, having passed n similar 
> cases already; I try to focus on finding logic problems but I catch many 
> many style things too while at it).

Fine :-) Thanks for the explanations, no problem I completely understand.
So I looked around for occurrences of the same things to fix into others already existing functions, and I carefully applied the change where appropriate.

>> If not, I suppose this may be the matter of another future and unrelated 
>> patch submission. 
>>
>> Thanks again for the review work.
> 
> You can always improve the patches beyond what is mentioned by the review 
> comments! :-)

Good to know!
Prepared the v3 for the next round (tested it and checked with scripts/checkpatch.pl as always), and described the changes; I'm sending it now.