Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
On Wed, Apr 14, 2021 at 10:26:13AM -0700, Dmitry Torokhov wrote: > Hi Giulio, Peter, > > On Wed, Apr 14, 2021 at 01:22:55PM +0200, Giulio Benetti wrote: > > Hi Peter, Dmitry, > > > > On 4/14/21 8:46 AM, Peter Hutterer wrote: > > > On Tue, Apr 13, 2021 at 10:44:07PM -0700, Dmitry Torokhov wrote: > > > > Hi Giulio, > > > > > > > > On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > > > > > + > > > > > + input_mt_report_pointer_emulation(tsdata->input, true); > > > > > > > > For touchscreens it does not make much sense to report BTN_DOUBLETAP, > > > > BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed > > > > this to > > > > > > > > input_mt_report_pointer_emulation(tsdata->input, false); > > > > > > > > to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. > > > > > > Can you expand on this please, just to make sure I'm not misinterpreting > > > those codes? Those bits are just for how many fingers are down (but > > > without > > > position), dropping those bits means you restrict the device to a pure > > > single-touch screen. Or am I missing something here? > > They are indeed represent number of fingers on the surface. I think I > over-simplified this a bit by saying these events are only for > touchpads, as there is allowance for BTN_TOOL_*TAP in > Documentation/input/multi-touch-protocol.rst for MT devices that may > report more contacts than what they can distinctly track, and it is > not restricted to touchpads (but I believe in reality only used by a > couple of "semi-MT" touchpad drivers). fwiw, almost all touchpads on ps2 use that functionality - they can track 2 touchpoints but *detect* up to 5. There's significant insanity in libinput to deal with that because it is so common :) semi-mt is orthogonal to that, it's the an inability to track two touchpoints correctly (only get top-left and bottom-right, but it's guesswork which finger is in which corner). > What I meant to say that for ST fallback of MT-capable devices we > typically provide BTN_TOOL_*TAP for devices declared as INPUT_MT_POINTER > (which is touchpads) and for INPUT_MT_DIRECT and others we only provide > ABS_X, ABS_Y, ABS_PRESSURE and BTN_TOUCH (see input_mt_sync_frame()), > and I think this driver should follow the suit. ah, right. that makes sense, thanks for the clarification. Cheers, Peter
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Giulio, Peter, On Wed, Apr 14, 2021 at 01:22:55PM +0200, Giulio Benetti wrote: > Hi Peter, Dmitry, > > On 4/14/21 8:46 AM, Peter Hutterer wrote: > > On Tue, Apr 13, 2021 at 10:44:07PM -0700, Dmitry Torokhov wrote: > > > Hi Giulio, > > > > > > On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > > > > + > > > > + input_mt_report_pointer_emulation(tsdata->input, true); > > > > > > For touchscreens it does not make much sense to report BTN_DOUBLETAP, > > > BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed > > > this to > > > > > > input_mt_report_pointer_emulation(tsdata->input, false); > > > > > > to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. > > > > Can you expand on this please, just to make sure I'm not misinterpreting > > those codes? Those bits are just for how many fingers are down (but without > > position), dropping those bits means you restrict the device to a pure > > single-touch screen. Or am I missing something here? They are indeed represent number of fingers on the surface. I think I over-simplified this a bit by saying these events are only for touchpads, as there is allowance for BTN_TOOL_*TAP in Documentation/input/multi-touch-protocol.rst for MT devices that may report more contacts than what they can distinctly track, and it is not restricted to touchpads (but I believe in reality only used by a couple of "semi-MT" touchpad drivers). What I meant to say that for ST fallback of MT-capable devices we typically provide BTN_TOOL_*TAP for devices declared as INPUT_MT_POINTER (which is touchpads) and for INPUT_MT_DIRECT and others we only provide ABS_X, ABS_Y, ABS_PRESSURE and BTN_TOUCH (see input_mt_sync_frame()), and I think this driver should follow the suit. > > I've re-tested the driver after setting input_mt_report_pointer_emulation() > use_count to false. It works correctly with all touch points expected. That > use_count refers to finger count of Touchpad[1]. Also you can see that even > with use_count=false this for loop[2] is entered by counting all the > mt->slots and then input_event() reports all of them. That is not quite true. The loop in question locates the oldest contact still on the surface and MT->ST mapping uses it as the "primary" point for ST events. It is reported in addition to sending all contacts via MT protocol as ABS_MT_SLOT, ABS_MT_POSITION_X, ABS_MT_POSITION_Y, ... > > I hope I've understood correctly :-) > > [1]: > https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L190 > [2]: > https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L208 > > > then again, MT support has been in the kernel for long enough that by now > > everything should understand it, so there's a certain "meh" factor. Yeah, there is that too. Thanks. -- Dmitry
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
On 4/14/21 7:44 AM, Dmitry Torokhov wrote: Hi Giulio, On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: + + input_mt_report_pointer_emulation(tsdata->input, true); For touchscreens it does not make much sense to report BTN_DOUBLETAP, BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed this to input_mt_report_pointer_emulation(tsdata->input, false); to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. Thanks. Thank you, I've re-tested and it works correctly, I've answered to Peter Hutterer with what I've understood about that. Please correct me if I'm wrong. Thanks a lot for reviewing Best regards -- Giulio Benetti Benetti Engineering sas
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Peter, Dmitry, On 4/14/21 8:46 AM, Peter Hutterer wrote: On Tue, Apr 13, 2021 at 10:44:07PM -0700, Dmitry Torokhov wrote: Hi Giulio, On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: + + input_mt_report_pointer_emulation(tsdata->input, true); For touchscreens it does not make much sense to report BTN_DOUBLETAP, BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed this to input_mt_report_pointer_emulation(tsdata->input, false); to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. Can you expand on this please, just to make sure I'm not misinterpreting those codes? Those bits are just for how many fingers are down (but without position), dropping those bits means you restrict the device to a pure single-touch screen. Or am I missing something here? I've re-tested the driver after setting input_mt_report_pointer_emulation() use_count to false. It works correctly with all touch points expected. That use_count refers to finger count of Touchpad[1]. Also you can see that even with use_count=false this for loop[2] is entered by counting all the mt->slots and then input_event() reports all of them. I hope I've understood correctly :-) [1]: https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L190 [2]: https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L208 then again, MT support has been in the kernel for long enough that by now everything should understand it, so there's a certain "meh" factor. Cheers, Peter Best regards -- Giulio Benetti Benetti Engineering sas
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
On Tue, Apr 13, 2021 at 10:44:07PM -0700, Dmitry Torokhov wrote: > Hi Giulio, > > On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > > + > > + input_mt_report_pointer_emulation(tsdata->input, true); > > For touchscreens it does not make much sense to report BTN_DOUBLETAP, > BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed > this to > > input_mt_report_pointer_emulation(tsdata->input, false); > > to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. Can you expand on this please, just to make sure I'm not misinterpreting those codes? Those bits are just for how many fingers are down (but without position), dropping those bits means you restrict the device to a pure single-touch screen. Or am I missing something here? then again, MT support has been in the kernel for long enough that by now everything should understand it, so there's a certain "meh" factor. Cheers, Peter
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Giulio, On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > + > + input_mt_report_pointer_emulation(tsdata->input, true); For touchscreens it does not make much sense to report BTN_DOUBLETAP, BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed this to input_mt_report_pointer_emulation(tsdata->input, false); to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. Thanks. -- Dmitry
