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@redhat.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 */
+ 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);
+ }
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;
+ }
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);
+ }
- 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;
+}
+
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)
{
}
#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);
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;
__s32 battery_capacity;
--
2.51.1On 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@redhat.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
>
On Thu, Nov 13, 2025 at 11:08 AM Benjamin Tissoires <bentiss@kernel.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@redhat.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
© 2016 - 2025 Red Hat, Inc.