[PATCH] HID: wacom: Remove AES power_supply after extended inactivity

[PATCH] HID: wacom: Remove AES power_supply after extended inactivity

[Tatsunosuke Tobita]

Even if a user does not use their AES pen for an extended period,
the battery power supply attributes continue to exist.
This results in the desktop showing battery status for a pen
that is no longer in use and which may in fact be in a different
state (e.g. the user may be charging the pen).
To avoid confusion and ensure userspace has an accurate view
of the battery state, this patch automatically removes
the power_supply after 30 minutes of inactivity.

Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
Reviewed-by: Jason Gerecke <Jason.Gerecke@wacom.com>
Reviewed-by: Aaron Skomra <aaron.skomra@wacom.com>
Reviewed-by: Josh Dickens <joshua.dickens@wacom.com>
---
 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c |  8 ++++++++
 drivers/hid/wacom_wac.c | 12 +++++++++++-
 drivers/hid/wacom_wac.h |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 166a76c9bcad..77c5fb26cd14 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -164,6 +164,7 @@ struct wacom {
 	struct work_struct battery_work;
 	struct work_struct remote_work;
 	struct delayed_work init_work;
+	struct delayed_work aes_battery_work;
 	struct wacom_remote *remote;
 	struct work_struct mode_change_work;
 	struct timer_list idleprox_timer;
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3f704b8072e8..b613f11ed949 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1813,6 +1813,13 @@ static void wacom_destroy_battery(struct wacom *wacom)
 	}
 }
 
+static void wacom_aes_battery_handler(struct work_struct *work)
+{
+	struct wacom *wacom = container_of(work, struct wacom, aes_battery_work.work);
+
+	wacom_destroy_battery(wacom);
+}
+
 static ssize_t wacom_show_speed(struct device *dev,
 				struct device_attribute
 				*attr, char *buf)
@@ -2794,6 +2801,7 @@ static int wacom_probe(struct hid_device *hdev,
 
 	mutex_init(&wacom->lock);
 	INIT_DELAYED_WORK(&wacom->init_work, wacom_init_work);
+	INIT_DELAYED_WORK(&wacom->aes_battery_work, wacom_aes_battery_handler);
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 	INIT_WORK(&wacom->remote_work, wacom_remote_work);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 471db78dbbf0..c205198ded11 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2528,11 +2528,12 @@ static void wacom_wac_pen_report(struct hid_device *hdev,
 	struct input_dev *input = wacom_wac->pen_input;
 	bool range = wacom_wac->hid_data.inrange_state;
 	bool sense = wacom_wac->hid_data.sense_state;
+	bool entering_range = !wacom_wac->tool[0] && range;
 
 	if (wacom_wac->is_invalid_bt_frame)
 		return;
 
-	if (!wacom_wac->tool[0] && range) { /* first in range */
+	if (entering_range) { /* first in range */
 		/* Going into range select tool */
 		if (wacom_wac->hid_data.invert_state)
 			wacom_wac->tool[0] = BTN_TOOL_RUBBER;
@@ -2583,6 +2584,15 @@ static void wacom_wac_pen_report(struct hid_device *hdev,
 		input_sync(input);
 	}
 
+	/* Handle AES battery timeout behavior */
+	if (wacom_wac->features.quirks & WACOM_QUIRK_AESPEN) {
+		if (entering_range)
+			cancel_delayed_work(&wacom->aes_battery_work);
+		if (!sense)
+			schedule_delayed_work(&wacom->aes_battery_work,
+					      msecs_to_jiffies(WACOM_AES_BATTERY_TIMEOUT));
+	}
+
 	if (!sense) {
 		wacom_wac->tool[0] = 0;
 		wacom_wac->id[0] = 0;
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 57e185f18d53..e63b1e806e34 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -14,6 +14,7 @@
 #define WACOM_MAX_REMOTES	5
 #define WACOM_STATUS_UNKNOWN	255
 #define WACOM_REMOTE_BATTERY_TIMEOUT	21000000000ll
+#define WACOM_AES_BATTERY_TIMEOUT       1800000
 
 /* packet length for individual models */
 #define WACOM_PKGLEN_BBFUN	 9
-- 
2.34.1

Re: [PATCH] HID: wacom: Remove AES power_supply after extended inactivity

[Benjamin Tissoires]

On Wed, 15 Nov 2023 08:57:29 +0900, Tatsunosuke Tobita wrote:
> Even if a user does not use their AES pen for an extended period,
> the battery power supply attributes continue to exist.
> This results in the desktop showing battery status for a pen
> that is no longer in use and which may in fact be in a different
> state (e.g. the user may be charging the pen).
> To avoid confusion and ensure userspace has an accurate view
> of the battery state, this patch automatically removes
> the power_supply after 30 minutes of inactivity.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.8/wacom), thanks!

[1/1] HID: wacom: Remove AES power_supply after extended inactivity
      https://git.kernel.org/hid/hid/c/fd2a9b29dc9c

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>

Re: [PATCH] HID: wacom: Remove AES power_supply after extended inactivity

[Benjamin Tissoires]

On Wed, Dec 6, 2023 at 9:32 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Wed, 15 Nov 2023 08:57:29 +0900, Tatsunosuke Tobita wrote:
> > Even if a user does not use their AES pen for an extended period,
> > the battery power supply attributes continue to exist.
> > This results in the desktop showing battery status for a pen
> > that is no longer in use and which may in fact be in a different
> > state (e.g. the user may be charging the pen).
> > To avoid confusion and ensure userspace has an accurate view
> > of the battery state, this patch automatically removes
> > the power_supply after 30 minutes of inactivity.
> >
> > [...]
>
> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.8/wacom), thanks!
>
> [1/1] HID: wacom: Remove AES power_supply after extended inactivity
>       https://git.kernel.org/hid/hid/c/fd2a9b29dc9c

FWIW, I have applied the changes, but please fix your workflow.
checkpatch complains about:
```
WARNING: From:/Signed-off-by: email address mismatch:
'From: Tatsunosuke Tobita <tatsunosuke.wacom@gmail.com>' !=
'Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>'
```

It should be a matter of configuring your git client or remember to
add the correct "from" line on top of your patch.

Cheers,
Benjamin

>
> Cheers,
> --
> Benjamin Tissoires <bentiss@kernel.org>
>

[PATCH] HID: wacom: Remove AES power_supply after extended inactivity

[Tatsunosuke]

From: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>

Even if a user does not use their AES pen for an extended period,
the battery power supply attributes continue to exist.
This results in the desktop showing battery status for a pen
that is no longer in use and which may in fact be in a different
state (e.g. the user may be charging the pen).
To avoid confusion and ensure userspace has an accurate view
of the battery state, this patch automatically removes
the power_supply after 30 minutes of inactivity.

Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
Reviewed-by: Jason Gerecke <Jason.Gerecke@wacom.com>
Reviewed-by: Aaron Skomra <aaron.skomra@wacom.com>
Reviewed-by: Josh Dickens <joshua.dickens@wacom.com>
---
 drivers/hid/wacom.h     |  2 ++
 drivers/hid/wacom_sys.c | 52 ++++++++++++++++++++++++++++++-----------
 drivers/hid/wacom_wac.c | 19 +++++++++++----
 drivers/hid/wacom_wac.h |  5 ++--
 4 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 4da50e19808e..77c5fb26cd14 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -150,6 +150,7 @@ struct wacom_remote {
 		struct input_dev *input;
 		bool registered;
 		struct wacom_battery battery;
+		ktime_t active_time;
 	} remotes[WACOM_MAX_REMOTES];
 };
 
@@ -163,6 +164,7 @@ struct wacom {
 	struct work_struct battery_work;
 	struct work_struct remote_work;
 	struct delayed_work init_work;
+	struct delayed_work aes_battery_work;
 	struct wacom_remote *remote;
 	struct work_struct mode_change_work;
 	struct timer_list idleprox_timer;
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 76e5353aca0c..b613f11ed949 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1813,6 +1813,13 @@ static void wacom_destroy_battery(struct wacom *wacom)
 	}
 }
 
+static void wacom_aes_battery_handler(struct work_struct *work)
+{
+	struct wacom *wacom = container_of(work, struct wacom, aes_battery_work.work);
+
+	wacom_destroy_battery(wacom);
+}
+
 static ssize_t wacom_show_speed(struct device *dev,
 				struct device_attribute
 				*attr, char *buf)
@@ -1997,7 +2004,7 @@ static int wacom_initialize_remotes(struct wacom *wacom)
 	spin_lock_init(&remote->remote_lock);
 
 	error = kfifo_alloc(&remote->remote_fifo,
-			5 * sizeof(struct wacom_remote_data),
+			5 * sizeof(struct wacom_remote_work_data),
 			GFP_KERNEL);
 	if (error) {
 		hid_err(wacom->hdev, "failed allocating remote_fifo\n");
@@ -2523,6 +2530,18 @@ static void wacom_wireless_work(struct work_struct *work)
 	return;
 }
 
+static void wacom_remote_destroy_battery(struct wacom *wacom, int index)
+{
+	struct wacom_remote *remote = wacom->remote;
+
+	if (remote->remotes[index].battery.battery) {
+		devres_release_group(&wacom->hdev->dev,
+				     &remote->remotes[index].battery.bat_desc);
+		remote->remotes[index].battery.battery = NULL;
+		remote->remotes[index].active_time = 0;
+	}
+}
+
 static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 {
 	struct wacom_remote *remote = wacom->remote;
@@ -2537,9 +2556,7 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 			remote->remotes[i].registered = false;
 			spin_unlock_irqrestore(&remote->remote_lock, flags);
 
-			if (remote->remotes[i].battery.battery)
-				devres_release_group(&wacom->hdev->dev,
-						     &remote->remotes[i].battery.bat_desc);
+			wacom_remote_destroy_battery(wacom, i);
 
 			if (remote->remotes[i].group.name)
 				devres_release_group(&wacom->hdev->dev,
@@ -2547,7 +2564,6 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 
 			remote->remotes[i].serial = 0;
 			remote->remotes[i].group.name = NULL;
-			remote->remotes[i].battery.battery = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -2632,6 +2648,9 @@ static int wacom_remote_attach_battery(struct wacom *wacom, int index)
 	if (remote->remotes[index].battery.battery)
 		return 0;
 
+	if (!remote->remotes[index].active_time)
+		return 0;
+
 	if (wacom->led.groups[index].select == WACOM_STATUS_UNKNOWN)
 		return 0;
 
@@ -2647,17 +2666,19 @@ static void wacom_remote_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, remote_work);
 	struct wacom_remote *remote = wacom->remote;
-	struct wacom_remote_data data;
+	ktime_t kt = ktime_get();
+	struct wacom_remote_work_data remote_work_data;
 	unsigned long flags;
 	unsigned int count;
-	u32 serial;
+	u32 work_serial;
 	int i;
 
 	spin_lock_irqsave(&remote->remote_lock, flags);
 
-	count = kfifo_out(&remote->remote_fifo, &data, sizeof(data));
+	count = kfifo_out(&remote->remote_fifo, &remote_work_data,
+			  sizeof(remote_work_data));
 
-	if (count != sizeof(data)) {
+	if (count != sizeof(remote_work_data)) {
 		hid_err(wacom->hdev,
 			"workitem triggered without status available\n");
 		spin_unlock_irqrestore(&remote->remote_lock, flags);
@@ -2670,10 +2691,14 @@ static void wacom_remote_work(struct work_struct *work)
 	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		serial = data.remote[i].serial;
-		if (data.remote[i].connected) {
+		work_serial = remote_work_data.remote[i].serial;
+		if (work_serial) {
+
+			if (kt - remote->remotes[i].active_time > WACOM_REMOTE_BATTERY_TIMEOUT
+			    && remote->remotes[i].active_time != 0)
+				wacom_remote_destroy_battery(wacom, i);
 
-			if (remote->remotes[i].serial == serial) {
+			if (remote->remotes[i].serial == work_serial) {
 				wacom_remote_attach_battery(wacom, i);
 				continue;
 			}
@@ -2681,7 +2706,7 @@ static void wacom_remote_work(struct work_struct *work)
 			if (remote->remotes[i].serial)
 				wacom_remote_destroy_one(wacom, i);
 
-			wacom_remote_create_one(wacom, serial, i);
+			wacom_remote_create_one(wacom, work_serial, i);
 
 		} else if (remote->remotes[i].serial) {
 			wacom_remote_destroy_one(wacom, i);
@@ -2776,6 +2801,7 @@ static int wacom_probe(struct hid_device *hdev,
 
 	mutex_init(&wacom->lock);
 	INIT_DELAYED_WORK(&wacom->init_work, wacom_init_work);
+	INIT_DELAYED_WORK(&wacom->aes_battery_work, wacom_aes_battery_handler);
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 	INIT_WORK(&wacom->remote_work, wacom_remote_work);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 174bf03908d7..c205198ded11 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1134,6 +1134,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	if (index < 0 || !remote->remotes[index].registered)
 		goto out;
 
+	remote->remotes[i].active_time = ktime_get();
 	input = remote->remotes[index].input;
 
 	input_report_key(input, BTN_0, (data[9] & 0x01));
@@ -1196,22 +1197,20 @@ static void wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	unsigned char *data = wacom_wac->data;
 	struct wacom_remote *remote = wacom->remote;
-	struct wacom_remote_data remote_data;
+	struct wacom_remote_work_data remote_data;
 	unsigned long flags;
 	int i, ret;
 
 	if (data[0] != WACOM_REPORT_DEVICE_LIST)
 		return;
 
-	memset(&remote_data, 0, sizeof(struct wacom_remote_data));
+	memset(&remote_data, 0, sizeof(struct wacom_remote_work_data));
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		int j = i * 6;
 		int serial = (data[j+6] << 16) + (data[j+5] << 8) + data[j+4];
-		bool connected = data[j+2];
 
 		remote_data.remote[i].serial = serial;
-		remote_data.remote[i].connected = connected;
 	}
 
 	spin_lock_irqsave(&remote->remote_lock, flags);
@@ -2529,11 +2528,12 @@ static void wacom_wac_pen_report(struct hid_device *hdev,
 	struct input_dev *input = wacom_wac->pen_input;
 	bool range = wacom_wac->hid_data.inrange_state;
 	bool sense = wacom_wac->hid_data.sense_state;
+	bool entering_range = !wacom_wac->tool[0] && range;
 
 	if (wacom_wac->is_invalid_bt_frame)
 		return;
 
-	if (!wacom_wac->tool[0] && range) { /* first in range */
+	if (entering_range) { /* first in range */
 		/* Going into range select tool */
 		if (wacom_wac->hid_data.invert_state)
 			wacom_wac->tool[0] = BTN_TOOL_RUBBER;
@@ -2584,6 +2584,15 @@ static void wacom_wac_pen_report(struct hid_device *hdev,
 		input_sync(input);
 	}
 
+	/* Handle AES battery timeout behavior */
+	if (wacom_wac->features.quirks & WACOM_QUIRK_AESPEN) {
+		if (entering_range)
+			cancel_delayed_work(&wacom->aes_battery_work);
+		if (!sense)
+			schedule_delayed_work(&wacom->aes_battery_work,
+					      msecs_to_jiffies(WACOM_AES_BATTERY_TIMEOUT));
+	}
+
 	if (!sense) {
 		wacom_wac->tool[0] = 0;
 		wacom_wac->id[0] = 0;
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index ee21bb260f22..e63b1e806e34 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -13,6 +13,8 @@
 #define WACOM_NAME_MAX		64
 #define WACOM_MAX_REMOTES	5
 #define WACOM_STATUS_UNKNOWN	255
+#define WACOM_REMOTE_BATTERY_TIMEOUT	21000000000ll
+#define WACOM_AES_BATTERY_TIMEOUT       1800000
 
 /* packet length for individual models */
 #define WACOM_PKGLEN_BBFUN	 9
@@ -327,10 +329,9 @@ struct hid_data {
 	ktime_t time_delayed;
 };
 
-struct wacom_remote_data {
+struct wacom_remote_work_data {
 	struct {
 		u32 serial;
-		bool connected;
 	} remote[WACOM_MAX_REMOTES];
 };
 
-- 
2.34.1