linux-kernel - Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YHcltT7HkJoXrX0f@google.com>
Date:   Wed, 14 Apr 2021 10:26:13 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Giulio Benetti <giulio.benetti@...ettiengineering.com>
Cc:     Peter Hutterer <peter.hutterer@...-t.net>,
        Rob Herring <robh@...nel.org>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        Henrik Rydberg <rydberg@...math.org>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: 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