Message-ID: <CAOOg__Ds=0EU=pS3ZxYONSqr1rncmz89pn1RpRbgTqvtTdRXgQ@mail.gmail.com>
Date: Wed, 12 Nov 2025 21:36:51 +0000
From: Lucas Zampieri <lzampier@...hat.com>
To: Bastien Nocera <hadess@...ess.net>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>, 
	Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH 1/1] HID: input: Add support for multiple batteries
 per device

Hey Bastien,

On Wed, Nov 12, 2025 at 2:46 PM Bastien Nocera <hadess@...ess.net> wrote:
>
> Hey Lucas,
>
> (Follow-up to a chat we had about this patch privately)
>
> On Tue, 2025-11-11 at 10:56 +0000, Lucas Zampieri wrote:
> > Add support for multiple batteries per HID device by introducing
> > struct hid_battery to encapsulate individual battery state and using
> > a list to track multiple batteries identified by report ID. The
> > legacy
> > dev->battery field is maintained for backwards compatibility.
>
> The cover letter mentions specific hardware, you probably want to
> mention this in the commit message itself, as the cover letter will be
> disconnected from this commit once this gets merged. Don't hesitate to
> link to product pages directly if you want to show specific products as
> potential users of that capability.
>
Got it, I'll update the commits in the v2 with that in mind.

> You mentioned that you tested this patchset with a custom firmware for
> a split keyboard. It would be great if the firmware could be made
> available to show how this was tested and mention that in the commit
> message.
>
I've pushed my custom firmware to
https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting,
if this series gets merged, I'll also propose that change to upstream
zmk project.
I'll also add links to in the v2 of the cover-letter

> bentiss will also likely want a hid-recorder output for the device that
> shows the batteries being instantiated. This would also likely be used
> to test whether upower continues working as expected.
>
Ack, I'll get the hid-recorder output and add to the testing section
of my cover letter.

> Talking of upower, I think we'll need an systemd/hwdb + upower changes
> to differentiate batteries within a single device, as I don't think we
> can have enough metadata in the HID report to differentiate them.
>
> Last comment about the patch itself, do you think it would be feasible
> to split this in 2 or 3? One to introduce the hid_battery struct,
> another to use it to replace direct power_supply access, and finally
> one to allow a list of hid_batteries?
>
> Don't hesitate to CC: on future versions.
>
For sure, thanks for the feedback

> Cheers
>
> >
> > Signed-off-by: Lucas Zampieri <lzampier@...hat.com>
> > ---
> >  drivers/hid/hid-core.c  |   4 +
> >  drivers/hid/hid-input.c | 193 +++++++++++++++++++++++++++-----------
> > --
> >  include/linux/hid.h     |  42 ++++++++-
> >  3 files changed, 176 insertions(+), 63 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 e56e7de53279..071df319775b 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -454,7 +454,8 @@ static int hidinput_get_battery_property(struct
> > power_supply *psy,
> >                                        enum power_supply_property
> > prop,
> >                                        union power_supply_propval
> > *val)
> >  {
> > -     struct hid_device *dev = power_supply_get_drvdata(psy);
> > +     struct hid_battery *bat = power_supply_get_drvdata(psy);
> > +     struct hid_device *dev = bat->dev;
> >       int value;
> >       int ret = 0;
> >
> > @@ -465,13 +466,13 @@ static int hidinput_get_battery_property(struct
> > power_supply *psy,
> >               break;
> >
> >       case POWER_SUPPLY_PROP_CAPACITY:
> > -             if (dev->battery_status != HID_BATTERY_REPORTED &&
> > -                 !dev->battery_avoid_query) {
> > +             if (bat->status != HID_BATTERY_REPORTED &&
> > +                 !bat->avoid_query) {
> >                       value =
> > hidinput_query_battery_capacity(dev);
> >                       if (value < 0)
> >                               return value;
> >               } else  {
> > -                     value = dev->battery_capacity;
> > +                     value = bat->capacity;
> >               }
> >
> >               val->intval = value;
> > @@ -482,20 +483,20 @@ static int hidinput_get_battery_property(struct
> > power_supply *psy,
> >               break;
> >
> >       case POWER_SUPPLY_PROP_STATUS:
> > -             if (dev->battery_status != HID_BATTERY_REPORTED &&
> > -                 !dev->battery_avoid_query) {
> > +             if (bat->status != HID_BATTERY_REPORTED &&
> > +                 !bat->avoid_query) {
> >                       value =
> > hidinput_query_battery_capacity(dev);
> >                       if (value < 0)
> >                               return value;
> >
> > -                     dev->battery_capacity = value;
> > -                     dev->battery_status = HID_BATTERY_QUERIED;
> > +                     bat->capacity = value;
> > +                     bat->status = HID_BATTERY_QUERIED;
> >               }
> >
> > -             if (dev->battery_status == HID_BATTERY_UNKNOWN)
> > +             if (bat->status == HID_BATTERY_UNKNOWN)
> >                       val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> >               else
> > -                     val->intval = dev->battery_charge_status;
> > +                     val->intval = bat->charge_status;
> >               break;
> >
> >       case POWER_SUPPLY_PROP_SCOPE:
> > @@ -513,33 +514,53 @@ static int hidinput_get_battery_property(struct
> > power_supply *psy,
> >  static int hidinput_setup_battery(struct hid_device *dev, unsigned
> > report_type,
> >                                 struct hid_field *field, bool
> > is_percentage)
> >  {
> > +     struct hid_battery *bat;
> >       struct power_supply_desc *psy_desc;
> > -     struct power_supply_config psy_cfg = { .drv_data = dev, };
> > +     struct power_supply_config psy_cfg;
> >       unsigned quirks;
> >       s32 min, max;
> >       int error;
> > +     int battery_num = 0;
> >
> > -     if (dev->battery)
> > -             return 0;       /* already initialized? */
> > +     list_for_each_entry(bat, &dev->batteries, list) {
> > +             if (bat->report_id == field->report->id)
> > +                     return 0;       /* already initialized */
> > +             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;
> >
> > -     psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
> > -     if (!psy_desc)
> > +     bat = kzalloc(sizeof(*bat), GFP_KERNEL);
> > +     if (!bat)
> >               return -ENOMEM;
> >
> > -     psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> > -                                strlen(dev->uniq) ?
> > -                                     dev->uniq : dev_name(&dev-
> > >dev));
> > +     psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
> > +     if (!psy_desc) {
> > +             error = -ENOMEM;
> > +             goto err_free_bat;
> > +     }
> > +
> > +     /* 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);
> > +     }
> >       if (!psy_desc->name) {
> >               error = -ENOMEM;
> > -             goto err_free_mem;
> > +             goto err_free_desc;
> >       }
> >
> >       psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > @@ -559,98 +580,148 @@ static int hidinput_setup_battery(struct
> > hid_device *dev, unsigned report_type,
> >       if (quirks & HID_BATTERY_QUIRK_FEATURE)
> >               report_type = HID_FEATURE_REPORT;
> >
> > -     dev->battery_min = min;
> > -     dev->battery_max = max;
> > -     dev->battery_report_type = report_type;
> > -     dev->battery_report_id = field->report->id;
> > -     dev->battery_charge_status =
> > POWER_SUPPLY_STATUS_DISCHARGING;
> > +     /* Initialize battery structure */
> > +     bat->dev = dev;
> > +     bat->min = min;
> > +     bat->max = max;
> > +     bat->report_type = report_type;
> > +     bat->report_id = field->report->id;
> > +     bat->charge_status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +     bat->status = HID_BATTERY_UNKNOWN;
> >
> >       /*
> >        * Stylus is normally not connected to the device and thus
> > we
> >        * can't query the device and get meaningful battery
> > strength.
> >        * We have to wait for the device to report it on its own.
> >        */
> > -     dev->battery_avoid_query = report_type == HID_INPUT_REPORT
> > &&
> > -                                field->physical == HID_DG_STYLUS;
> > +     bat->avoid_query = report_type == HID_INPUT_REPORT &&
> > +                        field->physical == HID_DG_STYLUS;
> >
> >       if (quirks & HID_BATTERY_QUIRK_AVOID_QUERY)
> > -             dev->battery_avoid_query = true;
> > +             bat->avoid_query = true;
> >
> > -     dev->battery = power_supply_register(&dev->dev, psy_desc,
> > &psy_cfg);
> > -     if (IS_ERR(dev->battery)) {
> > -             error = PTR_ERR(dev->battery);
> > +     psy_cfg.drv_data = bat;
> > +     bat->ps = power_supply_register(&dev->dev, psy_desc,
> > &psy_cfg);
> > +     if (IS_ERR(bat->ps)) {
> > +             error = PTR_ERR(bat->ps);
> >               hid_warn(dev, "can't register power supply: %d\n",
> > error);
> >               goto err_free_name;
> >       }
> >
> > -     power_supply_powers(dev->battery, &dev->dev);
> > +     power_supply_powers(bat->ps, &dev->dev);
> > +
> > +     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;
> > +     }
> > +
> >       return 0;
> >
> >  err_free_name:
> >       kfree(psy_desc->name);
> > -err_free_mem:
> > +err_free_desc:
> >       kfree(psy_desc);
> > -     dev->battery = NULL;
> > +err_free_bat:
> > +     kfree(bat);
> >       return error;
> >  }
> >
> >  static void hidinput_cleanup_battery(struct hid_device *dev)
> >  {
> > +     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);
> > +     }
> >
> > -     psy_desc = dev->battery->desc;
> > -     power_supply_unregister(dev->battery);
> > -     kfree(psy_desc->name);
> > -     kfree(psy_desc);
> >       dev->battery = NULL;
> >  }
> >
> > -static bool hidinput_update_battery_charge_status(struct hid_device
> > *dev,
> > +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;
> > +     }Tested with Dactyl 5x6 split keyboard usin
> > +     return NULL;
> > +}
> > +
> > +static bool hidinput_update_battery_charge_status(struct hid_battery
> > *bat,
> >                                                 unsigned int
> > usage, int value)
> >  {
> >       switch (usage) {
> >       case HID_BAT_CHARGING:
> > -             dev->battery_charge_status = value ?
> > -
> > POWER_SUPPLY_STATUS_CHARGING :
> > -
> > POWER_SUPPLY_STATUS_DISCHARGING;
> > +             bat->charge_status = value ?
> > +                                  POWER_SUPPLY_STATUS_CHARGING :
> > +
> > POWER_SUPPLY_STATUS_DISCHARGING;
> > +             if (bat->dev->battery == bat->ps)
> > +                     bat->dev->battery_charge_status = bat-
> > >charge_status;
> >               return true;
> >       }
> >
> >       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;
> >
> > -     if (hidinput_update_battery_charge_status(dev, usage,
> > value)) {
> > -             power_supply_changed(dev->battery);
> > +     if (hidinput_update_battery_charge_status(bat, usage,
> > value)) {
> > +             power_supply_changed(bat->ps);
> >               return;
> >       }
> >
> >       if ((usage & HID_USAGE_PAGE) == HID_UP_DIGITIZER && value ==
> > 0)
> >               return;
> >
> > -     if (value < dev->battery_min || value > dev->battery_max)
> > +     if (value < bat->min || value > bat->max)
> >               return;
> >
> >       capacity = hidinput_scale_battery_capacity(dev, value);
> >
> > -     if (dev->battery_status != HID_BATTERY_REPORTED ||
> > -         capacity != dev->battery_capacity ||
> > -         ktime_after(ktime_get_coarse(), dev-
> > >battery_ratelimit_time)) {
> > -             dev->battery_capacity = capacity;
> > -             dev->battery_status = HID_BATTERY_REPORTED;
> > -             dev->battery_ratelimit_time =
> > +     if (bat->status != HID_BATTERY_REPORTED ||
> > +         capacity != bat->capacity ||
> > +         ktime_after(ktime_get_coarse(), bat->ratelimit_time)) {
> > +             bat->capacity = capacity;
> > +             bat->status = HID_BATTERY_REPORTED;
> > +             bat->ratelimit_time =
> >                       ktime_add_ms(ktime_get_coarse(), 30 * 1000);
> > -             power_supply_changed(dev->battery);
> > +
> > +             if (dev->battery == bat->ps) {
> > +                     dev->battery_capacity = bat->capacity;
> > +                     dev->battery_status = bat->status;
> > +                     dev->battery_ratelimit_time = bat-
> > >ratelimit_time;
> > +             }
> > +
> > +             power_supply_changed(bat->ps);
> >       }
> >  }
> >  #else  /* !CONFIG_HID_BATTERY_STRENGTH */
> > @@ -664,8 +735,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)
> >  {
> >  }
> >  #endif       /* CONFIG_HID_BATTERY_STRENGTH */
> > @@ -1533,7 +1604,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);
> >               return;
> >       }
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index a4ddb94e3ee5..a6e36835fb3c 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -634,6 +634,36 @@ enum hid_battery_status {
> >       HID_BATTERY_REPORTED,           /* Device sent unsolicited
> > battery strength report */
> >  };
> >
> > +/**
> > + * struct hid_battery - represents a single battery power supply
> > + * @list: list node for linking into hid_device's battery list
> > + * @dev: pointer to the parent hid_device
> > + * @ps: the power supply device
> > + * @capacity: current battery capacity
> > + * @min: minimum battery value
> > + * @max: maximum battery value
> > + * @report_type: type of report (HID_INPUT_REPORT,
> > HID_FEATURE_REPORT)
> > + * @report_id: report ID for this battery
> > + * @charge_status: current charge status
> > + * @status: battery status (unknown, queried, reported)
> > + * @avoid_query: if true, don't query battery (wait for device
> > reports)
> > + * @ratelimit_time: time for rate limiting battery updates
> > + */
> > +struct hid_battery {
> > +     struct list_head list;
> > +     struct hid_device *dev;
> > +     struct power_supply *ps;
> > +     __s32 capacity;
> > +     __s32 min;
> > +     __s32 max;
> > +     __s32 report_type;
> > +     __s32 report_id;
> > +     __s32 charge_status;
> > +     enum hid_battery_status status;
> > +     bool avoid_query;
> > +     ktime_t ratelimit_time;
> > +};
> > +
> >  struct hid_driver;
> >  struct hid_ll_driver;
> >
> > @@ -670,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;
> >       __s32 battery_capacity;
>
Best,

Powered by blists - more mailing lists