Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code

From: Ping Cheng <hidden>
Date: 2016-07-19 20:07:32

On Tue, Jul 19, 2016 at 11:37 AM, Dmitry Torokhov
[off-list ref] wrote:

On Sat, Jul 16, 2016 at 03:33:12PM -0700, Ping Cheng wrote:

quoted

On Sat, Jul 16, 2016 at 2:58 PM, Dmitry Torokhov
[off-list ref] wrote:

quoted

On July 16, 2016 2:32:27 PM PDT, Ping Cheng [off-list ref] wrote:

quoted

On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov
[off-list ref] wrote:

quoted

On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote:

quoted

On Friday, July 15, 2016, Dmitry Torokhov

[off-list ref] wrote:

quoted

On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote:

quoted

input_mt_sync_frame is used by other wacom devices in

wacom_wac.c

quoted

to close the frame and emulate pointer events. Let's follow

them.

quoted

Touch events aren't multiplexed over the same device anymore,

the

quoted

use of ABS_MT_TOOL_TYPE is superfluous.

All of our touchscreen report the finger tool events, even if it

is the

quoted

only "tool" that is being used. Does it cause problems?

No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being

set.

quoted

Testing result shows all touch data are reported whether we

explicitly set

quoted

ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set

in

quoted

wacom.ko. We'd like to be in sync with wacom.ko.

The following comments for input_mt_report_slot_state in input-mt.c
explains the reason:

"* Reports a contact via ABS_MT_TRACKING_ID, and optionally
 * ABS_MT_TOOL_TYPE. If active is true and the slot is currently
 * inactive, or if the tool type is changed, a new tracking id is
 * assigned to the slot. The tool type is only reported if the
 * corresponding absbit field is set."

MT_TOOL_TYPE is necessary if we report both pen and touch on the

same

quoted

interface. That is not true for this series of devices any more..

quoted

Signed-off-by: Ping Cheng <redacted>
Signed-off-by: Peter Hutterer <redacted>
---
v2: moved input_abs_set_res into a separate patch, as suggested

by

quoted

Dmitry.
---
 drivers/input/touchscreen/wacom_w8001.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c

b/drivers/input/touchscreen/wacom_w8001.c

quoted

index 7e807af..541a8df 100644

--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c

@@ -170,16 +170,8 @@ static void parse_multi_touch(struct w8001

*w8001)

quoted

              }
      }

-     /* emulate single touch events when stylus is out of

proximity.

quoted

-      * This is to make single touch backward support

consistent

quoted

-      * across all Wacom single touch devices.
-      */
-     if (w8001->type != BTN_TOOL_PEN &&
-                         w8001->type != BTN_TOOL_RUBBER) {
-             w8001->type = count == 1 ? BTN_TOOL_FINGER :

KEY_RESERVED;

quoted

-             input_mt_report_pointer_emulation(dev, true);
-     }
-
+     w8001->type = KEY_RESERVED;
+     input_mt_sync_frame(dev);
      input_sync(dev);
 }

@@ -508,7 +500,6 @@ static int w8001_setup_touch(struct w8001

*w8001,

quoted

char *basename,

quoted

      case 5:
              w8001->pktlen = W8001_PKTLEN_TOUCH2FG;

-             __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);

Who is setting this bit then?

All MT related bits are set by input_mt_init_slots in input-mt.c,

including

quoted

BTN_TOOL_DOUBLETAP, if necessary.

Doesn't this only happen if you call input_mt_init_slots with
INPUT_MT_POINTER flag?

You are right. And that is what we want this driver to do - to be in
sync with wacom.ko and rely on input-mt for common actions.

But unless I missed some other patch from you this driver passes 0 as flags to input_mt_init_slots() and by removing the double-tap bit we are breaking the driver.

Oh, I see where you are ;). I assumed INPUT_MT_DIRECT was there. My
bad eyes ;-)!  We'd be better to change

input_mt_init_slots(dev, 2, 0);

to

input_mt_init_slots(dev, 2, INPUT_MT_DIRECT);

Do you want me to submit a new version or can you update it locally?

Ping,

If you look at input_mt_init_slots() we only set the doubletap bit
if we are using INPUT_MT_POINTER, but you are asking me to set
INPUT_MT_DIRECT (which is correct property given that we are dealing
with the touchscreen). Also, input_mt_sync_frame() passes use_count ==
true to input_mt_report_pointer_emulation() (which would enable
reporting of <n>-tap events) only if device has INPUT_MT_POINTER
property.

The while premise of this patch seems wrong. wacom.ko deals with
tablets, wacom_w8001 is for a touchscreen. They behave differently.

I see your point and I understand your concern. But, wacom.ko also
supports touchscreen under USB connection. It supports display tablets
with touch as well, which are direct touch devices (in absolute
movement). Those two types of tablets are initialized with
INPUT_MT_DIRECT in wacom.ko.

There was a bit of history in the idea of only emulating pointer
events for INPUT_MT_POINTR. Those devices are in relative
movement/mode. When we started to support type B MT in kernel 2.6.38,
we treated both absolute and relative mode devices the same. That is,
both types of devices would report emulated pointer events. At that
time, most of the common code in input-mt.c now were in individual
drivers.

Later in kernel 3.7, when we tried to unify the common code, we
realized that direct touch should be treated differently. That's why
and when pointer emulation was disabled for direct touch.

However, not all drivers were updated to fully utilize the unified
routines. I submitted two patches [1] [2] in 2012 to bring wacom.ko in
sync with input-mt.c. By default, Henrik made the routine backward
compatible with 0 as the value for the new entry of
input_mt_init_slots. So, unless someone explicitly updated a driver,
the last entry of input_mt_init_slots would be 0, by default. That is
where wacom_w8001.c stays.

I would have kept wacom_w8001.c as is if it didn't have the real issue
in patch 1. Since I was on the page, it felt bad for not cleaning it
up (another OCD symptom ;-).

I am fine if we don't merge this patch.

Thank you.

Ping

[1] https://sourceforge.net/p/linuxwacom/input-wacom/ci/dff630c2ccb4a91bebbd8ddf181f8b549d63bccb/

[2] https://sourceforge.net/p/linuxwacom/input-wacom/ci/f1467294120ba8d0c10c2aa6d05272dfdbc1cef0/

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help