Re: [PATCH] [PATCH] platform/x86: Add Battery Threshold support

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

On Sat, 6 Jun 2026, yahia wrote:

Thanks for the patch.

One PATCH is enough on the subject lines. :-)

> Hello,
> I aim in this patch to add support to
> Battery threshold via SBCT and GBCT
> acpi methods from the acpi table

acpi -> ACPI

Please write a proper changelog using imperative tone and real English 
sentences.

Don't use "I" (or "we") nor "this patch" but start with the verb:

Add support ...

> 
> Signed-off-by: yahia <yahia.a.abdrabou@gmail.com>

Please add also your surname before the email address.

For more information, please see 
Documentation/process/submitting-patches.rst

> ---
>  drivers/platform/x86/hp/hp-wmi.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index f63bc00d9a9b..3d035ad9f03d 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -59,6 +59,8 @@ enum hp_ec_offsets {
>  #define HP_POWER_LIMIT_DEFAULT	 0x00
>  #define HP_POWER_LIMIT_NO_CHANGE 0xFF
>  
> +#define HP_BATTERY_THRESHOLD_CAP 0x37
> +
>  #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
>  
>  enum hp_thermal_profile_omen_v0 {
> @@ -448,6 +450,7 @@ static struct notifier_block platform_power_source_nb;
>  static enum platform_profile_option active_platform_profile;
>  static bool platform_profile_support;
>  static bool zero_insize_support;
> +static bool battery_threshold_support;
>  
>  static struct rfkill *wifi_rfkill;
>  static struct rfkill *bluetooth_rfkill;
> @@ -1130,6 +1133,22 @@ static struct attribute *hp_wmi_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(hp_wmi);
>  
> +static int hp_battery_threshold_check(void)
> +{
> +	u8 buffer[128] = {0};

= {}; is enough to initialize it.

> +	int ret;

Add an empty line after local vars.

> +	ret = hp_wmi_perform_query(HP_BATTERY_THRESHOLD_CAP, HPWMI_READ, buffer, 128, 128);

sizeof(buffer)

> +	if (ret != 0) {
> +		battery_threshold_support = false;

battery_threshold_support is initialized to false so do you need this?

> +		return -1;

You should generally pass the error code onwards. Though the positive 
returns you should handle too so it would be better to split it into two 
if()s.

> +	}
> +	if (buffer[28] == 0xAA) {

Please name 0xaa with a define.

Maybe 28 as well should be named with a define.

> +		battery_threshold_support = true;
> +		return 0;
> +	}
> +	return false;

Returning false from int function???

> +}
> +
>  static void hp_wmi_notify(union acpi_object *obj, void *context)
>  {
>  	u32 event_id, event_data;
> @@ -2287,6 +2306,12 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  			hp_wmi_rfkill2_setup(device);
>  	}
>  
> +	err = hp_battery_threshold_check();
> +
> +	if (err == 0) {

Please don't leave empty lines in between call and it's error handling 
(but see the next comment below).

> +		pr_info("Battery Threshold is supported");

Success path should be silent.

After correcting these issues, please send v2.

> +	}
> +
>  	err = hp_wmi_hwmon_init();
>  
>  	if (err < 0)
> 

-- 
 i.