Re: [PATCH v3] platform/x86: lenovo-wmi-other: Deconflict battery with thinkpad_acpi

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

On Fri, 13 Nov 2026, Aaron Ma wrote:

> ThinkPads expose battery charge thresholds and charge_behaviour through
> thinkpad_acpi's ACPI battery hook using raw sysfs attribute groups. The
> Lenovo WMI "Other Mode" driver also registers a power_supply_ext for
> charge_behaviour and charge_types on systems advertising those WMI
> capabilities.
> 
> When lenovo-wmi-other registers first, thinkpad_acpi fails to create a
> duplicate charge_behaviour sysfs attribute and unregisters its battery
> extension. Conversely, registering or updating any power_supply_ext on
> the same battery runs sysfs_update_groups(), which removes standard
> power-supply attributes not owned by power_supply_core and wipes out
> thinkpad_acpi's charge_control_start_threshold,
> charge_control_end_threshold, and charge_behaviour files.
> 
> Deconflict both ideapad_laptop (VPC2004 with GBMD and SBMC) and
> thinkpad_acpi (LEN0268, LEN0068, or IBM0068 with BDSG and BICG) via a
> per-driver table guarded by IS_ENABLED() in Kconfig, and update the
> force_load_psy_ext description accordingly.
> 
> Fixes: 9ca8fc065b88b ("platform/x86: lenovo-wmi-other: Add WMI battery charge limiting")
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Co-developed-by: Rong Zhang <i@rong.moe>
> Signed-off-by: Rong Zhang <i@rong.moe>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
> Changes in v3:
> - Wrap deconfliction table entries in #if IS_ENABLED(CONFIG_*) and drop
>   driver_enabled, as suggested by Rong Zhang.
> - Group deconfliction entries per driver (HIDs list + methods list).
> 
> Changes in v2:
> - Consolidate IdeaPad and ThinkPad deconfliction into a table of ACPI
>   device IDs, method pairs, and driver configuration checks, as suggested
>   by Derek J. Clark.
> - Check GBMD and SBMC for IdeaPads, and BDSG and BICG for ThinkPads,
>   instead of relying only on ThinkPad ACPI device presence.
> - Check whether the corresponding driver is enabled before looking up
>   its ACPI device, including for ideapad_laptop.
> - Update the force_load_psy_ext description to cover both drivers.
> 
> v2: https://lore.kernel.org/platform-driver-x86/20261112074823.853819-1-aaron.ma@canonical.com/
> v1: https://lore.kernel.org/platform-driver-x86/20261111083529.727285-1-aaron.ma@canonical.com/
> 
>  drivers/platform/x86/lenovo/wmi-other.c | 66 ++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index bd64e7b44669d..340159df5f7cc 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -189,10 +189,10 @@ MODULE_PARM_DESC(relax_fan_constraint,
>  static bool force_load_psy_ext;
>  module_param(force_load_psy_ext, bool, 0444);
>  MODULE_PARM_DESC(force_load_psy_ext,
> -	"This option will skip checking if the ideapad_laptop driver will conflict "
> +	"This option will skip checking if the ideapad_laptop or thinkpad_acpi driver will conflict "
>  	"with adding an extension to set the battery charge behavior and battery charge "
>  	"control end threshold. It will also skip checking if the BIOS reports that "
> -	"those features are fully supported. It is recommended to blacklist the ideapad "
> +	"those features are fully supported. It is recommended to blacklist the conflicting "
>  	"driver before using this option.");
>  
>  /* ======== HWMON (component: lenovo-wmi-capdata 00 & fan) ======== */
> @@ -905,17 +905,41 @@ static acpi_status lwmi_acpi_match(acpi_handle handle, u32 lvl,
>  	return AE_CTRL_TERMINATE;
>  }
>  
> +struct lwmi_psy_deconflict {
> +	char * const *hids;

Who is only the pointer const, not the char?

> +	char * const *methods;

If acpi_has_method would input const char * (to me it seems convertable 
as it only calls acpi_get_handle that takes const char *), this too could 
be const char.

> +};
> +
> +#if IS_ENABLED(CONFIG_IDEAPAD_LAPTOP) || IS_ENABLED(CONFIG_THINKPAD_ACPI)
> +#define LWMI_PSY_DECONFLICT_STRS(...) \
> +	((char * const[]) { __VA_ARGS__, NULL })
> +#endif

What exactly does ifdef'ing this construct achieve? Does something hunt 
unused macros?

> +
> +static const struct lwmi_psy_deconflict lwmi_psy_deconflict_list[] = {
> +#if IS_ENABLED(CONFIG_IDEAPAD_LAPTOP)
> +	{
> +		.hids = LWMI_PSY_DECONFLICT_STRS("VPC2004"),
> +		.methods = LWMI_PSY_DECONFLICT_STRS("GBMD", "SBMC"),
> +	},
> +#endif /* IS_ENABLED(CONFIG_IDEAPAD_LAPTOP) */
> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> +	{
> +		.hids = LWMI_PSY_DECONFLICT_STRS("LEN0268", "LEN0068", "IBM0068"),
> +		.methods = LWMI_PSY_DECONFLICT_STRS("BDSG", "BICG"),
> +	},
> +#endif /* IS_ENABLED(CONFIG_THINKPAD_ACPI) */
> +};
> +
>  /**
>   * lwmi_om_psy_ext_init() - Hooks power supply extension to device battery
>   * @priv: Pointer to the lwmi_om_priv drvdata.
>   *
> - * Checks if the ideapad_laptop driver is going to manage charge attributes first,
> - * then if not, hooks the battery to our WMI methods if they are supported.
> + * Checks if the ideapad_laptop or thinkpad_acpi driver is going to manage
> + * charge attributes first, then if not, hooks the battery to our WMI methods
> + * if they are supported.
>   */
>  static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
>  {
> -	static const char * const ideapad_hid = "VPC2004";
> -	acpi_handle handle = NULL;
>  	unsigned int props = 0;
>  	int ret;
>  
> @@ -928,14 +952,30 @@ static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
>  	if (force_load_psy_ext)
>  		goto load_psy_ext;
>  
> -	/* Deconflict ideapad_laptop driver */
> -	ret = acpi_get_devices(ideapad_hid, lwmi_acpi_match, &handle, NULL);
> -	if (ret)
> -		return;
> +	for (unsigned int i = 0; i < ARRAY_SIZE(lwmi_psy_deconflict_list); i++) {
> +		const struct lwmi_psy_deconflict *entry = &lwmi_psy_deconflict_list[i];
>  
> -	if (handle && acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC")) {
> -		dev_dbg(&priv->wdev->dev, "ideapad_laptop driver manages battery for device\n");
> -		return;
> +		for (unsigned int j = 0; entry->hids[j]; j++) {
> +			const char *hid = entry->hids[j];
> +			acpi_handle handle = NULL;
> +			bool has_methods = true;
> +
> +			ret = acpi_get_devices(hid, lwmi_acpi_match, &handle, NULL);
> +			if (ret || !handle)
> +				continue;
> +
> +			for (unsigned int k = 0; entry->methods[k]; k++) {
> +				has_methods = acpi_has_method(handle, entry->methods[k]);
> +				if (!has_methods)
> +					break;

So this goes to dev_dbg + return, why complicate the code flow with extra 
break + check that is never true?

If indentation is the worry here, the innermost bodies don't seem to 
require much from the outer ones so they could be moved to a function 
(j body + k loop).

> +			}
> +			if (!has_methods)
> +				continue;
> +
> +			dev_dbg(&priv->wdev->dev, "ACPI device %s manages battery for device\n",
> +				hid);
> +			return;
> +		}
>  	}
>  
>  load_psy_ext:
> 

-- 
 i.