Refactor the battery handling code to use the newly introduced
struct hid_battery internally, replacing direct access to individual
power_supply and state fields.
The legacy dev->battery and dev->battery_* fields are maintained and
synchronized for backward compatibility. This refactoring prepares
the code for supporting multiple batteries per device in a subsequent
patch.
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
drivers/hid/hid-input.c | 125 ++++++++++++++++++++++++++--------------
1 file changed, 83 insertions(+), 42 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index e56e7de53279..0e71efea9da3 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,8 +514,9 @@ 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;
@@ -530,16 +532,22 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
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 = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
+ if (!psy_desc) {
+ error = -ENOMEM;
+ goto err_free_bat;
+ }
+
psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
strlen(dev->uniq) ?
dev->uniq : dev_name(&dev->dev));
if (!psy_desc->name) {
error = -ENOMEM;
- goto err_free_mem;
+ goto err_free_desc;
}
psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
@@ -559,63 +567,85 @@ 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);
+
+ /* 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;
+
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;
const struct power_supply_desc *psy_desc;
if (!dev->battery)
return;
+ 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 bool hidinput_update_battery_charge_status(struct hid_device *dev,
+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;
+ /* Update legacy field for backward compatibility */
+ if (bat->dev->battery == bat->ps)
+ bat->dev->battery_charge_status = bat->charge_status;
return true;
}
@@ -625,32 +655,43 @@ static bool hidinput_update_battery_charge_status(struct hid_device *dev,
static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
int value)
{
+ struct hid_battery *bat;
int capacity;
if (!dev->battery)
return;
- if (hidinput_update_battery_charge_status(dev, usage, value)) {
- power_supply_changed(dev->battery);
+ bat = power_supply_get_drvdata(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);
+
+ /* Update legacy fields for backward compatibility */
+ 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 */
--
2.51.1On Nov 13 2025, Lucas Zampieri wrote:
> Refactor the battery handling code to use the newly introduced
> struct hid_battery internally, replacing direct access to individual
> power_supply and state fields.
>
> The legacy dev->battery and dev->battery_* fields are maintained and
> synchronized for backward compatibility. This refactoring prepares
> the code for supporting multiple batteries per device in a subsequent
> patch.
>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> ---
> drivers/hid/hid-input.c | 125 ++++++++++++++++++++++++++--------------
> 1 file changed, 83 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index e56e7de53279..0e71efea9da3 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,8 +514,9 @@ 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;
> @@ -530,16 +532,22 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
> 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 = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
> + if (!psy_desc) {
> + error = -ENOMEM;
> + goto err_free_bat;
> + }
> +
> psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> strlen(dev->uniq) ?
> dev->uniq : dev_name(&dev->dev));
> if (!psy_desc->name) {
> error = -ENOMEM;
> - goto err_free_mem;
> + goto err_free_desc;
I wonder if a devm conversion of the whole battery support handling
would not be beneficial here.
> }
>
> psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> @@ -559,63 +567,85 @@ 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);
> +
> + /* 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;
Already mentioned in the cover letter, but what's the point of keeping
those legacy fields when the exact same data is stored in bat?
> +
> 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;
> const struct power_supply_desc *psy_desc;
>
> if (!dev->battery)
> return;
>
> + 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 bool hidinput_update_battery_charge_status(struct hid_device *dev,
> +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;
> + /* Update legacy field for backward compatibility */
> + if (bat->dev->battery == bat->ps)
> + bat->dev->battery_charge_status = bat->charge_status;
> return true;
> }
>
> @@ -625,32 +655,43 @@ static bool hidinput_update_battery_charge_status(struct hid_device *dev,
> static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> int value)
> {
> + struct hid_battery *bat;
> int capacity;
>
> if (!dev->battery)
> return;
>
> - if (hidinput_update_battery_charge_status(dev, usage, value)) {
> - power_supply_changed(dev->battery);
> + bat = power_supply_get_drvdata(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);
> +
> + /* Update legacy fields for backward compatibility */
> + 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 */
> --
> 2.51.1
>
Cheers,
Benjamin
On Thu, Nov 13, 2025 at 10:54 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Nov 13 2025, Lucas Zampieri wrote:
> > Refactor the battery handling code to use the newly introduced
> > struct hid_battery internally, replacing direct access to individual
> > power_supply and state fields.
> >
> > The legacy dev->battery and dev->battery_* fields are maintained and
> > synchronized for backward compatibility. This refactoring prepares
> > the code for supporting multiple batteries per device in a subsequent
> > patch.
> >
> > Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> > ---
> > drivers/hid/hid-input.c | 125 ++++++++++++++++++++++++++--------------
> > 1 file changed, 83 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index e56e7de53279..0e71efea9da3 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,8 +514,9 @@ 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;
> > @@ -530,16 +532,22 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
> > 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 = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
> > + if (!psy_desc) {
> > + error = -ENOMEM;
> > + goto err_free_bat;
> > + }
> > +
> > psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> > strlen(dev->uniq) ?
> > dev->uniq : dev_name(&dev->dev));
> > if (!psy_desc->name) {
> > error = -ENOMEM;
> > - goto err_free_mem;
> > + goto err_free_desc;
>
> I wonder if a devm conversion of the whole battery support handling
> would not be beneficial here.
>
I'll give it a try and see how it looks.
> > }
> >
> > psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > @@ -559,63 +567,85 @@ 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);
> > +
> > + /* 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;
>
> Already mentioned in the cover letter, but what's the point of keeping
> those legacy fields when the exact same data is stored in bat?
>
Yep, now that I know that is better to have cohereand-API instead of
maintaining backwards compatiliby to a single battery structure that
was only internal, I'll update those as well.
> > +
> > 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;
> > const struct power_supply_desc *psy_desc;
> >
> > if (!dev->battery)
> > return;
> >
> > + 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 bool hidinput_update_battery_charge_status(struct hid_device *dev,
> > +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;
> > + /* Update legacy field for backward compatibility */
> > + if (bat->dev->battery == bat->ps)
> > + bat->dev->battery_charge_status = bat->charge_status;
> > return true;
> > }
> >
> > @@ -625,32 +655,43 @@ static bool hidinput_update_battery_charge_status(struct hid_device *dev,
> > static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> > int value)
> > {
> > + struct hid_battery *bat;
> > int capacity;
> >
> > if (!dev->battery)
> > return;
> >
> > - if (hidinput_update_battery_charge_status(dev, usage, value)) {
> > - power_supply_changed(dev->battery);
> > + bat = power_supply_get_drvdata(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);
> > +
> > + /* Update legacy fields for backward compatibility */
> > + 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 */
> > --
> > 2.51.1
> >
>
> Cheers,
> Benjamin
>
Best,
Lucas Zampieri
© 2016 - 2025 Red Hat, Inc.