[PATCH 1/6] HID: wacom: Re-enable touch by default for Cintiq 24HDT / 27QHDT

[PATCH 1/6] HID: wacom: Re-enable touch by default for Cintiq 24HDT / 27QHDT

[Jason Gerecke]

Commit 670e90924bfe ("HID: wacom: support named keys on older devices")
added support for sending named events from the soft buttons on the
24HDT and 27QHDT. In the process, however, it inadvertantly disabled the
touchscreen of the 24HDT and 27QHDT by default. The
`wacom_set_shared_values` function would normally enable touch by default
but because it checks the state of the non-shared `has_mute_touch_switch`
flag and `wacom_setup_touch_input_capabilities` sets the state of the
/shared/ version, touch ends up being disabled by default.

This patch sets the non-shared flag, letting `wacom_set_shared_values`
take care of copying the value over to the shared version and setting
the default touch state to "on".

Fixes: 670e90924bfe ("HID: wacom: support named keys on older devices")
CC: stable@vger.kernel.org # 5.4+
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
---
 drivers/hid/wacom_wac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 81d7d12bcf34..496a000ef862 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3831,7 +3831,7 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 		    wacom_wac->shared->touch->product == 0xF6) {
 			input_dev->evbit[0] |= BIT_MASK(EV_SW);
 			__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
-			wacom_wac->shared->has_mute_touch_switch = true;
+			wacom_wac->has_mute_touch_switch = true;
 		}
 		fallthrough;
 
-- 
2.32.0

[PATCH 2/6] HID: wacom: set initial hardware touch switch state to 'off'

[Jason Gerecke]

From: Ping Cheng <ping.cheng@wacom.com>

Wacom touch devices have two types of touch switches: softkey touch
toggle and hardware touch switch. For softkey toggle, we assume
touch is on by default in the driver. However the hardware touch
switch is controlled by end users. We don't know if it's on or off
before getting the status event.

This patch sets touch off for devices with a hardware switch until we
get the status. This is a bit safer for users who leave the switch "off"
and don't want any accidental touches. The tradeoff is a slight delay
between device connection and touch becoming enabled for users who
leave the switch "on".

Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>
Tested-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c |  8 +++++++-
 drivers/hid/wacom_wac.c | 12 ++++++++----
 drivers/hid/wacom_wac.h |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 57bfa0ae9836..713a2504092f 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2287,7 +2287,13 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
 
 	if (wacom_wac->has_mute_touch_switch) {
 		wacom_wac->shared->has_mute_touch_switch = true;
-		wacom_wac->shared->is_touch_on = true;
+		/* Hardware touch switch may be off. Wait until
+		 * we know the switch state to decide is_touch_on.
+		 * Softkey state should be initialized to "on" to
+		 * match historic default.
+		 */
+		if (wacom_wac->is_soft_touch_switch)
+			wacom_wac->shared->is_touch_on = true;
 	}
 
 	if (wacom_wac->shared->has_mute_touch_switch &&
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 496a000ef862..b87fbd2a81a6 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1987,14 +1987,17 @@ static void wacom_wac_pad_usage_mapping(struct hid_device *hdev,
 		features->numbered_buttons++;
 		features->device_type |= WACOM_DEVICETYPE_PAD;
 		break;
-	case WACOM_HID_WD_TOUCHONOFF:
 	case WACOM_HID_WD_MUTE_DEVICE:
+		/* softkey touch switch */
+		wacom_wac->is_soft_touch_switch = true;
+		fallthrough;
+	case WACOM_HID_WD_TOUCHONOFF:
 		/*
-		 * This usage, which is used to mute touch events, comes
-		 * from the pad packet, but is reported on the touch
+		 * These two usages, which are used to mute touch events, come
+		 * from the pad packet, but are reported on the touch
 		 * interface. Because the touch interface may not have
 		 * been created yet, we cannot call wacom_map_usage(). In
-		 * order to process this usage when we receive it, we set
+		 * order to process the usages when we receive them, we set
 		 * the usage type and code directly.
 		 */
 		wacom_wac->has_mute_touch_switch = true;
@@ -3832,6 +3835,7 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 			input_dev->evbit[0] |= BIT_MASK(EV_SW);
 			__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
 			wacom_wac->has_mute_touch_switch = true;
+			wacom_wac->is_soft_touch_switch = true;
 		}
 		fallthrough;
 
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 8f16654eca09..4e9eb0c1eff6 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -352,6 +352,7 @@ struct wacom_wac {
 	int mode_value;
 	struct hid_data hid_data;
 	bool has_mute_touch_switch;
+	bool is_soft_touch_switch;
 	bool has_mode_change;
 	bool is_direct_mode;
 	bool is_invalid_bt_frame;
-- 
2.32.0

[PATCH 3/6] HID: wacom: Short-circuit processing of touch when it is disabled

[Jason Gerecke]

Avoid doing unnecessary work when touch is disabled by detecting this
condition and returning early. Note that the probe process sends GET
FEATURE requests to discover e.g. HID_DG_CONTACTMAX, so we can't start
ignoring touch reports until probe finishes.

Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c |  1 +
 drivers/hid/wacom_wac.c | 12 ++++++++++++
 drivers/hid/wacom_wac.h |  1 +
 3 files changed, 14 insertions(+)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 713a2504092f..93f49b766376 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2797,6 +2797,7 @@ static int wacom_probe(struct hid_device *hdev,
 				 error);
 	}
 
+	wacom_wac->probe_complete = true;
 	return 0;
 }
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index b87fbd2a81a6..27587070a192 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2581,6 +2581,12 @@ static void wacom_wac_finger_event(struct hid_device *hdev,
 	unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
 	struct wacom_features *features = &wacom->wacom_wac.features;
 
+	/* don't process touch events when touch is off */
+	if (wacom_wac->probe_complete &&
+	    !wacom_wac->shared->is_touch_on &&
+	    !wacom_wac->shared->touch_down)
+		return;
+
 	if (wacom_wac->is_invalid_bt_frame)
 		return;
 
@@ -2630,6 +2636,12 @@ static void wacom_wac_finger_pre_report(struct hid_device *hdev,
 	struct hid_data* hid_data = &wacom_wac->hid_data;
 	int i;
 
+	/* don't process touch events when touch is off */
+	if (wacom_wac->probe_complete &&
+	    !wacom_wac->shared->is_touch_on &&
+	    !wacom_wac->shared->touch_down)
+		return;
+
 	wacom_wac->is_invalid_bt_frame = false;
 
 	for (i = 0; i < report->maxfield; i++) {
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 4e9eb0c1eff6..8b2d4e5b2303 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -337,6 +337,7 @@ struct wacom_wac {
 	int tool[2];
 	int id[2];
 	__u64 serial[2];
+	bool probe_complete;
 	bool reporting_data;
 	struct wacom_features features;
 	struct wacom_shared *shared;
-- 
2.32.0

[PATCH 4/6] HID: wacom: Skip processing of touches with negative slot values

[Jason Gerecke]

The `input_mt_get_slot_by_key` function may return a negative value
if an error occurs (e.g. running out of slots). If this occurs we
should really avoid reporting any data for the slot.

Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 27587070a192..ce9e8e9b48b6 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2551,6 +2551,9 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
 		int slot;
 
 		slot = input_mt_get_slot_by_key(input, hid_data->id);
+		if (slot < 0)
+			return;
+
 		input_mt_slot(input, slot);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
 	}
-- 
2.32.0

[PATCH 5/6] HID: wacom: Avoid sending empty sync events

[Jason Gerecke]

Empty sync events clutter up logs and are a waste of CPU cycles. We can
avoid sending mt_sync events if touch is disabled or a specific slot is
unused. We can avoid sending full sync events if no events were generated.

Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index ce9e8e9b48b6..3f992c9dca4d 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2551,8 +2551,17 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
 		int slot;
 
 		slot = input_mt_get_slot_by_key(input, hid_data->id);
-		if (slot < 0)
+		if (slot < 0) {
 			return;
+		} else {
+			struct input_mt_slot *ps = &input->mt->slots[slot];
+			int mt_id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
+
+			if (!prox && mt_id < 0) {
+				// No data to send for this slot; short-circuit
+				return;
+			}
+		}
 
 		input_mt_slot(input, slot);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
@@ -2696,6 +2705,10 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
 	struct input_dev *input = wacom_wac->touch_input;
 	unsigned touch_max = wacom_wac->features.touch_max;
 
+	/* if there was nothing to process, don't send an empty sync */
+	if (wacom_wac->hid_data.num_expected == 0)
+		return;
+
 	/* If more packets of data are expected, give us a chance to
 	 * process them rather than immediately syncing a partial
 	 * update.
-- 
2.32.0

[PATCH 6/6] HID: wacom: Refactor touch input mute checks into a common function

[Jason Gerecke]

We perform this same set of tests to see if touch input is muted in
several places. We might as well replace these independent copies with
an inline function.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 3f992c9dca4d..fd51769d0994 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -824,6 +824,13 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
 	return 0;
 }
 
+static inline bool touch_is_muted(struct wacom_wac *wacom_wac)
+{
+	return wacom_wac->probe_complete &&
+	       wacom_wac->shared->has_mute_touch_switch &&
+	       !wacom_wac->shared->is_touch_on;
+}
+
 static inline bool report_touch_events(struct wacom_wac *wacom)
 {
 	return (touch_arbitration ? !wacom->shared->stylus_in_proximity : 1);
@@ -1525,11 +1532,8 @@ static int wacom_24hdt_irq(struct wacom_wac *wacom)
 	int byte_per_packet = WACOM_BYTES_PER_24HDT_PACKET;
 	int y_offset = 2;
 
-	if (wacom->shared->has_mute_touch_switch &&
-	    !wacom->shared->is_touch_on) {
-		if (!wacom->shared->touch_down)
-			return 0;
-	}
+	if (touch_is_muted(wacom) && !wacom->shared->touch_down)
+		return 0;
 
 	if (wacom->features.type == WACOM_27QHDT) {
 		current_num_contacts = data[63];
@@ -2536,8 +2540,7 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
 	bool prox = hid_data->tipswitch &&
 		    report_touch_events(wacom_wac);
 
-	if (wacom_wac->shared->has_mute_touch_switch &&
-	    !wacom_wac->shared->is_touch_on) {
+	if (touch_is_muted(wacom_wac)) {
 		if (!wacom_wac->shared->touch_down)
 			return;
 		prox = false;
@@ -2593,10 +2596,7 @@ static void wacom_wac_finger_event(struct hid_device *hdev,
 	unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
 	struct wacom_features *features = &wacom->wacom_wac.features;
 
-	/* don't process touch events when touch is off */
-	if (wacom_wac->probe_complete &&
-	    !wacom_wac->shared->is_touch_on &&
-	    !wacom_wac->shared->touch_down)
+	if (touch_is_muted(wacom_wac) && !wacom_wac->shared->touch_down)
 		return;
 
 	if (wacom_wac->is_invalid_bt_frame)
@@ -2648,10 +2648,7 @@ static void wacom_wac_finger_pre_report(struct hid_device *hdev,
 	struct hid_data* hid_data = &wacom_wac->hid_data;
 	int i;
 
-	/* don't process touch events when touch is off */
-	if (wacom_wac->probe_complete &&
-	    !wacom_wac->shared->is_touch_on &&
-	    !wacom_wac->shared->touch_down)
+	if (touch_is_muted(wacom_wac) && !wacom_wac->shared->touch_down)
 		return;
 
 	wacom_wac->is_invalid_bt_frame = false;
-- 
2.32.0

Re: [PATCH 1/6] HID: wacom: Re-enable touch by default for Cintiq 24HDT / 27QHDT

[Jiri Kosina]

On Mon, 19 Jul 2021, Jason Gerecke wrote:

> Commit 670e90924bfe ("HID: wacom: support named keys on older devices")
> added support for sending named events from the soft buttons on the
> 24HDT and 27QHDT. In the process, however, it inadvertantly disabled the
> touchscreen of the 24HDT and 27QHDT by default. The
> `wacom_set_shared_values` function would normally enable touch by default
> but because it checks the state of the non-shared `has_mute_touch_switch`
> flag and `wacom_setup_touch_input_capabilities` sets the state of the
> /shared/ version, touch ends up being disabled by default.
> 
> This patch sets the non-shared flag, letting `wacom_set_shared_values`
> take care of copying the value over to the shared version and setting
> the default touch state to "on".
> 
> Fixes: 670e90924bfe ("HID: wacom: support named keys on older devices")
> CC: stable@vger.kernel.org # 5.4+
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> ---
>  drivers/hid/wacom_wac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 81d7d12bcf34..496a000ef862 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3831,7 +3831,7 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  		    wacom_wac->shared->touch->product == 0xF6) {
>  			input_dev->evbit[0] |= BIT_MASK(EV_SW);
>  			__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> -			wacom_wac->shared->has_mute_touch_switch = true;
> +			wacom_wac->has_mute_touch_switch = true;
>  		}
>  		fallthrough;
>  

This patch series looks strangely like not really a series at all :) I am 
applying 1/6 and 4/6 for 5.14 and queuing the rest for 5.15. Please shout 
if you disagree with that. Thanks,

-- 
Jiri Kosina
SUSE Labs