[PATCH v3] Input: zinitix - Do not report shadow fingers

[PATCH v3] Input: zinitix - Do not report shadow fingers

[Linus Walleij]

I observed the following problem with the BT404 touch pad
running the Phosh UI:

When e.g. typing on the virtual keyboard pressing "g" would
produce "ggg".

After some analysis it turns out the firmware reports that three
fingers hit that coordinate at the same time, finger 0, 2 and
4 (of the five available 0,1,2,3,4).

DOWN
  Zinitix-TS 3-0020: finger 0 down (246, 395)
  Zinitix-TS 3-0020: finger 1 up (0, 0)
  Zinitix-TS 3-0020: finger 2 down (246, 395)
  Zinitix-TS 3-0020: finger 3 up (0, 0)
  Zinitix-TS 3-0020: finger 4 down (246, 395)
UP
  Zinitix-TS 3-0020: finger 0 up (246, 395)
  Zinitix-TS 3-0020: finger 2 up (246, 395)
  Zinitix-TS 3-0020: finger 4 up (246, 395)

This is one touch and release: i.e. this is all reported on
touch (down) and release.

There is a field in the struct touch_event called finger_cnt
which is actually a bitmask of the fingers active in the
event.

Rename this field finger_mask as this matches the use contents
better, then use for_each_set_bit() to iterate over just the
fingers that are actally active.

Factor out a finger reporting function zinitix_report_fingers()
to handle all fingers.

Also be more careful in reporting finger down/up: we were
reporting every event with input_mt_report_slot_state(..., true);
but this should only be reported on finger down or move,
not on finger up, so also add code to check p->sub_status
to see what is happening and report correctly.

After this my Zinitix BT404 touchscreen report fingers
flawlessly.

The vendor drive I have notably does not use the "finger_cnt"
and contains obviously incorrect code like this:

  if (touch_dev->touch_info.finger_cnt > MAX_SUPPORTED_FINGER_NUM)
      touch_dev->touch_info.finger_cnt = MAX_SUPPORTED_FINGER_NUM;

As MAX_SUPPORTED_FINGER_NUM is an ordinal and the field is
a bitmask this seems quite confused.

Cc: Michael Srba <Michael.Srba@seznam.cz>
Cc: Nikita Travkin <nikita@trvn.ru>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Fix a !() parenthesing issue reported by Dan Carpenter and the
  kernel test robot. Incorporated as a review comment as the patch
  is not yet upstream.
ChangeLog v1->v2:
- Rewrite to use the mask of active fingers after Dmitry's
  observation that there is this field in the struct touch_event
  that is unused (also in the vendor driver).
---
 drivers/input/touchscreen/zinitix.c | 60 ++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index 129ebc810de8..49794a545c64 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -135,7 +135,7 @@ struct point_coord {
 
 struct touch_event {
 	__le16	status;
-	u8	finger_cnt;
+	u8	finger_mask;
 	u8	time_stamp;
 	struct point_coord point_coord[MAX_SUPPORTED_FINGER_NUM];
 };
@@ -319,14 +319,52 @@ static int zinitix_send_power_on_sequence(struct bt541_ts_data *bt541)
 	return 0;
 }
 
-static void zinitix_report_finger(struct bt541_ts_data *bt541, int slot,
-				  const struct point_coord *p)
+static void zinitix_report_fingers(struct bt541_ts_data *bt541, struct touch_event *te)
 {
-	input_mt_slot(bt541->input_dev, slot);
-	input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
-	touchscreen_report_pos(bt541->input_dev, &bt541->prop,
-			       le16_to_cpu(p->x), le16_to_cpu(p->y), true);
-	input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
+	struct point_coord *p;
+	u16 x, y;
+	unsigned long fmask;
+	int i;
+
+	/*
+	 * If the corresponding finger is not active, do not report
+	 * what is happening on it.
+	 */
+	fmask = te->finger_mask;
+	for_each_set_bit(i, &fmask, MAX_SUPPORTED_FINGER_NUM) {
+		p = &te->point_coord[i];
+
+		/* Skip nonexisting fingers */
+		if (!(p->sub_status & SUB_BIT_EXIST))
+			continue;
+
+		x = le16_to_cpu(p->x);
+		y = le16_to_cpu(p->y);
+
+		input_mt_slot(bt541->input_dev, i);
+
+		if (p->sub_status & BIT_DOWN) {
+			/* Finger down */
+			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
+			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
+			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
+			dev_dbg(&bt541->client->dev, "finger %d down (%u, %u)\n", i, x, y);
+		} else if (p->sub_status & BIT_UP) {
+			/* Release finger */
+			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, false);
+			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
+			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, 0);
+			dev_dbg(&bt541->client->dev, "finger %d up (%u, %u)\n", i, x, y);
+		} else if (p->sub_status & BIT_MOVE) {
+			/* Finger moves while pressed down */
+			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
+			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
+			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
+			dev_dbg(&bt541->client->dev, "finger %d move (%u, %u)\n", i, x, y);
+		} else {
+			dev_dbg(&bt541->client->dev, "unknown finger event\n");
+		}
+	}
 }
 
 static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
@@ -335,7 +373,6 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 	struct i2c_client *client = bt541->client;
 	struct touch_event touch_event;
 	int error;
-	int i;
 
 	memset(&touch_event, 0, sizeof(struct touch_event));
 
@@ -346,10 +383,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 		goto out;
 	}
 
-	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++)
-		if (touch_event.point_coord[i].sub_status & SUB_BIT_EXIST)
-			zinitix_report_finger(bt541, i,
-					      &touch_event.point_coord[i]);
+	zinitix_report_fingers(bt541, &touch_event);
 
 	input_mt_sync_frame(bt541->input_dev);
 	input_sync(bt541->input_dev);
-- 
2.34.1

Re: [PATCH v3] Input: zinitix - Do not report shadow fingers

[Dmitry Torokhov]

Hi Linus,

On Tue, Mar 01, 2022 at 12:30:17AM +0100, Linus Walleij wrote:
> +static void zinitix_report_fingers(struct bt541_ts_data *bt541, struct touch_event *te)
>  {
> -	input_mt_slot(bt541->input_dev, slot);
> -	input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
> -	touchscreen_report_pos(bt541->input_dev, &bt541->prop,
> -			       le16_to_cpu(p->x), le16_to_cpu(p->y), true);
> -	input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
> +	struct point_coord *p;
> +	u16 x, y;
> +	unsigned long fmask;
> +	int i;
> +
> +	/*
> +	 * If the corresponding finger is not active, do not report
> +	 * what is happening on it.
> +	 */
> +	fmask = te->finger_mask;
> +	for_each_set_bit(i, &fmask, MAX_SUPPORTED_FINGER_NUM) {
> +		p = &te->point_coord[i];
> +
> +		/* Skip nonexisting fingers */
> +		if (!(p->sub_status & SUB_BIT_EXIST))
> +			continue;
> +
> +		x = le16_to_cpu(p->x);
> +		y = le16_to_cpu(p->y);
> +
> +		input_mt_slot(bt541->input_dev, i);
> +
> +		if (p->sub_status & BIT_DOWN) {
> +			/* Finger down */
> +			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
> +			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
> +			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
> +			dev_dbg(&bt541->client->dev, "finger %d down (%u, %u)\n", i, x, y);
> +		} else if (p->sub_status & BIT_UP) {
> +			/* Release finger */
> +			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, false);
> +			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
> +			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, 0);
> +			dev_dbg(&bt541->client->dev, "finger %d up (%u, %u)\n", i, x, y);

I think reporting releases should be a priority, or at least we should
not be skipping it if for some reason both up and down bits are set.
Also I believe we should be using SUB_BIT_* defines here (even though
they are the same).

> +		} else if (p->sub_status & BIT_MOVE) {
> +			/* Finger moves while pressed down */
> +			input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
> +			touchscreen_report_pos(bt541->input_dev, &bt541->prop, x, y, true);
> +			input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
> +			dev_dbg(&bt541->client->dev, "finger %d move (%u, %u)\n", i, x, y);
> +		} else {
> +			dev_dbg(&bt541->client->dev, "unknown finger event\n");
> +		}
> +	}
>  }
>  
>  static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
> @@ -335,7 +373,6 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  	struct i2c_client *client = bt541->client;
>  	struct touch_event touch_event;
>  	int error;
> -	int i;
>  
>  	memset(&touch_event, 0, sizeof(struct touch_event));
>  
> @@ -346,10 +383,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  		goto out;
>  	}
>  
> -	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++)
> -		if (touch_event.point_coord[i].sub_status & SUB_BIT_EXIST)
> -			zinitix_report_finger(bt541, i,
> -					      &touch_event.point_coord[i]);
> +	zinitix_report_fingers(bt541, &touch_event);

I actually liked that we iterated over individual contacts here. I took
the liberty to rearrange your patch a bit, could you please tell me if
the version below looks OK to you?

Thanks!

-- 
Dmitry

Input: zinitix - do not report shadow fingers

From: Linus Walleij <linus.walleij@linaro.org>

I observed the following problem with the BT404 touch pad
running the Phosh UI:

When e.g. typing on the virtual keyboard pressing "g" would
produce "ggg".

After some analysis it turns out the firmware reports that three
fingers hit that coordinate at the same time, finger 0, 2 and
4 (of the five available 0,1,2,3,4).

DOWN
  Zinitix-TS 3-0020: finger 0 down (246, 395)
  Zinitix-TS 3-0020: finger 1 up (0, 0)
  Zinitix-TS 3-0020: finger 2 down (246, 395)
  Zinitix-TS 3-0020: finger 3 up (0, 0)
  Zinitix-TS 3-0020: finger 4 down (246, 395)
UP
  Zinitix-TS 3-0020: finger 0 up (246, 395)
  Zinitix-TS 3-0020: finger 2 up (246, 395)
  Zinitix-TS 3-0020: finger 4 up (246, 395)

This is one touch and release: i.e. this is all reported on
touch (down) and release.

There is a field in the struct touch_event called finger_cnt
which is actually a bitmask of the fingers active in the
event.

Rename this field finger_mask as this matches the use contents
better, then use for_each_set_bit() to iterate over just the
fingers that are actally active.

Factor out a finger reporting function zinitix_report_fingers()
to handle all fingers.

Also be more careful in reporting finger down/up: we were
reporting every event with input_mt_report_slot_state(..., true);
but this should only be reported on finger down or move,
not on finger up, so also add code to check p->sub_status
to see what is happening and report correctly.

After this my Zinitix BT404 touchscreen report fingers
flawlessly.

The vendor drive I have notably does not use the "finger_cnt"
and contains obviously incorrect code like this:

  if (touch_dev->touch_info.finger_cnt > MAX_SUPPORTED_FINGER_NUM)
      touch_dev->touch_info.finger_cnt = MAX_SUPPORTED_FINGER_NUM;

As MAX_SUPPORTED_FINGER_NUM is an ordinal and the field is
a bitmask this seems quite confused.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/zinitix.c |   44 ++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index 129ebc810de8..8bd03278ad9a 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -135,7 +135,7 @@ struct point_coord {
 
 struct touch_event {
 	__le16	status;
-	u8	finger_cnt;
+	u8	finger_mask;
 	u8	time_stamp;
 	struct point_coord point_coord[MAX_SUPPORTED_FINGER_NUM];
 };
@@ -322,11 +322,32 @@ static int zinitix_send_power_on_sequence(struct bt541_ts_data *bt541)
 static void zinitix_report_finger(struct bt541_ts_data *bt541, int slot,
 				  const struct point_coord *p)
 {
+	u16 x, y;
+
+	if (unlikely(!(p->sub_status &
+		       (SUB_BIT_UP | SUB_BIT_DOWN | SUB_BIT_MOVE)))) {
+		dev_dbg(&bt541->client->dev, "unknown finger event %#02x\n",
+			p->sub_status);
+		return;
+	}
+
+	x = le16_to_cpu(p->x);
+	y = le16_to_cpu(p->y);
+
 	input_mt_slot(bt541->input_dev, slot);
-	input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
-	touchscreen_report_pos(bt541->input_dev, &bt541->prop,
-			       le16_to_cpu(p->x), le16_to_cpu(p->y), true);
-	input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR, p->width);
+	if (input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER,
+				       !(p->sub_status & SUB_BIT_UP))) {
+		touchscreen_report_pos(bt541->input_dev,
+				       &bt541->prop, x, y, true);
+		input_report_abs(bt541->input_dev,
+				 ABS_MT_TOUCH_MAJOR, p->width);
+		dev_dbg(&bt541->client->dev, "finger %d %s (%u, %u)\n",
+			slot, p->sub_status & SUB_BIT_DOWN ? "down" : "move",
+			x, y);
+	} else {
+		dev_dbg(&bt541->client->dev, "finger %d up (%u, %u)\n",
+			slot, x, y);
+	}
 }
 
 static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
@@ -334,6 +355,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 	struct bt541_ts_data *bt541 = bt541_handler;
 	struct i2c_client *client = bt541->client;
 	struct touch_event touch_event;
+	unsigned long finger_mask;
 	int error;
 	int i;
 
@@ -346,10 +368,14 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 		goto out;
 	}
 
-	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++)
-		if (touch_event.point_coord[i].sub_status & SUB_BIT_EXIST)
-			zinitix_report_finger(bt541, i,
-					      &touch_event.point_coord[i]);
+	finger_mask = touch_event.finger_mask;
+	for_each_set_bit(i, &finger_mask, MAX_SUPPORTED_FINGER_NUM) {
+		const struct point_coord *p = &touch_event.point_coord[i];
+
+		/* Only process contacts that are actually reported */
+		if (p->sub_status & SUB_BIT_EXIST)
+			zinitix_report_finger(bt541, i, p);
+	}
 
 	input_mt_sync_frame(bt541->input_dev);
 	input_sync(bt541->input_dev);

Re: [PATCH v3] Input: zinitix - Do not report shadow fingers

[Linus Walleij]

Hi Dmitry,

On Sat, Mar 5, 2022 at 3:15 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> I actually liked that we iterated over individual contacts here. I took
> the liberty to rearrange your patch a bit, could you please tell me if
> the version below looks OK to you?

Looks good and works good, gave it a spin on the hardware to make
sure!
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3] Input: zinitix - Do not report shadow fingers

[Dmitry Torokhov]

On Sun, Mar 06, 2022 at 01:32:57AM +0100, Linus Walleij wrote:
> Hi Dmitry,
> 
> On Sat, Mar 5, 2022 at 3:15 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > I actually liked that we iterated over individual contacts here. I took
> > the liberty to rearrange your patch a bit, could you please tell me if
> > the version below looks OK to you?
> 
> Looks good and works good, gave it a spin on the hardware to make
> sure!
> Tested-by: Linus Walleij <linus.walleij@linaro.org>

Awesome, thank you.

-- 
Dmitry