Message-ID: <CAOOg__BnRj78Govo8Dchtxng7uBB30ZUdD2zJhZYszQgfB9N2g@mail.gmail.com>
Date: Sun, 16 Nov 2025 11:54:32 +0000
From: Lucas Zampieri <lzampier@...hat.com>
To: Benjamin Tissoires <bentiss@...nel.org>
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 Thu, Nov 13, 2025 at 11:08 AM Benjamin Tissoires <bentiss@...nel.org> wrote:
>
> 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.
>
I'll add that, and test it in my custom fw.

> > +             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.
>
Agree completely, honestly that was the ugly bit for me, and the only
reason I keep that was for 'backwards compatibility' but now I know
better, and Ill made those consistent.

> >       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?
>
Ack, and updating those on v3.

> >
> >       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()?
>
"backwards compatibility" lol, but yeah in v2 I implemented it
half-way, in v3 I'll make a proper use of it with the full list
conversion.

> > +
> >  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)?
>
I see, this is much more elegant, as I can avoid the lookup inside the
funcion that way, let me use that on the v3.

> >  {
> >  }
> >  #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.
>
Ack
> >               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 ;)
>
Ack, and that was the knowledge that I was missing and why I stuck to
much to making it backwards compatible, thanks for that really.

> Cheers,
> Benjamin
>
> >       __s32 battery_capacity;
> > --
> > 2.51.1
> >
>

Best,
Lucas Zampieri

Powered by blists - more mailing lists