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

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

[Hans de Goede]

Fix alphabetic sorting of mt_devices hid_device_id table.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 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

[PATCH v3 2/3] HID: multitouch: Properly deal with Win8 PTP reports with 0 touches

[Hans de Goede]

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 <hdegoede@redhat.com>
---
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

[PATCH v3 3/3] HID: multitouch: Only send button events when we have a complete frame

[Hans de Goede]

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 <hdegoede@redhat.com>
---
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

Re: [PATCH v3 3/3] HID: multitouch: Only send button events when we have a complete frame

[Benjamin Tissoires]

On Nov 07 2017 or thereabouts, Hans de Goede wrote:
> 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 <hdegoede@redhat.com>
> ---
> 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?
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
> 

Re: [PATCH v3 2/3] HID: multitouch: Properly deal with Win8 PTP reports with 0 touches

[Benjamin Tissoires]

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 <hdegoede@redhat.com>
> ---

This one is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 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
> 

Re: [PATCH v3 3/3] HID: multitouch: Only send button events when we have a complete frame

[Hans de Goede]

Hi,

On 13-11-17 09:47, Benjamin Tissoires wrote:
> On Nov 07 2017 or thereabouts, Hans de Goede wrote:
>> 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 <hdegoede@redhat.com>
>> ---
>> 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
> 
>>   			if (usage->type)
>>   				input_event(input, usage->type, usage->code,
>>   						value);
>> -- 
>> 2.14.3
>>

Re: [PATCH v3 3/3] HID: multitouch: Only send button events when we have a complete frame

[Benjamin Tissoires]

On Nov 13 2017 or thereabouts, Hans de Goede wrote:
> Hi,
> 
> On 13-11-17 09:47, Benjamin Tissoires wrote:
> > On Nov 07 2017 or thereabouts, Hans de Goede wrote:
> > > 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 <hdegoede@redhat.com>
> > > ---
> > > 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
> 
> 
> > 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
> > >