Re: [PATCH 2/2] HID: input: Add HID_BATTERY_QUIRK_DYNAMIC for Elan touchscreens

[Sebastian Reichel <sebastian.reichel@collabora.com>]

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

Hi,

On Mon, Mar 02, 2026 at 10:19:59AM +0100, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 2-Mar-26 1:44 AM, Dmitry Torokhov wrote:
> > Hi Hans,
> > 
> > On Sat, Feb 28, 2026 at 03:52:58PM +0100, Hans de Goede wrote:
> >> Elan touchscreens have a HID-battery device for the stylus which is always
> >> there even if there is no stylus.
> >>
> >> This is causing upower to report an empty battery for the stylus and some
> >> desktop-environments will show a notification about this, which is quite
> >> annoying.
> >>
> >> Because of this the HID-battery is being ignored on all Elan I2c and USB
> >> touchscreens, but this causes there to be no battery reporting for
> >> the stylus at all.
> >>
> >> This adds a new HID_BATTERY_QUIRK_DYNAMIC and uses these for the Elan
> >> touchscreens.
> >>
> >> This new quirks causes the present value of the battery to start at 0,
> >> which will make userspace ignore it and only sets present to 1 after
> >> receiving a battery input report which only happens when the stylus
> >> gets in range.
> > 
> > My understanding was that "present" attribute is to be used for
> > removable batteries (i.e. when a battery can be extracted from either
> > from the system or from a peripheral) and we want to notify userspace of
> > that fact).
> 
> In this case we have a touchscreen and a stylus with a battery embedded
> in the stylus and we're setting present to 0 when the stylus (and thus
> the battery) is not there. I realize this is a bit different but it
> is close enough IMHO.
> 
> Also keep in mind that most people simply do not have the stylus at all,
> these styluses are typically an optional accessory and some models where
> the touchscreen supports this the vendor is not even selling a stylus to
> go with the 2-in-1 laptop (but you may use a compatible stylus from other
> vendors).
> 
> TL;DR; the user not having a stylus at all is the most common use case here.
> 
> > For example, if a laptop can have an additional battery UI
> > can show if it is installed or not, and it should not simply ignore such
> > power supplies.
> 
> As the screenshots of the KDE control-panel in the bug for this:
> https://bugzilla.kernel.org/show_bug.cgi?id=221118
> 
> Show it is not being ignored, but its charge-level is ignored,
> with the battery being shown as not present. Which is pretty much
> what we want.
> 
> > On the other hand the "Unknown" operating status signals that we
> > actually do not know the state of the battery, and fits better in our
> > situation.
> 
> Reporting "Unknown" in the status attribute is not something which
> drivers normally do. It is there in the ABI and upower seems to somewhat
> handle it, but upower only handles it in that it forwards it to upower
> DBUS API users it does not reset history, etc. as it does for present=0.
> 
> The problem with status=Unknown is that its meaning is not really well
> defined. Unknown status mostly means that it is unknown if the battery is
> charging / discharging / full for some reason. But if there still is a low
> capacity reading then userspace might still warn about the low capacity in
> this case, since the battery is still almost empty and presumably not
> charging.
> 
> Also when the stylus is not there, the battery state is not unknown,
> the entire stylus, including the battery is simply not there, so
> reporting present=0 seems more appropriate and its meaning is well
> defined and present=0 causes upower to not read/report any of the
> other attributes.
> 
> And as mentioned most users will not have the stylus I think having
> a non-present "ELAN Touchscreen Stylus battery" being reported makes
> some sense to end users, where as having the same battery being
> reported "Unknown" status and at 0% charge (Unknown status batteries
> do have a charge) is more confusing for users.
> 
> > Anyway, I think we need Sebastian's input here.
> 
> Ack, Sebastian your input would be appreciated here ?

The rationale and the patch LGTM. The other option would be to not
register the battery device in the first place and do that once the
first report arrived.

Greetings,

-- Sebastian

> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> >>
> >> Reported-by: ggrundik@gmail.com
> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221118
> >> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> >> ---
> >>  drivers/hid/hid-input.c | 14 +++++++++++---
> >>  include/linux/hid.h     |  1 +
> >>  2 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> >> index 67ca1e88ce13..8fc20df99b97 100644
> >> --- a/drivers/hid/hid-input.c
> >> +++ b/drivers/hid/hid-input.c
> >> @@ -354,6 +354,7 @@ static enum power_supply_property hidinput_battery_props[] = {
> >>  #define HID_BATTERY_QUIRK_FEATURE	(1 << 1) /* ask for feature report */
> >>  #define HID_BATTERY_QUIRK_IGNORE	(1 << 2) /* completely ignore the battery */
> >>  #define HID_BATTERY_QUIRK_AVOID_QUERY	(1 << 3) /* do not query the battery */
> >> +#define HID_BATTERY_QUIRK_DYNAMIC	(1 << 4) /* report present only after life signs */
> >>  
> >>  static const struct hid_device_id hid_battery_quirks[] = {
> >>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
> >> @@ -398,8 +399,8 @@ static const struct hid_device_id hid_battery_quirks[] = {
> >>  	 * Elan HID touchscreens seem to all report a non present battery,
> >>  	 * set HID_BATTERY_QUIRK_IGNORE for all Elan I2C and USB HID devices.
> >>  	 */
> >> -	{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, HID_ANY_ID), HID_BATTERY_QUIRK_IGNORE },
> >> -	{ HID_USB_DEVICE(USB_VENDOR_ID_ELAN, HID_ANY_ID), HID_BATTERY_QUIRK_IGNORE },
> >> +	{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, HID_ANY_ID), HID_BATTERY_QUIRK_DYNAMIC },
> >> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ELAN, HID_ANY_ID), HID_BATTERY_QUIRK_DYNAMIC },
> >>  	{}
> >>  };
> >>  
> >> @@ -456,11 +457,14 @@ static int hidinput_get_battery_property(struct power_supply *psy,
> >>  	int ret = 0;
> >>  
> >>  	switch (prop) {
> >> -	case POWER_SUPPLY_PROP_PRESENT:
> >>  	case POWER_SUPPLY_PROP_ONLINE:
> >>  		val->intval = 1;
> >>  		break;
> >>  
> >> +	case POWER_SUPPLY_PROP_PRESENT:
> >> +		val->intval = dev->battery_present;
> >> +		break;
> >> +
> >>  	case POWER_SUPPLY_PROP_CAPACITY:
> >>  		if (dev->battery_status != HID_BATTERY_REPORTED &&
> >>  		    !dev->battery_avoid_query) {
> >> @@ -573,6 +577,8 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
> >>  	if (quirks & HID_BATTERY_QUIRK_AVOID_QUERY)
> >>  		dev->battery_avoid_query = true;
> >>  
> >> +	dev->battery_present = (quirks & HID_BATTERY_QUIRK_DYNAMIC) ? false : true;
> >> +
> >>  	dev->battery = power_supply_register(&dev->dev, psy_desc, &psy_cfg);
> >>  	if (IS_ERR(dev->battery)) {
> >>  		error = PTR_ERR(dev->battery);
> >> @@ -628,6 +634,7 @@ static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> >>  		return;
> >>  
> >>  	if (hidinput_update_battery_charge_status(dev, usage, value)) {
> >> +		dev->battery_present = true;
> >>  		power_supply_changed(dev->battery);
> >>  		return;
> >>  	}
> >> @@ -643,6 +650,7 @@ static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
> >>  	if (dev->battery_status != HID_BATTERY_REPORTED ||
> >>  	    capacity != dev->battery_capacity ||
> >>  	    ktime_after(ktime_get_coarse(), dev->battery_ratelimit_time)) {
> >> +		dev->battery_present = true;
> >>  		dev->battery_capacity = capacity;
> >>  		dev->battery_status = HID_BATTERY_REPORTED;
> >>  		dev->battery_ratelimit_time =
> >> diff --git a/include/linux/hid.h b/include/linux/hid.h
> >> index dce862cafbbd..d9b54f0e8671 100644
> >> --- a/include/linux/hid.h
> >> +++ b/include/linux/hid.h
> >> @@ -682,6 +682,7 @@ struct hid_device {
> >>  	__s32 battery_charge_status;
> >>  	enum hid_battery_status battery_status;
> >>  	bool battery_avoid_query;
> >> +	bool battery_present;
> >>  	ktime_t battery_ratelimit_time;
> >>  #endif
> >>  
> > 
> > Thanks.
> > 
> 

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