[PATCH 0/4] reduce Surface Pro 3 multitouch jitter

[PATCH 0/4] reduce Surface Pro 3 multitouch jitter

[Joey Pabalinas]

The Surface Pro 3 firmware doesn't reliably send contact lift off
reports nor handle invalid report values gracefully.

To reduce touchscreen input jitter:
  - add MT_QUIRK_NOT_SEEN_MEANS_UP to the MT_CLS_WIN_8
  - drop invalid report values

Joey Pabalinas (4):
  HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  HID: multitouch: drop reports containing invalid values
  HID: multitouch: remove unneeded else conditional cases

 drivers/hid/hid-multitouch.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks

[Joey Pabalinas]

The firmware found in the touch screen of the Surface Pro 3 is slightly
buggy and occasionally doesn't send lift off reports for contacts; add
MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
reports.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 45968f7970f87775fa..a793076139d7d0db9b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_IGNORE_DUPLICATES |
 			MT_QUIRK_HOVERING |
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_STICKY_FINGERS |
-			MT_QUIRK_WIN8_PTP_BUTTONS },
+			MT_QUIRK_WIN8_PTP_BUTTONS |
+			MT_QUIRK_NOT_SEEN_MEANS_UP },
 	{ .name = MT_CLS_EXPORT_ALL_INPUTS,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_CONTACT_CNT_ACCURATE,
 		.export_all_inputs = true },
 	{ .name = MT_CLS_WIN_8_DUAL,
-- 
2.18.0

Re: [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks

[Benjamin Tissoires]

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> The firmware found in the touch screen of the Surface Pro 3 is slightly
> buggy and occasionally doesn't send lift off reports for contacts; add
> MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
> reports.
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 45968f7970f87775fa..a793076139d7d0db9b 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
>                 .quirks = MT_QUIRK_ALWAYS_VALID |
>                         MT_QUIRK_IGNORE_DUPLICATES |
>                         MT_QUIRK_HOVERING |
>                         MT_QUIRK_CONTACT_CNT_ACCURATE |
>                         MT_QUIRK_STICKY_FINGERS |
> -                       MT_QUIRK_WIN8_PTP_BUTTONS },
> +                       MT_QUIRK_WIN8_PTP_BUTTONS |
> +                       MT_QUIRK_NOT_SEEN_MEANS_UP },

NACK on this.
If the Surface has a buggy firmware, we should handle it as a special
case, not as a common failure.
Also, I am not sure this quirk is compatible with Win 8 specification.
Last, we now have a timeout for unreleased touches, so I really don't
think you need that in recent kernels.

Cheers,
Benjamin

>         { .name = MT_CLS_EXPORT_ALL_INPUTS,
>                 .quirks = MT_QUIRK_ALWAYS_VALID |
>                         MT_QUIRK_CONTACT_CNT_ACCURATE,
>                 .export_all_inputs = true },
>         { .name = MT_CLS_WIN_8_DUAL,
> --
> 2.18.0
>

Re: [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks

[Joey Pabalinas]

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

On Tue, Jul 03, 2018 at 09:53:19AM +0200, Benjamin Tissoires wrote:
> NACK on this.
> If the Surface has a buggy firmware, we should handle it as a special
> case, not as a common failure.
> Also, I am not sure this quirk is compatible with Win 8 specification.
> Last, we now have a timeout for unreleased touches, so I really don't
> think you need that in recent kernels.
> 
> Cheers,
> Benjamin
> 
> >         { .name = MT_CLS_EXPORT_ALL_INPUTS,
> >                 .quirks = MT_QUIRK_ALWAYS_VALID |
> >                         MT_QUIRK_CONTACT_CNT_ACCURATE,
> >                 .export_all_inputs = true },
> >         { .name = MT_CLS_WIN_8_DUAL,
> > --
> > 2.18.0
> >
Ah shoot, I had completely missed this reply to my earlier patchset, I
apologize. Now that you mention it, I think handling it as a special
case would be a better way to handle it, so I'll do a bit more research
on the quirk thing and then revise my patches (assuming they end up
as something still worth sending). Appreciate the comments, thanks.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RESEND 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks

[Joey Pabalinas]

The firmware found in the touch screen of the Surface Pro 3 is slightly
buggy and occasionally doesn't send lift off reports for contacts; add
MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
reports.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 45968f7970f87775fa..a793076139d7d0db9b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_IGNORE_DUPLICATES |
 			MT_QUIRK_HOVERING |
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_STICKY_FINGERS |
-			MT_QUIRK_WIN8_PTP_BUTTONS },
+			MT_QUIRK_WIN8_PTP_BUTTONS |
+			MT_QUIRK_NOT_SEEN_MEANS_UP },
 	{ .name = MT_CLS_EXPORT_ALL_INPUTS,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_CONTACT_CNT_ACCURATE,
 		.export_all_inputs = true },
 	{ .name = MT_CLS_WIN_8_DUAL,
-- 
2.18.0

Re: [PATCH RESEND 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks

[Joey Pabalinas]

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

On Thu, Aug 09, 2018 at 01:39:16PM -1000, Joey Pabalinas wrote:
> The firmware found in the touch screen of the Surface Pro 3 is slightly
> buggy and occasionally doesn't send lift off reports for contacts; add
> MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
> reports.
> 
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

Sorry, my mail filters are in need of some adjusting; I completely missed
the review of the first send of this patchset :(

Please disregard this resend, as I am going to revise it and submit a
v2, thanks.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol

[Joey Pabalinas]

The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
an unnecessary mt_post_parse_default_settings() call

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a793076139d7d0db9b..c0654db0b736543ca0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (!td->fields) {
 		dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
 		return -ENOMEM;
 	}
 
-	if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
+	if (id->vendor == HID_ANY_ID
+			&& id->product == HID_ANY_ID
+			&& id->group != HID_GROUP_MULTITOUCH_WIN_8)
 		td->serial_maybe = true;
 
 	/* This allows the driver to correctly support devices
 	 * that emit events over several HID messages.
 	 */
-- 
2.18.0

Re: [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol

[Benjamin Tissoires]

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
> protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
> an unnecessary mt_post_parse_default_settings() call
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index a793076139d7d0db9b..c0654db0b736543ca0 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (!td->fields) {
>                 dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
>                 return -ENOMEM;
>         }
>
> -       if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> +       if (id->vendor == HID_ANY_ID
> +                       && id->product == HID_ANY_ID
> +                       && id->group != HID_GROUP_MULTITOUCH_WIN_8)

There is a tiny difference between the HID group (this device looks
like it is used as a Win 8 device) and the device class (this is
effectively a Win8 device)

It makes sense to remove this check for Win8 devices, but I don't
think it should be done for all devices.

All in all, it won't change much.

Cheers,
Benjamin

>                 td->serial_maybe = true;
>
>         /* This allows the driver to correctly support devices
>          * that emit events over several HID messages.
>          */
> --
> 2.18.0
>

Re: [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol

[Joey Pabalinas]

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

On Tue, Jul 03, 2018 at 10:16:01AM +0200, Benjamin Tissoires wrote:
> There is a tiny difference between the HID group (this device looks
> like it is used as a Win 8 device) and the device class (this is
> effectively a Win8 device)
> 
> It makes sense to remove this check for Win8 devices, but I don't
> think it should be done for all devices.
> 
> All in all, it won't change much.

Hm, that sounds sane. I'll do a bit more research on this and see if
restricting it to just Win8 devices is simple enough to be worthwhile.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RESEND 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol

[Joey Pabalinas]

The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
an unnecessary mt_post_parse_default_settings() call

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a793076139d7d0db9b..c0654db0b736543ca0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (!td->fields) {
 		dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
 		return -ENOMEM;
 	}
 
-	if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
+	if (id->vendor == HID_ANY_ID
+			&& id->product == HID_ANY_ID
+			&& id->group != HID_GROUP_MULTITOUCH_WIN_8)
 		td->serial_maybe = true;
 
 	/* This allows the driver to correctly support devices
 	 * that emit events over several HID messages.
 	 */
-- 
2.18.0

[PATCH 3/4] HID: multitouch: drop reports containing invalid values

[Joey Pabalinas]

Avoid processing reports containing invalid values to reduce
multitouch input stutter.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0654db0b736543ca0..08b50e5908cecdda66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 {
 	if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
 	    td->num_received >= td->num_expected)
 		return;
 
+	/* drop invalid values after counting them */
+	if (td->curdata.x == 0xffff &&
+			td->curdata.y == 0xffff &&
+			td->curdata.w == 0xffff &&
+			td->curdata.h == 0xffff) {
+		td->num_received++;
+		return;
+	}
+
 	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
 		int active;
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
 		struct input_mt *mt = input->mt;
-- 
2.18.0

Re: [PATCH 3/4] HID: multitouch: drop reports containing invalid values

[Benjamin Tissoires]

Hi Joey,

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> Avoid processing reports containing invalid values to reduce
> multitouch input stutter.
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c0654db0b736543ca0..08b50e5908cecdda66 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  {
>         if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
>             td->num_received >= td->num_expected)
>                 return;
>
> +       /* drop invalid values after counting them */
> +       if (td->curdata.x == 0xffff &&
> +                       td->curdata.y == 0xffff &&
> +                       td->curdata.w == 0xffff &&
> +                       td->curdata.h == 0xffff) {

You can't really use plain values like that. There is a tiny chance
these values are valid on an other device.
IIRC, MS spec says that we should ignore out of band values if they
are tagged as such. Such input are tagged with NULL values
(http://www.usb.org/developers/hidpage/HID1_11.pdf page 31) and MS
spec mentioned this.

All in all, if you have this bit set, you need to compare the value
with the logical_max/min for each field.

I never encountered a device that required this, so you are probably
the lucky one :)

Cheers,
Benjamin

> +               td->num_received++;
> +               return;
> +       }
> +
>         if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>                 int active;
>                 int slotnum = mt_compute_slot(td, input);
>                 struct mt_slot *s = &td->curdata;
>                 struct input_mt *mt = input->mt;
> --
> 2.18.0
>

Re: [PATCH 3/4] HID: multitouch: drop reports containing invalid values

[Joey Pabalinas]

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

On Tue, Jul 03, 2018 at 10:13:54AM +0200, Benjamin Tissoires wrote:
> Hi Joey,
> You can't really use plain values like that. There is a tiny chance
> these values are valid on an other device.
> IIRC, MS spec says that we should ignore out of band values if they
> are tagged as such. Such input are tagged with NULL values
> (http://www.usb.org/developers/hidpage/HID1_11.pdf page 31) and MS
> spec mentioned this.
> 
> All in all, if you have this bit set, you need to compare the value
> with the logical_max/min for each field.
> 
> I never encountered a device that required this, so you are probably
> the lucky one :)

Ah, you are completely right. After giving that pdf a read over
I will definitely be dropping this patch from the v2.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RESEND 3/4] HID: multitouch: drop reports containing invalid values

[Joey Pabalinas]

Avoid processing reports containing invalid values to reduce
multitouch input stutter.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0654db0b736543ca0..08b50e5908cecdda66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 {
 	if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
 	    td->num_received >= td->num_expected)
 		return;
 
+	/* drop invalid values after counting them */
+	if (td->curdata.x == 0xffff &&
+			td->curdata.y == 0xffff &&
+			td->curdata.w == 0xffff &&
+			td->curdata.h == 0xffff) {
+		td->num_received++;
+		return;
+	}
+
 	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
 		int active;
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
 		struct input_mt *mt = input->mt;
-- 
2.18.0

[PATCH 4/4] HID: multitouch: remove unneeded else conditional cases

[Joey Pabalinas]

Elide lone `else` cases and replace `else if` clauses
with plain `if` conditionals when they occur immediately
after return statements.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
 
 static int cypress_compute_slot(struct mt_device *td)
 {
 	if (td->curdata.contactid != 0 || td->num_received == 0)
 		return td->curdata.contactid;
-	else
-		return -1;
+
+	return -1;
 }
 
 static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_DEFAULT,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
@@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
 	delta *= 100;
 
 	if (jdelta > MAX_TIMESTAMP_INTERVAL)
 		/* No data received for a while, resync the timestamp. */
 		return 0;
-	else
-		return td->timestamp + delta;
+
+	return td->timestamp + delta;
 }
 
 static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
@@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	 * HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
 	 * collection, but within the report ID.
 	 */
 	if (field->physical == HID_DG_STYLUS)
 		return 0;
-	else if ((field->physical == 0) &&
+
+	if ((field->physical == 0) &&
 		 (field->report->id != td->mt_report_id) &&
 		 (td->mt_report_id != -1))
 		return 0;
 
 	if (field->application == HID_DG_TOUCHSCREEN ||
-- 
2.18.0

Re: [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases

[Benjamin Tissoires]

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> Elide lone `else` cases and replace `else if` clauses
> with plain `if` conditionals when they occur immediately
> after return statements.
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>

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

Just FYI, I sent out a big refactor of the hid-multitouch code. Jiri
should be still reviewing it, so I am not so sure who will have to
rebase which series on top of the other, but someone between us will
have to do it :)

Cheers,
Benjamin

>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
>
>  static int cypress_compute_slot(struct mt_device *td)
>  {
>         if (td->curdata.contactid != 0 || td->num_received == 0)
>                 return td->curdata.contactid;
> -       else
> -               return -1;
> +
> +       return -1;
>  }
>
>  static struct mt_class mt_classes[] = {
>         { .name = MT_CLS_DEFAULT,
>                 .quirks = MT_QUIRK_ALWAYS_VALID |
> @@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
>         delta *= 100;
>
>         if (jdelta > MAX_TIMESTAMP_INTERVAL)
>                 /* No data received for a while, resync the timestamp. */
>                 return 0;
> -       else
> -               return td->timestamp + delta;
> +
> +       return td->timestamp + delta;
>  }
>
>  static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
>                                 struct hid_usage *usage, __s32 value)
>  {
> @@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>          * HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
>          * collection, but within the report ID.
>          */
>         if (field->physical == HID_DG_STYLUS)
>                 return 0;
> -       else if ((field->physical == 0) &&
> +
> +       if ((field->physical == 0) &&
>                  (field->report->id != td->mt_report_id) &&
>                  (td->mt_report_id != -1))
>                 return 0;
>
>         if (field->application == HID_DG_TOUCHSCREEN ||
> --
> 2.18.0
>

Re: [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases

[Joey Pabalinas]

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

On Tue, Jul 03, 2018 at 10:17:58AM +0200, Benjamin Tissoires wrote:
> This one looks good.
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Just FYI, I sent out a big refactor of the hid-multitouch code. Jiri
> should be still reviewing it, so I am not so sure who will have to
> rebase which series on top of the other, but someone between us will
> have to do it :)

Somehow also missed this reply... I guess I messed up my LKML
filters somewhere. Noted, thanks for the review; I'll check
your trees for the refactor when (if) I do my v2.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RESEND 4/4] HID: multitouch: remove unneeded else conditional cases

[Joey Pabalinas]

Elide lone `else` cases and replace `else if` clauses
with plain `if` conditionals when they occur immediately
after return statements.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
 
 static int cypress_compute_slot(struct mt_device *td)
 {
 	if (td->curdata.contactid != 0 || td->num_received == 0)
 		return td->curdata.contactid;
-	else
-		return -1;
+
+	return -1;
 }
 
 static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_DEFAULT,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
@@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
 	delta *= 100;
 
 	if (jdelta > MAX_TIMESTAMP_INTERVAL)
 		/* No data received for a while, resync the timestamp. */
 		return 0;
-	else
-		return td->timestamp + delta;
+
+	return td->timestamp + delta;
 }
 
 static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
@@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	 * HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
 	 * collection, but within the report ID.
 	 */
 	if (field->physical == HID_DG_STYLUS)
 		return 0;
-	else if ((field->physical == 0) &&
+
+	if ((field->physical == 0) &&
 		 (field->report->id != td->mt_report_id) &&
 		 (td->mt_report_id != -1))
 		return 0;
 
 	if (field->application == HID_DG_TOUCHSCREEN ||
-- 
2.18.0

[PATCH RESEND 0/4] reduce Surface Pro 3 multitouch jitter

[Joey Pabalinas]

The Surface Pro 3 firmware doesn't reliably send contact lift off
reports nor handle invalid report values gracefully.

To reduce touchscreen input jitter:
  - add MT_QUIRK_NOT_SEEN_MEANS_UP to the MT_CLS_WIN_8
  - drop invalid report values

Patches have been tested on my personal Surface Pro 3 for a
couple months without any problems, as well as being run in my
Arch Linux AUR kernel package [1] without a single complaint
so far.

[1] https://aur.archlinux.org/packages/linux-surfacepro3-git

Joey Pabalinas (4):
  HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  HID: multitouch: drop reports containing invalid values
  HID: multitouch: remove unneeded else conditional cases

 drivers/hid/hid-multitouch.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
2.18.0