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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Giulio Benetti <giulio.benetti@benettiengineering.com>
Cc: "Peter Hutterer" <peter.hutterer@who-t.net>,
	"Rob Herring" <robh@kernel.org>,
	"Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	"Henrik Rydberg" <rydberg@bitmath.org>,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Date: Wed, 14 Apr 2021 10:26:13 -0700	[thread overview]
Message-ID: <YHcltT7HkJoXrX0f@google.com> (raw)
In-Reply-To: <080a71d9-d629-5589-c943-4e65a7f414d8@benettiengineering.com>

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