Message-ID: <my3aleryrl57cp3lc7y2puh45rccgxmkvlkyib5537zvhjnnwc@bfbyknryvp2u>
Date: Thu, 13 Nov 2025 12:08:32 +0100
From: Benjamin Tissoires <bentiss@...nel.org>
To: Lucas Zampieri <lzampier@...hat.com>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jiri Kosina <jikos@...nel.org>, Sebastian Reichel <sre@...nel.org>, 
	Bastien Nocera <hadess@...ess.net>, linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH v2 3/3] HID: input: Add support for multiple
 batteries per device

On Nov 13 2025, Lucas Zampieri wrote:
> Enable HID devices to register and manage multiple batteries by
> maintaining a list of hid_battery structures, each identified by
> its report ID.
> 
> The legacy dev->battery field and related fields are maintained for
> backward compatibility, pointing to the first battery in the list.
> This allows existing code to continue working unchanged while
> enabling new functionality for multi-battery devices.
> 
> Example hardware that can benefit from this:
> - Gaming headsets with charging docks (e.g., SteelSeries Arctis Nova Pro
>   Wireless)
> - Graphics tablets with stylus batteries (Wacom)
> - Wireless earbuds with per-earbud batteries plus charging case
> - Split keyboards with independent battery per side
> 
> Signed-off-by: Lucas Zampieri <lzampier@...hat.com>
> ---
>  drivers/hid/hid-core.c  |  4 ++
>  drivers/hid/hid-input.c | 99 +++++++++++++++++++++++++++--------------
>  include/linux/hid.h     | 12 ++++-
>  3 files changed, 80 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index a5b3a8ca2fcb..76d628547e9a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2990,6 +2990,10 @@ struct hid_device *hid_allocate_device(void)
>  	mutex_init(&hdev->ll_open_lock);
>  	kref_init(&hdev->ref);
> 
> +#ifdef CONFIG_HID_BATTERY_STRENGTH
> +	INIT_LIST_HEAD(&hdev->batteries);
> +#endif
> +
>  	ret = hid_bpf_device_init(hdev);
>  	if (ret)
>  		goto out_err;
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 0e71efea9da3..9d0be3d4ce04 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -520,14 +520,20 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
>  	unsigned quirks;
>  	s32 min, max;
>  	int error;
> +	int battery_num = 0;
> 
> -	if (dev->battery)
> -		return 0;	/* already initialized? */
> +	/* Check if battery with this report_id already exists */
> +	list_for_each_entry(bat, &dev->batteries, list) {
> +		if (bat->report_id == field->report->id)
> +			return 0;	/* already initialized */

I wonder if we should not make this test stick out a little bit more.

Something like "get_battery(field)" which would return a battery if the
field matches. This way, if we ever encounter multiple batteries
reported on the same report ID, we can always split them by logical
collection, physical, or something else.

> +		battery_num++;
> +	}
> 
>  	quirks = find_battery_quirk(dev);
> 
> -	hid_dbg(dev, "device %x:%x:%x %d quirks %d\n",
> -		dev->bus, dev->vendor, dev->product, dev->version, quirks);
> +	hid_dbg(dev, "device %x:%x:%x %d quirks %d report_id %d\n",
> +		dev->bus, dev->vendor, dev->product, dev->version, quirks,
> +		field->report->id);
> 
>  	if (quirks & HID_BATTERY_QUIRK_IGNORE)
>  		return 0;
> @@ -542,9 +548,17 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
>  		goto err_free_bat;
>  	}
> 
> -	psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> -				   strlen(dev->uniq) ?
> -					dev->uniq : dev_name(&dev->dev));
> +	/* Create unique name for each battery based on report ID */
> +	if (battery_num == 0) {
> +		psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> +					   strlen(dev->uniq) ?
> +						dev->uniq : dev_name(&dev->dev));
> +	} else {
> +		psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery-%d",
> +					   strlen(dev->uniq) ?
> +						dev->uniq : dev_name(&dev->dev),
> +					   battery_num);
> +	}

Not sure how much conservative you need to be here, but I would prefer
we stick to the same naming pattern whether this is the first or second
battery.

>  	if (!psy_desc->name) {
>  		error = -ENOMEM;
>  		goto err_free_desc;
> @@ -597,15 +611,23 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
> 
>  	power_supply_powers(bat->ps, &dev->dev);
> 
> -	/* Maintain legacy single battery fields for backward compatibility */
> -	dev->battery = bat->ps;
> -	dev->battery_min = bat->min;
> -	dev->battery_max = bat->max;
> -	dev->battery_report_type = bat->report_type;
> -	dev->battery_report_id = bat->report_id;
> -	dev->battery_charge_status = bat->charge_status;
> -	dev->battery_status = bat->status;
> -	dev->battery_avoid_query = bat->avoid_query;
> +	list_add_tail(&bat->list, &dev->batteries);
> +
> +	/*
> +	 * The legacy single battery API is preserved by exposing the first
> +	 * discovered battery. Systems relying on a single battery view maintain
> +	 * unchanged behavior.
> +	 */
> +	if (battery_num == 0) {
> +		dev->battery = bat->ps;
> +		dev->battery_min = bat->min;
> +		dev->battery_max = bat->max;
> +		dev->battery_report_type = bat->report_type;
> +		dev->battery_report_id = bat->report_id;
> +		dev->battery_charge_status = bat->charge_status;
> +		dev->battery_status = bat->status;
> +		dev->battery_avoid_query = bat->avoid_query;
> +	}

Again, why keeping the old fields? Are they used anywhere?

> 
>  	return 0;
> 
> @@ -620,21 +642,33 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
> 
>  static void hidinput_cleanup_battery(struct hid_device *dev)
>  {
> -	struct hid_battery *bat;
> +	struct hid_battery *bat, *next;
>  	const struct power_supply_desc *psy_desc;
> 
> -	if (!dev->battery)
> -		return;
> +	list_for_each_entry_safe(bat, next, &dev->batteries, list) {
> +		psy_desc = bat->ps->desc;
> +		power_supply_unregister(bat->ps);
> +		kfree(psy_desc->name);
> +		kfree(psy_desc);
> +		list_del(&bat->list);
> +		kfree(bat);
> +	}

Really, switching to devm the batteries would greatly help not making
mistakes here.

> 
> -	bat = power_supply_get_drvdata(dev->battery);
> -	psy_desc = dev->battery->desc;
> -	power_supply_unregister(dev->battery);
> -	kfree(psy_desc->name);
> -	kfree(psy_desc);
> -	kfree(bat);
>  	dev->battery = NULL;
>  }
> 
> +static struct hid_battery *hidinput_find_battery(struct hid_device *dev,
> +						 int report_id)
> +{
> +	struct hid_battery *bat;
> +
> +	list_for_each_entry(bat, &dev->batteries, list) {
> +		if (bat->report_id == report_id)
> +			return bat;
> +	}
> +	return NULL;
> +}

Oh, so you already have this function. So why not making use of it in
hidinput_setup_battery()?

> +
>  static bool hidinput_update_battery_charge_status(struct hid_battery *bat,
>  						  unsigned int usage, int value)
>  {
> @@ -652,17 +686,16 @@ static bool hidinput_update_battery_charge_status(struct hid_battery *bat,
>  	return false;
>  }
> 
> -static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> -				    int value)
> +static void hidinput_update_battery(struct hid_device *dev, int report_id,
> +				    unsigned int usage, int value)
>  {
>  	struct hid_battery *bat;
>  	int capacity;
> 
> -	if (!dev->battery)
> +	bat = hidinput_find_battery(dev, report_id);
> +	if (!bat)
>  		return;
> 
> -	bat = power_supply_get_drvdata(dev->battery);
> -
>  	if (hidinput_update_battery_charge_status(bat, usage, value)) {
>  		power_supply_changed(bat->ps);
>  		return;
> @@ -705,8 +738,8 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
>  {
>  }
> 
> -static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> -				    int value)
> +static void hidinput_update_battery(struct hid_device *dev, int report_id,
> +				    unsigned int usage, int value)

Why not hidinput_update_battery(struct hid_battery *bat, unsigned int usage, int value)?

>  {
>  }
>  #endif	/* CONFIG_HID_BATTERY_STRENGTH */
> @@ -1574,7 +1607,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>  		return;
> 
>  	if (usage->type == EV_PWR) {
> -		hidinput_update_battery(hid, usage->hid, value);
> +		hidinput_update_battery(hid, report->id, usage->hid, value);

With the suggested change in hidinput_update_battery, first query the
battery and then change it.

>  		return;
>  	}
> 
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 63422130de20..a6e36835fb3c 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -700,8 +700,16 @@ struct hid_device {
>  #ifdef CONFIG_HID_BATTERY_STRENGTH
>  	/*
>  	 * Power supply information for HID devices which report
> -	 * battery strength. power_supply was successfully registered if
> -	 * battery is non-NULL.
> +	 * battery strength. Each battery is tracked separately in the
> +	 * batteries list.
> +	 */
> +	struct list_head batteries;		/* List of hid_battery structures */
> +
> +	/*
> +	 * Legacy single battery support - kept for backwards compatibility.
> +	 * Points to the first battery in the list if any exists.
> +	 * power_supply was successfully registered if battery is non-NULL.
> +	 * DEPRECATED: New code should iterate through batteries list instead.
>  	 */
>  	struct power_supply *battery;

Nah. hid_device is pure internal interface. So change all the users if
needed, but don't keep something around in the hope that others will do
the work for you.

For references there has been a very long discussion with Linus about
API changes, and the result was that any API change that introduced a
duplicate API was probably a bad design ;)

Cheers,
Benjamin

>  	__s32 battery_capacity;
> --
> 2.51.1
> 

Powered by blists - more mailing lists