* [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code @ 2016-07-15 23:26 Ping Cheng 2016-07-15 23:51 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Ping Cheng @ 2016-07-15 23:26 UTC (permalink / raw) To: linux-input, dmitry.torokhov; +Cc: Ping Cheng, Peter Hutterer input_mt_sync_frame is used by other wacom devices in wacom_wac.c to close the frame and emulate pointer events. Let's follow them. Touch events aren't multiplexed over the same device anymore, the use of ABS_MT_TOOL_TYPE is superfluous. Signed-off-by: Ping Cheng <pingc@wacom.com> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> --- v2: moved input_abs_set_res into a separate patch, as suggested by 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 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) } } - /* emulate single touch events when stylus is out of proximity. - * This is to make single touch backward support consistent - * 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; - 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, char *basename, case 5: w8001->pktlen = W8001_PKTLEN_TOUCH2FG; - __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit); error = input_mt_init_slots(dev, 2, 0); if (error) { pr_debug("w8001: failed to initialize MT slots: %d\n", error); @@ -519,8 +510,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename, 0, touch.x, 0, 0); input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, touch.y, 0, 0); - input_set_abs_params(dev, ABS_MT_TOOL_TYPE, - 0, MT_TOOL_MAX, 0, 0); input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res); input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-15 23:26 [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code Ping Cheng @ 2016-07-15 23:51 ` Dmitry Torokhov [not found] ` <CAF8JNhJ4wRnH+f2beOuip=M1cUums7nB-uu6NWTcN91EVTCs+Q@mail.gmail.com> 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-15 23:51 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input, Ping Cheng, Peter Hutterer On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: > input_mt_sync_frame is used by other wacom devices in wacom_wac.c > to close the frame and emulate pointer events. Let's follow them. > > Touch events aren't multiplexed over the same device anymore, the > use of ABS_MT_TOOL_TYPE is superfluous. All of our touchscreen report the finger tool events, even if it is the only "tool" that is being used. Does it cause problems? > > Signed-off-by: Ping Cheng <pingc@wacom.com> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > --- > v2: moved input_abs_set_res into a separate patch, as suggested by > 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 > 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) > } > } > > - /* emulate single touch events when stylus is out of proximity. > - * This is to make single touch backward support consistent > - * 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; > - 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, char *basename, > case 5: > w8001->pktlen = W8001_PKTLEN_TOUCH2FG; > > - __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit); Who is setting this bit then? > error = input_mt_init_slots(dev, 2, 0); > if (error) { > pr_debug("w8001: failed to initialize MT slots: %d\n", error); > @@ -519,8 +510,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename, > 0, touch.x, 0, 0); > input_set_abs_params(dev, ABS_MT_POSITION_Y, > 0, touch.y, 0, 0); > - input_set_abs_params(dev, ABS_MT_TOOL_TYPE, > - 0, MT_TOOL_MAX, 0, 0); > input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res); > input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res); > > -- > 1.8.3.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CAF8JNhJ4wRnH+f2beOuip=M1cUums7nB-uu6NWTcN91EVTCs+Q@mail.gmail.com>]
* Fwd: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code [not found] ` <CAF8JNhJ4wRnH+f2beOuip=M1cUums7nB-uu6NWTcN91EVTCs+Q@mail.gmail.com> @ 2016-07-16 1:56 ` Ping Cheng 2016-07-16 5:33 ` Dmitry Torokhov 1 sibling, 0 replies; 9+ messages in thread From: Ping Cheng @ 2016-07-16 1:56 UTC (permalink / raw) To: linux-input On Friday, July 15, 2016, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: > > input_mt_sync_frame is used by other wacom devices in wacom_wac.c > > to close the frame and emulate pointer events. Let's follow them. > > > > Touch events aren't multiplexed over the same device anymore, the > > use of ABS_MT_TOOL_TYPE is superfluous. > > All of our touchscreen report the finger tool events, even if it is the > only "tool" that is being used. Does it cause problems? No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being set. Testing result shows all touch data are reported whether we explicitly set ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set in 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 interface. That is not true for this series of devices any more.. > > > > > Signed-off-by: Ping Cheng <pingc@wacom.com> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > > --- > > v2: moved input_abs_set_res into a separate patch, as suggested by > > 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 > > 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) > > } > > } > > > > - /* emulate single touch events when stylus is out of proximity. > > - * This is to make single touch backward support consistent > > - * 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; > > - 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, char *basename, > > 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 BTN_TOOL_DOUBLETAP, if necessary. Hope this helps, Ping > > error = input_mt_init_slots(dev, 2, 0); > > if (error) { > > pr_debug("w8001: failed to initialize MT slots: %d\n", error); > > @@ -519,8 +510,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename, > > 0, touch.x, 0, 0); > > input_set_abs_params(dev, ABS_MT_POSITION_Y, > > 0, touch.y, 0, 0); > > - input_set_abs_params(dev, ABS_MT_TOOL_TYPE, > > - 0, MT_TOOL_MAX, 0, 0); > > input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res); > > input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res); > > > > -- > > 1.8.3.1 > > > > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code [not found] ` <CAF8JNhJ4wRnH+f2beOuip=M1cUums7nB-uu6NWTcN91EVTCs+Q@mail.gmail.com> 2016-07-16 1:56 ` Fwd: " Ping Cheng @ 2016-07-16 5:33 ` Dmitry Torokhov 2016-07-16 21:32 ` Ping Cheng 1 sibling, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-16 5:33 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input, Ping Cheng, Peter Hutterer On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: > On Friday, July 15, 2016, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: > > > input_mt_sync_frame is used by other wacom devices in wacom_wac.c > > > to close the frame and emulate pointer events. Let's follow them. > > > > > > Touch events aren't multiplexed over the same device anymore, the > > > use of ABS_MT_TOOL_TYPE is superfluous. > > > > All of our touchscreen report the finger tool events, even if it is the > > only "tool" that is being used. Does it cause problems? > > > > No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being set. > Testing result shows all touch data are reported whether we explicitly set > ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set in > 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 > interface. That is not true for this series of devices any more.. > > > > > > > > Signed-off-by: Ping Cheng <pingc@wacom.com> > > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > > > --- > > > v2: moved input_abs_set_res into a separate patch, as suggested by > > > 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 > > > 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) > > > } > > > } > > > > > > - /* emulate single touch events when stylus is out of proximity. > > > - * This is to make single touch backward support consistent > > > - * 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; > > > - 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, > > char *basename, > > > 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 > BTN_TOOL_DOUBLETAP, if necessary. Doesn't this only happen if you call input_mt_init_slots with INPUT_MT_POINTER flag? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-16 5:33 ` Dmitry Torokhov @ 2016-07-16 21:32 ` Ping Cheng 2016-07-16 21:58 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Ping Cheng @ 2016-07-16 21:32 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, Ping Cheng, Peter Hutterer On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: >> On Friday, July 15, 2016, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: >> >> > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: >> > > input_mt_sync_frame is used by other wacom devices in wacom_wac.c >> > > to close the frame and emulate pointer events. Let's follow them. >> > > >> > > Touch events aren't multiplexed over the same device anymore, the >> > > use of ABS_MT_TOOL_TYPE is superfluous. >> > >> > All of our touchscreen report the finger tool events, even if it is the >> > only "tool" that is being used. Does it cause problems? >> > >> >> No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being set. >> Testing result shows all touch data are reported whether we explicitly set >> ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set in >> 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 >> interface. That is not true for this series of devices any more.. >> >> >> > > >> > > Signed-off-by: Ping Cheng <pingc@wacom.com> >> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >> > > --- >> > > v2: moved input_abs_set_res into a separate patch, as suggested by >> > > 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 >> > > 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) >> > > } >> > > } >> > > >> > > - /* emulate single touch events when stylus is out of proximity. >> > > - * This is to make single touch backward support consistent >> > > - * 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; >> > > - 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, >> > char *basename, >> > > 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 >> 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. Cheers, Ping ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-16 21:32 ` Ping Cheng @ 2016-07-16 21:58 ` Dmitry Torokhov 2016-07-16 22:33 ` Ping Cheng 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-16 21:58 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input, Ping Cheng, Peter Hutterer On July 16, 2016 2:32:27 PM PDT, Ping Cheng <pinglinux@gmail.com> wrote: >On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov ><dmitry.torokhov@gmail.com> wrote: >> On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: >>> On Friday, July 15, 2016, Dmitry Torokhov ><dmitry.torokhov@gmail.com> wrote: >>> >>> > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: >>> > > input_mt_sync_frame is used by other wacom devices in >wacom_wac.c >>> > > to close the frame and emulate pointer events. Let's follow >them. >>> > > >>> > > Touch events aren't multiplexed over the same device anymore, >the >>> > > use of ABS_MT_TOOL_TYPE is superfluous. >>> > >>> > All of our touchscreen report the finger tool events, even if it >is the >>> > only "tool" that is being used. Does it cause problems? >>> > >>> >>> No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being >set. >>> Testing result shows all touch data are reported whether we >explicitly set >>> ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set >in >>> 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 >>> interface. That is not true for this series of devices any more.. >>> >>> >>> > > >>> > > Signed-off-by: Ping Cheng <pingc@wacom.com> >>> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >>> > > --- >>> > > v2: moved input_abs_set_res into a separate patch, as suggested >by >>> > > 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 >>> > > 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) >>> > > } >>> > > } >>> > > >>> > > - /* emulate single touch events when stylus is out of >proximity. >>> > > - * This is to make single touch backward support >consistent >>> > > - * 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; >>> > > - 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, >>> > char *basename, >>> > > 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 >>> 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. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-16 21:58 ` Dmitry Torokhov @ 2016-07-16 22:33 ` Ping Cheng 2016-07-19 18:37 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Ping Cheng @ 2016-07-16 22:33 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, Peter Hutterer On Sat, Jul 16, 2016 at 2:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On July 16, 2016 2:32:27 PM PDT, Ping Cheng <pinglinux@gmail.com> wrote: >>On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov >><dmitry.torokhov@gmail.com> wrote: >>> On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: >>>> On Friday, July 15, 2016, Dmitry Torokhov >><dmitry.torokhov@gmail.com> wrote: >>>> >>>> > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: >>>> > > input_mt_sync_frame is used by other wacom devices in >>wacom_wac.c >>>> > > to close the frame and emulate pointer events. Let's follow >>them. >>>> > > >>>> > > Touch events aren't multiplexed over the same device anymore, >>the >>>> > > use of ABS_MT_TOOL_TYPE is superfluous. >>>> > >>>> > All of our touchscreen report the finger tool events, even if it >>is the >>>> > only "tool" that is being used. Does it cause problems? >>>> > >>>> >>>> No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being >>set. >>>> Testing result shows all touch data are reported whether we >>explicitly set >>>> ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set >>in >>>> 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 >>>> interface. That is not true for this series of devices any more.. >>>> >>>> >>>> > > >>>> > > Signed-off-by: Ping Cheng <pingc@wacom.com> >>>> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >>>> > > --- >>>> > > v2: moved input_abs_set_res into a separate patch, as suggested >>by >>>> > > 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 >>>> > > 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) >>>> > > } >>>> > > } >>>> > > >>>> > > - /* emulate single touch events when stylus is out of >>proximity. >>>> > > - * This is to make single touch backward support >>consistent >>>> > > - * 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; >>>> > > - 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, >>>> > char *basename, >>>> > > 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 >>>> 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? Thank you for your time. Ping ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-16 22:33 ` Ping Cheng @ 2016-07-19 18:37 ` Dmitry Torokhov 2016-07-19 20:07 ` Ping Cheng 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-19 18:37 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input, Peter Hutterer On Sat, Jul 16, 2016 at 03:33:12PM -0700, Ping Cheng wrote: > On Sat, Jul 16, 2016 at 2:58 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On July 16, 2016 2:32:27 PM PDT, Ping Cheng <pinglinux@gmail.com> wrote: > >>On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov > >><dmitry.torokhov@gmail.com> wrote: > >>> On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: > >>>> On Friday, July 15, 2016, Dmitry Torokhov > >><dmitry.torokhov@gmail.com> wrote: > >>>> > >>>> > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: > >>>> > > input_mt_sync_frame is used by other wacom devices in > >>wacom_wac.c > >>>> > > to close the frame and emulate pointer events. Let's follow > >>them. > >>>> > > > >>>> > > Touch events aren't multiplexed over the same device anymore, > >>the > >>>> > > use of ABS_MT_TOOL_TYPE is superfluous. > >>>> > > >>>> > All of our touchscreen report the finger tool events, even if it > >>is the > >>>> > only "tool" that is being used. Does it cause problems? > >>>> > > >>>> > >>>> No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being > >>set. > >>>> Testing result shows all touch data are reported whether we > >>explicitly set > >>>> ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set > >>in > >>>> 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 > >>>> interface. That is not true for this series of devices any more.. > >>>> > >>>> > >>>> > > > >>>> > > Signed-off-by: Ping Cheng <pingc@wacom.com> > >>>> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > >>>> > > --- > >>>> > > v2: moved input_abs_set_res into a separate patch, as suggested > >>by > >>>> > > 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 > >>>> > > 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) > >>>> > > } > >>>> > > } > >>>> > > > >>>> > > - /* emulate single touch events when stylus is out of > >>proximity. > >>>> > > - * This is to make single touch backward support > >>consistent > >>>> > > - * 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; > >>>> > > - 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, > >>>> > char *basename, > >>>> > > 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 > >>>> 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. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code 2016-07-19 18:37 ` Dmitry Torokhov @ 2016-07-19 20:07 ` Ping Cheng 0 siblings, 0 replies; 9+ messages in thread From: Ping Cheng @ 2016-07-19 20:07 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, Peter Hutterer On Tue, Jul 19, 2016 at 11:37 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Sat, Jul 16, 2016 at 03:33:12PM -0700, Ping Cheng wrote: >> On Sat, Jul 16, 2016 at 2:58 PM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > On July 16, 2016 2:32:27 PM PDT, Ping Cheng <pinglinux@gmail.com> wrote: >> >>On Fri, Jul 15, 2016 at 10:33 PM, Dmitry Torokhov >> >><dmitry.torokhov@gmail.com> wrote: >> >>> On Fri, Jul 15, 2016 at 06:19:41PM -0700, Ping Cheng wrote: >> >>>> On Friday, July 15, 2016, Dmitry Torokhov >> >><dmitry.torokhov@gmail.com> wrote: >> >>>> >> >>>> > On Fri, Jul 15, 2016 at 04:26:25PM -0700, Ping Cheng wrote: >> >>>> > > input_mt_sync_frame is used by other wacom devices in >> >>wacom_wac.c >> >>>> > > to close the frame and emulate pointer events. Let's follow >> >>them. >> >>>> > > >> >>>> > > Touch events aren't multiplexed over the same device anymore, >> >>the >> >>>> > > use of ABS_MT_TOOL_TYPE is superfluous. >> >>>> > >> >>>> > All of our touchscreen report the finger tool events, even if it >> >>is the >> >>>> > only "tool" that is being used. Does it cause problems? >> >>>> > >> >>>> >> >>>> No, it does not cause problem, with/without ABS_MT_TOOL_TYPE being >> >>set. >> >>>> Testing result shows all touch data are reported whether we >> >>explicitly set >> >>>> ABS_MT_TOOL_TYPE upfront or not. Plus, ABS_MT_TOOL_TYPE is not set >> >>in >> >>>> 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 >> >>>> interface. That is not true for this series of devices any more.. >> >>>> >> >>>> >> >>>> > > >> >>>> > > Signed-off-by: Ping Cheng <pingc@wacom.com> >> >>>> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >> >>>> > > --- >> >>>> > > v2: moved input_abs_set_res into a separate patch, as suggested >> >>by >> >>>> > > 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 >> >>>> > > 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) >> >>>> > > } >> >>>> > > } >> >>>> > > >> >>>> > > - /* emulate single touch events when stylus is out of >> >>proximity. >> >>>> > > - * This is to make single touch backward support >> >>consistent >> >>>> > > - * 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; >> >>>> > > - 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, >> >>>> > char *basename, >> >>>> > > 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 >> >>>> 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-19 20:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-15 23:26 [PATCH 2/2 v2] input: wacom_w8001 - cleanup 2FG touch code Ping Cheng
2016-07-15 23:51 ` Dmitry Torokhov
[not found] ` <CAF8JNhJ4wRnH+f2beOuip=M1cUums7nB-uu6NWTcN91EVTCs+Q@mail.gmail.com>
2016-07-16 1:56 ` Fwd: " Ping Cheng
2016-07-16 5:33 ` Dmitry Torokhov
2016-07-16 21:32 ` Ping Cheng
2016-07-16 21:58 ` Dmitry Torokhov
2016-07-16 22:33 ` Ping Cheng
2016-07-19 18:37 ` Dmitry Torokhov
2016-07-19 20:07 ` Ping Cheng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.