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

[PATCH 1/2] 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>
---
 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 2/2] HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek mt touchpad

[Hans de Goede]

The Novatek 0603:0002 mt clickpad / keyboard combo found in some budget
Cherry Trail laptops, only reports the clickpad's button status in the
report for finger / slot 0. In the other reports the button field value
is always 0.

This leads to BTN_LEFT 0-1-0-1 events being reported to userspace when
the button is pressed and 2 fingers or more are down. Making it impossible
to (left)click with one finger and drag with another to e.g. select text.

This commit adds a quirk for this, ignoring the button field from reports
for the other fingers, fixing this.

Cc: Daniel Drake <drake@endlessm.com>
Cc: Carlo Caione <carlo@endlessm.com>
Cc: Robert McQueen <robert@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-ids.h        |  1 +
 drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index be2e005c3c51..45bc5d81208c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -797,6 +797,7 @@
 #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330
 
 #define USB_VENDOR_ID_NOVATEK		0x0603
+#define USB_DEVICE_ID_NOVATEK_MT_TP	0x0002
 #define USB_DEVICE_ID_NOVATEK_PCT	0x0600
 #define USB_DEVICE_ID_NOVATEK_MOUSE	0x1602
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index bb939f6990f1..28a58cfd2d3e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -73,6 +73,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_TOUCH_SIZE_SCALING	BIT(15)
 #define MT_QUIRK_STICKY_FINGERS		BIT(16)
 #define MT_QUIRK_ASUS_CUSTOM_UP		BIT(17)
+#define MT_QUIRK_BUTTON_IN_SLOT0_ONLY	BIT(18)
 
 #define MT_INPUTMODE_TOUCHSCREEN	0x02
 #define MT_INPUTMODE_TOUCHPAD		0x03
@@ -135,6 +136,7 @@ struct mt_device {
 	bool is_buttonpad;	/* is this device a button pad? */
 	bool serial_maybe;	/* need to check for serial protocol */
 	bool curvalid;		/* is the current contact valid? */
+	int curslot;		/* current linux mt slot */
 	unsigned mt_flags;	/* flags to pass to input-mt */
 };
 
@@ -173,6 +175,7 @@ static void mt_post_parse(struct mt_device *td);
 #define MT_CLS_ASUS				0x010b
 #define MT_CLS_VTL				0x0110
 #define MT_CLS_GOOGLE				0x0111
+#define MT_CLS_NOVATEK				0x0112
 
 #define MT_DEFAULT_MAXCONTACT	10
 #define MT_MAX_MAXCONTACT	250
@@ -307,6 +310,14 @@ static struct mt_class mt_classes[] = {
 			MT_QUIRK_SLOT_IS_CONTACTID |
 			MT_QUIRK_HOVERING
 	},
+	{ .name = MT_CLS_NOVATEK,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_IGNORE_DUPLICATES |
+			MT_QUIRK_HOVERING |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_STICKY_FINGERS |
+			MT_QUIRK_BUTTON_IN_SLOT0_ONLY
+	},
 	{ }
 };
 
@@ -676,6 +687,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 		active = (s->touch_state || s->inrange_state) &&
 							s->confidence_state;
 
+		td->curslot = slotnum;
 		input_mt_slot(input, slotnum);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
 		if (active) {
@@ -794,6 +806,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 			break;
 
 		default:
+			if ((quirks & MT_QUIRK_BUTTON_IN_SLOT0_ONLY) &&
+			    td->curslot != 0 &&
+			    usage->type == EV_KEY &&
+			    usage->code >= BTN_MOUSE &&
+			    usage->code < BTN_JOYSTICK)
+				return;
+
 			if (usage->type)
 				input_event(input, usage->type, usage->code,
 						value);
@@ -1605,6 +1624,12 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
 			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 
+	/* Novatek multi-touch touchpad / keyboard combo */
+	{ .driver_data = MT_CLS_NOVATEK,
+		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
+			USB_VENDOR_ID_NOVATEK,
+			USB_DEVICE_ID_NOVATEK_MT_TP) },
+
 	/* Novatek Panel */
 	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
-- 
2.14.3

Re: [PATCH 2/2] HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek mt touchpad

[Benjamin Tissoires]

On Nov 02 2017 or thereabouts, Hans de Goede wrote:
> The Novatek 0603:0002 mt clickpad / keyboard combo found in some budget
> Cherry Trail laptops, only reports the clickpad's button status in the
> report for finger / slot 0. In the other reports the button field value
> is always 0.
> 
> This leads to BTN_LEFT 0-1-0-1 events being reported to userspace when
> the button is pressed and 2 fingers or more are down. Making it impossible
> to (left)click with one finger and drag with another to e.g. select text.
> 
> This commit adds a quirk for this, ignoring the button field from reports
> for the other fingers, fixing this.

Hi Hans,

may I have a look at the hid-record output when you press the button
with one and with two fingers?

Because the quirk is pretty awful, and I wonder if there is not
something fishy in our implementation.

Cheers,
Benjamin

> 
> Cc: Daniel Drake <drake@endlessm.com>
> Cc: Carlo Caione <carlo@endlessm.com>
> Cc: Robert McQueen <robert@endlessm.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/hid-ids.h        |  1 +
>  drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index be2e005c3c51..45bc5d81208c 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -797,6 +797,7 @@
>  #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330
>  
>  #define USB_VENDOR_ID_NOVATEK		0x0603
> +#define USB_DEVICE_ID_NOVATEK_MT_TP	0x0002
>  #define USB_DEVICE_ID_NOVATEK_PCT	0x0600
>  #define USB_DEVICE_ID_NOVATEK_MOUSE	0x1602
>  
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index bb939f6990f1..28a58cfd2d3e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -73,6 +73,7 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_TOUCH_SIZE_SCALING	BIT(15)
>  #define MT_QUIRK_STICKY_FINGERS		BIT(16)
>  #define MT_QUIRK_ASUS_CUSTOM_UP		BIT(17)
> +#define MT_QUIRK_BUTTON_IN_SLOT0_ONLY	BIT(18)
>  
>  #define MT_INPUTMODE_TOUCHSCREEN	0x02
>  #define MT_INPUTMODE_TOUCHPAD		0x03
> @@ -135,6 +136,7 @@ struct mt_device {
>  	bool is_buttonpad;	/* is this device a button pad? */
>  	bool serial_maybe;	/* need to check for serial protocol */
>  	bool curvalid;		/* is the current contact valid? */
> +	int curslot;		/* current linux mt slot */
>  	unsigned mt_flags;	/* flags to pass to input-mt */
>  };
>  
> @@ -173,6 +175,7 @@ static void mt_post_parse(struct mt_device *td);
>  #define MT_CLS_ASUS				0x010b
>  #define MT_CLS_VTL				0x0110
>  #define MT_CLS_GOOGLE				0x0111
> +#define MT_CLS_NOVATEK				0x0112
>  
>  #define MT_DEFAULT_MAXCONTACT	10
>  #define MT_MAX_MAXCONTACT	250
> @@ -307,6 +310,14 @@ static struct mt_class mt_classes[] = {
>  			MT_QUIRK_SLOT_IS_CONTACTID |
>  			MT_QUIRK_HOVERING
>  	},
> +	{ .name = MT_CLS_NOVATEK,
> +		.quirks = MT_QUIRK_ALWAYS_VALID |
> +			MT_QUIRK_IGNORE_DUPLICATES |
> +			MT_QUIRK_HOVERING |
> +			MT_QUIRK_CONTACT_CNT_ACCURATE |
> +			MT_QUIRK_STICKY_FINGERS |
> +			MT_QUIRK_BUTTON_IN_SLOT0_ONLY
> +	},
>  	{ }
>  };
>  
> @@ -676,6 +687,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  		active = (s->touch_state || s->inrange_state) &&
>  							s->confidence_state;
>  
> +		td->curslot = slotnum;
>  		input_mt_slot(input, slotnum);
>  		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
>  		if (active) {
> @@ -794,6 +806,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>  			break;
>  
>  		default:
> +			if ((quirks & MT_QUIRK_BUTTON_IN_SLOT0_ONLY) &&
> +			    td->curslot != 0 &&
> +			    usage->type == EV_KEY &&
> +			    usage->code >= BTN_MOUSE &&
> +			    usage->code < BTN_JOYSTICK)
> +				return;
> +
>  			if (usage->type)
>  				input_event(input, usage->type, usage->code,
>  						value);
> @@ -1605,6 +1624,12 @@ static const struct hid_device_id mt_devices[] = {
>  		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
>  			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>  
> +	/* Novatek multi-touch touchpad / keyboard combo */
> +	{ .driver_data = MT_CLS_NOVATEK,
> +		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
> +			USB_VENDOR_ID_NOVATEK,
> +			USB_DEVICE_ID_NOVATEK_MT_TP) },
> +
>  	/* Novatek Panel */
>  	{ .driver_data = MT_CLS_NSMU,
>  		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
> -- 
> 2.14.3
> 

Re: [PATCH 2/2] HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek mt touchpad

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 5344 bytes --]

Hi,

On 03-11-17 10:56, Benjamin Tissoires wrote:
> On Nov 02 2017 or thereabouts, Hans de Goede wrote:
>> The Novatek 0603:0002 mt clickpad / keyboard combo found in some budget
>> Cherry Trail laptops, only reports the clickpad's button status in the
>> report for finger / slot 0. In the other reports the button field value
>> is always 0.
>>
>> This leads to BTN_LEFT 0-1-0-1 events being reported to userspace when
>> the button is pressed and 2 fingers or more are down. Making it impossible
>> to (left)click with one finger and drag with another to e.g. select text.
>>
>> This commit adds a quirk for this, ignoring the button field from reports
>> for the other fingers, fixing this.
> 
> Hi Hans,
> 
> may I have a look at the hid-record output when you press the button
> with one and with two fingers?
> 
> Because the quirk is pretty awful, and I wonder if there is not
> something fishy in our implementation.

Ok, recording of:

1) Clicking left-bottom of touchpad (left-button click on clickpad)
2) Dragging with a 2nd finger from left to right to select some text
3) Releasing 2nd finger
4) Releasing left-bottom / the click-button and the 1st finger

Attached.

Note AFAICT what is happening is really simply, the touchpad
sends reports containing data for 1 finger, so once 2 fingers
are down it alternately sends report for contactid 0 and contactid 1
and only the reports with contactid 0 contain a valid value for
the BTN_LEFT field, in the reports with contactid 1 the field
which we map to BTN_LEFT is always 0, so I really see no other
solution then ignoring that field for contactid != 0.

Regards,

Hans




> 
> Cheers,
> Benjamin
> 
>>
>> Cc: Daniel Drake <drake@endlessm.com>
>> Cc: Carlo Caione <carlo@endlessm.com>
>> Cc: Robert McQueen <robert@endlessm.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/hid/hid-ids.h        |  1 +
>>   drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index be2e005c3c51..45bc5d81208c 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -797,6 +797,7 @@
>>   #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330
>>   
>>   #define USB_VENDOR_ID_NOVATEK		0x0603
>> +#define USB_DEVICE_ID_NOVATEK_MT_TP	0x0002
>>   #define USB_DEVICE_ID_NOVATEK_PCT	0x0600
>>   #define USB_DEVICE_ID_NOVATEK_MOUSE	0x1602
>>   
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index bb939f6990f1..28a58cfd2d3e 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -73,6 +73,7 @@ MODULE_LICENSE("GPL");
>>   #define MT_QUIRK_TOUCH_SIZE_SCALING	BIT(15)
>>   #define MT_QUIRK_STICKY_FINGERS		BIT(16)
>>   #define MT_QUIRK_ASUS_CUSTOM_UP		BIT(17)
>> +#define MT_QUIRK_BUTTON_IN_SLOT0_ONLY	BIT(18)
>>   
>>   #define MT_INPUTMODE_TOUCHSCREEN	0x02
>>   #define MT_INPUTMODE_TOUCHPAD		0x03
>> @@ -135,6 +136,7 @@ struct mt_device {
>>   	bool is_buttonpad;	/* is this device a button pad? */
>>   	bool serial_maybe;	/* need to check for serial protocol */
>>   	bool curvalid;		/* is the current contact valid? */
>> +	int curslot;		/* current linux mt slot */
>>   	unsigned mt_flags;	/* flags to pass to input-mt */
>>   };
>>   
>> @@ -173,6 +175,7 @@ static void mt_post_parse(struct mt_device *td);
>>   #define MT_CLS_ASUS				0x010b
>>   #define MT_CLS_VTL				0x0110
>>   #define MT_CLS_GOOGLE				0x0111
>> +#define MT_CLS_NOVATEK				0x0112
>>   
>>   #define MT_DEFAULT_MAXCONTACT	10
>>   #define MT_MAX_MAXCONTACT	250
>> @@ -307,6 +310,14 @@ static struct mt_class mt_classes[] = {
>>   			MT_QUIRK_SLOT_IS_CONTACTID |
>>   			MT_QUIRK_HOVERING
>>   	},
>> +	{ .name = MT_CLS_NOVATEK,
>> +		.quirks = MT_QUIRK_ALWAYS_VALID |
>> +			MT_QUIRK_IGNORE_DUPLICATES |
>> +			MT_QUIRK_HOVERING |
>> +			MT_QUIRK_CONTACT_CNT_ACCURATE |
>> +			MT_QUIRK_STICKY_FINGERS |
>> +			MT_QUIRK_BUTTON_IN_SLOT0_ONLY
>> +	},
>>   	{ }
>>   };
>>   
>> @@ -676,6 +687,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>   		active = (s->touch_state || s->inrange_state) &&
>>   							s->confidence_state;
>>   
>> +		td->curslot = slotnum;
>>   		input_mt_slot(input, slotnum);
>>   		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
>>   		if (active) {
>> @@ -794,6 +806,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>>   			break;
>>   
>>   		default:
>> +			if ((quirks & MT_QUIRK_BUTTON_IN_SLOT0_ONLY) &&
>> +			    td->curslot != 0 &&
>> +			    usage->type == EV_KEY &&
>> +			    usage->code >= BTN_MOUSE &&
>> +			    usage->code < BTN_JOYSTICK)
>> +				return;
>> +
>>   			if (usage->type)
>>   				input_event(input, usage->type, usage->code,
>>   						value);
>> @@ -1605,6 +1624,12 @@ static const struct hid_device_id mt_devices[] = {
>>   		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
>>   			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>>   
>> +	/* Novatek multi-touch touchpad / keyboard combo */
>> +	{ .driver_data = MT_CLS_NOVATEK,
>> +		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
>> +			USB_VENDOR_ID_NOVATEK,
>> +			USB_DEVICE_ID_NOVATEK_MT_TP) },
>> +
>>   	/* Novatek Panel */
>>   	{ .driver_data = MT_CLS_NSMU,
>>   		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
>> -- 
>> 2.14.3
>>

[-- Attachment #2: log.gz --]
[-- Type: application/gzip, Size: 5526 bytes --]

Re: [PATCH 2/2] HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek mt touchpad

[Benjamin Tissoires]

On Nov 03 2017 or thereabouts, Hans de Goede wrote:
> Hi,
> 
> On 03-11-17 10:56, Benjamin Tissoires wrote:
> > On Nov 02 2017 or thereabouts, Hans de Goede wrote:
> > > The Novatek 0603:0002 mt clickpad / keyboard combo found in some budget
> > > Cherry Trail laptops, only reports the clickpad's button status in the
> > > report for finger / slot 0. In the other reports the button field value
> > > is always 0.
> > > 
> > > This leads to BTN_LEFT 0-1-0-1 events being reported to userspace when
> > > the button is pressed and 2 fingers or more are down. Making it impossible
> > > to (left)click with one finger and drag with another to e.g. select text.
> > > 
> > > This commit adds a quirk for this, ignoring the button field from reports
> > > for the other fingers, fixing this.
> > 
> > Hi Hans,
> > 
> > may I have a look at the hid-record output when you press the button
> > with one and with two fingers?
> > 
> > Because the quirk is pretty awful, and I wonder if there is not
> > something fishy in our implementation.
> 
> Ok, recording of:
> 
> 1) Clicking left-bottom of touchpad (left-button click on clickpad)
> 2) Dragging with a 2nd finger from left to right to select some text
> 3) Releasing 2nd finger
> 4) Releasing left-bottom / the click-button and the 1st finger
> 
> Attached.
> 
> Note AFAICT what is happening is really simply, the touchpad
> sends reports containing data for 1 finger, so once 2 fingers
> are down it alternately sends report for contactid 0 and contactid 1
> and only the reports with contactid 0 contain a valid value for
> the BTN_LEFT field, in the reports with contactid 1 the field
> which we map to BTN_LEFT is always 0, so I really see no other
> solution then ignoring that field for contactid != 0.

Thanks. So this is more or less what I expected, we are not fully
"compliant" with how Windows handle the touchpads:
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections

This touchpad is using "single finger hybrid mode". The first report is
marked with Contact Count > 0 and the subsequent ones have a contact
count of 0.
Given that the button is exported in all reports, it makes "sense" to
actually only rely on the first report of the button, but not in the
subsequent ones.

So I think we should fix the general protocol handling, not add a quirk
for this one particular model. On top of that, you assume the contact ID
0 will always be reported with the button flags, while my guess is that
if you press one finger to reach the button threshold, put a second
finger, keeping the force hard enough, release the first, still keeping
the button down, and then releasing the second finger, you might get the
button stuck because at one point, the button information will be
relayed through the second finger (contact ID 1). It might also simply
release it too soon.

Anyway, a proper fix should be to tag which reports are primary (with
contact count > 0, or with only buttons information, and/or that are
not sharing the same scan time than the previous report), and only take
into account buttons for these primary reports.

Of course, this should only apply to Win8 touchpads, given that Win7
ones are dying or need special quirks.

Cheers,
Benjamin
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > 
> > Cheers,
> > Benjamin
> > 
> > > 
> > > Cc: Daniel Drake <drake@endlessm.com>
> > > Cc: Carlo Caione <carlo@endlessm.com>
> > > Cc: Robert McQueen <robert@endlessm.com>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/hid/hid-ids.h        |  1 +
> > >   drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
> > >   2 files changed, 26 insertions(+)
> > > 
> > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > index be2e005c3c51..45bc5d81208c 100644
> > > --- a/drivers/hid/hid-ids.h
> > > +++ b/drivers/hid/hid-ids.h
> > > @@ -797,6 +797,7 @@
> > >   #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330
> > >   #define USB_VENDOR_ID_NOVATEK		0x0603
> > > +#define USB_DEVICE_ID_NOVATEK_MT_TP	0x0002
> > >   #define USB_DEVICE_ID_NOVATEK_PCT	0x0600
> > >   #define USB_DEVICE_ID_NOVATEK_MOUSE	0x1602
> > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > > index bb939f6990f1..28a58cfd2d3e 100644
> > > --- a/drivers/hid/hid-multitouch.c
> > > +++ b/drivers/hid/hid-multitouch.c
> > > @@ -73,6 +73,7 @@ MODULE_LICENSE("GPL");
> > >   #define MT_QUIRK_TOUCH_SIZE_SCALING	BIT(15)
> > >   #define MT_QUIRK_STICKY_FINGERS		BIT(16)
> > >   #define MT_QUIRK_ASUS_CUSTOM_UP		BIT(17)
> > > +#define MT_QUIRK_BUTTON_IN_SLOT0_ONLY	BIT(18)
> > >   #define MT_INPUTMODE_TOUCHSCREEN	0x02
> > >   #define MT_INPUTMODE_TOUCHPAD		0x03
> > > @@ -135,6 +136,7 @@ struct mt_device {
> > >   	bool is_buttonpad;	/* is this device a button pad? */
> > >   	bool serial_maybe;	/* need to check for serial protocol */
> > >   	bool curvalid;		/* is the current contact valid? */
> > > +	int curslot;		/* current linux mt slot */
> > >   	unsigned mt_flags;	/* flags to pass to input-mt */
> > >   };
> > > @@ -173,6 +175,7 @@ static void mt_post_parse(struct mt_device *td);
> > >   #define MT_CLS_ASUS				0x010b
> > >   #define MT_CLS_VTL				0x0110
> > >   #define MT_CLS_GOOGLE				0x0111
> > > +#define MT_CLS_NOVATEK				0x0112
> > >   #define MT_DEFAULT_MAXCONTACT	10
> > >   #define MT_MAX_MAXCONTACT	250
> > > @@ -307,6 +310,14 @@ static struct mt_class mt_classes[] = {
> > >   			MT_QUIRK_SLOT_IS_CONTACTID |
> > >   			MT_QUIRK_HOVERING
> > >   	},
> > > +	{ .name = MT_CLS_NOVATEK,
> > > +		.quirks = MT_QUIRK_ALWAYS_VALID |
> > > +			MT_QUIRK_IGNORE_DUPLICATES |
> > > +			MT_QUIRK_HOVERING |
> > > +			MT_QUIRK_CONTACT_CNT_ACCURATE |
> > > +			MT_QUIRK_STICKY_FINGERS |
> > > +			MT_QUIRK_BUTTON_IN_SLOT0_ONLY
> > > +	},
> > >   	{ }
> > >   };
> > > @@ -676,6 +687,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
> > >   		active = (s->touch_state || s->inrange_state) &&
> > >   							s->confidence_state;
> > > +		td->curslot = slotnum;
> > >   		input_mt_slot(input, slotnum);
> > >   		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
> > >   		if (active) {
> > > @@ -794,6 +806,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
> > >   			break;
> > >   		default:
> > > +			if ((quirks & MT_QUIRK_BUTTON_IN_SLOT0_ONLY) &&
> > > +			    td->curslot != 0 &&
> > > +			    usage->type == EV_KEY &&
> > > +			    usage->code >= BTN_MOUSE &&
> > > +			    usage->code < BTN_JOYSTICK)
> > > +				return;
> > > +
> > >   			if (usage->type)
> > >   				input_event(input, usage->type, usage->code,
> > >   						value);
> > > @@ -1605,6 +1624,12 @@ static const struct hid_device_id mt_devices[] = {
> > >   		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
> > >   			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
> > > +	/* Novatek multi-touch touchpad / keyboard combo */
> > > +	{ .driver_data = MT_CLS_NOVATEK,
> > > +		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
> > > +			USB_VENDOR_ID_NOVATEK,
> > > +			USB_DEVICE_ID_NOVATEK_MT_TP) },
> > > +
> > >   	/* Novatek Panel */
> > >   	{ .driver_data = MT_CLS_NSMU,
> > >   		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
> > > -- 
> > > 2.14.3
> > > 

Re: [PATCH 2/2] HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek mt touchpad

[Hans de Goede]

Hi,

On 03-11-17 14:09, Benjamin Tissoires wrote:
> On Nov 03 2017 or thereabouts, Hans de Goede wrote:
>> Hi,
>>
>> On 03-11-17 10:56, Benjamin Tissoires wrote:
>>> On Nov 02 2017 or thereabouts, Hans de Goede wrote:
>>>> The Novatek 0603:0002 mt clickpad / keyboard combo found in some budget
>>>> Cherry Trail laptops, only reports the clickpad's button status in the
>>>> report for finger / slot 0. In the other reports the button field value
>>>> is always 0.
>>>>
>>>> This leads to BTN_LEFT 0-1-0-1 events being reported to userspace when
>>>> the button is pressed and 2 fingers or more are down. Making it impossible
>>>> to (left)click with one finger and drag with another to e.g. select text.
>>>>
>>>> This commit adds a quirk for this, ignoring the button field from reports
>>>> for the other fingers, fixing this.
>>>
>>> Hi Hans,
>>>
>>> may I have a look at the hid-record output when you press the button
>>> with one and with two fingers?
>>>
>>> Because the quirk is pretty awful, and I wonder if there is not
>>> something fishy in our implementation.
>>
>> Ok, recording of:
>>
>> 1) Clicking left-bottom of touchpad (left-button click on clickpad)
>> 2) Dragging with a 2nd finger from left to right to select some text
>> 3) Releasing 2nd finger
>> 4) Releasing left-bottom / the click-button and the 1st finger
>>
>> Attached.
>>
>> Note AFAICT what is happening is really simply, the touchpad
>> sends reports containing data for 1 finger, so once 2 fingers
>> are down it alternately sends report for contactid 0 and contactid 1
>> and only the reports with contactid 0 contain a valid value for
>> the BTN_LEFT field, in the reports with contactid 1 the field
>> which we map to BTN_LEFT is always 0, so I really see no other
>> solution then ignoring that field for contactid != 0.
> 
> Thanks. So this is more or less what I expected, we are not fully
> "compliant" with how Windows handle the touchpads:
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections
> 
> This touchpad is using "single finger hybrid mode". The first report is
> marked with Contact Count > 0 and the subsequent ones have a contact
> count of 0.
> Given that the button is exported in all reports, it makes "sense" to
> actually only rely on the first report of the button, but not in the
> subsequent ones.
> 
> So I think we should fix the general protocol handling, not add a quirk
> for this one particular model. On top of that, you assume the contact ID
> 0 will always be reported with the button flags, while my guess is that
> if you press one finger to reach the button threshold, put a second
> finger, keeping the force hard enough, release the first, still keeping
> the button down, and then releasing the second finger, you might get the
> button stuck because at one point, the button information will be
> relayed through the second finger (contact ID 1). It might also simply
> release it too soon.
> 
> Anyway, a proper fix should be to tag which reports are primary (with
> contact count > 0, or with only buttons information, and/or that are
> not sharing the same scan time than the previous report), and only take
> into account buttons for these primary reports.
> 
> Of course, this should only apply to Win8 touchpads, given that Win7
> ones are dying or need special quirks.

Ok.

So I ended up hitting a related bug on a different Chinese laptop
(yeah cheap hardware) and I came up with a single fix for both issues.

The commit message of the new version explains this, so I will let
that one speak for itself.

Regards,

Hans



> 
> Cheers,
> Benjamin
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>
>>> Cheers,
>>> Benjamin
>>>
>>>>
>>>> Cc: Daniel Drake <drake@endlessm.com>
>>>> Cc: Carlo Caione <carlo@endlessm.com>
>>>> Cc: Robert McQueen <robert@endlessm.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    drivers/hid/hid-ids.h        |  1 +
>>>>    drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
>>>>    2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>>> index be2e005c3c51..45bc5d81208c 100644
>>>> --- a/drivers/hid/hid-ids.h
>>>> +++ b/drivers/hid/hid-ids.h
>>>> @@ -797,6 +797,7 @@
>>>>    #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330
>>>>    #define USB_VENDOR_ID_NOVATEK		0x0603
>>>> +#define USB_DEVICE_ID_NOVATEK_MT_TP	0x0002
>>>>    #define USB_DEVICE_ID_NOVATEK_PCT	0x0600
>>>>    #define USB_DEVICE_ID_NOVATEK_MOUSE	0x1602
>>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>>>> index bb939f6990f1..28a58cfd2d3e 100644
>>>> --- a/drivers/hid/hid-multitouch.c
>>>> +++ b/drivers/hid/hid-multitouch.c
>>>> @@ -73,6 +73,7 @@ MODULE_LICENSE("GPL");
>>>>    #define MT_QUIRK_TOUCH_SIZE_SCALING	BIT(15)
>>>>    #define MT_QUIRK_STICKY_FINGERS		BIT(16)
>>>>    #define MT_QUIRK_ASUS_CUSTOM_UP		BIT(17)
>>>> +#define MT_QUIRK_BUTTON_IN_SLOT0_ONLY	BIT(18)
>>>>    #define MT_INPUTMODE_TOUCHSCREEN	0x02
>>>>    #define MT_INPUTMODE_TOUCHPAD		0x03
>>>> @@ -135,6 +136,7 @@ struct mt_device {
>>>>    	bool is_buttonpad;	/* is this device a button pad? */
>>>>    	bool serial_maybe;	/* need to check for serial protocol */
>>>>    	bool curvalid;		/* is the current contact valid? */
>>>> +	int curslot;		/* current linux mt slot */
>>>>    	unsigned mt_flags;	/* flags to pass to input-mt */
>>>>    };
>>>> @@ -173,6 +175,7 @@ static void mt_post_parse(struct mt_device *td);
>>>>    #define MT_CLS_ASUS				0x010b
>>>>    #define MT_CLS_VTL				0x0110
>>>>    #define MT_CLS_GOOGLE				0x0111
>>>> +#define MT_CLS_NOVATEK				0x0112
>>>>    #define MT_DEFAULT_MAXCONTACT	10
>>>>    #define MT_MAX_MAXCONTACT	250
>>>> @@ -307,6 +310,14 @@ static struct mt_class mt_classes[] = {
>>>>    			MT_QUIRK_SLOT_IS_CONTACTID |
>>>>    			MT_QUIRK_HOVERING
>>>>    	},
>>>> +	{ .name = MT_CLS_NOVATEK,
>>>> +		.quirks = MT_QUIRK_ALWAYS_VALID |
>>>> +			MT_QUIRK_IGNORE_DUPLICATES |
>>>> +			MT_QUIRK_HOVERING |
>>>> +			MT_QUIRK_CONTACT_CNT_ACCURATE |
>>>> +			MT_QUIRK_STICKY_FINGERS |
>>>> +			MT_QUIRK_BUTTON_IN_SLOT0_ONLY
>>>> +	},
>>>>    	{ }
>>>>    };
>>>> @@ -676,6 +687,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>>>    		active = (s->touch_state || s->inrange_state) &&
>>>>    							s->confidence_state;
>>>> +		td->curslot = slotnum;
>>>>    		input_mt_slot(input, slotnum);
>>>>    		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
>>>>    		if (active) {
>>>> @@ -794,6 +806,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>>>>    			break;
>>>>    		default:
>>>> +			if ((quirks & MT_QUIRK_BUTTON_IN_SLOT0_ONLY) &&
>>>> +			    td->curslot != 0 &&
>>>> +			    usage->type == EV_KEY &&
>>>> +			    usage->code >= BTN_MOUSE &&
>>>> +			    usage->code < BTN_JOYSTICK)
>>>> +				return;
>>>> +
>>>>    			if (usage->type)
>>>>    				input_event(input, usage->type, usage->code,
>>>>    						value);
>>>> @@ -1605,6 +1624,12 @@ static const struct hid_device_id mt_devices[] = {
>>>>    		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
>>>>    			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>>>> +	/* Novatek multi-touch touchpad / keyboard combo */
>>>> +	{ .driver_data = MT_CLS_NOVATEK,
>>>> +		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
>>>> +			USB_VENDOR_ID_NOVATEK,
>>>> +			USB_DEVICE_ID_NOVATEK_MT_TP) },
>>>> +
>>>>    	/* Novatek Panel */
>>>>    	{ .driver_data = MT_CLS_NSMU,
>>>>    		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
>>>> -- 
>>>> 2.14.3
>>>>
> 
> 

Re: [PATCH 1/2] HID: multitouch: Fix alphabetic sorting of mt_devices table.

[Benjamin Tissoires]

On Nov 02 2017 or thereabouts, Hans de Goede wrote:
> Fix alphabetic sorting of mt_devices hid_device_id table.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

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

Cheers,
Benjamin

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