Re: [PATCH v3 1/4] Input: ts-overlay - Add touchscreen overlay object handling

[Javier Carrasco <javier.carrasco@wolfvision.net>]

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.

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.

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