mbox series

[v7,0/3] Input: add Hycon HY46XX Touchscreen controller

Message ID 20210413144446.2277817-1-giulio.benetti@benettiengineering.com
Headers show
Series Input: add Hycon HY46XX Touchscreen controller | expand

Message

Giulio Benetti April 13, 2021, 2:44 p.m. UTC
This patchset adds Hycon vendor, HY46XX touchscreen controller driver
and its .yaml binding.

---
V1->V2:
* changed authorship and SoBs to @benettiengineering.com domain
* fixed vendor commit log according to Jonathan Neuschäfer's suggestion
* fixed hy46xx bindings according to Rob Herring's suggestions
* fixed hy46xx driver according to Dmitry Torokhov's suggestions
further details are listed in single patches
V2->V3:
* fixed hy46xx bindings according to Jonathan Neuschäfer's suggestion
* fixed hy46xx driver according to Jonathan Neuschäfer's suggestion
further details are listed in single patches
V3->V4:
* fixed binding compatible string as suggested by Jonathan Neuschäfer
V4->V5:
* fixed hy46xx bindings and driver according to Rob Herring's suggestions
further details are listed in single patches
V5->V6:
* changed report-speed property name into report-speed-hz according to
Rob Herring's suggestion
V6->V7:
* added missing patch to series
---

Giulio Benetti (3):
  dt-bindings: Add Hycon Technology vendor prefix
  dt-bindings: touchscreen: Add HY46XX bindings
  Input: add driver for the Hycon HY46XX touchpanel series

 .../input/touchscreen/hycon,hy46xx.yaml       | 119 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/input/touchscreen/Kconfig             |  11 +
 drivers/input/touchscreen/Makefile            |   1 +
 drivers/input/touchscreen/hycon-hy46xx.c      | 591 ++++++++++++++++++
 6 files changed, 731 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
 create mode 100644 drivers/input/touchscreen/hycon-hy46xx.c

Comments

Dmitry Torokhov April 14, 2021, 5:44 a.m. UTC | #1
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 Torokhov April 14, 2021, 5:44 a.m. UTC | #2
On Tue, Apr 13, 2021 at 04:44:44PM +0200, Giulio Benetti wrote:
> Update Documentation/devicetree/bindings/vendor-prefixes.yaml to

> include "hycon" as a vendor prefix for "Hycon Technology".

> Company website: https://www.hycontek.com/

> 

> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>

> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

> Acked-by: Rob Herring <robh@kernel.org>


Applied, thank you.

-- 
Dmitry
Peter Hutterer April 14, 2021, 6:46 a.m. UTC | #3
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
Giulio Benetti April 14, 2021, 11:22 a.m. UTC | #4
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
Giulio Benetti April 14, 2021, 11:24 a.m. UTC | #5
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
Dmitry Torokhov April 14, 2021, 5:26 p.m. UTC | #6
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.
Peter Hutterer April 15, 2021, 6:16 a.m. UTC | #7
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