Linux Input/HID development
 help / color / mirror / Atom feed
* Re: [RFC PATCH 1/1] HID: input: Add support for multiple batteries per device
From: Bastien Nocera @ 2025-11-12 14:15 UTC (permalink / raw)
  To: Lucas Zampieri, linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
	linux-pm
In-Reply-To: <20251111105634.1684751-2-lzampier@redhat.com>

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.

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.

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.

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.

Cheers

> 
> Signed-off-by: Lucas Zampieri <lzampier@redhat.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;
> +	}
> +	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;

^ permalink raw reply

* [PATCH v3] hid/hid-multitouch: Keep latency normal on deactivate for reactivation gesture
From: Werner Sembach @ 2025-11-12 14:47 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires; +Cc: Werner Sembach, linux-input, linux-kernel

Uniwill devices have a built in gesture in the touchpad to de- and
reactivate it by double taping the upper left corner. This gesture stops
working when latency is set to high, so this patch keeps the latency on
normal.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Cc: stable@vger.kernel.org
---
V1->V2: Use a quirk to narrow down the devices this is applied to.
V2->V3: Fix this patch breaking touchpads on some devices.
        Add another device ID.

I have three Uniwill devices at hand right now that have at least two
physically different touchpads, but same Vendor + Product ID combination.
Maybe the vendor uses this product ID for all i2c connected touchpads, or
it is used as some kind of subvendor ID to indicate Uniwill?

To be able to really narrow it down to Uniwill only devices I would need to
check DMI strings, but then I will probably narrow it down to much as I
only know what we at TUXEDO use there.

 drivers/hid/hid-multitouch.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 179dc316b4b51..ed9eb4e0d5038 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -76,6 +76,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_DISABLE_WAKEUP		BIT(21)
 #define MT_QUIRK_ORIENTATION_INVERT	BIT(22)
 #define MT_QUIRK_APPLE_TOUCHBAR		BIT(23)
+#define MT_QUIRK_KEEP_LATENCY_ON_CLOSE	BIT(24)
 
 #define MT_INPUTMODE_TOUCHSCREEN	0x02
 #define MT_INPUTMODE_TOUCHPAD		0x03
@@ -211,6 +212,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
 #define MT_CLS_WIN_8_DISABLE_WAKEUP		0x0016
 #define MT_CLS_WIN_8_NO_STICKY_FINGERS		0x0017
 #define MT_CLS_WIN_8_FORCE_MULTI_INPUT_NSMU	0x0018
+#define MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE	0x0019
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -330,6 +332,15 @@ static const struct mt_class mt_classes[] = {
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_WIN8_PTP_BUTTONS,
 		.export_all_inputs = true },
+	{ .name = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_IGNORE_DUPLICATES |
+			MT_QUIRK_HOVERING |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_STICKY_FINGERS |
+			MT_QUIRK_WIN8_PTP_BUTTONS |
+			MT_QUIRK_KEEP_LATENCY_ON_CLOSE,
+		.export_all_inputs = true },
 
 	/*
 	 * vendor specific classes
@@ -1998,7 +2009,12 @@ static void mt_on_hid_hw_open(struct hid_device *hdev)
 
 static void mt_on_hid_hw_close(struct hid_device *hdev)
 {
-	mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
+	struct mt_device *td = hid_get_drvdata(hdev);
+
+	if (td->mtclass.quirks & MT_QUIRK_KEEP_LATENCY_ON_CLOSE)
+		mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_NONE);
+	else
+		mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
 }
 
 /*
@@ -2375,6 +2391,14 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
 			USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
 
+	/* Uniwill touchpads */
+	{ .driver_data = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
+		HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
+			USB_VENDOR_ID_PIXART, 0x0255) },
+	{ .driver_data = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
+		HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
+			USB_VENDOR_ID_PIXART, 0x0274) },
+
 	/* VTL panels */
 	{ .driver_data = MT_CLS_VTL,
 		MT_USB_DEVICE(USB_VENDOR_ID_VTL,
-- 
2.43.0


^ permalink raw reply related

* Re: [PATCH v3] hid/hid-multitouch: Keep latency normal on deactivate for reactivation gesture
From: Werner Sembach @ 2025-11-12 15:51 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20251112144837.499782-1-wse@tuxedocomputers.com>


Am 12.11.25 um 15:47 schrieb Werner Sembach:
> Uniwill devices have a built in gesture in the touchpad to de- and
> reactivate it by double taping the upper left corner. This gesture stops
> working when latency is set to high, so this patch keeps the latency on
> normal.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: stable@vger.kernel.org
> ---
> V1->V2: Use a quirk to narrow down the devices this is applied to.
> V2->V3: Fix this patch breaking touchpads on some devices.
>          Add another device ID.
>
> I have three Uniwill devices at hand right now that have at least two
> physically different touchpads, but same Vendor + Product ID combination.
> Maybe the vendor uses this product ID for all i2c connected touchpads, or
> it is used as some kind of subvendor ID to indicate Uniwill?
>
> To be able to really narrow it down to Uniwill only devices I would need to
> check DMI strings, but then I will probably narrow it down to much as I
> only know what we at TUXEDO use there.
>
>   drivers/hid/hid-multitouch.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 179dc316b4b51..ed9eb4e0d5038 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -76,6 +76,7 @@ MODULE_LICENSE("GPL");
>   #define MT_QUIRK_DISABLE_WAKEUP		BIT(21)
>   #define MT_QUIRK_ORIENTATION_INVERT	BIT(22)
>   #define MT_QUIRK_APPLE_TOUCHBAR		BIT(23)
> +#define MT_QUIRK_KEEP_LATENCY_ON_CLOSE	BIT(24)
>   
>   #define MT_INPUTMODE_TOUCHSCREEN	0x02
>   #define MT_INPUTMODE_TOUCHPAD		0x03
> @@ -211,6 +212,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
>   #define MT_CLS_WIN_8_DISABLE_WAKEUP		0x0016
>   #define MT_CLS_WIN_8_NO_STICKY_FINGERS		0x0017
>   #define MT_CLS_WIN_8_FORCE_MULTI_INPUT_NSMU	0x0018
> +#define MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE	0x0019
A college realized that at some points in the code some, but not all, of the 
MT_CLS_WIN_8* classes are checked for directly. Should I add my new class there 
too?
>   
>   /* vendor specific classes */
>   #define MT_CLS_3M				0x0101
> @@ -330,6 +332,15 @@ static const struct mt_class mt_classes[] = {
>   			MT_QUIRK_CONTACT_CNT_ACCURATE |
>   			MT_QUIRK_WIN8_PTP_BUTTONS,
>   		.export_all_inputs = true },
> +	{ .name = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
> +		.quirks = MT_QUIRK_ALWAYS_VALID |
> +			MT_QUIRK_IGNORE_DUPLICATES |
> +			MT_QUIRK_HOVERING |
> +			MT_QUIRK_CONTACT_CNT_ACCURATE |
> +			MT_QUIRK_STICKY_FINGERS |
> +			MT_QUIRK_WIN8_PTP_BUTTONS |
> +			MT_QUIRK_KEEP_LATENCY_ON_CLOSE,
> +		.export_all_inputs = true },
>   
>   	/*
>   	 * vendor specific classes
> @@ -1998,7 +2009,12 @@ static void mt_on_hid_hw_open(struct hid_device *hdev)
>   
>   static void mt_on_hid_hw_close(struct hid_device *hdev)
>   {
> -	mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +
> +	if (td->mtclass.quirks & MT_QUIRK_KEEP_LATENCY_ON_CLOSE)
> +		mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_NONE);
> +	else
> +		mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
>   }
>   
>   /*
> @@ -2375,6 +2391,14 @@ static const struct hid_device_id mt_devices[] = {
>   		MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
>   			USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
>   
> +	/* Uniwill touchpads */
> +	{ .driver_data = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
> +		HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> +			USB_VENDOR_ID_PIXART, 0x0255) },
> +	{ .driver_data = MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE,
> +		HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> +			USB_VENDOR_ID_PIXART, 0x0274) },
> +
>   	/* VTL panels */
>   	{ .driver_data = MT_CLS_VTL,
>   		MT_USB_DEVICE(USB_VENDOR_ID_VTL,

^ permalink raw reply

* [RESEND PATCH v2] Input: edt-ft5x06 - fix report rate handling by sysfs
From: Dario Binacchi @ 2025-11-12 17:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-amarula, Dario Binacchi, Dmitry Torokhov, Jens Reidel,
	Wolfram Sang, linux-input

In the driver probe, the report-rate-hz value from device tree is written
directly to the M12 controller register, while for the M06 it is divided
by 10 since the controller expects the value in units of 10 Hz. That logic
was missing in the sysfs handling, leading to inconsistent behavior
depending on whether the value came from device tree or sysfs.

This patch makes the report-rate handling consistent by applying the same
logic in both cases. Two dedicated functions, report_rate_hz_{show,store},
were added for the following reasons:

- Avoid modifying the more generic edt_ft5x06_setting_{show,store} and
  thus prevent regressions.
- Properly enforce lower and upper limits for the M06 case. The previous
  version accepted invalid values for M06, since it relied on the M12
  limits.
- Return an error when the property is not supported (e.g. M09), to avoid
  misleading users into thinking the property is handled by the
  controller.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v2:
- Drop the patch:
  1/2 Input: edt-ft5x06 - rename sysfs attribute report_rate to report_rate_hz
  because not accepted.

 drivers/input/touchscreen/edt-ft5x06.c | 158 +++++++++++++++++++++----
 1 file changed, 135 insertions(+), 23 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index bf498bd4dea9..d7a269a0528f 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -516,9 +516,136 @@ static EDT_ATTR(offset_y, S_IWUSR | S_IRUGO, NO_REGISTER, NO_REGISTER,
 /* m06: range 20 to 80, m09: range 0 to 30, m12: range 1 to 255... */
 static EDT_ATTR(threshold, S_IWUSR | S_IRUGO, WORK_REGISTER_THRESHOLD,
 		M09_REGISTER_THRESHOLD, EV_REGISTER_THRESHOLD, 0, 255);
-/* m06: range 3 to 14, m12: range 1 to 255 */
-static EDT_ATTR(report_rate, S_IWUSR | S_IRUGO, WORK_REGISTER_REPORT_RATE,
-		M12_REGISTER_REPORT_RATE, NO_REGISTER, 0, 255);
+
+static int edt_ft5x06_report_rate_get(struct edt_ft5x06_ts_data *tsdata)
+{
+	unsigned int val;
+	int error;
+
+	if (tsdata->reg_addr.reg_report_rate == NO_REGISTER)
+		return -EOPNOTSUPP;
+
+	error = regmap_read(tsdata->regmap, tsdata->reg_addr.reg_report_rate,
+			    &val);
+	if (error)
+		return error;
+
+	if (tsdata->version == EDT_M06)
+		val *= 10;
+
+	if (val != tsdata->report_rate) {
+		dev_warn(&tsdata->client->dev,
+			 "report-rate: read (%d) and stored value (%d) differ\n",
+			 val, tsdata->report_rate);
+		tsdata->report_rate = val;
+	}
+
+	return 0;
+}
+
+static ssize_t report_rate_show(struct device *dev,
+				struct device_attribute *dattr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
+	size_t count;
+	int error;
+
+	mutex_lock(&tsdata->mutex);
+
+	if (tsdata->factory_mode) {
+		error = -EIO;
+		goto out;
+	}
+
+	error = edt_ft5x06_report_rate_get(tsdata);
+	if (error) {
+		dev_err(&tsdata->client->dev,
+			"Failed to fetch attribute %s, error %d\n",
+			dattr->attr.name, error);
+		goto out;
+	}
+
+	count = sysfs_emit(buf, "%d\n", tsdata->report_rate);
+out:
+	mutex_unlock(&tsdata->mutex);
+	return error ?: count;
+}
+
+static int edt_ft5x06_report_rate_set(struct edt_ft5x06_ts_data *tsdata,
+				      unsigned int val)
+{
+	if (tsdata->reg_addr.reg_report_rate == NO_REGISTER)
+		return -EOPNOTSUPP;
+
+	if (tsdata->version == EDT_M06)
+		tsdata->report_rate = clamp_val(val, 30, 140);
+	else
+		tsdata->report_rate = clamp_val(val, 1, 255);
+
+	if (val != tsdata->report_rate) {
+		dev_warn(&tsdata->client->dev,
+			 "report-rate %dHz is unsupported, use %dHz\n",
+			 val, tsdata->report_rate);
+		val = tsdata->report_rate;
+	}
+
+	if (tsdata->version == EDT_M06)
+		val /= 10;
+
+	return regmap_write(tsdata->regmap, tsdata->reg_addr.reg_report_rate,
+			    val);
+}
+
+static ssize_t report_rate_store(struct device *dev,
+				 struct device_attribute *dattr,
+				 const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
+	unsigned int val;
+	u8 limit_low;
+	u8 limit_high;
+	int error;
+
+	mutex_lock(&tsdata->mutex);
+
+	if (tsdata->factory_mode) {
+		error = -EIO;
+		goto out;
+	}
+
+	error = kstrtouint(buf, 0, &val);
+	if (error)
+		goto out;
+
+	if (tsdata->version == EDT_M06) {
+		limit_low = 30;
+		limit_high = 140;
+	} else {
+		limit_low = 1;
+		limit_high = 255;
+	}
+
+	if (val < limit_low || val > limit_high) {
+		error = -ERANGE;
+		goto out;
+	}
+
+	error = edt_ft5x06_report_rate_set(tsdata, val);
+	if (error) {
+		dev_err(&tsdata->client->dev,
+			"Failed to update attribute %s, error: %d\n",
+			dattr->attr.name, error);
+		goto out;
+	}
+
+out:
+	mutex_unlock(&tsdata->mutex);
+	return error ?: count;
+}
+
+static DEVICE_ATTR_RW(report_rate);
 
 static ssize_t model_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
@@ -572,7 +699,7 @@ static struct attribute *edt_ft5x06_attrs[] = {
 	&edt_ft5x06_attr_offset_x.dattr.attr,
 	&edt_ft5x06_attr_offset_y.dattr.attr,
 	&edt_ft5x06_attr_threshold.dattr.attr,
-	&edt_ft5x06_attr_report_rate.dattr.attr,
+	&dev_attr_report_rate.attr,
 	&dev_attr_model.attr,
 	&dev_attr_fw_version.attr,
 	&dev_attr_header_errors.attr,
@@ -595,8 +722,7 @@ static void edt_ft5x06_restore_reg_parameters(struct edt_ft5x06_ts_data *tsdata)
 	if (reg_addr->reg_offset_y != NO_REGISTER)
 		regmap_write(regmap, reg_addr->reg_offset_y, tsdata->offset_y);
 	if (reg_addr->reg_report_rate != NO_REGISTER)
-		regmap_write(regmap, reg_addr->reg_report_rate,
-			     tsdata->report_rate);
+		edt_ft5x06_report_rate_set(tsdata, tsdata->report_rate);
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -1029,8 +1155,8 @@ static void edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
 	if (reg_addr->reg_offset_y != NO_REGISTER)
 		regmap_read(regmap, reg_addr->reg_offset_y, &tsdata->offset_y);
 	if (reg_addr->reg_report_rate != NO_REGISTER)
-		regmap_read(regmap, reg_addr->reg_report_rate,
-			    &tsdata->report_rate);
+		edt_ft5x06_report_rate_get(tsdata);
+
 	tsdata->num_x = EDT_DEFAULT_NUM_X;
 	if (reg_addr->reg_num_x != NO_REGISTER) {
 		if (!regmap_read(regmap, reg_addr->reg_num_x, &val))
@@ -1289,21 +1415,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client)
 	if (tsdata->reg_addr.reg_report_rate != NO_REGISTER &&
 	    !device_property_read_u32(&client->dev,
 				      "report-rate-hz", &report_rate)) {
-		if (tsdata->version == EDT_M06)
-			tsdata->report_rate = clamp_val(report_rate, 30, 140);
-		else
-			tsdata->report_rate = clamp_val(report_rate, 1, 255);
-
-		if (report_rate != tsdata->report_rate)
-			dev_warn(&client->dev,
-				 "report-rate %dHz is unsupported, use %dHz\n",
-				 report_rate, tsdata->report_rate);
-
-		if (tsdata->version == EDT_M06)
-			tsdata->report_rate /= 10;
-
-		regmap_write(tsdata->regmap, tsdata->reg_addr.reg_report_rate,
-			     tsdata->report_rate);
+		edt_ft5x06_report_rate_set(tsdata, report_rate);
 	}
 
 	dev_dbg(&client->dev,
-- 
2.43.0

base-commit: 24172e0d79900908cf5ebf366600616d29c9b417
branch: edt-ft5x06-fix-report-rate

^ permalink raw reply related

* Re: [PATCH v2] Apply the quirk HID_QUIRK_ALWAYS_POLL to the Edifier QR30 (2d99:a101).
From: Alan Stern @ 2025-11-12 17:25 UTC (permalink / raw)
  To: Terry Junge
  Cc: Rodrigo Lugathe da Conceição Alves, michal.pecio,
	bentiss, dmitry.torokhov, jikos, linux-input, linux-sound,
	linux-usb, linuxsound
In-Reply-To: <ab81f525-b4ea-4ac7-94a8-9d8eabca957a@cosmicgizmosystems.com>

On Tue, Nov 11, 2025 at 09:20:29PM -0800, Terry Junge wrote:
> Hi Rodrigo,
> 
> You might want to wait a couple of days before sending a v3 so other 
> reviewers can get a chance to make comments.
> 
> You should include the HID: tag in the subject--
> ...] HID: Apply...
> 
> The patch should be sent
> 
> To:  the HID subsystem maintainers
> bentiss@kernel.org
> jikos@kernel.org
> 
> Cc:  at a minimum
> linux-input@vger.kernel.org
> linux-kernel@vger.kernel.org
> 
> plus any of the others already part of this thread.
> 
> On 11/11/2025 5:53 PM, Rodrigo Lugathe da Conceição Alves wrote:
> > The USB speaker has a bug that causes it to reboot when changing the
> > brightness using the physical knob.
> > 
> > Add a new vendor and product ID entry in hid-ids.h, and register
> > the corresponding device in hid-quirks.c with the required quirk.
> > 
> > ---
> > v2:
> >   - Fixed title
> >   - Simplified commit message
> > 
> > Signed-off-by: Rodrigo Lugathe da Conceição Alves <lugathe2@gmail.com>
> > ---

One more thing about patch style: The Signed-off-by: line should go 
above the --- line, whereas the revision information goes below it (as 
it is here).  The second --- line isn't needed (but it doesn't hurt).

Look at other patch submissions in the email archives to see more 
examples of the style.

Alan Stern

^ permalink raw reply

* Re: [PATCH v2 02/11] dt-bindings: display: panel: ronbo,rb070d30: panel-common ref
From: Conor Dooley @ 2025-11-12 18:26 UTC (permalink / raw)
  To: Josua Mayer
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	Dmitry Torokhov, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, Laurent Pinchart, Lad Prabhakar, Thierry Reding,
	Jon Nettleton, Mikhail Anikin, Yazan Shhady,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-input@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org
In-Reply-To: <14492bcb-aa74-4fce-b9e6-3d33b08c682f@solid-run.com>

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Wed, Nov 12, 2025 at 11:51:55AM +0000, Josua Mayer wrote:
> Hi Conor,
> 
> Am 07.11.25 um 18:48 schrieb Conor Dooley:
> > On Fri, Nov 07, 2025 at 12:46:09PM +0100, Josua Mayer wrote:
> >> Add missing ref on panel-common.yaml for this dsi panel so that common
> >> properties can be shared.
> >>
> >> Drop reset-gpios and backlight as they are already in panel-common.
> >>
> >> Switch from additionalProperties to unevaluatedProperties so that common
> >> panel properties are available without repeating them in this binding.
> >>
> >> Notably panel-common defines the "port" property for linking panels to a
> >> source - which was missing from this panel. Mark it as required.
> >>
> >> Signed-off-by: Josua Mayer <josua@solid-run.com>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Is your ack compatible with Frank Li requesting to move the
> ref on panel-common further towards the end of the file?

I don't care what you do with that. I will say that it is pretty normal
to have it at the top of the file and move it if there ends up being a
need, and it's not worth resubmitting for that alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* Re: [PATCH v3 1/2] dt-bindings: input: i2c-hid: Introduce FocalTech FT8112
From: Conor Dooley @ 2025-11-12 18:53 UTC (permalink / raw)
  To: daniel_peng
  Cc: Dmitry Torokhov, linux-input, LKML, Conor Dooley,
	Krzysztof Kozlowski, Rob Herring, devicetree
In-Reply-To: <20251112195751.v3.1.I894dde5015f4acad94cb5bada61e5811c5142395@changeid>

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

On Wed, Nov 12, 2025 at 07:59:04PM +0800, daniel_peng@pegatron.corp-partner.google.com wrote:
> From: Daniel Peng <Daniel_Peng@pegatron.corp-partner.google.com>
> 
> The FocalTech FT8112 touch screen chip same as Ilitek ili2901 controller
> has a reset gpio. The difference is that they have different
> post_gpio_reset_on_delay_ms.
> FocalTech FT8112 also uses 3.3V power supply.
> 
> Signed-off-by: Daniel Peng <Daniel_Peng@pegatron.corp-partner.google.com>

> - Modified the subject description.
> - Modified maintainers to myself of this binding file.
> - Fixed the extra '>' on section of "interrupts = <15 IRQ_TYPE_LEVEL_LOW>;" and confirm command 'make dt_binding_check' correctly.
> - Restored MAINTAINERS file.

The patch seems okay me me, other than this which a) should be below the
--- and also does not appear to even be accurate? The last bullet in
doesn't match the patch contents.
Move this out of the commit and then
Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: changes-requested

Thanks,
Conor.

> 
> ---
> 
> (no changes since v1)
> 
>  .../bindings/input/focaltech,ft8112.yaml      | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml b/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
> new file mode 100644
> index 000000000000..197f30b14d45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/focaltech,ft8112.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/focaltech,ft8112.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FocalTech FT8112 touchscreen controller
> +
> +maintainers:
> +  - Daniel Peng <Daniel_Peng@pegatron.corp-partner.google.com>
> +
> +description:
> +  Supports the FocalTech FT8112 touchscreen controller.
> +  This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
> +
> +allOf:
> +  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - focaltech,ft8112
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  panel: true
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vcc33-supply: true
> +
> +  vccio-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - vcc33-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      touchscreen@38 {
> +        compatible = "focaltech,ft8112";
> +        reg = <0x38>;
> +
> +        interrupt-parent = <&pio>;
> +        interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
> +
> +        reset-gpios = <&pio 126 GPIO_ACTIVE_LOW>;
> +        vcc33-supply = <&pp3300_tchscr_x>;
> +      };
> +    };
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* [PATCH v1 2/5] Input: gpio_decoder - unify messages with help of dev_err_probe()
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20251112191412.2088105-1-andriy.shevchenko@linux.intel.com>

Unify error messages that might appear during probe phase by
switching to use dev_err_probe().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/misc/gpio_decoder.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index 459abc749a49..a2ae400790f9 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/input.h>
 #include <linux/kernel.h>
@@ -73,15 +74,12 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 	device_property_read_u32(dev, "linux,axis", &decoder->axis);
 
 	decoder->input_gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
-	if (IS_ERR(decoder->input_gpios)) {
-		dev_err(dev, "unable to acquire input gpios\n");
-		return PTR_ERR(decoder->input_gpios);
-	}
+	if (IS_ERR(decoder->input_gpios))
+		return dev_err_probe(dev, PTR_ERR(decoder->input_gpios),
+				     "unable to acquire input gpios\n");
 
-	if (decoder->input_gpios->ndescs < 2) {
-		dev_err(dev, "not enough gpios found\n");
-		return -EINVAL;
-	}
+	if (decoder->input_gpios->ndescs < 2)
+		return dev_err_probe(dev, -EINVAL, "not enough gpios found\n");
 
 	if (device_property_read_u32(dev, "decoder-max-value", &max))
 		max = (1U << decoder->input_gpios->ndescs) - 1;
@@ -97,16 +95,12 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 	input_set_abs_params(input, decoder->axis, 0, max, 0, 0);
 
 	err = input_setup_polling(input, gpio_decoder_poll_gpios);
-	if (err) {
-		dev_err(dev, "failed to set up polling\n");
-		return err;
-	}
+	if (err)
+		return dev_err_probe(dev, err, "failed to set up polling\n");
 
 	err = input_register_device(input);
-	if (err) {
-		dev_err(dev, "failed to register input device\n");
-		return err;
-	}
+	if (err)
+		return dev_err_probe(dev, err, "failed to register input device\n");
 
 	return 0;
 }
-- 
2.50.1


^ permalink raw reply related

* [PATCH v1 4/5] Input: gpio_decoder - make use of the macros from bits.h
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20251112191412.2088105-1-andriy.shevchenko@linux.intel.com>

Make use of BIT() where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/misc/gpio_decoder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index 8c07f7ff66e5..299f16d3bf4d 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -60,7 +60,7 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct gpio_decoder *decoder;
 	struct input_dev *input;
-	u32  max;
+	u32 max;
 	int err;
 
 	decoder = devm_kzalloc(dev, sizeof(*decoder), GFP_KERNEL);
@@ -82,7 +82,7 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -EINVAL, "too many gpios found\n");
 
 	if (device_property_read_u32(dev, "decoder-max-value", &max))
-		max = (1U << decoder->input_gpios->ndescs) - 1;
+		max = BIT(decoder->input_gpios->ndescs) - 1;
 
 	input = devm_input_allocate_device(dev);
 	if (!input)
-- 
2.50.1


^ permalink raw reply related

* [PATCH v1 0/5] Input: gpio_decoder - update driver to use modern APIs
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov

Update gpio_decoder driver to use modern in-kernel APIs.

Andy Shevchenko (5):
  Input: gpio_decoder - make use of device properties
  Input: gpio_decoder - unify messages with help of dev_err_probe()
  Input: gpio_decoder - replace custom loop by
    gpiod_get_array_value_cansleep()
  Input: gpio_decoder - make use of the macros from bits.h
  Input: gpio_decoder - don't use "proxy" headers

 drivers/input/misc/gpio_decoder.c | 72 ++++++++++++++-----------------
 1 file changed, 33 insertions(+), 39 deletions(-)

-- 
2.50.1


^ permalink raw reply

* [PATCH v1 5/5] Input: gpio_decoder - don't use "proxy" headers
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20251112191412.2088105-1-andriy.shevchenko@linux.intel.com>

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/misc/gpio_decoder.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index 299f16d3bf4d..5fe56ae58183 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -7,16 +7,17 @@
  */
 
 #include <linux/bitmap.h>
-#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/input.h>
-#include <linux/kernel.h>
 #include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/types.h>
 
 struct gpio_decoder {
 	struct gpio_descs *input_gpios;
-- 
2.50.1


^ permalink raw reply related

* [PATCH v1 1/5] Input: gpio_decoder - make use of device properties
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20251112191412.2088105-1-andriy.shevchenko@linux.intel.com>

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/misc/gpio_decoder.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index ee668eba302f..459abc749a49 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -10,9 +10,10 @@
 #include <linux/gpio/consumer.h>
 #include <linux/input.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 struct gpio_decoder {
 	struct gpio_descs *input_gpios;
@@ -110,19 +111,17 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id gpio_decoder_of_match[] = {
 	{ .compatible = "gpio-decoder", },
-	{ },
+	{ }
 };
 MODULE_DEVICE_TABLE(of, gpio_decoder_of_match);
-#endif
 
 static struct platform_driver gpio_decoder_driver = {
 	.probe		= gpio_decoder_probe,
 	.driver		= {
 		.name	= "gpio-decoder",
-		.of_match_table = of_match_ptr(gpio_decoder_of_match),
+		.of_match_table = gpio_decoder_of_match,
 	}
 };
 module_platform_driver(gpio_decoder_driver);
-- 
2.50.1


^ permalink raw reply related

* [PATCH v1 3/5] Input: gpio_decoder - replace custom loop by gpiod_get_array_value_cansleep()
From: Andy Shevchenko @ 2025-11-12 19:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20251112191412.2088105-1-andriy.shevchenko@linux.intel.com>

There is a custom loop that repeats parts of gpiod_get_array_value_cansleep().
Use that in conjunction with bitmap API to make code shorter and easier to
follow.

With this done, add an upper check for amount of GPIOs given based on
the driver's code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/misc/gpio_decoder.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index a2ae400790f9..8c07f7ff66e5 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -6,11 +6,13 @@
  * encoded numeric value into an input event.
  */
 
+#include <linux/bitmap.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/input.h>
 #include <linux/kernel.h>
+#include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -26,23 +28,18 @@ struct gpio_decoder {
 static int gpio_decoder_get_gpios_state(struct gpio_decoder *decoder)
 {
 	struct gpio_descs *gpios = decoder->input_gpios;
-	unsigned int ret = 0;
-	int i, val;
+	DECLARE_BITMAP(values, 32);
+	unsigned int size;
+	int err;
 
-	for (i = 0; i < gpios->ndescs; i++) {
-		val = gpiod_get_value_cansleep(gpios->desc[i]);
-		if (val < 0) {
-			dev_err(decoder->dev,
-				"Error reading gpio %d: %d\n",
-				desc_to_gpio(gpios->desc[i]), val);
-			return val;
-		}
-
-		val = !!val;
-		ret = (ret << 1) | val;
+	size = min(gpios->ndescs, 32U);
+	err = gpiod_get_array_value_cansleep(size, gpios->desc, gpios->info, values);
+	if (err) {
+		dev_err(decoder->dev, "Error reading GPIO: %d\n", val);
+		return err;
 	}
 
-	return ret;
+	return bitmap_read(values, 0, size);
 }
 
 static void gpio_decoder_poll_gpios(struct input_dev *input)
@@ -81,6 +78,9 @@ static int gpio_decoder_probe(struct platform_device *pdev)
 	if (decoder->input_gpios->ndescs < 2)
 		return dev_err_probe(dev, -EINVAL, "not enough gpios found\n");
 
+	if (decoder->input_gpios->ndescs > 31)
+		return dev_err_probe(dev, -EINVAL, "too many gpios found\n");
+
 	if (device_property_read_u32(dev, "decoder-max-value", &max))
 		max = (1U << decoder->input_gpios->ndescs) - 1;
 
-- 
2.50.1


^ permalink raw reply related

* Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling
From: Denis Benato @ 2025-11-12 19:51 UTC (permalink / raw)
  To: Antheas Kapenekakis, Ilpo Järvinen
  Cc: platform-driver-x86, linux-input, LKML, Jiri Kosina,
	Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede
In-Reply-To: <CAGwozwGpacR=wYXpf3vOiwWNxaV6pJ6CdE-E-G1gRRpO4VHVMg@mail.gmail.com>


On 11/12/25 14:41, Antheas Kapenekakis wrote:
> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>>
>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>> This is a two part series which does the following:
>>>>   - Clean-up init sequence
>>>>   - Unify backlight handling to happen under asus-wmi so that all Aura
>>>>     devices have synced brightness controls and the backlight button works
>>>>     properly when it is on a USB laptop keyboard instead of one w/ WMI.
>>>>
>>>> For more context, see cover letter of V1. Since V5, I removed some patches
>>>> to make this easier to merge.
>>> Small bump for this.
>> I looked at v8 earlier but then got an impression some of Denis' comments
>> against v7 were not taken into account in v8, which implies there will be
>> delay until I've time to delve into the details (I need to understand
>> things pretty deeply in such a case, which does take lots of time).
>>
>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
>> so much time on it, but somehow I've a feeling he isn't happy with v8
>> but just hasn't voiced it again...
>>
>> Please do realize that ignoring reviewer feedback without a very very good
>> reason always risks adding delay or friction into getting things
>> upstreamed. Especially, when the review comes from a person who has been
>> around for me to learn to trust their reviews or from a maintainer of the
>> code in question.
> Sure, sorry if it came out this way. Dennis had two comments on the V7
> version of the series.
>
> The first one was that asusctl has a hang bug, which he hasn't had
> time to look into yet. This should have been fixed by dropping the
> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> bit of a NOOP that does not need to be added in the future.
So it is supposed to not regress it now, correct?
> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> said (back in March) that it is fine to drop 0x5e because it is only
> used for ANIME displays. However, for 0x5d, it is hard to verify some
> of the older laptops because they have only been tested with 0x5d and
> we do not have the hardware in question to test.
>
> For this series, I re-added "initialize LED endpoint early for old
> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> tested again so this comment should be resolved too. With that in
> mind, we do end up with an additional quirk/command that may be
> unneeded and will remain there forever, but since it was a point of
> contention, it is not worth arguing over.
>
> So both comments should be resolved
The driver should also not late-initialize anything.

Windows doesn't do it and the official asus application
can freely send LEDs changing commands to either WMI or USB
so I don't see any reason to do things differently [than windows]
and not prepare every USB endpoint to receive commands,
this has not been addressed unless I confused v7 and v8?
> @Denis: can give an ack if this is the case?
>
> As for Derek's comment, he has a PR for his project where he removes
> the name match for Ally devices with ample time for it to be merged
> until kernel 6.19 is released. In addition, that specific software for
> full functionality relies on OOT drivers on the distros it ships with,
> so it is minimally affected in either case.
The part we are talking about depends on this driver (hid-asus)
and there are people on asus-linux community using inputplumber
for non-ally devices (the OOT driver is only for ally devices)
therefore it is very important to us (and various other distributions)
not to break that software in any way.

Weighting pros and cons of changing the name I am not sure
changing name brings any benefit? Or am I missing something here?
It's simply used by userspace so the hardware should be loading
regardless of the name...

Along with IP and your tool and asusctl there is also openrgb,
and a newborn application for asus devices (I don't have contacts
with that dev nor I remember the name of the tool)
and I am not even that sure these are all asus-related
applications.

Excercise EXTRA care touching this area as these are enough changes
to make it difficult to understand what exactly is the problem if
someone shows up with LEDs malfunctioning, laptop not entering sleep
anymore or something else entirely. Plus over time
ASUS has used various workarounds for windows problems
and I am not eager to find out what those are since there is only
a realistic way it's going to happen....
> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> as the kernel kicks one of the RGB endpoints because it does not
> register an input device (the check skipped by early return) so
> userspace becomes unable to control RGB on a stock kernel
> (hidraw/hiddev nodes are gone). For more context here, this specific
> endpoint implements the RGB Lamparray protocol for Windows dynamic
> lighting, and I think in an attempt to make it work properly in
> Windows, Asus made it so Windows has to first disable dynamic lighting
> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> hid page).
Yes once ASUS introduces something new it sticks with that for
future models so it's expected more and more laptops will have
the same problem: I am not questioning if these patches are needed
as they very clearly are; I am questioning if everything that these
patches are doing are worth doing and aren't breaking/regressing
either tools or the flow of actions between the EC and these USB devices.
> Hopefully this clears things up
>
> Antheas
>
>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
>>> Perhaps it would help with spam
>> I see BADSIG very often these days from b4 (thanks to gmail expiring
>> things after 7 days or so, I recall hearing somewhere), I just ignore them
>> entirely.
>>
>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
>> that is what you thought is happening here, it's not the case.
>> If your patch does appear in the pdx86 patchwork, there's even less reason
>> to worry as I mostly pick patches to process using patchwork's list.
> Turns out I had to update my DNS records. It should be good now.
>
>> --
>>  i.
>>
>>> Antheas
>>>
>>>> ---
>>>> V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
>>>> V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
>>>> V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
>>>> V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
>>>> V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
>>>> V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
>>>> V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>>>>
>>>> Changes since V7:
>>>>   - Readd legacy init quirk for Dennis
>>>>   - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
>>>>   - Fix warning due to enum_backlight receiving negative values
>>>>
>>>> Changes since V6:
>>>>   - Split initialization refactor into three patches, update commit text
>>>>     to be clearer in what it does
>>>>   - Replace spinlock accesses with guard and scoped guard in all patches
>>>>   - Add missing includes mentioned by Ilpo
>>>>   - Reflow, tweak comment in prevent binding to all HID devices on ROG
>>>>   - Replace asus_ref.asus with local reference in all patches
>>>>   - Add missing kernel doc comments
>>>>   - Other minor nits from Ilpo
>>>>   - User reported warning due to scheduling work while holding a spinlock.
>>>>     Restructure patch for multiple handlers to limit when spinlock is held to
>>>>     variable access only. In parallel, setup a workqueue to handle registration
>>>>     of led device and setting brightness. This is required as registering the
>>>>     led device triggers kbd_led_get which needs to hold the spinlock to
>>>>     protect the led_wk value. The workqueue is also required for the hid
>>>>     event passthrough to avoid scheduling work while holding the spinlock.
>>>>     Apply the workqueue to wmi brightness buttons as well, as that was
>>>>     omitted before this series and WMI access was performed.
>>>>   - On "HID: asus: prevent binding to all HID devices on ROG", rename
>>>>     quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
>>>>     This allows other quirks to apply (applies quirk that fixes keyboard
>>>>     being named as a pointer device).
>>>>
>>>> Changes since V5:
>>>>   - It's been a long time
>>>>   - Remove addition of RGB as that had some comments I need to work on
>>>>   - Remove folio patch (already merged)
>>>>   - Remove legacy fix patch 11 from V4. There is a small chance that
>>>>     without this patch, some old NKEY keyboards might not respond to
>>>>     RGB commands according to Luke, but the kernel driver does not do
>>>>     RGB currently. The 0x5d init is done by Armoury crate software in
>>>>     Windows. If an issue is found, we can re-add it or just remove patches
>>>>     1/2 before merging. However, init could use the cleanup.
>>>>
>>>> Changes since V4:
>>>>   - Fix KConfig (reported by kernel robot)
>>>>   - Fix Ilpo's nits, if I missed anything lmk
>>>>
>>>> Changes since V3:
>>>>   - Add initializer for 0x5d for old NKEY keyboards until it is verified
>>>>     that it is not needed for their media keys to function.
>>>>   - Cover init in asus-wmi with spinlock as per Hans
>>>>   - If asus-wmi registers WMI handler with brightness, init the brightness
>>>>     in USB Asus keyboards, per Hans.
>>>>   - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
>>>>   - Fix oops when unregistering asus-wmi by moving unregister outside of
>>>>     the spin lock (but after the asus reference is set to null)
>>>>
>>>> Changes since V2:
>>>>   - Check lazy init succeds in asus-wmi before setting register variable
>>>>   - make explicit check in asus_hid_register_listener for listener existing
>>>>     to avoid re-init
>>>>   - rename asus_brt to asus_hid in most places and harmonize everything
>>>>   - switch to a spinlock instead of a mutex to avoid kernel ooops
>>>>   - fixup hid device quirks to avoid multiple RGB devices while still exposing
>>>>     all input vendor devices. This includes moving rgb init to probe
>>>>     instead of the input_configured callbacks.
>>>>   - Remove fan key (during retest it appears to be 0xae that is already
>>>>     supported by hid-asus)
>>>>   - Never unregister asus::kbd_backlight while asus-wmi is active, as that
>>>>   - removes fds from userspace and breaks backlight functionality. All
>>>>   - current mainline drivers do not support backlight hotplugging, so most
>>>>     userspace software (e.g., KDE, UPower) is built with that assumption.
>>>>     For the Ally, since it disconnects its controller during sleep, this
>>>>     caused the backlight slider to not work in KDE.
>>>>
>>>> Changes since V1:
>>>>   - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
>>>>   - Fix ifdef else having an invalid signature (reported by kernel robot)
>>>>   - Restore input arguments to init and keyboard function so they can
>>>>     be re-used for RGB controls.
>>>>   - Remove Z13 delay (it did not work to fix the touchpad) and replace it
>>>>     with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
>>>>     keyboard rename into it.
>>>>   - Unregister brightness listener before removing work queue to avoid
>>>>     a race condition causing corruption
>>>>   - Remove spurious mutex unlock in asus_brt_event
>>>>   - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
>>>>     relocking the mutex and causing a deadlock when unregistering leds
>>>>   - Add extra check during unregistering to avoid calling unregister when
>>>>     no led device is registered.
>>>>   - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
>>>>     the driver to create 4 RGB handlers per device. I also suspect some
>>>>     extra events sneak through (KDE had the @@@@@@).
>>>>
>>>> Antheas Kapenekakis (10):
>>>>   HID: asus: simplify RGB init sequence
>>>>   HID: asus: use same report_id in response
>>>>   HID: asus: fortify keyboard handshake
>>>>   HID: asus: prevent binding to all HID devices on ROG
>>>>   HID: asus: initialize LED endpoint early for old NKEY keyboards
>>>>   platform/x86: asus-wmi: Add support for multiple kbd led handlers
>>>>   HID: asus: listen to the asus-wmi brightness device instead of
>>>>     creating one
>>>>   platform/x86: asus-wmi: remove unused keyboard backlight quirk
>>>>   platform/x86: asus-wmi: add keyboard brightness event handler
>>>>   HID: asus: add support for the asus-wmi brightness handler
>>>>
>>>>  drivers/hid/hid-asus.c                     | 222 +++++++++++----------
>>>>  drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
>>>>  include/linux/platform_data/x86/asus-wmi.h |  70 +++----
>>>>  3 files changed, 331 insertions(+), 175 deletions(-)
>>>>
>>>>
>>>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
>>>> --
>>>> 2.51.2
>>>>
>>>>

^ permalink raw reply

* Re: [RFC PATCH 1/1] HID: input: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-12 21:36 UTC (permalink / raw)
  To: Bastien Nocera
  Cc: linux-input, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Sebastian Reichel, linux-pm
In-Reply-To: <82ec5223f83eaa89278997fe95ee9ea83236a4a1.camel@hadess.net>

Hey Bastien,

On Wed, Nov 12, 2025 at 2:46 PM Bastien Nocera <hadess@hadess.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@redhat.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,


^ permalink raw reply

* Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling
From: Antheas Kapenekakis @ 2025-11-12 22:08 UTC (permalink / raw)
  To: Denis Benato
  Cc: Ilpo Järvinen, platform-driver-x86, linux-input, LKML,
	Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
	Hans de Goede
In-Reply-To: <74f91d3c-6494-4754-a10f-4d8c1d45f7ff@gmail.com>

On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> > On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> >> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>
> >>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>> This is a two part series which does the following:
> >>>>   - Clean-up init sequence
> >>>>   - Unify backlight handling to happen under asus-wmi so that all Aura
> >>>>     devices have synced brightness controls and the backlight button works
> >>>>     properly when it is on a USB laptop keyboard instead of one w/ WMI.
> >>>>
> >>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>> to make this easier to merge.
> >>> Small bump for this.
> >> I looked at v8 earlier but then got an impression some of Denis' comments
> >> against v7 were not taken into account in v8, which implies there will be
> >> delay until I've time to delve into the details (I need to understand
> >> things pretty deeply in such a case, which does take lots of time).
> >>
> >> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >> so much time on it, but somehow I've a feeling he isn't happy with v8
> >> but just hasn't voiced it again...
> >>
> >> Please do realize that ignoring reviewer feedback without a very very good
> >> reason always risks adding delay or friction into getting things
> >> upstreamed. Especially, when the review comes from a person who has been
> >> around for me to learn to trust their reviews or from a maintainer of the
> >> code in question.
> > Sure, sorry if it came out this way. Dennis had two comments on the V7
> > version of the series.
> >
> > The first one was that asusctl has a hang bug, which he hasn't had
> > time to look into yet. This should have been fixed by dropping the
> > HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> > bit of a NOOP that does not need to be added in the future.
> So it is supposed to not regress it now, correct?
> > The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> > said (back in March) that it is fine to drop 0x5e because it is only
> > used for ANIME displays. However, for 0x5d, it is hard to verify some
> > of the older laptops because they have only been tested with 0x5d and
> > we do not have the hardware in question to test.
> >
> > For this series, I re-added "initialize LED endpoint early for old
> > NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> > tested again so this comment should be resolved too. With that in
> > mind, we do end up with an additional quirk/command that may be
> > unneeded and will remain there forever, but since it was a point of
> > contention, it is not worth arguing over.
> >
> > So both comments should be resolved
> The driver should also not late-initialize anything.
>
> Windows doesn't do it and the official asus application
> can freely send LEDs changing commands to either WMI or USB
> so I don't see any reason to do things differently [than windows]
> and not prepare every USB endpoint to receive commands,
> this has not been addressed unless I confused v7 and v8?

Yes, it's been added on v8. 0x5d is init for the laptops it is
problematic for. Not because it does not work, but because it has not
been verified to work for those laptops.

> > @Denis: can give an ack if this is the case?
> >
> > As for Derek's comment, he has a PR for his project where he removes
> > the name match for Ally devices with ample time for it to be merged
> > until kernel 6.19 is released. In addition, that specific software for
> > full functionality relies on OOT drivers on the distros it ships with,
> > so it is minimally affected in either case.
> The part we are talking about depends on this driver (hid-asus)
> and there are people on asus-linux community using inputplumber
> for non-ally devices (the OOT driver is only for ally devices)
> therefore it is very important to us (and various other distributions)
> not to break that software in any way.

This driver is only used for Ally devices. If you mean that people
remap their keyboards using inputplumber I guess they could but I have
not seen it.

> Weighting pros and cons of changing the name I am not sure
> changing name brings any benefit? Or am I missing something here?
> It's simply used by userspace so the hardware should be loading
> regardless of the name...

Users see the name of their devices in their settings menu. They
should be accurate. Also, the early entry needs to be added anyway to
prevent kicking devices.

> Along with IP and your tool and asusctl there is also openrgb,
> and a newborn application for asus devices (I don't have contacts
> with that dev nor I remember the name of the tool)
> and I am not even that sure these are all asus-related
> applications.

My tool never checked for names, it briefly did for around a month
after its creation for some devices until capability matches. Around
6.1 and 6.7 the kernel changed the names of most USB devices and that
caused issues. It currently only uses name matches for VID/PID 0/0
devices created by the kernel. Specifically, WMI and AT Keyboards. I
am not sure there is a workaround for those. Asusctl also does not use
names either.

> Excercise EXTRA care touching this area as these are enough changes
> to make it difficult to understand what exactly is the problem if
> someone shows up with LEDs malfunctioning, laptop not entering sleep
> anymore or something else entirely. Plus over time
> ASUS has used various workarounds for windows problems
> and I am not eager to find out what those are since there is only
> a realistic way it's going to happen....

These changes are not doing something extraordinary. It's just a minor cleanup.

> > Moreover, that specific commit is needed for Xbox Ally devices anyway,
> > as the kernel kicks one of the RGB endpoints because it does not
> > register an input device (the check skipped by early return) so
> > userspace becomes unable to control RGB on a stock kernel
> > (hidraw/hiddev nodes are gone). For more context here, this specific
> > endpoint implements the RGB Lamparray protocol for Windows dynamic
> > lighting, and I think in an attempt to make it work properly in
> > Windows, Asus made it so Windows has to first disable dynamic lighting
> > for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> > hid page).
> Yes once ASUS introduces something new it sticks with that for
> future models so it's expected more and more laptops will have
> the same problem: I am not questioning if these patches are needed
> as they very clearly are; I am questioning if everything that these
> patches are doing are worth doing and aren't breaking/regressing
> either tools or the flow of actions between the EC and these USB devices.

Well, this series is needed to account for that. Sending the disable
command is out of scope for now though.

Antheas

> > Hopefully this clears things up
> >
> > Antheas
> >
> >>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>> Perhaps it would help with spam
> >> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >> entirely.
> >>
> >> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >> that is what you thought is happening here, it's not the case.
> >> If your patch does appear in the pdx86 patchwork, there's even less reason
> >> to worry as I mostly pick patches to process using patchwork's list.
> > Turns out I had to update my DNS records. It should be good now.
> >
> >> --
> >>  i.
> snipp
> >>>> 2.51.2
> >>>>
> >>>>
>


^ permalink raw reply

* [PATCH] Input: adxl34x: Add warning on shared compatible with adxl345
From: Jorge Marques @ 2025-11-12 22:26 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Jorge Marques

INPUT_ADXL34X and ADXL345 share compatibles, previously it
wasn't possible to compile ADXL345 if INPUT_ADXL34X was enabled.
Changed to allow both as modules instead, then the user should blacklist
to block loading one, if his config selects both.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
There are two drivers for the compatible:

- adi,adxl345

* IIO:
  drivers/iio/accel/adxl345_core.c
  drivers/iio/accel/adxl345_spi.c
  drivers/iio/accel/adxl345_i2c.c
* Inputs:
  drivers/input/misc/adxl34x-spi.c
  drivers/input/misc/adxl34x-i2c.c

To disallows both being complied, the depends INPUT_ADXL34X=n
was added to ADXL345 symbols. However, it should be possible to compile
both as modules, then blacklist one of them in the /etc/modprobe.d/blacklist.conf
file.
So patch
https://lore.kernel.org/linux-iio/20251112-adxl345-allow-adxl34x-mod-v2-1-5b1561eae5a0@analog.com/T/#u
changes the rule to !(INPUT_ADXL34X) to allow both as modules, but still
disallow INPUT_ADXL34X to be built-in and ADXL345 as module.

Add warning to the input Kconfig to inform users.
---
 drivers/input/misc/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index cc2558630..df5c425c9 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -769,7 +769,9 @@ config INPUT_ADXL34X_I2C
 	  Say Y here if you have ADXL345/6 hooked to an I2C bus.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called adxl34x-i2c.
+	  module will be called adxl34x-i2c. ADXL345 share compatibles
+	  with this driver, choosing Y won't allow to select ADXL345.
+	  Do not add both modules to the kernel.
 
 config INPUT_ADXL34X_SPI
 	tristate "support SPI bus connection"
@@ -779,7 +781,9 @@ config INPUT_ADXL34X_SPI
 	  Say Y here if you have ADXL345/6 hooked to a SPI bus.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called adxl34x-spi.
+	  module will be called adxl34x-spi. ADXL345 share compatibles
+	  with this driver, choosing Y won't allow to select ADXL345.
+	  Do not add both modules to the kernel.
 
 config INPUT_IBM_PANEL
 	tristate "IBM Operation Panel driver"

---
base-commit: a311c777f2987e6ddba2d2dd2f82f2135d65f8aa
change-id: 20251112-input-adxl34x-1972753e2c62
prerequisite-message-id: 20251031-adxl345-allow-adxl34x-mod-v1-1-cd65749ba89c@analog.com
prerequisite-patch-id: ee005752c4daeb450e458f20d9de0ce7b24743ca

Best regards,
-- 
Jorge Marques <jorge.marques@analog.com>


^ permalink raw reply related

* Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling
From: Antheas Kapenekakis @ 2025-11-12 23:22 UTC (permalink / raw)
  To: Denis Benato
  Cc: Ilpo Järvinen, platform-driver-x86, linux-input, LKML,
	Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
	Hans de Goede
In-Reply-To: <CAGwozwEKqqJxxmtjJhy2MzNVhmBTMmy8xG5TZGkKJqJCgK=X5w@mail.gmail.com>

On Wed, 12 Nov 2025 at 23:08, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> snip
> > > Sure, sorry if it came out this way. Dennis had two comments on the V7
> > > version of the series.
> > >
> > > The first one was that asusctl has a hang bug, which he hasn't had
> > > time to look into yet. This should have been fixed by dropping the
> > > HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> > > bit of a NOOP that does not need to be added in the future.
> > So it is supposed to not regress it now, correct?

I missed this. Spaces. Yes, quirk input per app created around 3 more
input devices per USB device, plus the dynamic lighting one, so you
went from 2 to 6, and there seems to be an issue with asusctl and a
large number of event devices, that caused some intermittent freezing
when the user typed on certain keyboards. I removed it. Although, not
a problem with the kernel itself, per say.

Antheas

> > > The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> > > said (back in March) that it is fine to drop 0x5e because it is only
> > > > snip
> > >>>>
> >


^ permalink raw reply

* Re: [PATCH v2 1/2] Input: Add lid switch notifier
From: Jonathan Denose @ 2025-11-12 23:45 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <s7nk2xerfa7ocscyaaez7qnygkmrdy2ch355uziodda654ws5p@zswljdmx3747>

Hi Dmitry,

Thanks for your review.

On Tue, Nov 11, 2025 at 4:34 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Jonathan,
>
> On Tue, Nov 11, 2025 at 09:34:06PM +0000, Jonathan Denose wrote:
> > This change creates a new input handler which can be included in the
> > build via a new Kconfig option CONFIG_INPUT_LID_NOTIFIER. This input
> > handler listens for lid switch events and publishes them through an
> > atomic notification chain. Other modules may register for events
> > through this notification chain with register_lid_notifier.
> >
> > Signed-off-by: Jonathan Denose <jdenose@google.com>
> > ---
> >  drivers/input/Kconfig        | 11 +++++
> >  drivers/input/Makefile       |  1 +
> >  drivers/input/lid-notifier.c | 98 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/input.h        |  2 +
> >  4 files changed, 112 insertions(+)
> >
> > diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
> > index 88ecdf5218ee9ba35e1efec6341f8605b621bd49..16f6d24fd04ac8cb5af9d36cc47155ea9be0e177 100644
> > --- a/drivers/input/Kconfig
> > +++ b/drivers/input/Kconfig
> > @@ -38,6 +38,17 @@ config INPUT_LEDS
> >         To compile this driver as a module, choose M here: the
> >         module will be called input-leds.
> >
> > +config INPUT_LID_NOTIFIER
> > +     tristate "Include notifier for lid switch events"
> > +     help
> > +       Say Y here if you would like to create a notifier to publish lid switch
> > +             events.
> > +
> > +       If unsure, say N.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called lid-notifier.
>
> I think this better not surfaced to users but rather interested drivers
> 'select' it.

That makes sense to me, but how could I get the lid-notifier.c file to
compile in that case? That was my only reason for adding it as a
config option.

> > +
> >  config INPUT_FF_MEMLESS
> >       tristate "Support for memoryless force-feedback devices"
> >       help
> > diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> > index 2cd6e1c9a77844fe09cd3d99533e5d3efb038c7d..1efdba04f79a97e2a122b9198341b18a1855b4b9 100644
> > --- a/drivers/input/Makefile
> > +++ b/drivers/input/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_INPUT_MATRIXKMAP)      += matrix-keymap.o
> >  obj-$(CONFIG_INPUT_VIVALDIFMAP)      += vivaldi-fmap.o
> >
> >  obj-$(CONFIG_INPUT_LEDS)     += input-leds.o
> > +obj-$(CONFIG_INPUT_LID_NOTIFIER)     += lid-notifier.o
> >  obj-$(CONFIG_INPUT_MOUSEDEV) += mousedev.o
> >  obj-$(CONFIG_INPUT_JOYDEV)   += joydev.o
> >  obj-$(CONFIG_INPUT_EVDEV)    += evdev.o
> > diff --git a/drivers/input/lid-notifier.c b/drivers/input/lid-notifier.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..954b9855532dbd0514860e309d0b76982e947673
> > --- /dev/null
> > +++ b/drivers/input/lid-notifier.c
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  Lid event notifier
> > + *
> > + *  Copyright (c) 2025 Jonathan Denose <jdenose@google.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/notifier.h>
> > +
> > +static struct input_handler lid_handler;
> > +static struct atomic_notifier_head input_notifier_head;
> > +
> > +int register_lid_notifier(struct notifier_block *notifier)
> > +{
> > +     return atomic_notifier_chain_register(&input_notifier_head, notifier);
> > +}
> > +EXPORT_SYMBOL(register_lid_notifier);
>
> I wonder if we want to expose the "raw" notifier or if we want to
> provide a higher-level API that would allocate a notifier blocki, set up
> the callback, and return a "cookie" that can be used to free notifier
> block later. This way we do not need to worry that some enterprising
> driver suppresses notifications for the rest by returning NOTIFY_STOP.
>
> > +
> > +static int lid_handler_connect(struct input_handler *handler,
> > +             struct input_dev *input_dev, const struct input_device_id *id)
>
> Proper alignment of the arguments please.
>
> > +{
> > +     struct input_handle *handle;
> > +     int error;
> > +
> > +     handle = devm_kzalloc(&input_dev->dev, sizeof(struct input_handle), GFP_KERNEL);
>
> This is not driver probe path so devm_kzalloc must not be used here.
> Also "sizeof(*handle)".

So in that case, would I just use kzalloc?

> > +     if (!handle)
> > +             return -ENOMEM;
> > +
> > +     handle->dev = input_dev;
> > +     handle->handler = handler;
> > +     handle->name = "lid";
> > +
> > +     error = input_register_handle(handle);
> > +     if (error)
> > +             goto err_free_handle;
> > +
> > +     error = input_open_device(handle);
> > +     if (error)
> > +             goto err_unregister_handle;
> > +
> > +     return 0;
> > +
> > + err_unregister_handle:
> > +     input_unregister_handle(handle);
> > + err_free_handle:
> > +     kfree(handle);
>
> Just FYI: One must never use kfree() with devm_kalloc()ed memory.

Noted!

> > +     return error;
> > +}
> > +
> > +static void lid_handler_disconnect(struct input_handle *handle)
> > +{
> > +     input_close_device(handle);
> > +     input_unregister_handle(handle);
>
>         kfree(handle);
>
> > +}
> > +
> > +static void lid_handler_event(struct input_handle *handle, unsigned int type,
> > +             unsigned int code, int value)
> > +{
> > +     if (type == EV_SW && code == SW_LID)
> > +             atomic_notifier_call_chain(&input_notifier_head, value, handle->dev);
>
> Why do you need to pass the device from which SW_LID originated?

It isn't needed, I'll pass NULL instead.

> > +}
> > +
> > +static const struct input_device_id lid_handler_ids[] = {
> > +     {
> > +             .flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT
> > +                                             | INPUT_DEVICE_ID_MATCH_BUS,
> > +             .evbit = { BIT_MASK(EV_SW) },
> > +             .swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
> > +             .bustype = 0x19
>
> Why do we need to match in bus type? The LID does not have to always
> come from ACPI.

On the specific chromebook where this touchpad is found, when the lid
switch is opened or closed, `cros_ec_buttons` also sends SW_LID
events, so lid_handler_event was unnecessarily called twice. To
prevent that, I'm filtering on the bus type to get only the events
originating from the lid switch.

> > +     },
> > +     { },
> > +};
> > +
> > +static struct input_handler lid_handler = {
> > +     .connect = lid_handler_connect,
> > +     .disconnect = lid_handler_disconnect,
> > +     .event = lid_handler_event,
> > +     .name = "lid",
> > +     .id_table = lid_handler_ids
> > +};
> > +
> > +static int __init lid_notifier_init(void)
> > +{
> > +     return input_register_handler(&lid_handler);
> > +}
> > +module_init(lid_notifier_init);
> > +
> > +static void __exit lid_notifier_exit(void)
> > +{
> > +     input_unregister_handler(&lid_handler);
> > +}
> > +module_exit(lid_notifier_exit);
> > +
> > +MODULE_AUTHOR("Jonathan Denose <jdenose@google.com>");
> > +MODULE_DESCRIPTION("Lid event notifier");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index 7d7cb0593a63e93c4906c49cde430188db2d1ab5..023eb92c77d9e8721d482b9787632a671671de08 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -592,3 +592,5 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
> >               int (*play_effect)(struct input_dev *, void *, struct ff_effect *));
> >
> >  #endif
>
> I think this should go into include/linux/lid-notifier.h.
>
> > +
> > +int register_lid_notifier(struct notifier_block *notifier);
>
> Thanks.
>
> --
> Dmitry
-- 
Jonathan

^ permalink raw reply

* Re: [PATCH v2 2/2] HID: multitouch: Toggle touch surface on Elan touchpad on lid event
From: Jonathan Denose @ 2025-11-12 23:49 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <iazucnirwewmavtsxudcynuoy2afzxhzr7nre2wke4d7glypcu@jtotz5oivpsu>

Hi Dmitry,

Thanks for your review.

On Tue, Nov 11, 2025 at 4:37 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Jonathan,
>
> On Tue, Nov 11, 2025 at 09:34:07PM +0000, Jonathan Denose wrote:
> > Many touchpad modules have a pin which is expected to be connected to the
> > lid angle sensor in laptops. The pin sends a signal to the touchpad module
> > about the lid state and each touchpad vendor handles this notification in
> > their firmware.
> >
> > The Elan touchpad with VID 323b does not always have this aforementioned
> > pin, which then causes interference between the lid and the touchpad when
> > the lid is closed. This interference causes a few seconds delay before the
> > touchpad works again, or it causes it to be come completely unresponsive.
> > To circumvent this hardware issue in software, implement a device quirk
> > which will allow the hid-multitouch driver to register a notifier_block
> > to listen for lid switch events. When the lid switch closes, the
> > touchpad surface will be turned off and when the lid switch opens, the
> > touchpad surgace will be turned on. This triggers recalibration which
> > resolves interference issues when the lid is closed.
> >
> > Signed-off-by: Jonathan Denose <jdenose@google.com>
> > ---
> >  drivers/hid/hid-multitouch.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 2879e65cf303b1456311ac06115adda5a78a2600..9a89913c193bc110a0a821a901aebd97892c66bd 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/device.h>
> >  #include <linux/hid.h>
> >  #include <linux/module.h>
> > +#include <linux/notifier.h>
> >  #include <linux/slab.h>
> >  #include <linux/input/mt.h>
> >  #include <linux/jiffies.h>
> > @@ -76,6 +77,7 @@ MODULE_LICENSE("GPL");
> >  #define MT_QUIRK_DISABLE_WAKEUP              BIT(21)
> >  #define MT_QUIRK_ORIENTATION_INVERT  BIT(22)
> >  #define MT_QUIRK_APPLE_TOUCHBAR              BIT(23)
> > +#define MT_QUIRK_REGISTER_LID_NOTIFIER BIT(24)
> >
> >  #define MT_INPUTMODE_TOUCHSCREEN     0x02
> >  #define MT_INPUTMODE_TOUCHPAD                0x03
> > @@ -183,6 +185,8 @@ struct mt_device {
> >       struct list_head reports;
> >  };
> >
> > +static struct hid_device *lid_notify_hdev;
>
> This should really be per-device.

Just to be sure I'm following correctly, the initialization of
lid_notify_hdev should be per-device?

> > +
> >  static void mt_post_parse_default_settings(struct mt_device *td,
> >                                          struct mt_application *app);
> >  static void mt_post_parse(struct mt_device *td, struct mt_application *app);
> > @@ -227,6 +231,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
> >  #define MT_CLS_SMART_TECH                    0x0113
> >  #define MT_CLS_APPLE_TOUCHBAR                        0x0114
> >  #define MT_CLS_SIS                           0x0457
> > +#define MT_CLS_REGISTER_LID_NOTIFIER 0x0115
> >
> >  #define MT_DEFAULT_MAXCONTACT        10
> >  #define MT_MAX_MAXCONTACT    250
> > @@ -327,7 +332,9 @@ static const struct mt_class mt_classes[] = {
> >                       MT_QUIRK_CONTACT_CNT_ACCURATE |
> >                       MT_QUIRK_WIN8_PTP_BUTTONS,
> >               .export_all_inputs = true },
> > -
> > +     { .name = MT_CLS_REGISTER_LID_NOTIFIER,
> > +             .quirks = MT_QUIRK_REGISTER_LID_NOTIFIER,
> > +             .export_all_inputs = true },
> >       /*
> >        * vendor specific classes
> >        */
> > @@ -1840,6 +1847,20 @@ static void mt_expired_timeout(struct timer_list *t)
> >       clear_bit_unlock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags);
> >  }
> >
> > +static int mt_input_notifier(struct notifier_block *nb, unsigned long action, void *dev)
> > +{
> > +     if (action)
> > +             mt_set_modes(lid_notify_hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_NONE);
> > +     else if (!action)
> > +             mt_set_modes(lid_notify_hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct notifier_block mt_lid_notifier_block = {
> > +     .notifier_call = mt_input_notifier
> > +};
> > +
> >  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >  {
> >       int ret, i;
> > @@ -1920,6 +1941,11 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >       if (hdev->vendor == USB_VENDOR_ID_SIS_TOUCH)
> >               hdev->quirks |= HID_QUIRK_NOGET;
> >
> > +     if (mtclass->quirks & MT_CLS_REGISTER_LID_NOTIFIER) {
> > +             lid_notify_hdev = hdev;
> > +             register_lid_notifier(&mt_lid_notifier_block);
> > +     }
> > +
> >       ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> >       if (ret)
> >               return ret;
> > @@ -2150,6 +2176,10 @@ static const struct hid_device_id mt_devices[] = {
> >               HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> >                       USB_VENDOR_ID_ELAN, 0x32ae) },
> >
> > +     { .driver_data = MT_CLS_REGISTER_LID_NOTIFIER,
> > +             HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> > +                     USB_VENDOR_ID_ELAN, 0x323b) },
>
> The need to have special handling of LID events is a quirk of board
> design, not quire of a controller. So I think it needs to be triggered
> by DMI quirk.
> > +
> >       /* Elitegroup panel */
> >       { .driver_data = MT_CLS_SERIAL,
> >               MT_USB_DEVICE(USB_VENDOR_ID_ELITEGROUP,
> >
>
> Thanks.
>
> --
> Dmitry
-- 
Jonathan

^ permalink raw reply

* [RFC PATCH v2 0/3] HID: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-13  0:15 UTC (permalink / raw)
  To: linux-input
  Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Sebastian Reichel, Bastien Nocera, linux-pm

This RFC introduces support for multiple batteries per HID device, addressing
a long-standing architectural limitation in the HID battery reporting subsystem.

## Background

The current HID implementation explicitly prevents multiple batteries per device
through an early return in hidinput_setup_battery() that enforces a single-battery
assumption. Linux treats peripheral batteries (scope=Device) differently from system
batteries, with desktop environments often displaying them separately or ignoring
them entirely. However, this design doesn't account for modern multi-battery hardware patterns.

## Problem Statement

Multiple battery scenarios that cannot be properly reported today:

1. Gaming headsets with charging docks (e.g., SteelSeries Arctis Nova Pro
   Wireless) - headset battery reported, dock battery invisible
2. Graphics tablets with stylus batteries (Wacom) - requires driver-specific
   workarounds
3. Wireless earbuds with per-earbud batteries plus charging case
4. Multi-device receivers (Logitech Unifying) - requires proprietary HID++
   protocol parsing

This forces manufacturers to use proprietary protocols and vendor-specific
software. Community projects parse USB packets directly because standard HID
battery reporting cannot handle multi-battery scenarios.

## Why This Matters

The current limitation creates a cycle: OS lacks support, so manufacturers
implement proprietary protocols, which makes vendor software necessary, which
reduces pressure to fix the OS limitation. Improving HID core support for
multiple batteries would enable standardized reporting, reduce the need for
vendor software, improve OS integration, reduce driver duplication, and provide
a foundation for future multi-battery devices.

## Proposed Solution

This series introduces struct hid_battery to encapsulate individual battery
state, refactors the code to use this structure internally, and adds support
for multiple batteries tracked in a list within struct hid_device. Batteries
are identified by report ID. The implementation maintains full backwards
compatibility with existing single-battery code.

## Testing

Tested with split keyboard hardware (Dactyl 5x6) using custom ZMK firmware
that implements per-side HID battery reporting. Each battery (left and right
keyboard halves) reports independently through the power supply interface with
distinct report IDs (0x05 and 0x06).

Test firmware available on my personal fork at:
https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting
If this series gets merged, these changes will be proposed to upstream ZMK.

HID descriptor and recording captured with hid-recorder:

D: 0
R: 162 05 01 09 06 a1 01 85 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 05 07 75 08 95 01 81 03 05 07 15 00 25 01 19 00 29 67 75 01 95 68 81 02 c0 05 0c 09 01 a1 01 85 02 05 0c 15 00 26 ff 0f 19 00 2a ff 0f 75 10 95 06 81 00 c0 05 84 09 05 a1 01 05 85 85 05 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0 05 84 09 05 a1 01 05 85 85 06 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0
N: ZMK Project Dactyl 5x6
P: usb-0000:2d:00.3-4.2/input2
I: 3 1d50 615e
D: 0
E: 0.000000 3 05 00 56
E: 0.000977 3 05 00 56
E: 1.490974 3 06 00 52
E: 1.491958 3 06 00 52
E: 6.492979 3 06 00 53
E: 6.493962 3 06 00 53

The recording shows both batteries reporting with different charge levels
(Report ID 05: 86%, Report ID 06: 82%-83%), demonstrating the multi-battery
functionality. This can be used to verify UPower compatibility.

## Future Work: Userspace Integration

As suggested by Bastien, semantic battery differentiation (e.g., "left
earbud" vs "right earbud") requires userspace coordination, as HID
reports typically lack role metadata.

This will require:
1. systemd/hwdb entries for device-specific battery role mappings
2. UPower updates to enumerate and group multi-battery devices
3. Desktop environment changes to display batteries with meaningful labels

This kernel infrastructure is a prerequisite for that userspace work.

## Request for Comments

Is list-based storage appropriate or would another structure work better?
Should we support usage-based identification in addition to report ID for
devices using the same report ID? Is sequential naming (battery-N) sufficient
or should batteries have semantic role identifiers like "main", "stylus", "dock"?

To HID maintainers (Jiri Kosina, Benjamin Tissoires): Does this belong in
hid-input.c or should it be separate? Any concerns about the backwards
compatibility approach? Meaning, should I have removed the whole
dev->bat legacy mapping and use the new struct?

To power supply maintainers (Sebastian Reichel): Any issues with multiple
power_supply devices from a single HID device?

Related commits:
- c6838eeef2fb: HID: hid-input: occasionally report stylus battery
- a608dc1c0639: HID: input: map battery system charging
- fd2a9b29dc9c: HID: wacom: Remove AES power_supply after inactivity

Community projects demonstrating the need:
- HeadsetControl: https://github.com/Sapd/HeadsetControl
- Solaar: https://github.com/pwr-Solaar/Solaar
- OpenRazer: https://github.com/openrazer/openrazer

Lucas Zampieri (3):
  HID: input: Introduce struct hid_battery
  HID: input: Refactor battery code to use struct hid_battery
  HID: input: Add support for multiple batteries per device

Changes in v2:
- Split the monolithic v1 patch into three logical patches for easier review:
  1. Introduce struct hid_battery (pure structure addition)
  2. Refactor existing code to use the new structure (internal changes)
  3. Add multi-battery support (new functionality)
- Added detailed testing section with hardware specifics
- Added hid-recorder output (dactyl-hid-recording.txt) demonstrating two-battery
  HID descriptor for UPower validation
- Added "Future Work: Userspace Integration" section addressing Bastien's feedback
  about semantic battery differentiation
- Added hardware examples with product links to commit messages (per Bastien's
  suggestion)
- No functional changes from v1, only improved patch organization and documentation

 drivers/hid/hid-core.c  |   4 +
 drivers/hid/hid-input.c | 196 +++++++++++++++++++++++++++-------------
 include/linux/hid.h     |  42 ++++++++-
 3 files changed, 179 insertions(+), 63 deletions(-)

--
2.51.1


^ permalink raw reply

* [RFC PATCH v2 1/3] HID: input: Introduce struct hid_battery
From: Lucas Zampieri @ 2025-11-13  0:15 UTC (permalink / raw)
  To: linux-input
  Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251113001508.713574-1-lzampier@redhat.com>

Add struct hid_battery to encapsulate battery state per HID device.
This structure will allow tracking individual battery properties
including capacity, min/max values, report information, and status.

The structure includes a list node to enable support for multiple
batteries per device in subsequent patches.

This is a preparation step for transitioning from direct power_supply
access to a more flexible battery management system.

Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
 include/linux/hid.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index a4ddb94e3ee5..63422130de20 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;
 
-- 
2.51.1


^ permalink raw reply related

* [RFC PATCH v2 2/3] HID: input: Refactor battery code to use struct hid_battery
From: Lucas Zampieri @ 2025-11-13  0:15 UTC (permalink / raw)
  To: linux-input
  Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251113001508.713574-1-lzampier@redhat.com>

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.1


^ permalink raw reply related

* [RFC PATCH v2 3/3] HID: input: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-13  0:15 UTC (permalink / raw)
  To: linux-input
  Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251113001508.713574-1-lzampier@redhat.com>

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.1


^ permalink raw reply related

* Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling
From: Denis Benato @ 2025-11-13  0:45 UTC (permalink / raw)
  To: Antheas Kapenekakis
  Cc: Ilpo Järvinen, platform-driver-x86, linux-input, LKML,
	Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
	Hans de Goede
In-Reply-To: <CAGwozwH4_5LnJ4kt-sbdwzL5Ezt74ddPOpGqvTvKiiVzkorSMQ@mail.gmail.com>


On 11/13/25 00:22, Antheas Kapenekakis wrote:
> On Wed, 12 Nov 2025 at 23:08, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>> snip
>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
>>>> version of the series.
>>>>
>>>> The first one was that asusctl has a hang bug, which he hasn't had
>>>> time to look into yet. This should have been fixed by dropping the
>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
>>>> bit of a NOOP that does not need to be added in the future.
>>> So it is supposed to not regress it now, correct?
> I missed this. Spaces. Yes, quirk input per app created around 3 more
> input devices per USB device, plus the dynamic lighting one, so you
> went from 2 to 6, and there seems to be an issue with asusctl and a
> large number of event devices, that caused some intermittent freezing
> when the user typed on certain keyboards. I removed it. Although, not
> a problem with the kernel itself, per say.
Okay, I will ask asus-linux kernel man to load this patchset then.
> Antheas
>
>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
>>>> said (back in March) that it is fine to drop 0x5e because it is only
>>>>> snip

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox