[PATCH 1/2] HID: wacom: Lazy-init batteries

[PATCH 1/2] HID: wacom: Lazy-init batteries

[Jason Gerecke]

From: Jason Gerecke <killertofu@gmail.com>

Rather than creating batteries as part of the initial device probe, let's
make the process lazy. This gives us the opportunity to prevent batteries
from being created in situations where they are unnecessary.

There are two cases in particular where batteries are being unnecessarily
created at initialization. These are AES sensors (for which we don't know
any battery status information until a battery-powered pen actually comes
into prox) peripheral tablets which share HID descriptors between the
wired-only and wireless-capable SKUs of a family of devices.

This patch will delay battery initialization of the former until a pen
actually comes into prox. It will delay battery initialization of the
latter until either a pen comes into prox or a "heartbeat" packet is
processed.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Tested-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/wacom_sys.c | 10 ----------
 drivers/hid/wacom_wac.c | 13 ++++++-------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index fb538a6c4add8..8214896adadad 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2372,13 +2372,6 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	if (error)
 		goto fail;
 
-	if (!(features->device_type & WACOM_DEVICETYPE_WL_MONITOR) &&
-	     (features->quirks & WACOM_QUIRK_BATTERY)) {
-		error = wacom_initialize_battery(wacom);
-		if (error)
-			goto fail;
-	}
-
 	error = wacom_register_inputs(wacom);
 	if (error)
 		goto fail;
@@ -2509,9 +2502,6 @@ static void wacom_wireless_work(struct work_struct *work)
 
 		strscpy(wacom_wac->name, wacom_wac1->name,
 			sizeof(wacom_wac->name));
-		error = wacom_initialize_battery(wacom);
-		if (error)
-			goto fail;
 	}
 
 	return;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 4cfa51416dbcb..391fde5bf6024 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -113,6 +113,11 @@ static void wacom_notify_battery(struct wacom_wac *wacom_wac,
 	bool bat_connected, bool ps_connected)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+	bool bat_initialized = wacom->battery.battery;
+	bool has_quirk = wacom_wac->features.quirks & WACOM_QUIRK_BATTERY;
+
+	if (bat_initialized != has_quirk)
+		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 
 	__wacom_notify_battery(&wacom->battery, bat_status, bat_capacity,
 			       bat_charging, bat_connected, ps_connected);
@@ -3391,19 +3396,13 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		int battery = (data[8] & 0x3f) * 100 / 31;
 		bool charging = !!(data[8] & 0x80);
 
+		features->quirks |= WACOM_QUIRK_BATTERY;
 		wacom_notify_battery(wacom_wac, WACOM_POWER_SUPPLY_STATUS_AUTO,
 				     battery, charging, battery || charging, 1);
-
-		if (!wacom->battery.battery &&
-		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
-			features->quirks |= WACOM_QUIRK_BATTERY;
-			wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
-		}
 	}
 	else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
 		 wacom->battery.battery) {
 		features->quirks &= ~WACOM_QUIRK_BATTERY;
-		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		wacom_notify_battery(wacom_wac, POWER_SUPPLY_STATUS_UNKNOWN, 0, 0, 0, 0);
 	}
 	return 0;
-- 
2.40.0

[PATCH 2/2] HID: wacom: generic: Set battery quirk only when we see battery data

[Jason Gerecke]

From: Jason Gerecke <killertofu@gmail.com>

Some devices will include battery status usages in the HID descriptor
but we won't see that battery data for one reason or another. For example,
AES sensors won't send battery data unless an AES pen is in proximity.
If a user does not have an AES pen but instead only interacts with the
AES touchscreen with their fingers then there is no need for us to create
a battery object. Similarly, if a family of peripherals shares the same
HID descriptor between wired-only and wireless-capable SKUs, users of the
former may never see a battery event and will not want a power_supply
object created.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Tested-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/wacom_wac.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 391fde5bf6024..5db7de7a6b171 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1960,18 +1960,7 @@ static void wacom_map_usage(struct input_dev *input, struct hid_usage *usage,
 static void wacom_wac_battery_usage_mapping(struct hid_device *hdev,
 		struct hid_field *field, struct hid_usage *usage)
 {
-	struct wacom *wacom = hid_get_drvdata(hdev);
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
-	struct wacom_features *features = &wacom_wac->features;
-	unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
-
-	switch (equivalent_usage) {
-	case HID_DG_BATTERYSTRENGTH:
-	case WACOM_HID_WD_BATTERY_LEVEL:
-	case WACOM_HID_WD_BATTERY_CHARGING:
-		features->quirks |= WACOM_QUIRK_BATTERY;
-		break;
-	}
+	return;
 }
 
 static void wacom_wac_battery_event(struct hid_device *hdev, struct hid_field *field,
@@ -1992,18 +1981,21 @@ static void wacom_wac_battery_event(struct hid_device *hdev, struct hid_field *f
 			wacom_wac->hid_data.bat_connected = 1;
 			wacom_wac->hid_data.bat_status = WACOM_POWER_SUPPLY_STATUS_AUTO;
 		}
+		wacom_wac->features.quirks |= WACOM_QUIRK_BATTERY;
 		break;
 	case WACOM_HID_WD_BATTERY_LEVEL:
 		value = value * 100 / (field->logical_maximum - field->logical_minimum);
 		wacom_wac->hid_data.battery_capacity = value;
 		wacom_wac->hid_data.bat_connected = 1;
 		wacom_wac->hid_data.bat_status = WACOM_POWER_SUPPLY_STATUS_AUTO;
+		wacom_wac->features.quirks |= WACOM_QUIRK_BATTERY;
 		break;
 	case WACOM_HID_WD_BATTERY_CHARGING:
 		wacom_wac->hid_data.bat_charging = value;
 		wacom_wac->hid_data.ps_connected = value;
 		wacom_wac->hid_data.bat_connected = 1;
 		wacom_wac->hid_data.bat_status = WACOM_POWER_SUPPLY_STATUS_AUTO;
+		wacom_wac->features.quirks |= WACOM_QUIRK_BATTERY;
 		break;
 	}
 }
@@ -2019,18 +2011,15 @@ static void wacom_wac_battery_report(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
-	struct wacom_features *features = &wacom_wac->features;
 
-	if (features->quirks & WACOM_QUIRK_BATTERY) {
-		int status = wacom_wac->hid_data.bat_status;
-		int capacity = wacom_wac->hid_data.battery_capacity;
-		bool charging = wacom_wac->hid_data.bat_charging;
-		bool connected = wacom_wac->hid_data.bat_connected;
-		bool powered = wacom_wac->hid_data.ps_connected;
+	int status = wacom_wac->hid_data.bat_status;
+	int capacity = wacom_wac->hid_data.battery_capacity;
+	bool charging = wacom_wac->hid_data.bat_charging;
+	bool connected = wacom_wac->hid_data.bat_connected;
+	bool powered = wacom_wac->hid_data.ps_connected;
 
-		wacom_notify_battery(wacom_wac, status, capacity, charging,
-				     connected, powered);
-	}
+	wacom_notify_battery(wacom_wac, status, capacity, charging,
+			     connected, powered);
 }
 
 static void wacom_wac_pad_usage_mapping(struct hid_device *hdev,
-- 
2.40.0

RE: [PATCH 1/2] HID: wacom: Lazy-init batteries

[Limonciello, Mario]

[Public]

> From: Jason Gerecke <killertofu@gmail.com>
> 
> Rather than creating batteries as part of the initial device probe, let's
> make the process lazy. This gives us the opportunity to prevent batteries
> from being created in situations where they are unnecessary.
> 
> There are two cases in particular where batteries are being unnecessarily
> created at initialization. These are AES sensors (for which we don't know
> any battery status information until a battery-powered pen actually comes
> into prox) peripheral tablets which share HID descriptors between the
> wired-only and wireless-capable SKUs of a family of devices.
> 
> This patch will delay battery initialization of the former until a pen
> actually comes into prox. It will delay battery initialization of the
> latter until either a pen comes into prox or a "heartbeat" packet is
> processed.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>

Some other tags to add:

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217062
Link: https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/2354

> ---
>  drivers/hid/wacom_sys.c | 10 ----------
>  drivers/hid/wacom_wac.c | 13 ++++++-------
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index fb538a6c4add8..8214896adadad 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2372,13 +2372,6 @@ static int wacom_parse_and_register(struct
> wacom *wacom, bool wireless)
>  	if (error)
>  		goto fail;
> 
> -	if (!(features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
> &&
> -	     (features->quirks & WACOM_QUIRK_BATTERY)) {
> -		error = wacom_initialize_battery(wacom);
> -		if (error)
> -			goto fail;
> -	}
> -
>  	error = wacom_register_inputs(wacom);
>  	if (error)
>  		goto fail;
> @@ -2509,9 +2502,6 @@ static void wacom_wireless_work(struct
> work_struct *work)
> 
>  		strscpy(wacom_wac->name, wacom_wac1->name,
>  			sizeof(wacom_wac->name));
> -		error = wacom_initialize_battery(wacom);
> -		if (error)
> -			goto fail;
>  	}
> 
>  	return;
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 4cfa51416dbcb..391fde5bf6024 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -113,6 +113,11 @@ static void wacom_notify_battery(struct
> wacom_wac *wacom_wac,
>  	bool bat_connected, bool ps_connected)
>  {
>  	struct wacom *wacom = container_of(wacom_wac, struct wacom,
> wacom_wac);
> +	bool bat_initialized = wacom->battery.battery;
> +	bool has_quirk = wacom_wac->features.quirks &
> WACOM_QUIRK_BATTERY;
> +
> +	if (bat_initialized != has_quirk)
> +		wacom_schedule_work(wacom_wac,
> WACOM_WORKER_BATTERY);
> 
>  	__wacom_notify_battery(&wacom->battery, bat_status,
> bat_capacity,
>  			       bat_charging, bat_connected, ps_connected);
> @@ -3391,19 +3396,13 @@ static int wacom_status_irq(struct wacom_wac
> *wacom_wac, size_t len)
>  		int battery = (data[8] & 0x3f) * 100 / 31;
>  		bool charging = !!(data[8] & 0x80);
> 
> +		features->quirks |= WACOM_QUIRK_BATTERY;
>  		wacom_notify_battery(wacom_wac,
> WACOM_POWER_SUPPLY_STATUS_AUTO,
>  				     battery, charging, battery || charging, 1);
> -
> -		if (!wacom->battery.battery &&
> -		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
> -			features->quirks |= WACOM_QUIRK_BATTERY;
> -			wacom_schedule_work(wacom_wac,
> WACOM_WORKER_BATTERY);
> -		}
>  	}
>  	else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
>  		 wacom->battery.battery) {
>  		features->quirks &= ~WACOM_QUIRK_BATTERY;
> -		wacom_schedule_work(wacom_wac,
> WACOM_WORKER_BATTERY);
>  		wacom_notify_battery(wacom_wac,
> POWER_SUPPLY_STATUS_UNKNOWN, 0, 0, 0, 0);
>  	}
>  	return 0;
> --
> 2.40.0

Re: [PATCH 1/2] HID: wacom: Lazy-init batteries

[Jiri Kosina]

On Thu, 13 Apr 2023, Jason Gerecke wrote:

> From: Jason Gerecke <killertofu@gmail.com>
> 
> Rather than creating batteries as part of the initial device probe, let's
> make the process lazy. This gives us the opportunity to prevent batteries
> from being created in situations where they are unnecessary.
> 
> There are two cases in particular where batteries are being unnecessarily
> created at initialization. These are AES sensors (for which we don't know
> any battery status information until a battery-powered pen actually comes
> into prox) peripheral tablets which share HID descriptors between the
> wired-only and wireless-capable SKUs of a family of devices.
> 
> This patch will delay battery initialization of the former until a pen
> actually comes into prox. It will delay battery initialization of the
> latter until either a pen comes into prox or a "heartbeat" packet is
> processed.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>

I have added the Link: tags suggested by Mario and applied, thanks.

-- 
Jiri Kosina
SUSE Labs