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

[PATCH v2 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 v2 2/3] HID: multitouch: Properly deal with 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) report
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 num_received == 0.

The only other case where we may get a contact count value of 0
when num_received == 0 is when we're somehow out of sync and in
that case setting num_expected to 0 so that we ignore contacts
until we're back in sync again also is the right thing to do.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-multitouch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index bb939f6990f1..76a3e9e074ba 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -827,7 +827,7 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
 	if (td->cc_index >= 0) {
 		struct hid_field *field = report->field[td->cc_index];
 		int value = field->value[td->cc_value_index];
-		if (value)
+		if (value || td->num_received == 0)
 			td->num_expected = value;
 	}
 
-- 
2.14.3

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

[Benjamin Tissoires]

On Nov 06 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) report
> 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 num_received == 0.

Please add something here that explains that num_received == 0 means
that we are at the beginning of a multi-report frame. I had a hard time
remembering that actually.

> 
> The only other case where we may get a contact count value of 0
> when num_received == 0 is when we're somehow out of sync and in
> that case setting num_expected to 0 so that we ignore contacts
> until we're back in sync again also is the right thing to do.

Are you sure about that? I mean, if you set num_expected to 0, all
incoming report will call mt_sync_frame(), which basically flushes all
the events to the user space.

I am fine accepting a contact count value of 0 if we are at the
beginning of a multi report frame, but I'd be more happy if you take
into account the Scan Time HID field value into account instead of a
computed value.

Cheers,
Benjamin

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/hid-multitouch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index bb939f6990f1..76a3e9e074ba 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -827,7 +827,7 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
>  	if (td->cc_index >= 0) {
>  		struct hid_field *field = report->field[td->cc_index];
>  		int value = field->value[td->cc_value_index];
> -		if (value)
> +		if (value || td->num_received == 0)
>  			td->num_expected = value;
>  	}
>  
> -- 
> 2.14.3
> 

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

[Hans de Goede]

Hi,

On 06-11-17 18:56, Benjamin Tissoires wrote:
> On Nov 06 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) report
>> 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 num_received == 0.
> 
> Please add something here that explains that num_received == 0 means
> that we are at the beginning of a multi-report frame. I had a hard time
> remembering that actually.
> 
>>
>> The only other case where we may get a contact count value of 0
>> when num_received == 0 is when we're somehow out of sync and in
>> that case setting num_expected to 0 so that we ignore contacts
>> until we're back in sync again also is the right thing to do.
> 
> Are you sure about that? I mean, if you set num_expected to 0, all
> incoming report will call mt_sync_frame(), which basically flushes all
> the events to the user space.
> 
> I am fine accepting a contact count value of 0 if we are at the
> beginning of a multi report frame, but I'd be more happy if you take
> into account the Scan Time HID field value into account instead of a
> computed value.

Ok, I've prepared a v3 which only sets num_expected to 0 for Win8 PTP
devices when both num_received == 0 and the timestamp has changed since
the previous report.

Regards,

Hans


> 
> Cheers,
> Benjamin
> 
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/hid/hid-multitouch.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index bb939f6990f1..76a3e9e074ba 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -827,7 +827,7 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
>>   	if (td->cc_index >= 0) {
>>   		struct hid_field *field = report->field[td->cc_index];
>>   		int value = field->value[td->cc_value_index];
>> -		if (value)
>> +		if (value || td->num_received == 0)
>>   			td->num_expected = value;
>>   	}
>>   
>> -- 
>> 2.14.3
>>

[PATCH v2 3/3] HID: multitouch: Fix BTN_LEFT on some touchpads

[Hans de Goede]

This commit fixes 2 different issues with BTN_LEFT 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 SYNA3602
   i2c mt touchpads.

This commit fixes both issues by not immediately reporting BTN_LEFT when
we parse the report, but instead or-ing the values of all fields mapped
to BTN_LEFT together 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
---
 drivers/hid/hid-multitouch.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 76a3e9e074ba..bacbd1ba75ba 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -117,6 +117,7 @@ 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 btn_left;		/* BTN_LEFT 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 */
@@ -717,9 +718,11 @@ 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)
 {
+	input_event(input, EV_KEY, BTN_LEFT, td->btn_left);
 	input_mt_sync_frame(input);
 	input_sync(input);
 	td->num_received = 0;
+	td->btn_left = 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
@@ -794,6 +797,17 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 			break;
 
 		default:
+			/*
+			 * On some hardware we get multiple BTN_LEFT reports
+			 * per frame, with only 1 report reporting 1 if pressed.
+			 * So for BTN_LEFT we or the values together and
+			 * report the combined state in mt_sync_frame()
+			 */
+			if (usage->type == EV_KEY && usage->code == BTN_LEFT) {
+				td->btn_left |= value;
+				return;
+			}
+
 			if (usage->type)
 				input_event(input, usage->type, usage->code,
 						value);
-- 
2.14.3

Re: [PATCH v2 3/3] HID: multitouch: Fix BTN_LEFT on some touchpads

[Benjamin Tissoires]

On Nov 06 2017 or thereabouts, Hans de Goede wrote:
> This commit fixes 2 different issues with BTN_LEFT 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 SYNA3602
>    i2c mt touchpads.
> 
> This commit fixes both issues by not immediately reporting BTN_LEFT when
> we parse the report, but instead or-ing the values of all fields mapped
> to BTN_LEFT together 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
> ---
>  drivers/hid/hid-multitouch.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 76a3e9e074ba..bacbd1ba75ba 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -117,6 +117,7 @@ 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 btn_left;		/* BTN_LEFT 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 */
> @@ -717,9 +718,11 @@ 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)
>  {
> +	input_event(input, EV_KEY, BTN_LEFT, td->btn_left);
>  	input_mt_sync_frame(input);
>  	input_sync(input);
>  	td->num_received = 0;
> +	td->btn_left = 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
> @@ -794,6 +797,17 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>  			break;
>  
>  		default:
> +			/*
> +			 * On some hardware we get multiple BTN_LEFT reports
> +			 * per frame, with only 1 report reporting 1 if pressed.
> +			 * So for BTN_LEFT we or the values together and
> +			 * report the combined state in mt_sync_frame()
> +			 */
> +			if (usage->type == EV_KEY && usage->code == BTN_LEFT) {
> +				td->btn_left |= value;

Nah... We need to delay all the non multitouch events in the report if
we are not in the first report. This only fixes BTN_LEFT, but BTN_RIGHT
and others should also be fixed.

And for the point 2 you are raising, we should probably not map the
first button in case of a Precision Touchpad which is a non clickpad
one.

I still think you should rely on the scan time to detect the beginning
of the frame. If you store the current scan time and remember if we are
at the beginning or not of the frame (in mt_touch_report()), this should
be pretty straightforward to fix.

Cheers,
Benjamin

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

Re: [PATCH v2 3/3] HID: multitouch: Fix BTN_LEFT on some touchpads

[Hans de Goede]

Hi,

On 06-11-17 19:05, Benjamin Tissoires wrote:
> On Nov 06 2017 or thereabouts, Hans de Goede wrote:
>> This commit fixes 2 different issues with BTN_LEFT 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 SYNA3602
>>     i2c mt touchpads.
>>
>> This commit fixes both issues by not immediately reporting BTN_LEFT when
>> we parse the report, but instead or-ing the values of all fields mapped
>> to BTN_LEFT together 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
>> ---
>>   drivers/hid/hid-multitouch.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 76a3e9e074ba..bacbd1ba75ba 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -117,6 +117,7 @@ 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 btn_left;		/* BTN_LEFT 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 */
>> @@ -717,9 +718,11 @@ 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)
>>   {
>> +	input_event(input, EV_KEY, BTN_LEFT, td->btn_left);
>>   	input_mt_sync_frame(input);
>>   	input_sync(input);
>>   	td->num_received = 0;
>> +	td->btn_left = 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
>> @@ -794,6 +797,17 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>>   			break;
>>   
>>   		default:
>> +			/*
>> +			 * On some hardware we get multiple BTN_LEFT reports
>> +			 * per frame, with only 1 report reporting 1 if pressed.
>> +			 * So for BTN_LEFT we or the values together and
>> +			 * report the combined state in mt_sync_frame()
>> +			 */
>> +			if (usage->type == EV_KEY && usage->code == BTN_LEFT) {
>> +				td->btn_left |= value;
> 
> Nah... We need to delay all the non multitouch events in the report if
> we are not in the first report. This only fixes BTN_LEFT, but BTN_RIGHT
> and others should also be fixed.

Storing all non multi-touch events in the state is going to be tricky,
so for v3 I've gone with all buttons.

> And for the point 2 you are raising, we should probably not map the
> first button in case of a Precision Touchpad which is a non clickpad
> one.

This is actually a clickpad, the 2 usages for external left / right
buttons are bogus in the sense that there are no external buttons
hooked up. But I can imagine having a clickpad with both a "click"
and 2 external-buttons (e.g. buttons on the top for a trackpoint).

So I believe that just or-ing usages mapping to the same BTN code
together in a single frame is best, and something which we need to
do to delay reporting button presses anyway.

> I still think you should rely on the scan time to detect the beginning
> of the frame. If you store the current scan time and remember if we are
> at the beginning or not of the frame (in mt_touch_report()), this should
> be pretty straightforward to fix.

I'm looking at the scan_time now to reset num_expected.

v3 with these changes is coming up now.

Regards,

Hans



> 
> Cheers,
> Benjamin
> 
>> +				return;
>> +			}
>> +
>>   			if (usage->type)
>>   				input_event(input, usage->type, usage->code,
>>   						value);
>> -- 
>> 2.14.3
>>