[PATCH v3 1/3] HID: multitouch: Fix alphabetic sorting of mt_devices table.

Fix alphabetic sorting of mt_devices hid_device_id table.

Signed-off-by: Hans de Goede <redacted>
Reviewed-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 9e8c4d2ba11d..bb939f6990f1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -1605,14 +1605,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
 			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 
-	/* Panasonic panels */
-	{ .driver_data = MT_CLS_PANASONIC,
-		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
-			USB_DEVICE_ID_PANABOARD_UBT780) },
-	{ .driver_data = MT_CLS_PANASONIC,
-		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
-			USB_DEVICE_ID_PANABOARD_UBT880) },
-
 	/* Novatek Panel */
 	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,

@@ -1623,6 +1615,14 @@ static const struct hid_device_id mt_devices[] = {
 		HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
 			USB_VENDOR_ID_NTRIG, 0x1b05) },
 
+	/* Panasonic panels */
+	{ .driver_data = MT_CLS_PANASONIC,
+		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
+			USB_DEVICE_ID_PANABOARD_UBT780) },
+	{ .driver_data = MT_CLS_PANASONIC,
+		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
+			USB_DEVICE_ID_PANABOARD_UBT880) },
+
 	/* PixArt optical touch screen */
 	{ .driver_data = MT_CLS_INRANGE_CONTACTNUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_PIXART,

-- 
2.14.3

The Windows Precision Touchpad spec "Figure 4 Button Only Down and Up"
and "Table 9 Report Sequence for Button Only Down and Up" indicate
that the first packet of a (possibly hybrid mode multi-packet) frame
may contain a contact-count of 0 if only a button is pressed and no
fingers are detected.

This means that a value of 0 for contact-count is a valid value and
should be used as expected contact count when it is the first packet
(num_received == 0), as extra check to make sure that this is the first
packet of a buttons only frame, we also check that the timestamp is
different.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-New patch in v2 of this patch-set

Changes in v3:
-Also check timestamps to make sure we're dealing with the first-packet
 of a frame
---
 drivers/hid/hid-multitouch.c | 34 ++++++++++++++++++++++++++++++++--
 include/linux/hid.h          |  1 +
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index bb939f6990f1..5d3904d0e89d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -117,6 +117,9 @@ struct mt_device {
 	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_*) */
 	int cc_index;	/* contact count field index in the report */
 	int cc_value_index;	/* contact count value index in the field */
+	int scantime_index;	/* scantime field index in the report */
+	int scantime_val_index;	/* scantime value index in the field */
+	int prev_scantime;	/* scantime reported in the previous packet */
 	unsigned last_slot_field;	/* the last field of a slot */
 	unsigned mt_report_id;	/* the report ID of the multitouch device */
 	unsigned long initial_quirks;	/* initial quirks state */

@@ -595,6 +598,14 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* we don't set td->last_slot_field as contactcount and
 			 * contact max are global to the report */
 			return -1;
+		case HID_DG_SCANTIME:
+			/* Ignore if indexes are out of bounds. */
+			if (field->index >= field->report->maxfield ||
+			    usage->usage_index >= field->report_count)
+				return 1;
+			td->scantime_index = field->index;
+			td->scantime_val_index = usage->usage_index;
+			return 1;
 		case HID_DG_TOUCH:
 			/* Legacy devices use TIPSWITCH and not TOUCH.
 			 * Let's just ignore this field. */

@@ -812,9 +823,10 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
 {
 	struct mt_device *td = hid_get_drvdata(hid);
+	__s32 cls = td->mtclass.name;
 	struct hid_field *field;
 	unsigned count;
-	int r, n;
+	int r, n, scantime = 0;
 
 	/* sticky fingers release in progress, abort */
 	if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))

@@ -824,12 +836,29 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
 	 * Includes multi-packet support where subsequent
 	 * packets are sent with zero contactcount.
 	 */
+	if (td->scantime_index >= 0) {
+		field = report->field[td->scantime_index];
+		scantime = field->value[td->scantime_val_index];
+	}
 	if (td->cc_index >= 0) {
 		struct hid_field *field = report->field[td->cc_index];
 		int value = field->value[td->cc_value_index];
-		if (value)
+
+		/*
+		 * For Win8 PTPs the first packet (td->num_received == 0) may
+		 * have a contactcount of 0 if there only is a button event.
+		 * We double check that this is not a continuation packet
+		 * of a possible multi-packet frame be checking that the
+		 * timestamp has changed.
+		 */
+		if ((cls == MT_CLS_WIN_8 || cls == MT_CLS_WIN_8_DUAL) &&
+		    td->num_received == 0 && td->prev_scantime != scantime)
+			td->num_expected = value;
+		/* A non 0 contact count always indicates a first packet */
+		else if (value)
 			td->num_expected = value;
 	}
+	td->prev_scantime = scantime;
 
 	for (r = 0; r < report->maxfield; r++) {
 		field = report->field[r];

@@ -1285,6 +1314,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->maxcontact_report_id = -1;
 	td->inputmode_value = MT_INPUTMODE_TOUCHSCREEN;
 	td->cc_index = -1;
+	td->scantime_index = -1;
 	td->mt_report_id = -1;
 	hid_set_drvdata(hdev, td);
 

diff --git a/include/linux/hid.h b/include/linux/hid.h
index ab05a86269dc..47dd962d9a7a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -289,6 +289,7 @@ struct hid_item {
 #define HID_DG_DEVICEINDEX	0x000d0053
 #define HID_DG_CONTACTCOUNT	0x000d0054
 #define HID_DG_CONTACTMAX	0x000d0055
+#define HID_DG_SCANTIME		0x000d0056
 #define HID_DG_BUTTONTYPE	0x000d0059
 #define HID_DG_BARRELSWITCH2	0x000d005a
 #define HID_DG_TOOLSERIALNUMBER	0x000d005b

-- 
2.14.3

This commit fixes 2 different issues with buttons on touchpads in one go:

1) Devices in "single finger hybrid mode" will send one report per finger,
   on some devices only the first report of such a multi-packet frame will
   contain a value for BTN_LEFT, in subsequent reports (if multiple fingers
   are down) the value is always 0, causing hid-mt to report BTN_LEFT going
   1 - 0 - 1 - 0 when pressing a clickpad and putting down a second finger.
   This happens for example on USB 0603:0002 mt touchpads.

2) According to the Win8 Precision Touchpad spec, inside the HID_UP_BUTTON
   usage-page usage 1 is for a clickpad getting clicked, 2 for an external
   left button and 3 for an external right button. Since Linux uses
   BTN_LEFT for a clickpad being clicked we end up mapping both usage 1
   and 2 to BTN_LEFT and if a single report contains both then we ended
   up always reporting the value of both in a single SYN, e.g. :
   BTN_LEFT 1, BTN_LEFT 0, SYN. This happens for example with Hantick
   HTT5288 i2c mt touchpads.

This commit fixes both issues by not immediately reporting buttons when
we parse the report, but instead storing the values of button fields and
reporting the result from mt_sync_frame() when we've a complete frame.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Rewrite of v1 of "HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek
 mt touchpad" to kill two birds with one stone

Changes in v3:
-Delay reporting for all buttons not just for BTN_LEFT
---
 drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5d3904d0e89d..bfbfa04d023a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -83,6 +83,8 @@ MODULE_LICENSE("GPL");
 #define MT_IO_FLAGS_ACTIVE_SLOTS	1
 #define MT_IO_FLAGS_PENDING_SLOTS	2
 
+#define BTN_COUNT			(BTN_JOYSTICK - BTN_MOUSE)
+
 struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
 	__s32 contactid;	/* the device ContactID assigned to this slot */

@@ -120,6 +122,7 @@ struct mt_device {
 	int scantime_index;	/* scantime field index in the report */
 	int scantime_val_index;	/* scantime value index in the field */
 	int prev_scantime;	/* scantime reported in the previous packet */
+	int buttons_state;	/* buttons state */
 	unsigned last_slot_field;	/* the last field of a slot */
 	unsigned mt_report_id;	/* the report ID of the multitouch device */
 	unsigned long initial_quirks;	/* initial quirks state */

@@ -728,9 +731,17 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
  */
 static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 {
+	int i;
+
+	/* For non Win8 devices buttons_state = 0, so this is a no-op */
+	for (i = 0; i < BTN_COUNT; i++)
+		input_event(input, EV_KEY, BTN_MOUSE + i,
+			    (td->buttons_state >> i) & 1);
+
 	input_mt_sync_frame(input);
 	input_sync(input);
 	td->num_received = 0;
+	td->buttons_state = 0;
 	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
 		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
 	else

@@ -752,6 +763,7 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
 	struct mt_device *td = hid_get_drvdata(hid);
+	__s32 cls = td->mtclass.name;
 	__s32 quirks = td->mtclass.quirks;
 	struct input_dev *input = field->hidinput->input;
 

@@ -805,6 +817,19 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 			break;
 
 		default:
+			/*
+			 * For Win8 PTP touchpads the button state should be
+			 * reported once we've a complete frame, so we store
+			 * the state here and report it in mt_sync_frame().
+			 */
+			if ((cls == MT_CLS_WIN_8 || cls == MT_CLS_WIN_8_DUAL) &&
+			    usage->type == EV_KEY && usage->code >= BTN_MOUSE &&
+			    usage->code < BTN_JOYSTICK) {
+				int btn = usage->code - BTN_MOUSE;
+				td->buttons_state |= (!!value) << btn;
+				return;
+			}
+
 			if (usage->type)
 				input_event(input, usage->type, usage->code,
 						value);

-- 
2.14.3

On Nov 07 2017 or thereabouts, Hans de Goede wrote:

I think there is no point in storing the data and only matching for
BTN_* events here.

How about you simply extend the test below in case the device is
following Win8 protocol and that we have a scantime matching the one
stored in td->prev_scantime?
This way we would only deal with non-multitouch events if they are
reported in the first frame, and ignore all of the others.
I do not believe there are other events than BTN_* that are handled
here, so we should be safe.

Cheers,
Benjamin

 			if (usage->type)
 				input_event(input, usage->type, usage->code,
 						value);
-- 
2.14.3

On Nov 07 2017 or thereabouts, Hans de Goede wrote:

The Windows Precision Touchpad spec "Figure 4 Button Only Down and Up"
and "Table 9 Report Sequence for Button Only Down and Up" indicate
that the first packet of a (possibly hybrid mode multi-packet) frame
may contain a contact-count of 0 if only a button is pressed and no
fingers are detected.

This means that a value of 0 for contact-count is a valid value and
should be used as expected contact count when it is the first packet
(num_received == 0), as extra check to make sure that this is the first
packet of a buttons only frame, we also check that the timestamp is
different.

Signed-off-by: Hans de Goede <redacted>
---

This one is
Reviewed-by: Benjamin Tissoires <redacted>

Cheers,
Benjamin

Hi,

On 13-11-17 09:47, Benjamin Tissoires wrote:

On Nov 07 2017 or thereabouts, Hans de Goede wrote:

quoted

This commit fixes 2 different issues with buttons on touchpads in one go:

1) Devices in "single finger hybrid mode" will send one report per finger,
    on some devices only the first report of such a multi-packet frame will
    contain a value for BTN_LEFT, in subsequent reports (if multiple fingers
    are down) the value is always 0, causing hid-mt to report BTN_LEFT going
    1 - 0 - 1 - 0 when pressing a clickpad and putting down a second finger.
    This happens for example on USB 0603:0002 mt touchpads.

2) According to the Win8 Precision Touchpad spec, inside the HID_UP_BUTTON
    usage-page usage 1 is for a clickpad getting clicked, 2 for an external
    left button and 3 for an external right button. Since Linux uses
    BTN_LEFT for a clickpad being clicked we end up mapping both usage 1
    and 2 to BTN_LEFT and if a single report contains both then we ended
    up always reporting the value of both in a single SYN, e.g. :
    BTN_LEFT 1, BTN_LEFT 0, SYN. This happens for example with Hantick
    HTT5288 i2c mt touchpads.

This commit fixes both issues by not immediately reporting buttons when
we parse the report, but instead storing the values of button fields and
reporting the result from mt_sync_frame() when we've a complete frame.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Rewrite of v1 of "HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek
  mt touchpad" to kill two birds with one stone

Changes in v3:
-Delay reporting for all buttons not just for BTN_LEFT
---
  drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5d3904d0e89d..bfbfa04d023a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -83,6 +83,8 @@ MODULE_LICENSE("GPL");
  #define MT_IO_FLAGS_ACTIVE_SLOTS	1
  #define MT_IO_FLAGS_PENDING_SLOTS	2
  
+#define BTN_COUNT			(BTN_JOYSTICK - BTN_MOUSE)
+
  struct mt_slot {
  	__s32 x, y, cx, cy, p, w, h;
  	__s32 contactid;	/* the device ContactID assigned to this slot */

@@ -120,6 +122,7 @@ struct mt_device {
  	int scantime_index;	/* scantime field index in the report */
  	int scantime_val_index;	/* scantime value index in the field */
  	int prev_scantime;	/* scantime reported in the previous packet */
+	int buttons_state;	/* buttons state */
  	unsigned last_slot_field;	/* the last field of a slot */
  	unsigned mt_report_id;	/* the report ID of the multitouch device */
  	unsigned long initial_quirks;	/* initial quirks state */

@@ -728,9 +731,17 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
   */
  static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
  {
+	int i;
+
+	/* For non Win8 devices buttons_state = 0, so this is a no-op */
+	for (i = 0; i < BTN_COUNT; i++)
+		input_event(input, EV_KEY, BTN_MOUSE + i,
+			    (td->buttons_state >> i) & 1);
+
  	input_mt_sync_frame(input);
  	input_sync(input);
  	td->num_received = 0;
+	td->buttons_state = 0;
  	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
  		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
  	else

@@ -752,6 +763,7 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
  				struct hid_usage *usage, __s32 value)
  {
  	struct mt_device *td = hid_get_drvdata(hid);
+	__s32 cls = td->mtclass.name;
  	__s32 quirks = td->mtclass.quirks;
  	struct input_dev *input = field->hidinput->input;
  

@@ -805,6 +817,19 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
  			break;
  
  		default:
+			/*
+			 * For Win8 PTP touchpads the button state should be
+			 * reported once we've a complete frame, so we store
+			 * the state here and report it in mt_sync_frame().
+			 */
+			if ((cls == MT_CLS_WIN_8 || cls == MT_CLS_WIN_8_DUAL) &&
+			    usage->type == EV_KEY && usage->code >= BTN_MOUSE &&
+			    usage->code < BTN_JOYSTICK) {
+				int btn = usage->code - BTN_MOUSE;
+				td->buttons_state |= (!!value) << btn;
+				return;
+			}
+

I think there is no point in storing the data and only matching for
BTN_* events here.

How about you simply extend the test below in case the device is
following Win8 protocol and that we have a scantime matching the one
stored in td->prev_scantime?

Since we properly reset num_expected based on scantime now and
num_received when num_received >= num_expected I would prefer to
use num_received == 0, in case of buggy firmware which either:
a) reports different scantime-s for different packets of a single frame
b) always reports 0.

Also this will fix the issue where the BTN_LEFT field is always 0
in "extra" packets in a multi-packet frame with with the USB 0603:0002
mt touchpads, but ...

It will do nothing for the Hantick HTT5288 i2c mt touchpads, which
contain both usage 1 and usage 2 buttons in their descriptors, which
both get mapped to BTN_LEFT. It only has a click (usage 1) not an
external left button (usage 2), so the field with usage 2 always reports
a value of 0, resulting in: BTN_LEFT 1, BTN_LEFT 0, SYN being
reported when the pad is clicked, with the BTN_LEFT 1 being the
result of the field with usage 1 and the BTN_LEFT 0 of the field
with usage 2. To fix this we need to either not map usage 2
(check if there already is a BTN_LEFT mapped, skip or quirk?),
or or together all BTN_LEFT values and report the result at the
end of the frame.

Regards,

Hans

This way we would only deal with non-multitouch events if they are
reported in the first frame, and ignore all of the others.
I do not believe there are other events than BTN_* that are handled
here, so we should be safe.

Cheers,
Benjamin

quoted

  			if (usage->type)
  				input_event(input, usage->type, usage->code,
  						value);
-- 
2.14.3

On Nov 13 2017 or thereabouts, Hans de Goede wrote:

Hi,

On 13-11-17 09:47, Benjamin Tissoires wrote:

quoted

On Nov 07 2017 or thereabouts, Hans de Goede wrote:

quoted

This commit fixes 2 different issues with buttons on touchpads in one go:

1) Devices in "single finger hybrid mode" will send one report per finger,
    on some devices only the first report of such a multi-packet frame will
    contain a value for BTN_LEFT, in subsequent reports (if multiple fingers
    are down) the value is always 0, causing hid-mt to report BTN_LEFT going
    1 - 0 - 1 - 0 when pressing a clickpad and putting down a second finger.
    This happens for example on USB 0603:0002 mt touchpads.

2) According to the Win8 Precision Touchpad spec, inside the HID_UP_BUTTON
    usage-page usage 1 is for a clickpad getting clicked, 2 for an external
    left button and 3 for an external right button. Since Linux uses
    BTN_LEFT for a clickpad being clicked we end up mapping both usage 1
    and 2 to BTN_LEFT and if a single report contains both then we ended
    up always reporting the value of both in a single SYN, e.g. :
    BTN_LEFT 1, BTN_LEFT 0, SYN. This happens for example with Hantick
    HTT5288 i2c mt touchpads.

This commit fixes both issues by not immediately reporting buttons when
we parse the report, but instead storing the values of button fields and
reporting the result from mt_sync_frame() when we've a complete frame.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Rewrite of v1 of "HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek
  mt touchpad" to kill two birds with one stone

Changes in v3:
-Delay reporting for all buttons not just for BTN_LEFT
---
  drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5d3904d0e89d..bfbfa04d023a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -83,6 +83,8 @@ MODULE_LICENSE("GPL");
  #define MT_IO_FLAGS_ACTIVE_SLOTS	1
  #define MT_IO_FLAGS_PENDING_SLOTS	2
+#define BTN_COUNT			(BTN_JOYSTICK - BTN_MOUSE)
+
  struct mt_slot {
  	__s32 x, y, cx, cy, p, w, h;
  	__s32 contactid;	/* the device ContactID assigned to this slot */

@@ -120,6 +122,7 @@ struct mt_device {
  	int scantime_index;	/* scantime field index in the report */
  	int scantime_val_index;	/* scantime value index in the field */
  	int prev_scantime;	/* scantime reported in the previous packet */
+	int buttons_state;	/* buttons state */
  	unsigned last_slot_field;	/* the last field of a slot */
  	unsigned mt_report_id;	/* the report ID of the multitouch device */
  	unsigned long initial_quirks;	/* initial quirks state */

@@ -728,9 +731,17 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
   */
  static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
  {
+	int i;
+
+	/* For non Win8 devices buttons_state = 0, so this is a no-op */
+	for (i = 0; i < BTN_COUNT; i++)
+		input_event(input, EV_KEY, BTN_MOUSE + i,
+			    (td->buttons_state >> i) & 1);
+
  	input_mt_sync_frame(input);
  	input_sync(input);
  	td->num_received = 0;
+	td->buttons_state = 0;
  	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
  		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
  	else

@@ -752,6 +763,7 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
  				struct hid_usage *usage, __s32 value)
  {
  	struct mt_device *td = hid_get_drvdata(hid);
+	__s32 cls = td->mtclass.name;
  	__s32 quirks = td->mtclass.quirks;
  	struct input_dev *input = field->hidinput->input;

@@ -805,6 +817,19 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
  			break;
  		default:
+			/*
+			 * For Win8 PTP touchpads the button state should be
+			 * reported once we've a complete frame, so we store
+			 * the state here and report it in mt_sync_frame().
+			 */
+			if ((cls == MT_CLS_WIN_8 || cls == MT_CLS_WIN_8_DUAL) &&
+			    usage->type == EV_KEY && usage->code >= BTN_MOUSE &&
+			    usage->code < BTN_JOYSTICK) {
+				int btn = usage->code - BTN_MOUSE;
+				td->buttons_state |= (!!value) << btn;
+				return;
+			}
+

I think there is no point in storing the data and only matching for
BTN_* events here.

How about you simply extend the test below in case the device is
following Win8 protocol and that we have a scantime matching the one
stored in td->prev_scantime?

Since we properly reset num_expected based on scantime now and
num_received when num_received >= num_expected I would prefer to
use num_received == 0, in case of buggy firmware which either:
a) reports different scantime-s for different packets of a single frame
b) always reports 0.

We should not consider such buggy firmwares here. For a device to have
the Win8 certification, the hardware maker has to send a sample to
Microsoft and they will check that it is correct. There is a signature
that is integrated in the firmware that Windows checks. So if you have
such a buggy firmware, either MS is not doing proper quality check,
either the device was never sent to Microsoft and we should quirk it.

We are fixing the generic case here.

Anyway, I can deal with the num_received == 0 if you had an inlined
function named mt_touch_is_first_frame() (or something similar) so it is
less brain needed when re-reading the code.

Also this will fix the issue where the BTN_LEFT field is always 0
in "extra" packets in a multi-packet frame with with the USB 0603:0002
mt touchpads, but ...

It will do nothing for the Hantick HTT5288 i2c mt touchpads, which
contain both usage 1 and usage 2 buttons in their descriptors, which
both get mapped to BTN_LEFT. It only has a click (usage 1) not an
external left button (usage 2), so the field with usage 2 always reports
a value of 0, resulting in: BTN_LEFT 1, BTN_LEFT 0, SYN being
reported when the pad is clicked, with the BTN_LEFT 1 being the
result of the field with usage 1 and the BTN_LEFT 0 of the field
with usage 2. To fix this we need to either not map usage 2
(check if there already is a BTN_LEFT mapped, skip or quirk?),
or or together all BTN_LEFT values and report the result at the
end of the frame.

Right. I forgot about that (this was precisely why I did not answered
right away I think). We probably need a specific temporary storage for
BTN_0 then (in addition to the check on the first report).

I think I really will have to refactor the code for Windows 8 devices
and append it into hid-input.c This way hid-multitouch will stay for
quirked devices and hid-input will handle the correct devices, with what
we learned for the past 6 years...

Cheers,
Benjamin

Regards,

Hans

quoted

This way we would only deal with non-multitouch events if they are
reported in the first frame, and ignore all of the others.
I do not believe there are other events than BTN_* that are handled
here, so we should be safe.

Cheers,
Benjamin

quoted

  			if (usage->type)
  				input_event(input, usage->type, usage->code,
  						value);
-- 
2.14.3