Message-ID: <ZKYmVpHq1GftUSxf@nixie71>
Date:   Wed, 5 Jul 2023 21:26:30 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Javier Carrasco <javier.carrasco@...fvision.net>
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 v3 1/4] Input: ts-overlay - Add touchscreen overlay
 object handling

Hi Javier,

On Sat, Jul 01, 2023 at 10:58:54PM +0200, Javier Carrasco wrote:
> Hi Jeff,
> 
> On 26.06.23 04:35, Jeff LaBundy wrote:
> >> +
> >> +		dev_info(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
> >> +			 btn[j].shape.x_origin, btn[j].shape.y_origin,
> >> +			 btn[j].shape.x_size, btn[j].shape.y_size, btn[j].key);
> > 
> > This seems like a dev_dbg() to me.
> > 
> >> +		j++;
> >> +	}
> >> +
> >> +	return 0;
> >> +
> >> +button_prop_cleanup:
> >> +	fwnode_handle_put(child_btn);
> >> +	return rc;
> >> +}
> >> +
> >> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> >> +				struct input_dev *dev)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < map->button_count; i++)
> >> +		input_set_capability(dev, EV_KEY, map->buttons[i].key);
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_set_button_caps);
> > 
> > I don't see a need to expose this function and require participating drivers
> > to call it; we should just have one over-arching function for processing the
> > overlay(s), akin to touchscreen_parse_properties but for the button input
> > device in case the driver separates the button and touchscreen input devices.
> > 
> > That one function would then be responsible for parsing the overlay(s) and
> > calling input_set_capability() on each button.
> > 
> I just realized that I did not reply anything about this, even though
> there is a reason why I exposed this function and now that I am working
> on the v4, some clarification might be required.
> 
> After it was decided that this feature should work with two different
> devices (a touchscreen and a keypad), I registered a keypad in the
> st1232.c probe function if overlay buttons were defined. I did not
> register the device inside the new functions because I thought that I
> would be hiding too much magic from the driver.
> 
> Instead I provided a function to check if a keypad was defined and
> another one to set the capabilities (the one we are discussing). The
> driver could just set any parameters it wants before registering the
> device and use this function within that process. The parsing is not
> missing, it is carried out before and the programmer does not need to
> know the key capabilities to call this function.

I looked back at patch [3/4] with these points in mind, but I still feel
there is too much burden placed on the consuming driver. I imagine that
almost every driver would call ts_overlay_set_button_caps() if
ts_overlay_mapped_buttons() returned true; this would result in a lot of
repeated code that your module should simply do on the driver's behalf.

I think modeling after the touchscreen helpers is a good start and would
be most natural for future customers. Where we have this:

void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
				  struct touchscreen_properties *prop)

Perhaps we need something like this:

int touch_overlay_parse_properties(struct input_dev *input,
				   struct list_head overlay_head)

The latter would do the following:

1. Walk the parent node of *input to find each overlay/button ("segment")
2. Allocate sizeof(struct touch_segment)'s worth of memory and add it to
   the linked list
3. Parse the dimensions and keycode (if present)
4. Call input_set_capability() on *input with the given keycode
5. Return to step (2) for all remaining button(s)

There are two problems with this:

1. *input needs to be allocated ahead-of-time, but you don't know whether
   or not you actually needed it until after the function returns.
2. After the function returns, you need to know whether or not the input
   device is empty (i.e. no childen defined); otherwise there is no point
   in registering the second device.

Part (2) seems pretty easy to solve; the consuming driver could simply call
list_empty() on overlay_head and then decide whether to proceed to populate
the rest of the *input struct and register the device.

Perhaps one way to solve part (1) would be to simply expect the consuming
driver to allocate *input ahead-of-time, as is the case for the touchscreen
helpers. If the same call to list_empty() above returns false, the driver
can call devm_free() on it instead of registering it.

Please note that this complexity only exists for the case where the driver
elects to separate the touchscreen and button devices. Both problems go
away for the simple case where the driver clubs all functions into a single
input device.

> 
> So the process is as follows:
> 1.- Map overlay objects if they are defined.
> 2.- If there is a keypad, set the device properties you want it to have
> (name, etc). The event keys were already parsed and can be set with
> touch_overlay_set_button_caps()
> 3.- Register the device whenever and under the circumstances you like.
> 
> That is the current implementation, not necessarily the best one or the
> one the community would prefer.
> If that is preferred, the mapping function could for example register
> the keypad and return a pointer to the keyboard, inferring the device
> properties from the "main" device that will be registered anyways (e.g.
> using its name + "-keypad") or passing them as parameters, which might
> look a bit artificial. In that case the key capabilities would be
> automatically set and this function would not be exposed any more.
> 
> The question is if we would be missing flexibility when setting the
> device properties prior its registration and if the participating
> drivers want this to be done under the hood. My solution is the one I
> found less intrusive for the driver (at the cost of doing the
> registration itself), but if there are good reasons for a different
> implementation, I will be alright with it. If not, the driver will
> control the keypad registration and will use this function to set the
> key caps.

I think we should stick with the existing model where the consuming driver
is responsible for allocating and registering the input device as you have
done; this is the correct and common pattern. touch_overlay_parse_properties()
or its equivalent should not be managing memory or manipulating properties
of *input beyond those it is immediately concerned with (i.e. key reporting
capabilities).

What I do recommend to change, however, is to absorb what is currently called
ts_overlay_set_button_caps() into the existing parsing code. The consuming
driver will always call it if buttons are defined, and the parsing code knows
whether any are defined.

> 
> Sorry for the late reply to this particular point and especially for the
> long text. But a clarification here might save us from another
> discussion later on :)
> 
> Best regards,
> Javier Carrasco

Kind regards,
Jeff LaBundy

Powered by blists - more mailing lists