Re: [PATCH v3] hp-wmi: add battery threshold support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 8 Jun 2026, yahia wrote:

> From: yahia ahmed <yahia.a.abdrabou@gmail.com>
> 
> Implement battery threshold support using the SBCO and GBCO ACPI methods,
> Add battery_get to retrieve the current battery mode and provide a function
> to set the battery threshold.
> 
> Signed-off-by: yahia ahmed <yahia.a.abdrabou@gmail.com>
> ---
> v3:
> - Fix coding style errors and warning
> - Refine the changelogs for clarity
> 
> v2:
> - Simplify detection logic via battery_get
> - Add Battery Profiles enum table
> - Add functions to set the battery mode
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 37 ++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index f2180b61b84e..c50fe78099d9 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -11,7 +11,6 @@
>   * Copyright (C) 2005 Dmitry Torokhov <dtor@mail.ru>
>   */
>  
> -#include <asm-generic/errno.h>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/acpi.h>
> @@ -297,15 +296,16 @@ enum hp_wmi_event_ids {
>   * Profiles for hp battery threshold profile 0x05 and 0x06 map to 4 and 5 respectively
>   */
>  enum hp_battery_modes {
> -	HP_BATTERY_MODE_NORMAL = 0x00,
> +	HP_BATTERY_MODE_NORMAL   = 0x00,
>  	HP_BATTERY_MODE_ADAPTIVE = 0x02,
> -	HP_BATTERY_THRESHOLD_4 = 0x05,
> -	HP_BATTERY_THRESHOLD_5 = 0x06,
> +	HP_BATTERY_THRESHOLD_4   = 0x05,
> +	HP_BATTERY_THRESHOLD_5   = 0x06,
>  };
>  
> -#define HPWMI_BATTERY_THRESHOLD_SBCO 0x2B // SBCO Command id
> -#define HPWMI_AC_REQUIRED 0x35 // obscure edge case where the bios requires the user to be charged while setting a profile
> -/*
> +#define HPWMI_BATTERY_THRESHOLD_SBCO 0x2B /* SBCO Command id */
> +#define HPWMI_AC_REQUIRED	     0x35
> +/* obscure edge case where the bios requires the user to be charging while setting a profile
> + *
>   * struct bios_args buffer is dynamically allocated.  New WMI command types
>   * were introduced that exceeds 128-byte data size.  Changes to handle
>   * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
> @@ -1099,21 +1099,26 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> -static int battery_get(void) {
> +static int battery_get(void)
> +{
>  	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>  	int mode = -1;
> +
>  	if (ACPI_SUCCESS(acpi_evaluate_object(NULL, "\\_SB.WMID.GBCO", NULL, &output))) {
>  		union acpi_object *obj = (union acpi_object *)output.pointer;
> -		if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 3) { // standard package sent by bios
> -			mode = (int)obj->package.elements[2].buffer.pointer[0]; // bios returns the result in the third buffer 
> +
> +		if (obj && obj->type == ACPI_TYPE_PACKAGE  /* standard package sent by bios */
> +				&& obj->package.count >= 3) {
> +			 /* bios returns the result in the third buffer */
> +			mode = (int)obj->package.elements[2].buffer.pointer[0];
>  		}
>  		kfree(output.pointer);
> -	} else 
> +	} else
>  		mode = -EIO;
>  	return mode;
>  }
>  
> -static int battery_set(int profile) 
> +static int battery_set(int profile)
>  {
>  	if (!battery_threshold_support) {
>  		pr_err("Battery threshold is not supported");
> @@ -1122,18 +1127,21 @@ static int battery_set(int profile)
>  	union acpi_object args[4];
>  	acpi_status status;
>  	struct acpi_object_list arg_list = { .count = 4, .pointer = args };
> +
>  	memset(args, 0, sizeof(args));
>  	args[0].type = ACPI_TYPE_INTEGER;
>  	args[0].integer.value = 0;
>  	args[1].type = ACPI_TYPE_INTEGER;
>  	args[1].integer.value = profile;
> -	args[2].type = ACPI_TYPE_INTEGER; // padding required by bios
> +	args[2].type = ACPI_TYPE_INTEGER; /* padding required by bios */
>  	args[2].integer.value = 0;
>  	args[3].type = ACPI_TYPE_INTEGER;
>  	args[3].integer.value = 0;
>  	status = acpi_evaluate_object(NULL, "\\_SB.WMID.SBCO", &arg_list, NULL);
> +
>  	if (unlikely(status == HPWMI_AC_REQUIRED)) {
> -		pr_err("You have to be plugged in while setting a battery profile"); // random edge case in the bios
> +		 /* random edge case in the bios */
> +		pr_err("You have to be plugged in while setting a battery profile");
>  		return -EIO;
>  	}
>  	if (ACPI_FAILURE(status)) {
> @@ -2351,6 +2359,7 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  		return err;
>  
>  	int ret = battery_get();
> +
>  	if (ret >= 0)
>  		battery_threshold_support = true;
>  	thermal_profile_setup(device);

This patch wouldn't even apply.

Also, I'm very skeptical you reviewed it yourself before sending it as 
some of the changes are direct opposite of my earlier review comments 
(irrespective of whether the patch would apply or not).

-- 
 i.