Message-ID: <a56d5b63-bc21-47b4-bb58-cd7b82163e24@wolfvision.net>
Date:   Tue, 21 Nov 2023 08:23:41 +0100
From:   Javier Carrasco <javier.carrasco@...fvision.net>
To:     Jeff LaBundy <jeff@...undy.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Henrik Rydberg <rydberg@...math.org>,
        Bastian Hecht <hechtb@...il.com>,
        Michael Riesch <michael.riesch@...fvision.net>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen
 and overlay-buttons properties

Hi Jeff,

On 26.10.23 16:46, Jeff LaBundy wrote:
> I am still confused why the buttons need to live under an 'overlay-buttons'
> parent node, which seems like an imaginary boundary. In my view, the touch
> surface comprises the following types of rectangular areas:
> 
> 1. A touchscreen, wherein granular coordinates and pressure are reported.
> 2. A momentary button, wherein pressure is quantized into a binary value
>    (press or release), and coordinates are ignored.
> 
> Any contact that falls outside of (1) and (2) is presumed to be part of a
> border or matting, and is hence ignored.
> 
> Areas (1) and (2) exist in the same "plane", so why can they not reside
> under the same parent node? The following seems much more representative
> of the actual hardware we intend to describe in the device tree:
> 
> 	touchscreen {
> 		compatible = "...";
> 		reg = <...>;
> 
> 		/* raw coordinates reported here */
> 		touch-area-1 {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 		};
> 
> 		/* a button */
> 		touch-area-2a {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 			linux,code = <KEY_POWER>;
> 		};
> 
> 		/* another button */
> 		touch-area-2b {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 			linux,code = <KEY_INFO>;
> 		};
> 	};
> 
> With this method, the driver merely stores a list head. The parsing code
> then walks the client device node; for each touch* child encountered, it
> allocates memory for a structure of five members, and adds it to the list.
> 
> The event handling code then simply iterates through the list and checks
> if the coordinates reported by the hardware fall within each rectangle. If
> so, and the keycode in the list element is equal to KEY_RESERVED (zero),
> we assume the rectangle is of type (1); the coordinates are passed to
> touchscreen_report_pos() and the pressure is reported as well.
> 
> If the keycode is not equal to KEY_RESERVED (e.g. KEY_POWER), we assume
> the rectangle is of type (2); input_report_key() is called instead and
> the coordinates are ignored altogether.
> 
> Instead, the existing implementation has two separate structures, one of
> which inherits the other. Its corresponding properties are then parsed in
> a separate function to account for the fact that the two structures exist
> at different layers in the heirarchy.
> 
> My argument is that all nodes can be parsed in the same way, without having
> to understand whether they represent case (1) or (2). The existing method
> also has to count the nodes; this would not be necessary with a linked list.
> 
> Can you help me understand why the buttons must be "wrapped" in their own
> node? As I mention in v2, this isn't a deal breaker necessarily, but I'd
> like to understand the reasoning behind what I interpret as additional
> complexity, and a degree of separation from a natural description of the
> touch surface.
> 
> The only reason I can think to insert the 'overlay-buttons' parent is in
> case we want to specify a property within this node that applies to all
> buttons, but this does not exist in the current implementation, nor do I
> see a compelling reason to do so in the future.
> 
> Kind regards,
> Jeff LaBundy

I was about to send a gentle ping when I saw that you have reviewed the
last version almost a month ago. Somehow I overlooked your emails, sorry
for the late reply.

As I said in a previous discussion, there is no unavoidable reason why
the buttons should have their own node. I just wanted to make clear that
there are different objects with different properties and in case of
some new appear, there is no need to check several properties to build a
decision tree. Moreover, the device tree is self-documented and even if
you never saw this feature before, there is no need to explain anything:
every object type has its node and the boundary I would be drawing would
be logical, not physical.

On the other hand, with the current implementation a single keycode
property is enough to tell what object is being handled as there are
only two types of objects. If some new objects appear and the decision
tree ends up getting too complex, some other solution might be more
suitable. But until that happens (if ever), I can give up on the button
nodes and simplify the code with a list head.

I will work on that approach for v7. This will require some major
modifications in the code and the bindings, so it will take some time
until the next version is ready and gets proper testing.

Your comments on 1/4 do not require further discussion and will be
applied as well.

Thanks again for your thorough review and enriching feedback.

Best regards,
Javier Carrasco

Powered by blists - more mailing lists