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

  Some touchscreens provide mechanical overlays with different objects
like buttons or clipped touchscreen surfaces.

In order to support these objects, add a series of helper functions
to the input subsystem to transform them into overlay objects via
device tree nodes.

These overlay objects consume the raw touch events and report the
expected input events depending on the object properties.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 MAINTAINERS                            |   7 +
 drivers/input/touchscreen/Kconfig      |   9 +
 drivers/input/touchscreen/Makefile     |   1 +
 drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
 include/linux/input/ts-overlay.h       |  43 ++++
 5 files changed, 416 insertions(+)
  

Hi Javier,

On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
> Some touchscreens provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.
> 
> In order to support these objects, add a series of helper functions
> to the input subsystem to transform them into overlay objects via
> device tree nodes.
> 
> These overlay objects consume the raw touch events and report the
> expected input events depending on the object properties.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---

Excellent work; it's great to see this series move along.

>  MAINTAINERS                            |   7 +
>  drivers/input/touchscreen/Kconfig      |   9 +
>  drivers/input/touchscreen/Makefile     |   1 +
>  drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
>  include/linux/input/ts-overlay.h       |  43 ++++
>  5 files changed, 416 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7e0b87d5aa2e..db9427926a4c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21434,6 +21434,13 @@ W:	https://github.com/srcres258/linux-doc
>  T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
>  F:	Documentation/translations/zh_TW/
>  
> +TOUCHSCREEN OVERLAY OBJECTS
> +M:	Javier Carrasco <javier.carrasco@wolfvision.net>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +F:	drivers/input/touchscreen/ts-overlay.c
> +F:	include/linux/input/ts-overlay.h
> +
>  TTY LAYER
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 143ff43c67ae..8382a4d68326 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called himax_hx83112b.
>  
> +config TOUCHSCREEN_TS_OVERLAY
> +	bool "Touchscreen Overlay Objects"
> +	help
> +	  Say Y here if you are using a touchscreen driver that supports
> +	  printed overlays with keys or a clipped touchscreen area.
> +
> +	  Should be selected by the touchscren drivers that support
> +	  this feature.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..f554826706ff 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
>  obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B)	+= himax_hx83112b.o
> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY)	+= ts-overlay.o

It seems like this feature is useful for any two-dimensional touch surface
(e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
helpers in touchscreen.c were moved out of input/touchscreen and into input/
such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
of devices in input/misc are taking advantage of these.

Therefore, I think this feature should follow suit and be available to any
input device as is the case for touchscreen.c. As written, the newly updated
binding is misleading because one may believe that any device that includes
touchscreen.yaml can define an overlay child, but the code does not currently
support this.

To that end, it seems like touch-overlay would be a more descriptive name as
well. I understand that the name has changed once already, so hopefully this
feedback is not too annoying :)

> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
> new file mode 100644
> index 000000000000..7afa77d86c1f
> --- /dev/null
> +++ b/drivers/input/touchscreen/ts-overlay.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Helper functions for overlay objects on touchscreens
> + *
> + *  Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#include <linux/property.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +#include <linux/input/ts-overlay.h>
> +
> +enum ts_overlay_valid_objects {
> +	TOUCHSCREEN,
> +	BUTTON,

Please namespace these, i.e. TOUCH_OVERLAY_*.

> +};
> +
> +static const char *const ts_overlay_names[] = {
> +	[TOUCHSCREEN] = "overlay-touchscreen",

I'm a little confused why we need new code for this particular function; it's
what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
can't we keep using those?

> +	[BUTTON] = "overlay-buttons",
> +};
> +
> +struct ts_overlay_shape {
> +	u32 x_origin;
> +	u32 y_origin;
> +	u32 x_size;
> +	u32 y_size;
> +};
> +
> +struct ts_overlay_button {
> +	struct ts_overlay_shape shape;
> +	u32 key;
> +	bool pressed;
> +	int slot;
> +};
> +
> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
> +					   struct ts_overlay_shape *shape)
> +{
> +	int rc;

In input, the common practice is to use 'error' for return values that are either
zero or negative. The reasoning is because the variable quite literally represents
an error, or lack thereof. And then:

	error = ...
	if (error)
		return error;

> +
> +	rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
> +	if (rc < 0)
> +		return rc;

It seems like all of these properties are required; if so, please update the
binding to make this clear.

If the binding is correct and these properties are in fact optional, then you
must evaluate fwnode_property_read_u32() against -EINVAL.

> +
> +	rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static int ts_overlay_get_button_properties(struct device *dev,
> +					    struct fwnode_handle *child_node,
> +					    struct ts_overlay_button *btn)
> +{
> +	struct fwnode_handle *child_btn;
> +	int rc;
> +	int j = 0;
> +
> +	fwnode_for_each_child_node(child_node, child_btn) {
> +		rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
> +		if (rc < 0)
> +			goto button_prop_cleanup;
> +
> +		rc = fwnode_property_read_u32(child_btn, "linux,code",
> +					      &btn[j].key);
> +		if (rc < 0)
> +			goto button_prop_cleanup;

The binding needs to list this property as required, too.

> +
> +		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.

> +
> +static int ts_overlay_count_buttons(struct device *dev)
> +{
> +	struct fwnode_handle *child_node;
> +	struct fwnode_handle *child_button;

These names confused me; they're both children, but only the second is aptly
named. How about child_overlay and child_button, or perhaps overlay_node and
button_node?

> +	int count = 0;
> +
> +	child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
> +	if (!child_node)
> +		return 0;
> +
> +	fwnode_for_each_child_node(child_node, child_button)
> +		count++;
> +	fwnode_handle_put(child_node);
> +
> +	return count;
> +}
> +
> +static int ts_overlay_map_touchscreen(struct device *dev,
> +				      struct ts_overlay_map *map)
> +{
> +	struct fwnode_handle *child;

Same here; there are two layers of children, so please use more descriptive
variable names.

> +	int rc = 0;
> +
> +	child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
> +	if (!child)
> +		goto touchscreen_ret;

I don't think we need a label here; just return 0 directly.

> +
> +	map->touchscreen =
> +		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
> +	if (!map->touchscreen) {
> +		rc = -ENOMEM;
> +		goto touchscreen_handle;
> +	}
> +	rc = ts_overlay_get_shape_properties(child, map->touchscreen);
> +	if (rc < 0)
> +		goto touchscreen_free;
> +
> +	map->overlay_touchscreen = true;
> +	dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
> +		 map->touchscreen->x_origin, map->touchscreen->y_origin,
> +		 map->touchscreen->x_size, map->touchscreen->y_size);

dev_dbg()

> +
> +	rc = 0;

rc (error) can only be zero if we have gotten this far; I don't see a need
to assign it here.

> +	goto touchscreen_handle;

Please think about whether this can be reorganized to prevent jumping over
small bits of code; I found it hard to follow. Maybe one or more tasks can
be offloaded to a helper function?

> +
> +touchscreen_free:
> +	devm_kfree(dev, map->touchscreen);

This set off a red flag; it's unclear that it's necessary. Regardless of
whether the participating driver is smart enough to bail during probe()
if the overlay parsing fails, or it happily continues, this memory will
get freed when the driver tied to 'dev' is torn down.

Calling devm_kfree() is generally limited to special cases where you are
dynamically reallocating memory at runtime. In case I have misunderstood
the intent, please let me know.

> +touchscreen_handle:
> +	fwnode_handle_put(child);
> +touchscreen_ret:
> +	return rc;
> +}
> +
> +static int ts_overlay_map_buttons(struct device *dev,
> +				  struct ts_overlay_map *map,
> +				  struct input_dev *input)
> +{
> +	struct fwnode_handle *child;
> +	u32 button_count;
> +	int rc = 0;
> +
> +	button_count = ts_overlay_count_buttons(dev);
> +	if (button_count) {
> +		map->buttons = devm_kcalloc(dev, button_count,
> +					    sizeof(*map->buttons), GFP_KERNEL);
> +		if (!map->buttons) {
> +			rc = -ENOMEM;
> +			goto map_buttons_ret;
> +		}
> +		child = device_get_named_child_node(dev,
> +						    ts_overlay_names[BUTTON]);
> +		if (unlikely(!child))
> +			goto map_buttons_free;
> +
> +		rc = ts_overlay_get_button_properties(dev, child, map->buttons);
> +		if (rc < 0)
> +			goto map_buttons_free;
> +
> +		map->button_count = button_count;
> +	}
> +
> +	return 0;
> +
> +map_buttons_free:
> +	devm_kfree(dev, map->buttons);
> +map_buttons_ret:
> +	return rc;
> +}
> +
> +static bool ts_overlay_defined_objects(struct device *dev)
> +{
> +	struct fwnode_handle *child;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
> +		child = device_get_named_child_node(dev, ts_overlay_names[i]);
> +		if (child) {
> +			fwnode_handle_put(child);
> +			return true;
> +		}
> +		fwnode_handle_put(child);
> +	}
> +
> +	return false;
> +}
> +
> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> +					      struct input_dev *input)
> +{
> +	struct ts_overlay_map *map = NULL;
> +	int rc;
> +
> +	if (!ts_overlay_defined_objects(dev))
> +		return NULL;
> +
> +	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> +	if (!map) {
> +		rc = -ENOMEM;
> +		goto objects_err;
> +	}
> +	rc = ts_overlay_map_touchscreen(dev, map);
> +	if (rc < 0)
> +		goto objects_free;
> +
> +	rc = ts_overlay_map_buttons(dev, map, input);
> +	if (rc < 0)
> +		goto objects_free;
> +
> +	return map;
> +
> +objects_free:
> +	devm_kfree(dev, map);
> +objects_err:
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL(ts_overlay_map_objects);
> +
> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
> +{
> +	*x = map->touchscreen->x_size - 1;
> +	*y = map->touchscreen->y_size - 1;
> +}
> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
> +
> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
> +{
> +	if (!shape)
> +		return false;
> +
> +	if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
> +	    y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
> +					 u32 *x, u32 *y)
> +{
> +	if (ts_overlay_shape_event(touchscreen, *x, *y)) {
> +		*x -= touchscreen->x_origin;
> +		*y -= touchscreen->y_origin;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
> +{
> +	if (!map || !map->overlay_touchscreen)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
> +
> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
> +{
> +	if (!map || !map->button_count)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
> +
> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
> +{
> +	if (!ts_overlay_mapped_touchscreen(map))
> +		return true;
> +
> +	if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
> +
> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> +			     struct input_dev *input, u32 x, u32 y, u32 slot)
> +{
> +	int i;
> +
> +	if (!ts_overlay_mapped_buttons(map))
> +		return false;
> +
> +	for (i = 0; i < map->button_count; i++) {
> +		if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
> +			input_report_key(input, map->buttons[i].key, 1);
> +			map->buttons[i].pressed = true;
> +			map->buttons[i].slot = slot;
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(ts_overlay_button_press);

The level of abstraction here does not seem quite right. Rather than force
each participating driver to call a press and release function, I think it
is better to expose something like touch_overlay_process_buttons() which
then handles the press and release events internally.

You're also relying on each individual driver to decide whether a touch
coordinate is inside or outside the overlay, and selectively call the press
and release functions OR report coordinates which is non-optimal.

To me, this says we actually need one wrapper function that accepts handles
to both the touchscreen and button input devices (which may be the same at
the driver's discretion) as well as the coordinates. If the coordinate is
within an overlay area, handle press/release; if not, call touchscreen_report_pos().

> +
> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
> +{
> +	int i;
> +
> +	if (!map || !map->button_count)
> +		return false;
> +
> +	for (i = 0; i < map->button_count; i++) {
> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
> +
> +void ts_overlay_button_release(struct ts_overlay_map *map,
> +			       struct input_dev *input, u32 slot)
> +{
> +	int i;
> +
> +	if (!map || !map->button_count)
> +		return;
> +
> +	for (i = 0; i < map->button_count; i++) {
> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
> +			input_report_key(input, map->buttons[i].key, 0);
> +			map->buttons[i].pressed = false;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(ts_overlay_button_release);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
> new file mode 100644
> index 000000000000..b75df0dec3ab
> --- /dev/null
> +++ b/include/linux/input/ts-overlay.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TS_OVERLAY
> +#define _TS_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +struct device;
> +
> +struct ts_overlay_map {
> +	struct ts_overlay_shape *touchscreen;
> +	bool overlay_touchscreen;
> +	struct ts_overlay_button *buttons;
> +	u32 button_count;
> +};
> +
> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> +					      struct input_dev *input);
> +
> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
> +
> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
> +
> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
> +
> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
> +
> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> +			     struct input_dev *input, u32 x, u32 y, u32 slot);
> +
> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
> +
> +void ts_overlay_button_release(struct ts_overlay_map *map,
> +			       struct input_dev *input, u32 slot);
> +
> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> +				struct input_dev *dev);
> +
> +#endif
> 
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy
  

Hi Jeff,

On 26.06.23 04:35, Jeff LaBundy wrote:
> Hi Javier,
> 
> On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
>> Some touchscreens provide mechanical overlays with different objects
>> like buttons or clipped touchscreen surfaces.
>>
>> In order to support these objects, add a series of helper functions
>> to the input subsystem to transform them into overlay objects via
>> device tree nodes.
>>
>> These overlay objects consume the raw touch events and report the
>> expected input events depending on the object properties.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
> 
> Excellent work; it's great to see this series move along.
> 
>>  MAINTAINERS                            |   7 +
>>  drivers/input/touchscreen/Kconfig      |   9 +
>>  drivers/input/touchscreen/Makefile     |   1 +
>>  drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
>>  include/linux/input/ts-overlay.h       |  43 ++++
>>  5 files changed, 416 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7e0b87d5aa2e..db9427926a4c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21434,6 +21434,13 @@ W:	https://github.com/srcres258/linux-doc
>>  T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
>>  F:	Documentation/translations/zh_TW/
>>  
>> +TOUCHSCREEN OVERLAY OBJECTS
>> +M:	Javier Carrasco <javier.carrasco@wolfvision.net>
>> +L:	linux-input@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/input/touchscreen/ts-overlay.c
>> +F:	include/linux/input/ts-overlay.h
>> +
>>  TTY LAYER
>>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>  M:	Jiri Slaby <jirislaby@kernel.org>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 143ff43c67ae..8382a4d68326 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called himax_hx83112b.
>>  
>> +config TOUCHSCREEN_TS_OVERLAY
>> +	bool "Touchscreen Overlay Objects"
>> +	help
>> +	  Say Y here if you are using a touchscreen driver that supports
>> +	  printed overlays with keys or a clipped touchscreen area.
>> +
>> +	  Should be selected by the touchscren drivers that support
>> +	  this feature.
>> +
>>  endif
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..f554826706ff 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
>>  obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B)	+= himax_hx83112b.o
>> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY)	+= ts-overlay.o
> 
> It seems like this feature is useful for any two-dimensional touch surface
> (e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
> helpers in touchscreen.c were moved out of input/touchscreen and into input/
> such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
> of devices in input/misc are taking advantage of these.
> 
> Therefore, I think this feature should follow suit and be available to any
> input device as is the case for touchscreen.c. As written, the newly updated
> binding is misleading because one may believe that any device that includes
> touchscreen.yaml can define an overlay child, but the code does not currently
> support this.
> 
> To that end, it seems like touch-overlay would be a more descriptive name as
> well. I understand that the name has changed once already, so hopefully this
> feedback is not too annoying :)
> 
changing names is no problem at all as long as it makes the feature more
comprehensible and/or takes more suitable devices into account, which
would be the case. So I will move the code from touchscreen to input and
I will update the names and descriptions to make them more generic.

I guess then I will need to move the properties to a separate binding
for this feature because it will not be an addition to the touchscreen
bindings anymore, right?

>> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
>> new file mode 100644
>> index 000000000000..7afa77d86c1f
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/ts-overlay.c
>> @@ -0,0 +1,356 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + *  Helper functions for overlay objects on touchscreens
>> + *
>> + *  Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>> + */
>> +
>> +#include <linux/property.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/module.h>
>> +#include <linux/input/ts-overlay.h>
>> +
>> +enum ts_overlay_valid_objects {
>> +	TOUCHSCREEN,
>> +	BUTTON,
> 
> Please namespace these, i.e. TOUCH_OVERLAY_*.
> 
>> +};
>> +
>> +static const char *const ts_overlay_names[] = {
>> +	[TOUCHSCREEN] = "overlay-touchscreen",
> 
> I'm a little confused why we need new code for this particular function; it's
> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> can't we keep using those?
> 
According to the bindings, touchscreen-min-x/y define the minimum
reported values, but the overlay-touchscreen is actually setting a new
origin. Therefore I might be misusing those properties. On the other
hand touchscreen-size-x/y would make more sense, but I also considered
the case where someone would like to describe the real size of the
touchscreen outside of the overlay node as well as the clipped size
inside the node. In that case using the same property twice would be
confusing.
So in the end I thought that the origin/size properties are more precise
and applicable for all objects and not only the overlay touchscreen.
These properties are needed for the buttons anyways and in the future
more overlay would use the same properties.

>> +	[BUTTON] = "overlay-buttons",
>> +};
>> +
>> +struct ts_overlay_shape {
>> +	u32 x_origin;
>> +	u32 y_origin;
>> +	u32 x_size;
>> +	u32 y_size;
>> +};
>> +
>> +struct ts_overlay_button {
>> +	struct ts_overlay_shape shape;
>> +	u32 key;
>> +	bool pressed;
>> +	int slot;
>> +};
>> +
>> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
>> +					   struct ts_overlay_shape *shape)
>> +{
>> +	int rc;
> 
> In input, the common practice is to use 'error' for return values that are either
> zero or negative. The reasoning is because the variable quite literally represents
> an error, or lack thereof. And then:
> 
> 	error = ...
> 	if (error)
> 		return error;
> 
>> +
>> +	rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
>> +	if (rc < 0)
>> +		return rc;
> 
> It seems like all of these properties are required; if so, please update the
> binding to make this clear.
> 
> If the binding is correct and these properties are in fact optional, then you
> must evaluate fwnode_property_read_u32() against -EINVAL.
> 
If I end up writing new bindings for this feature, it will be more clear
what is required and what not because I will not be part only of the
touchscreen anymore. These properties will be required.
>> +
>> +	rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ts_overlay_get_button_properties(struct device *dev,
>> +					    struct fwnode_handle *child_node,
>> +					    struct ts_overlay_button *btn)
>> +{
>> +	struct fwnode_handle *child_btn;
>> +	int rc;
>> +	int j = 0;
>> +
>> +	fwnode_for_each_child_node(child_node, child_btn) {
>> +		rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
>> +		if (rc < 0)
>> +			goto button_prop_cleanup;
>> +
>> +		rc = fwnode_property_read_u32(child_btn, "linux,code",
>> +					      &btn[j].key);
>> +		if (rc < 0)
>> +			goto button_prop_cleanup;
> 
> The binding needs to list this property as required, too.
> 
Do you mean "linux,code"? It is already listed with the same pattern
that some other bindings use:

linux,code: true

Is that not right? I did not want to redefine an existing property that
other bindings already make use of.
>> +
>> +		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.
> 
>> +
>> +static int ts_overlay_count_buttons(struct device *dev)
>> +{
>> +	struct fwnode_handle *child_node;
>> +	struct fwnode_handle *child_button;
> 
> These names confused me; they're both children, but only the second is aptly
> named. How about child_overlay and child_button, or perhaps overlay_node and
> button_node?
> 
>> +	int count = 0;
>> +
>> +	child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
>> +	if (!child_node)
>> +		return 0;
>> +
>> +	fwnode_for_each_child_node(child_node, child_button)
>> +		count++;
>> +	fwnode_handle_put(child_node);
>> +
>> +	return count;
>> +}
>> +
>> +static int ts_overlay_map_touchscreen(struct device *dev,
>> +				      struct ts_overlay_map *map)
>> +{
>> +	struct fwnode_handle *child;
> 
> Same here; there are two layers of children, so please use more descriptive
> variable names.
> 
>> +	int rc = 0;
>> +
>> +	child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
>> +	if (!child)
>> +		goto touchscreen_ret;
> 
> I don't think we need a label here; just return 0 directly.
> 
>> +
>> +	map->touchscreen =
>> +		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
>> +	if (!map->touchscreen) {
>> +		rc = -ENOMEM;
>> +		goto touchscreen_handle;
>> +	}
>> +	rc = ts_overlay_get_shape_properties(child, map->touchscreen);
>> +	if (rc < 0)
>> +		goto touchscreen_free;
>> +
>> +	map->overlay_touchscreen = true;
>> +	dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
>> +		 map->touchscreen->x_origin, map->touchscreen->y_origin,
>> +		 map->touchscreen->x_size, map->touchscreen->y_size);
> 
> dev_dbg()
> 
>> +
>> +	rc = 0;
> 
> rc (error) can only be zero if we have gotten this far; I don't see a need
> to assign it here.
> 
>> +	goto touchscreen_handle;
> 
> Please think about whether this can be reorganized to prevent jumping over
> small bits of code; I found it hard to follow. Maybe one or more tasks can
> be offloaded to a helper function?
> 
>> +
>> +touchscreen_free:
>> +	devm_kfree(dev, map->touchscreen);
> 
> This set off a red flag; it's unclear that it's necessary. Regardless of
> whether the participating driver is smart enough to bail during probe()
> if the overlay parsing fails, or it happily continues, this memory will
> get freed when the driver tied to 'dev' is torn down.
> 
> Calling devm_kfree() is generally limited to special cases where you are
> dynamically reallocating memory at runtime. In case I have misunderstood
> the intent, please let me know.
> 
Would devm_kfree() not free the memory immediately if the parsing fails,
making it available for any process instead of waiting until the driver
is torn down, which might never happen? Otherwise that chunk of memory
will not be accessible even though it is useless because the operation
failed, right? Or am I missing something?
>> +touchscreen_handle:
>> +	fwnode_handle_put(child);
>> +touchscreen_ret:
>> +	return rc;
>> +}
>> +
>> +static int ts_overlay_map_buttons(struct device *dev,
>> +				  struct ts_overlay_map *map,
>> +				  struct input_dev *input)
>> +{
>> +	struct fwnode_handle *child;
>> +	u32 button_count;
>> +	int rc = 0;
>> +
>> +	button_count = ts_overlay_count_buttons(dev);
>> +	if (button_count) {
>> +		map->buttons = devm_kcalloc(dev, button_count,
>> +					    sizeof(*map->buttons), GFP_KERNEL);
>> +		if (!map->buttons) {
>> +			rc = -ENOMEM;
>> +			goto map_buttons_ret;
>> +		}
>> +		child = device_get_named_child_node(dev,
>> +						    ts_overlay_names[BUTTON]);
>> +		if (unlikely(!child))
>> +			goto map_buttons_free;
>> +
>> +		rc = ts_overlay_get_button_properties(dev, child, map->buttons);
>> +		if (rc < 0)
>> +			goto map_buttons_free;
>> +
>> +		map->button_count = button_count;
>> +	}
>> +
>> +	return 0;
>> +
>> +map_buttons_free:
>> +	devm_kfree(dev, map->buttons);
>> +map_buttons_ret:
>> +	return rc;
>> +}
>> +
>> +static bool ts_overlay_defined_objects(struct device *dev)
>> +{
>> +	struct fwnode_handle *child;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
>> +		child = device_get_named_child_node(dev, ts_overlay_names[i]);
>> +		if (child) {
>> +			fwnode_handle_put(child);
>> +			return true;
>> +		}
>> +		fwnode_handle_put(child);
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>> +					      struct input_dev *input)
>> +{
>> +	struct ts_overlay_map *map = NULL;
>> +	int rc;
>> +
>> +	if (!ts_overlay_defined_objects(dev))
>> +		return NULL;
>> +
>> +	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
>> +	if (!map) {
>> +		rc = -ENOMEM;
>> +		goto objects_err;
>> +	}
>> +	rc = ts_overlay_map_touchscreen(dev, map);
>> +	if (rc < 0)
>> +		goto objects_free;
>> +
>> +	rc = ts_overlay_map_buttons(dev, map, input);
>> +	if (rc < 0)
>> +		goto objects_free;
>> +
>> +	return map;
>> +
>> +objects_free:
>> +	devm_kfree(dev, map);
>> +objects_err:
>> +	return ERR_PTR(rc);
>> +}
>> +EXPORT_SYMBOL(ts_overlay_map_objects);
>> +
>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
>> +{
>> +	*x = map->touchscreen->x_size - 1;
>> +	*y = map->touchscreen->y_size - 1;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
>> +
>> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
>> +{
>> +	if (!shape)
>> +		return false;
>> +
>> +	if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
>> +	    y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
>> +					 u32 *x, u32 *y)
>> +{
>> +	if (ts_overlay_shape_event(touchscreen, *x, *y)) {
>> +		*x -= touchscreen->x_origin;
>> +		*y -= touchscreen->y_origin;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
>> +{
>> +	if (!map || !map->overlay_touchscreen)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
>> +
>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
>> +{
>> +	if (!map || !map->button_count)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
>> +
>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
>> +{
>> +	if (!ts_overlay_mapped_touchscreen(map))
>> +		return true;
>> +
>> +	if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
>> +
>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>> +			     struct input_dev *input, u32 x, u32 y, u32 slot)
>> +{
>> +	int i;
>> +
>> +	if (!ts_overlay_mapped_buttons(map))
>> +		return false;
>> +
>> +	for (i = 0; i < map->button_count; i++) {
>> +		if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
>> +			input_report_key(input, map->buttons[i].key, 1);
>> +			map->buttons[i].pressed = true;
>> +			map->buttons[i].slot = slot;
>> +			return true;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_button_press);
> 
> The level of abstraction here does not seem quite right. Rather than force
> each participating driver to call a press and release function, I think it
> is better to expose something like touch_overlay_process_buttons() which
> then handles the press and release events internally.
> 
> You're also relying on each individual driver to decide whether a touch
> coordinate is inside or outside the overlay, and selectively call the press
> and release functions OR report coordinates which is non-optimal.
> 
> To me, this says we actually need one wrapper function that accepts handles
> to both the touchscreen and button input devices (which may be the same at
> the driver's discretion) as well as the coordinates. If the coordinate is
> within an overlay area, handle press/release; if not, call touchscreen_report_pos().
> 
>> +
>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
>> +{
>> +	int i;
>> +
>> +	if (!map || !map->button_count)
>> +		return false;
>> +
>> +	for (i = 0; i < map->button_count; i++) {
>> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
>> +
>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>> +			       struct input_dev *input, u32 slot)
>> +{
>> +	int i;
>> +
>> +	if (!map || !map->button_count)
>> +		return;
>> +
>> +	for (i = 0; i < map->button_count; i++) {
>> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
>> +			input_report_key(input, map->buttons[i].key, 0);
>> +			map->buttons[i].pressed = false;
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL(ts_overlay_button_release);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
>> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
>> new file mode 100644
>> index 000000000000..b75df0dec3ab
>> --- /dev/null
>> +++ b/include/linux/input/ts-overlay.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>> + */
>> +
>> +#ifndef _TS_OVERLAY
>> +#define _TS_OVERLAY
>> +
>> +#include <linux/types.h>
>> +
>> +struct input_dev;
>> +struct device;
>> +
>> +struct ts_overlay_map {
>> +	struct ts_overlay_shape *touchscreen;
>> +	bool overlay_touchscreen;
>> +	struct ts_overlay_button *buttons;
>> +	u32 button_count;
>> +};
>> +
>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>> +					      struct input_dev *input);
>> +
>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
>> +
>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
>> +
>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
>> +
>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
>> +
>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>> +			     struct input_dev *input, u32 x, u32 y, u32 slot);
>> +
>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
>> +
>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>> +			       struct input_dev *input, u32 slot);
>> +
>> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
>> +				struct input_dev *dev);
>> +
>> +#endif
>>
>> -- 
>> 2.39.2
>>
> 
> Kind regards,
> Jeff LaBundy

Thanks again for your feedback, I really appreciate it. All the comments
without a reply can be taken as acknowledged and accepted as they are
without further discussion. I will work on them for the next version.

Thank you for your time and best regards,
Javier Carrasco
  

Hi Javier,

On Mon, Jun 26, 2023 at 12:31:21PM +0200, Javier Carrasco wrote:
> Hi Jeff,
> 
> On 26.06.23 04:35, Jeff LaBundy wrote:
> > Hi Javier,
> > 
> > On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
> >> Some touchscreens provide mechanical overlays with different objects
> >> like buttons or clipped touchscreen surfaces.
> >>
> >> In order to support these objects, add a series of helper functions
> >> to the input subsystem to transform them into overlay objects via
> >> device tree nodes.
> >>
> >> These overlay objects consume the raw touch events and report the
> >> expected input events depending on the object properties.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> >> ---
> > 
> > Excellent work; it's great to see this series move along.
> > 
> >>  MAINTAINERS                            |   7 +
> >>  drivers/input/touchscreen/Kconfig      |   9 +
> >>  drivers/input/touchscreen/Makefile     |   1 +
> >>  drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
> >>  include/linux/input/ts-overlay.h       |  43 ++++
> >>  5 files changed, 416 insertions(+)
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 7e0b87d5aa2e..db9427926a4c 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -21434,6 +21434,13 @@ W:	https://github.com/srcres258/linux-doc
> >>  T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
> >>  F:	Documentation/translations/zh_TW/
> >>  
> >> +TOUCHSCREEN OVERLAY OBJECTS
> >> +M:	Javier Carrasco <javier.carrasco@wolfvision.net>
> >> +L:	linux-input@vger.kernel.org
> >> +S:	Maintained
> >> +F:	drivers/input/touchscreen/ts-overlay.c
> >> +F:	include/linux/input/ts-overlay.h
> >> +
> >>  TTY LAYER
> >>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>  M:	Jiri Slaby <jirislaby@kernel.org>
> >> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> >> index 143ff43c67ae..8382a4d68326 100644
> >> --- a/drivers/input/touchscreen/Kconfig
> >> +++ b/drivers/input/touchscreen/Kconfig
> >> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
> >>  	  To compile this driver as a module, choose M here: the
> >>  	  module will be called himax_hx83112b.
> >>  
> >> +config TOUCHSCREEN_TS_OVERLAY
> >> +	bool "Touchscreen Overlay Objects"
> >> +	help
> >> +	  Say Y here if you are using a touchscreen driver that supports
> >> +	  printed overlays with keys or a clipped touchscreen area.
> >> +
> >> +	  Should be selected by the touchscren drivers that support
> >> +	  this feature.
> >> +
> >>  endif
> >> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> >> index 159cd5136fdb..f554826706ff 100644
> >> --- a/drivers/input/touchscreen/Makefile
> >> +++ b/drivers/input/touchscreen/Makefile
> >> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
> >>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
> >>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> >>  obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B)	+= himax_hx83112b.o
> >> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY)	+= ts-overlay.o
> > 
> > It seems like this feature is useful for any two-dimensional touch surface
> > (e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
> > helpers in touchscreen.c were moved out of input/touchscreen and into input/
> > such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
> > of devices in input/misc are taking advantage of these.
> > 
> > Therefore, I think this feature should follow suit and be available to any
> > input device as is the case for touchscreen.c. As written, the newly updated
> > binding is misleading because one may believe that any device that includes
> > touchscreen.yaml can define an overlay child, but the code does not currently
> > support this.
> > 
> > To that end, it seems like touch-overlay would be a more descriptive name as
> > well. I understand that the name has changed once already, so hopefully this
> > feedback is not too annoying :)
> > 
> changing names is no problem at all as long as it makes the feature more
> comprehensible and/or takes more suitable devices into account, which
> would be the case. So I will move the code from touchscreen to input and
> I will update the names and descriptions to make them more generic.
> 
> I guess then I will need to move the properties to a separate binding
> for this feature because it will not be an addition to the touchscreen
> bindings anymore, right?

Technically this feature was never bound to touchscreen.yaml in the first place.
touchscreen.yaml defines scalar properties under a parent input device; your new
binding defines a child node and subsequent properties under the same, or another
parent input device.

That said, it is highly unlikely that one would use your feature without also
using the properties from touchscreen.yaml. It is perfectly fine in my opinion,
and perhaps more convenient, to define the overlay child in touchscreen.yaml as
you have done so that binding authors need not reference two files.

I do agree that it seems more natural for code living in input to be bound by
bindings in dt-bindings/input and not dt-bindings/input/touchscreen/, but there
was push back when the same question came up for touchscreen.yaml [1] some time
ago.

[1] https://patchwork.kernel.org/patch/12042037/

> 
> >> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
> >> new file mode 100644
> >> index 000000000000..7afa77d86c1f
> >> --- /dev/null
> >> +++ b/drivers/input/touchscreen/ts-overlay.c
> >> @@ -0,0 +1,356 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + *  Helper functions for overlay objects on touchscreens
> >> + *
> >> + *  Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> >> + */
> >> +
> >> +#include <linux/property.h>
> >> +#include <linux/input.h>
> >> +#include <linux/input/mt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/input/ts-overlay.h>
> >> +
> >> +enum ts_overlay_valid_objects {
> >> +	TOUCHSCREEN,
> >> +	BUTTON,
> > 
> > Please namespace these, i.e. TOUCH_OVERLAY_*.
> > 
> >> +};
> >> +
> >> +static const char *const ts_overlay_names[] = {
> >> +	[TOUCHSCREEN] = "overlay-touchscreen",
> > 
> > I'm a little confused why we need new code for this particular function; it's
> > what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> > can't we keep using those?
> > 
> According to the bindings, touchscreen-min-x/y define the minimum
> reported values, but the overlay-touchscreen is actually setting a new
> origin. Therefore I might be misusing those properties. On the other
> hand touchscreen-size-x/y would make more sense, but I also considered
> the case where someone would like to describe the real size of the
> touchscreen outside of the overlay node as well as the clipped size
> inside the node. In that case using the same property twice would be
> confusing.
> So in the end I thought that the origin/size properties are more precise
> and applicable for all objects and not only the overlay touchscreen.
> These properties are needed for the buttons anyways and in the future
> more overlay would use the same properties.

Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
reported to input but they don't move the origin. I'm aligned with the reason
to introduce this function.

This does beg the question as to whether we need two separate types of children
and related parsing code. Can we not simply have one overlay definition, and
make the decision as to whether we are dealing with a border or virtual button
based on whether 'linux,code' is present?

> 
> >> +	[BUTTON] = "overlay-buttons",
> >> +};
> >> +
> >> +struct ts_overlay_shape {
> >> +	u32 x_origin;
> >> +	u32 y_origin;
> >> +	u32 x_size;
> >> +	u32 y_size;
> >> +};
> >> +
> >> +struct ts_overlay_button {
> >> +	struct ts_overlay_shape shape;
> >> +	u32 key;
> >> +	bool pressed;
> >> +	int slot;
> >> +};
> >> +
> >> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
> >> +					   struct ts_overlay_shape *shape)
> >> +{
> >> +	int rc;
> > 
> > In input, the common practice is to use 'error' for return values that are either
> > zero or negative. The reasoning is because the variable quite literally represents
> > an error, or lack thereof. And then:
> > 
> > 	error = ...
> > 	if (error)
> > 		return error;
> > 
> >> +
> >> +	rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
> >> +	if (rc < 0)
> >> +		return rc;
> > 
> > It seems like all of these properties are required; if so, please update the
> > binding to make this clear.
> > 
> > If the binding is correct and these properties are in fact optional, then you
> > must evaluate fwnode_property_read_u32() against -EINVAL.
> > 
> If I end up writing new bindings for this feature, it will be more clear
> what is required and what not because I will not be part only of the
> touchscreen anymore. These properties will be required.

The question of whether to split the overlay child definition from touchscreen.yaml
is orthogonal to this point. If the code fails in the absence of these properties,
then you must add a "required:" heading within the overlay child node definition to
call out these properties.

> >> +
> >> +	rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
> >> +	if (rc < 0)
> >> +		return rc;
> >> +
> >> +	rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
> >> +	if (rc < 0)
> >> +		return rc;
> >> +
> >> +	rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
> >> +	if (rc < 0)
> >> +		return rc;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int ts_overlay_get_button_properties(struct device *dev,
> >> +					    struct fwnode_handle *child_node,
> >> +					    struct ts_overlay_button *btn)
> >> +{
> >> +	struct fwnode_handle *child_btn;
> >> +	int rc;
> >> +	int j = 0;
> >> +
> >> +	fwnode_for_each_child_node(child_node, child_btn) {
> >> +		rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
> >> +		if (rc < 0)
> >> +			goto button_prop_cleanup;
> >> +
> >> +		rc = fwnode_property_read_u32(child_btn, "linux,code",
> >> +					      &btn[j].key);
> >> +		if (rc < 0)
> >> +			goto button_prop_cleanup;
> > 
> > The binding needs to list this property as required, too.
> > 
> Do you mean "linux,code"? It is already listed with the same pattern
> that some other bindings use:
> 
> linux,code: true
> 
> Is that not right? I did not want to redefine an existing property that
> other bindings already make use of.

These are separate things. You must additionally list 'linux,code' under a
'required:' heading if the code fails without the property defined. If you
adopt the idea above to decide whether we are dealing with a border or button
based on the presence of this property, then it goes back to being optional
of course.

> >> +
> >> +		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.
> > 
> >> +
> >> +static int ts_overlay_count_buttons(struct device *dev)
> >> +{
> >> +	struct fwnode_handle *child_node;
> >> +	struct fwnode_handle *child_button;
> > 
> > These names confused me; they're both children, but only the second is aptly
> > named. How about child_overlay and child_button, or perhaps overlay_node and
> > button_node?
> > 
> >> +	int count = 0;
> >> +
> >> +	child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
> >> +	if (!child_node)
> >> +		return 0;
> >> +
> >> +	fwnode_for_each_child_node(child_node, child_button)
> >> +		count++;
> >> +	fwnode_handle_put(child_node);
> >> +
> >> +	return count;
> >> +}
> >> +
> >> +static int ts_overlay_map_touchscreen(struct device *dev,
> >> +				      struct ts_overlay_map *map)
> >> +{
> >> +	struct fwnode_handle *child;
> > 
> > Same here; there are two layers of children, so please use more descriptive
> > variable names.
> > 
> >> +	int rc = 0;
> >> +
> >> +	child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
> >> +	if (!child)
> >> +		goto touchscreen_ret;
> > 
> > I don't think we need a label here; just return 0 directly.
> > 
> >> +
> >> +	map->touchscreen =
> >> +		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
> >> +	if (!map->touchscreen) {
> >> +		rc = -ENOMEM;
> >> +		goto touchscreen_handle;
> >> +	}
> >> +	rc = ts_overlay_get_shape_properties(child, map->touchscreen);
> >> +	if (rc < 0)
> >> +		goto touchscreen_free;
> >> +
> >> +	map->overlay_touchscreen = true;
> >> +	dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
> >> +		 map->touchscreen->x_origin, map->touchscreen->y_origin,
> >> +		 map->touchscreen->x_size, map->touchscreen->y_size);
> > 
> > dev_dbg()
> > 
> >> +
> >> +	rc = 0;
> > 
> > rc (error) can only be zero if we have gotten this far; I don't see a need
> > to assign it here.
> > 
> >> +	goto touchscreen_handle;
> > 
> > Please think about whether this can be reorganized to prevent jumping over
> > small bits of code; I found it hard to follow. Maybe one or more tasks can
> > be offloaded to a helper function?
> > 
> >> +
> >> +touchscreen_free:
> >> +	devm_kfree(dev, map->touchscreen);
> > 
> > This set off a red flag; it's unclear that it's necessary. Regardless of
> > whether the participating driver is smart enough to bail during probe()
> > if the overlay parsing fails, or it happily continues, this memory will
> > get freed when the driver tied to 'dev' is torn down.
> > 
> > Calling devm_kfree() is generally limited to special cases where you are
> > dynamically reallocating memory at runtime. In case I have misunderstood
> > the intent, please let me know.
> > 
> Would devm_kfree() not free the memory immediately if the parsing fails,
> making it available for any process instead of waiting until the driver
> is torn down, which might never happen? Otherwise that chunk of memory
> will not be accessible even though it is useless because the operation
> failed, right? Or am I missing something?

If the participating driver does not immediately fail to probe (and hence
free all of its device-managed resources) upon failure to parse this or any
other required properties, that is a bug in that driver.

> >> +touchscreen_handle:
> >> +	fwnode_handle_put(child);
> >> +touchscreen_ret:
> >> +	return rc;
> >> +}
> >> +
> >> +static int ts_overlay_map_buttons(struct device *dev,
> >> +				  struct ts_overlay_map *map,
> >> +				  struct input_dev *input)
> >> +{
> >> +	struct fwnode_handle *child;
> >> +	u32 button_count;
> >> +	int rc = 0;
> >> +
> >> +	button_count = ts_overlay_count_buttons(dev);
> >> +	if (button_count) {
> >> +		map->buttons = devm_kcalloc(dev, button_count,
> >> +					    sizeof(*map->buttons), GFP_KERNEL);
> >> +		if (!map->buttons) {
> >> +			rc = -ENOMEM;
> >> +			goto map_buttons_ret;
> >> +		}
> >> +		child = device_get_named_child_node(dev,
> >> +						    ts_overlay_names[BUTTON]);
> >> +		if (unlikely(!child))
> >> +			goto map_buttons_free;
> >> +
> >> +		rc = ts_overlay_get_button_properties(dev, child, map->buttons);
> >> +		if (rc < 0)
> >> +			goto map_buttons_free;
> >> +
> >> +		map->button_count = button_count;
> >> +	}
> >> +
> >> +	return 0;
> >> +
> >> +map_buttons_free:
> >> +	devm_kfree(dev, map->buttons);
> >> +map_buttons_ret:
> >> +	return rc;
> >> +}
> >> +
> >> +static bool ts_overlay_defined_objects(struct device *dev)
> >> +{
> >> +	struct fwnode_handle *child;
> >> +	int i;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
> >> +		child = device_get_named_child_node(dev, ts_overlay_names[i]);
> >> +		if (child) {
> >> +			fwnode_handle_put(child);
> >> +			return true;
> >> +		}
> >> +		fwnode_handle_put(child);
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> >> +					      struct input_dev *input)
> >> +{
> >> +	struct ts_overlay_map *map = NULL;
> >> +	int rc;
> >> +
> >> +	if (!ts_overlay_defined_objects(dev))
> >> +		return NULL;
> >> +
> >> +	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> >> +	if (!map) {
> >> +		rc = -ENOMEM;
> >> +		goto objects_err;
> >> +	}
> >> +	rc = ts_overlay_map_touchscreen(dev, map);
> >> +	if (rc < 0)
> >> +		goto objects_free;
> >> +
> >> +	rc = ts_overlay_map_buttons(dev, map, input);
> >> +	if (rc < 0)
> >> +		goto objects_free;
> >> +
> >> +	return map;
> >> +
> >> +objects_free:
> >> +	devm_kfree(dev, map);
> >> +objects_err:
> >> +	return ERR_PTR(rc);
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_map_objects);
> >> +
> >> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
> >> +{
> >> +	*x = map->touchscreen->x_size - 1;
> >> +	*y = map->touchscreen->y_size - 1;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
> >> +
> >> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
> >> +{
> >> +	if (!shape)
> >> +		return false;
> >> +
> >> +	if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
> >> +	    y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
> >> +		return true;
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
> >> +					 u32 *x, u32 *y)
> >> +{
> >> +	if (ts_overlay_shape_event(touchscreen, *x, *y)) {
> >> +		*x -= touchscreen->x_origin;
> >> +		*y -= touchscreen->y_origin;
> >> +		return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
> >> +{
> >> +	if (!map || !map->overlay_touchscreen)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
> >> +
> >> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
> >> +{
> >> +	if (!map || !map->button_count)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
> >> +
> >> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
> >> +{
> >> +	if (!ts_overlay_mapped_touchscreen(map))
> >> +		return true;
> >> +
> >> +	if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
> >> +
> >> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> >> +			     struct input_dev *input, u32 x, u32 y, u32 slot)
> >> +{
> >> +	int i;
> >> +
> >> +	if (!ts_overlay_mapped_buttons(map))
> >> +		return false;
> >> +
> >> +	for (i = 0; i < map->button_count; i++) {
> >> +		if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
> >> +			input_report_key(input, map->buttons[i].key, 1);
> >> +			map->buttons[i].pressed = true;
> >> +			map->buttons[i].slot = slot;
> >> +			return true;
> >> +		}
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_button_press);
> > 
> > The level of abstraction here does not seem quite right. Rather than force
> > each participating driver to call a press and release function, I think it
> > is better to expose something like touch_overlay_process_buttons() which
> > then handles the press and release events internally.
> > 
> > You're also relying on each individual driver to decide whether a touch
> > coordinate is inside or outside the overlay, and selectively call the press
> > and release functions OR report coordinates which is non-optimal.
> > 
> > To me, this says we actually need one wrapper function that accepts handles
> > to both the touchscreen and button input devices (which may be the same at
> > the driver's discretion) as well as the coordinates. If the coordinate is
> > within an overlay area, handle press/release; if not, call touchscreen_report_pos().
> > 
> >> +
> >> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
> >> +{
> >> +	int i;
> >> +
> >> +	if (!map || !map->button_count)
> >> +		return false;
> >> +
> >> +	for (i = 0; i < map->button_count; i++) {
> >> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot)
> >> +			return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
> >> +
> >> +void ts_overlay_button_release(struct ts_overlay_map *map,
> >> +			       struct input_dev *input, u32 slot)
> >> +{
> >> +	int i;
> >> +
> >> +	if (!map || !map->button_count)
> >> +		return;
> >> +
> >> +	for (i = 0; i < map->button_count; i++) {
> >> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
> >> +			input_report_key(input, map->buttons[i].key, 0);
> >> +			map->buttons[i].pressed = false;
> >> +		}
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_button_release);
> >> +
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
> >> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
> >> new file mode 100644
> >> index 000000000000..b75df0dec3ab
> >> --- /dev/null
> >> +++ b/include/linux/input/ts-overlay.h
> >> @@ -0,0 +1,43 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> >> + */
> >> +
> >> +#ifndef _TS_OVERLAY
> >> +#define _TS_OVERLAY
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +struct input_dev;
> >> +struct device;
> >> +
> >> +struct ts_overlay_map {
> >> +	struct ts_overlay_shape *touchscreen;
> >> +	bool overlay_touchscreen;
> >> +	struct ts_overlay_button *buttons;
> >> +	u32 button_count;
> >> +};
> >> +
> >> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> >> +					      struct input_dev *input);
> >> +
> >> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
> >> +
> >> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
> >> +
> >> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
> >> +
> >> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
> >> +
> >> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> >> +			     struct input_dev *input, u32 x, u32 y, u32 slot);
> >> +
> >> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
> >> +
> >> +void ts_overlay_button_release(struct ts_overlay_map *map,
> >> +			       struct input_dev *input, u32 slot);
> >> +
> >> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> >> +				struct input_dev *dev);
> >> +
> >> +#endif
> >>
> >> -- 
> >> 2.39.2
> >>
> > 
> > Kind regards,
> > Jeff LaBundy
> 
> Thanks again for your feedback, I really appreciate it. All the comments
> without a reply can be taken as acknowledged and accepted as they are
> without further discussion. I will work on them for the next version.

Sure thing! Thank you for your efforts.

> 
> Thank you for your time and best regards,
> Javier Carrasco

Kind regards,
Jeff LaBundy
  

Hi Jeff,

On 26.06.23 15:47, Jeff LaBundy wrote:
> Hi Javier,
> 
> On Mon, Jun 26, 2023 at 12:31:21PM +0200, Javier Carrasco wrote:
>> Hi Jeff,
>>
>> On 26.06.23 04:35, Jeff LaBundy wrote:
>>> Hi Javier,
>>>
>>> On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
>>>> Some touchscreens provide mechanical overlays with different objects
>>>> like buttons or clipped touchscreen surfaces.
>>>>
>>>> In order to support these objects, add a series of helper functions
>>>> to the input subsystem to transform them into overlay objects via
>>>> device tree nodes.
>>>>
>>>> These overlay objects consume the raw touch events and report the
>>>> expected input events depending on the object properties.
>>>>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>>>> ---
>>>
>>> Excellent work; it's great to see this series move along.
>>>
>>>>  MAINTAINERS                            |   7 +
>>>>  drivers/input/touchscreen/Kconfig      |   9 +
>>>>  drivers/input/touchscreen/Makefile     |   1 +
>>>>  drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
>>>>  include/linux/input/ts-overlay.h       |  43 ++++
>>>>  5 files changed, 416 insertions(+)
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 7e0b87d5aa2e..db9427926a4c 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -21434,6 +21434,13 @@ W:	https://github.com/srcres258/linux-doc
>>>>  T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
>>>>  F:	Documentation/translations/zh_TW/
>>>>  
>>>> +TOUCHSCREEN OVERLAY OBJECTS
>>>> +M:	Javier Carrasco <javier.carrasco@wolfvision.net>
>>>> +L:	linux-input@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	drivers/input/touchscreen/ts-overlay.c
>>>> +F:	include/linux/input/ts-overlay.h
>>>> +
>>>>  TTY LAYER
>>>>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>  M:	Jiri Slaby <jirislaby@kernel.org>
>>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>>> index 143ff43c67ae..8382a4d68326 100644
>>>> --- a/drivers/input/touchscreen/Kconfig
>>>> +++ b/drivers/input/touchscreen/Kconfig
>>>> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
>>>>  	  To compile this driver as a module, choose M here: the
>>>>  	  module will be called himax_hx83112b.
>>>>  
>>>> +config TOUCHSCREEN_TS_OVERLAY
>>>> +	bool "Touchscreen Overlay Objects"
>>>> +	help
>>>> +	  Say Y here if you are using a touchscreen driver that supports
>>>> +	  printed overlays with keys or a clipped touchscreen area.
>>>> +
>>>> +	  Should be selected by the touchscren drivers that support
>>>> +	  this feature.
>>>> +
>>>>  endif
>>>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>>>> index 159cd5136fdb..f554826706ff 100644
>>>> --- a/drivers/input/touchscreen/Makefile
>>>> +++ b/drivers/input/touchscreen/Makefile
>>>> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>>>>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>>>>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
>>>>  obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B)	+= himax_hx83112b.o
>>>> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY)	+= ts-overlay.o
>>>
>>> It seems like this feature is useful for any two-dimensional touch surface
>>> (e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
>>> helpers in touchscreen.c were moved out of input/touchscreen and into input/
>>> such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
>>> of devices in input/misc are taking advantage of these.
>>>
>>> Therefore, I think this feature should follow suit and be available to any
>>> input device as is the case for touchscreen.c. As written, the newly updated
>>> binding is misleading because one may believe that any device that includes
>>> touchscreen.yaml can define an overlay child, but the code does not currently
>>> support this.
>>>
>>> To that end, it seems like touch-overlay would be a more descriptive name as
>>> well. I understand that the name has changed once already, so hopefully this
>>> feedback is not too annoying :)
>>>
>> changing names is no problem at all as long as it makes the feature more
>> comprehensible and/or takes more suitable devices into account, which
>> would be the case. So I will move the code from touchscreen to input and
>> I will update the names and descriptions to make them more generic.
>>
>> I guess then I will need to move the properties to a separate binding
>> for this feature because it will not be an addition to the touchscreen
>> bindings anymore, right?
> 
> Technically this feature was never bound to touchscreen.yaml in the first place.
> touchscreen.yaml defines scalar properties under a parent input device; your new
> binding defines a child node and subsequent properties under the same, or another
> parent input device.
> 
> That said, it is highly unlikely that one would use your feature without also
> using the properties from touchscreen.yaml. It is perfectly fine in my opinion,
> and perhaps more convenient, to define the overlay child in touchscreen.yaml as
> you have done so that binding authors need not reference two files.
> 
> I do agree that it seems more natural for code living in input to be bound by
> bindings in dt-bindings/input and not dt-bindings/input/touchscreen/, but there
> was push back when the same question came up for touchscreen.yaml [1] some time
> ago.
> 
> [1] https://patchwork.kernel.org/patch/12042037/
> 
I will move this feature from input/touchscreen to input and add the
object to the input-core as it is done with the touchscreen object,
making it available for the rest of input devices. On the other hand the
bindings will stay where they are inside of touchscreen.yaml as you
suggested.
>>
>>>> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
>>>> new file mode 100644
>>>> index 000000000000..7afa77d86c1f
>>>> --- /dev/null
>>>> +++ b/drivers/input/touchscreen/ts-overlay.c
>>>> @@ -0,0 +1,356 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + *  Helper functions for overlay objects on touchscreens
>>>> + *
>>>> + *  Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>>>> + */
>>>> +
>>>> +#include <linux/property.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/input/mt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/input/ts-overlay.h>
>>>> +
>>>> +enum ts_overlay_valid_objects {
>>>> +	TOUCHSCREEN,
>>>> +	BUTTON,
>>>
>>> Please namespace these, i.e. TOUCH_OVERLAY_*.
>>>
>>>> +};
>>>> +
>>>> +static const char *const ts_overlay_names[] = {
>>>> +	[TOUCHSCREEN] = "overlay-touchscreen",
>>>
>>> I'm a little confused why we need new code for this particular function; it's
>>> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
>>> can't we keep using those?
>>>
>> According to the bindings, touchscreen-min-x/y define the minimum
>> reported values, but the overlay-touchscreen is actually setting a new
>> origin. Therefore I might be misusing those properties. On the other
>> hand touchscreen-size-x/y would make more sense, but I also considered
>> the case where someone would like to describe the real size of the
>> touchscreen outside of the overlay node as well as the clipped size
>> inside the node. In that case using the same property twice would be
>> confusing.
>> So in the end I thought that the origin/size properties are more precise
>> and applicable for all objects and not only the overlay touchscreen.
>> These properties are needed for the buttons anyways and in the future
>> more overlay would use the same properties.
> 
> Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
> reported to input but they don't move the origin. I'm aligned with the reason
> to introduce this function.
> 
> This does beg the question as to whether we need two separate types of children
> and related parsing code. Can we not simply have one overlay definition, and
> make the decision as to whether we are dealing with a border or virtual button
> based on whether 'linux,code' is present?
> 
A single overlay definition would be possible, but in case more objects
are added in the future, looking for single properties and then deciding
what object it is might get messy pretty fast. You could end up needing
a decision tree and the definition in the DT would get more complex.

Now the decision tree is straightforward (linux,code -> button), but
that might not always be the case. In the current implementation there
are well-defined objects and adding a new one will never affect the
parsing of the rest.
Therefore I would like to keep it more readable and easily extendable.
>>
>>>> +	[BUTTON] = "overlay-buttons",
>>>> +};
>>>> +
>>>> +struct ts_overlay_shape {
>>>> +	u32 x_origin;
>>>> +	u32 y_origin;
>>>> +	u32 x_size;
>>>> +	u32 y_size;
>>>> +};
>>>> +
>>>> +struct ts_overlay_button {
>>>> +	struct ts_overlay_shape shape;
>>>> +	u32 key;
>>>> +	bool pressed;
>>>> +	int slot;
>>>> +};
>>>> +
>>>> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
>>>> +					   struct ts_overlay_shape *shape)
>>>> +{
>>>> +	int rc;
>>>
>>> In input, the common practice is to use 'error' for return values that are either
>>> zero or negative. The reasoning is because the variable quite literally represents
>>> an error, or lack thereof. And then:
>>>
>>> 	error = ...
>>> 	if (error)
>>> 		return error;
>>>
>>>> +
>>>> +	rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
>>>> +	if (rc < 0)
>>>> +		return rc;
>>>
>>> It seems like all of these properties are required; if so, please update the
>>> binding to make this clear.
>>>
>>> If the binding is correct and these properties are in fact optional, then you
>>> must evaluate fwnode_property_read_u32() against -EINVAL.
>>>
>> If I end up writing new bindings for this feature, it will be more clear
>> what is required and what not because I will not be part only of the
>> touchscreen anymore. These properties will be required.
> 
> The question of whether to split the overlay child definition from touchscreen.yaml
> is orthogonal to this point. If the code fails in the absence of these properties,
> then you must add a "required:" heading within the overlay child node definition to
> call out these properties.
> 
>>>> +
>>>> +	rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
>>>> +	if (rc < 0)
>>>> +		return rc;
>>>> +
>>>> +	rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
>>>> +	if (rc < 0)
>>>> +		return rc;
>>>> +
>>>> +	rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
>>>> +	if (rc < 0)
>>>> +		return rc;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ts_overlay_get_button_properties(struct device *dev,
>>>> +					    struct fwnode_handle *child_node,
>>>> +					    struct ts_overlay_button *btn)
>>>> +{
>>>> +	struct fwnode_handle *child_btn;
>>>> +	int rc;
>>>> +	int j = 0;
>>>> +
>>>> +	fwnode_for_each_child_node(child_node, child_btn) {
>>>> +		rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
>>>> +		if (rc < 0)
>>>> +			goto button_prop_cleanup;
>>>> +
>>>> +		rc = fwnode_property_read_u32(child_btn, "linux,code",
>>>> +					      &btn[j].key);
>>>> +		if (rc < 0)
>>>> +			goto button_prop_cleanup;
>>>
>>> The binding needs to list this property as required, too.
>>>
>> Do you mean "linux,code"? It is already listed with the same pattern
>> that some other bindings use:
>>
>> linux,code: true
>>
>> Is that not right? I did not want to redefine an existing property that
>> other bindings already make use of.
> 
> These are separate things. You must additionally list 'linux,code' under a
> 'required:' heading if the code fails without the property defined. If you
> adopt the idea above to decide whether we are dealing with a border or button
> based on the presence of this property, then it goes back to being optional
> of course.
> 
As I mentioned above, I will keep the feature documentation in the
touchscreen.yaml as a node and add the 'required' to the properties
needed for the code not to fail.
>>>> +
>>>> +		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.
>>>
>>>> +
>>>> +static int ts_overlay_count_buttons(struct device *dev)
>>>> +{
>>>> +	struct fwnode_handle *child_node;
>>>> +	struct fwnode_handle *child_button;
>>>
>>> These names confused me; they're both children, but only the second is aptly
>>> named. How about child_overlay and child_button, or perhaps overlay_node and
>>> button_node?
>>>
>>>> +	int count = 0;
>>>> +
>>>> +	child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
>>>> +	if (!child_node)
>>>> +		return 0;
>>>> +
>>>> +	fwnode_for_each_child_node(child_node, child_button)
>>>> +		count++;
>>>> +	fwnode_handle_put(child_node);
>>>> +
>>>> +	return count;
>>>> +}
>>>> +
>>>> +static int ts_overlay_map_touchscreen(struct device *dev,
>>>> +				      struct ts_overlay_map *map)
>>>> +{
>>>> +	struct fwnode_handle *child;
>>>
>>> Same here; there are two layers of children, so please use more descriptive
>>> variable names.
>>>
>>>> +	int rc = 0;
>>>> +
>>>> +	child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
>>>> +	if (!child)
>>>> +		goto touchscreen_ret;
>>>
>>> I don't think we need a label here; just return 0 directly.
>>>
>>>> +
>>>> +	map->touchscreen =
>>>> +		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
>>>> +	if (!map->touchscreen) {
>>>> +		rc = -ENOMEM;
>>>> +		goto touchscreen_handle;
>>>> +	}
>>>> +	rc = ts_overlay_get_shape_properties(child, map->touchscreen);
>>>> +	if (rc < 0)
>>>> +		goto touchscreen_free;
>>>> +
>>>> +	map->overlay_touchscreen = true;
>>>> +	dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
>>>> +		 map->touchscreen->x_origin, map->touchscreen->y_origin,
>>>> +		 map->touchscreen->x_size, map->touchscreen->y_size);
>>>
>>> dev_dbg()
>>>
>>>> +
>>>> +	rc = 0;
>>>
>>> rc (error) can only be zero if we have gotten this far; I don't see a need
>>> to assign it here.
>>>
>>>> +	goto touchscreen_handle;
>>>
>>> Please think about whether this can be reorganized to prevent jumping over
>>> small bits of code; I found it hard to follow. Maybe one or more tasks can
>>> be offloaded to a helper function?
>>>
>>>> +
>>>> +touchscreen_free:
>>>> +	devm_kfree(dev, map->touchscreen);
>>>
>>> This set off a red flag; it's unclear that it's necessary. Regardless of
>>> whether the participating driver is smart enough to bail during probe()
>>> if the overlay parsing fails, or it happily continues, this memory will
>>> get freed when the driver tied to 'dev' is torn down.
>>>
>>> Calling devm_kfree() is generally limited to special cases where you are
>>> dynamically reallocating memory at runtime. In case I have misunderstood
>>> the intent, please let me know.
>>>
>> Would devm_kfree() not free the memory immediately if the parsing fails,
>> making it available for any process instead of waiting until the driver
>> is torn down, which might never happen? Otherwise that chunk of memory
>> will not be accessible even though it is useless because the operation
>> failed, right? Or am I missing something?
> 
> If the participating driver does not immediately fail to probe (and hence
> free all of its device-managed resources) upon failure to parse this or any
> other required properties, that is a bug in that driver.
> 
In that case I will remove the call to devm_kfree and trust the
participating drivers.
>>>> +touchscreen_handle:
>>>> +	fwnode_handle_put(child);
>>>> +touchscreen_ret:
>>>> +	return rc;
>>>> +}
>>>> +
>>>> +static int ts_overlay_map_buttons(struct device *dev,
>>>> +				  struct ts_overlay_map *map,
>>>> +				  struct input_dev *input)
>>>> +{
>>>> +	struct fwnode_handle *child;
>>>> +	u32 button_count;
>>>> +	int rc = 0;
>>>> +
>>>> +	button_count = ts_overlay_count_buttons(dev);
>>>> +	if (button_count) {
>>>> +		map->buttons = devm_kcalloc(dev, button_count,
>>>> +					    sizeof(*map->buttons), GFP_KERNEL);
>>>> +		if (!map->buttons) {
>>>> +			rc = -ENOMEM;
>>>> +			goto map_buttons_ret;
>>>> +		}
>>>> +		child = device_get_named_child_node(dev,
>>>> +						    ts_overlay_names[BUTTON]);
>>>> +		if (unlikely(!child))
>>>> +			goto map_buttons_free;
>>>> +
>>>> +		rc = ts_overlay_get_button_properties(dev, child, map->buttons);
>>>> +		if (rc < 0)
>>>> +			goto map_buttons_free;
>>>> +
>>>> +		map->button_count = button_count;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +map_buttons_free:
>>>> +	devm_kfree(dev, map->buttons);
>>>> +map_buttons_ret:
>>>> +	return rc;
>>>> +}
>>>> +
>>>> +static bool ts_overlay_defined_objects(struct device *dev)
>>>> +{
>>>> +	struct fwnode_handle *child;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
>>>> +		child = device_get_named_child_node(dev, ts_overlay_names[i]);
>>>> +		if (child) {
>>>> +			fwnode_handle_put(child);
>>>> +			return true;
>>>> +		}
>>>> +		fwnode_handle_put(child);
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>>>> +					      struct input_dev *input)
>>>> +{
>>>> +	struct ts_overlay_map *map = NULL;
>>>> +	int rc;
>>>> +
>>>> +	if (!ts_overlay_defined_objects(dev))
>>>> +		return NULL;
>>>> +
>>>> +	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
>>>> +	if (!map) {
>>>> +		rc = -ENOMEM;
>>>> +		goto objects_err;
>>>> +	}
>>>> +	rc = ts_overlay_map_touchscreen(dev, map);
>>>> +	if (rc < 0)
>>>> +		goto objects_free;
>>>> +
>>>> +	rc = ts_overlay_map_buttons(dev, map, input);
>>>> +	if (rc < 0)
>>>> +		goto objects_free;
>>>> +
>>>> +	return map;
>>>> +
>>>> +objects_free:
>>>> +	devm_kfree(dev, map);
>>>> +objects_err:
>>>> +	return ERR_PTR(rc);
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_map_objects);
>>>> +
>>>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
>>>> +{
>>>> +	*x = map->touchscreen->x_size - 1;
>>>> +	*y = map->touchscreen->y_size - 1;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
>>>> +
>>>> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
>>>> +{
>>>> +	if (!shape)
>>>> +		return false;
>>>> +
>>>> +	if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
>>>> +	    y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
>>>> +		return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
>>>> +					 u32 *x, u32 *y)
>>>> +{
>>>> +	if (ts_overlay_shape_event(touchscreen, *x, *y)) {
>>>> +		*x -= touchscreen->x_origin;
>>>> +		*y -= touchscreen->y_origin;
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
>>>> +{
>>>> +	if (!map || !map->overlay_touchscreen)
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
>>>> +
>>>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
>>>> +{
>>>> +	if (!map || !map->button_count)
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
>>>> +
>>>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
>>>> +{
>>>> +	if (!ts_overlay_mapped_touchscreen(map))
>>>> +		return true;
>>>> +
>>>> +	if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
>>>> +
>>>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>>>> +			     struct input_dev *input, u32 x, u32 y, u32 slot)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!ts_overlay_mapped_buttons(map))
>>>> +		return false;
>>>> +
>>>> +	for (i = 0; i < map->button_count; i++) {
>>>> +		if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
>>>> +			input_report_key(input, map->buttons[i].key, 1);
>>>> +			map->buttons[i].pressed = true;
>>>> +			map->buttons[i].slot = slot;
>>>> +			return true;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_button_press);
>>>
>>> The level of abstraction here does not seem quite right. Rather than force
>>> each participating driver to call a press and release function, I think it
>>> is better to expose something like touch_overlay_process_buttons() which
>>> then handles the press and release events internally.
>>>
>>> You're also relying on each individual driver to decide whether a touch
>>> coordinate is inside or outside the overlay, and selectively call the press
>>> and release functions OR report coordinates which is non-optimal.
>>>
>>> To me, this says we actually need one wrapper function that accepts handles
>>> to both the touchscreen and button input devices (which may be the same at
>>> the driver's discretion) as well as the coordinates. If the coordinate is
>>> within an overlay area, handle press/release; if not, call touchscreen_report_pos().
>>>
>>>> +
>>>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!map || !map->button_count)
>>>> +		return false;
>>>> +
>>>> +	for (i = 0; i < map->button_count; i++) {
>>>> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot)
>>>> +			return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
>>>> +
>>>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>>>> +			       struct input_dev *input, u32 slot)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!map || !map->button_count)
>>>> +		return;
>>>> +
>>>> +	for (i = 0; i < map->button_count; i++) {
>>>> +		if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
>>>> +			input_report_key(input, map->buttons[i].key, 0);
>>>> +			map->buttons[i].pressed = false;
>>>> +		}
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(ts_overlay_button_release);
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
>>>> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
>>>> new file mode 100644
>>>> index 000000000000..b75df0dec3ab
>>>> --- /dev/null
>>>> +++ b/include/linux/input/ts-overlay.h
>>>> @@ -0,0 +1,43 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>>>> + */
>>>> +
>>>> +#ifndef _TS_OVERLAY
>>>> +#define _TS_OVERLAY
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct input_dev;
>>>> +struct device;
>>>> +
>>>> +struct ts_overlay_map {
>>>> +	struct ts_overlay_shape *touchscreen;
>>>> +	bool overlay_touchscreen;
>>>> +	struct ts_overlay_button *buttons;
>>>> +	u32 button_count;
>>>> +};
>>>> +
>>>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>>>> +					      struct input_dev *input);
>>>> +
>>>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
>>>> +
>>>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
>>>> +
>>>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
>>>> +
>>>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
>>>> +
>>>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>>>> +			     struct input_dev *input, u32 x, u32 y, u32 slot);
>>>> +
>>>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
>>>> +
>>>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>>>> +			       struct input_dev *input, u32 slot);
>>>> +
>>>> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
>>>> +				struct input_dev *dev);
>>>> +
>>>> +#endif
>>>>
>>>> -- 
>>>> 2.39.2
>>>>
>>>
>>> Kind regards,
>>> Jeff LaBundy
>>
>> Thanks again for your feedback, I really appreciate it. All the comments
>> without a reply can be taken as acknowledged and accepted as they are
>> without further discussion. I will work on them for the next version.
> 
> Sure thing! Thank you for your efforts.
> 
>>
>> Thank you for your time and best regards,
>> Javier Carrasco
> 
> Kind regards,
> Jeff LaBundy
Thanks again, I will start working on the next version asap.

Best regards,
Javier Carrasco
  

Hi Javier,

On Wed, Jun 28, 2023 at 08:44:51AM +0200, Javier Carrasco wrote:

[...]

> >>>> +static const char *const ts_overlay_names[] = {
> >>>> +	[TOUCHSCREEN] = "overlay-touchscreen",
> >>>
> >>> I'm a little confused why we need new code for this particular function; it's
> >>> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> >>> can't we keep using those?
> >>>
> >> According to the bindings, touchscreen-min-x/y define the minimum
> >> reported values, but the overlay-touchscreen is actually setting a new
> >> origin. Therefore I might be misusing those properties. On the other
> >> hand touchscreen-size-x/y would make more sense, but I also considered
> >> the case where someone would like to describe the real size of the
> >> touchscreen outside of the overlay node as well as the clipped size
> >> inside the node. In that case using the same property twice would be
> >> confusing.
> >> So in the end I thought that the origin/size properties are more precise
> >> and applicable for all objects and not only the overlay touchscreen.
> >> These properties are needed for the buttons anyways and in the future
> >> more overlay would use the same properties.
> > 
> > Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
> > reported to input but they don't move the origin. I'm aligned with the reason
> > to introduce this function.
> > 
> > This does beg the question as to whether we need two separate types of children
> > and related parsing code. Can we not simply have one overlay definition, and
> > make the decision as to whether we are dealing with a border or virtual button
> > based on whether 'linux,code' is present?
> > 
> A single overlay definition would be possible, but in case more objects
> are added in the future, looking for single properties and then deciding
> what object it is might get messy pretty fast. You could end up needing
> a decision tree and the definition in the DT would get more complex.
> 
> Now the decision tree is straightforward (linux,code -> button), but
> that might not always be the case. In the current implementation there
> are well-defined objects and adding a new one will never affect the
> parsing of the rest.
> Therefore I would like to keep it more readable and easily extendable.

As a potential customer of this feature, I'm ultimately looking to describe
the hardware as succinctly as possible. Currently we have two overlay types,
a border and button(s). The former will never have linux,code defined, while
the latter will. From my naive perspective, it seems redundant to define the
overlay types differently when their properties imply the difference already.

Ultimately it seems we are simply dealing with generic "segments" scattered
throughout a larger touch surface. These segments start to look something
like the following:

struct touch_segment {
	unsigned int x_origin;
	unsigned int y_origin;
	unsigned int x_size;
	unsigned int y_size;
	unsigned int code;
};

You then have one exported function akin to touchscreen_parse_properties() that
simply walks the parent device looking for children named "touch-segment-0",
"touch-segment-1", etc. and parses the five properties, with the fifth (keycode)
being optional.

And then, you have one last exported function akin to touchscreen_report_pos()
that processes the touch coordinates. If the coordinates are in a given segment
and segment->code == KEY_RESERVED (i.e. linux,code was never given), then this
function simply passes the shifted coordinates to touchscreen_report_pos().

If however segment->code != KEY_RESERVED, it calls input_report_key() based on
whether the coordinates are within the segment. If this simplified solution
shrinks the code enough, it may even make sense to keep it in touchscreen.c
which this new feature is so tightly coupled to anyway.

I'm sure the devil is in the details however, and I understand the value in
future-proofing. Can you help me understand a potential future case where this
simplified view would break, and the existing definitions would be better?

Kind regards,
Jeff LaBundy
  

Hi Jeff,

On 29.06.23 05:29, Jeff LaBundy wrote:
> Hi Javier,
> 
> On Wed, Jun 28, 2023 at 08:44:51AM +0200, Javier Carrasco wrote:
> 
> [...]
> 
>>>>>> +static const char *const ts_overlay_names[] = {
>>>>>> +	[TOUCHSCREEN] = "overlay-touchscreen",
>>>>>
>>>>> I'm a little confused why we need new code for this particular function; it's
>>>>> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
>>>>> can't we keep using those?
>>>>>
>>>> According to the bindings, touchscreen-min-x/y define the minimum
>>>> reported values, but the overlay-touchscreen is actually setting a new
>>>> origin. Therefore I might be misusing those properties. On the other
>>>> hand touchscreen-size-x/y would make more sense, but I also considered
>>>> the case where someone would like to describe the real size of the
>>>> touchscreen outside of the overlay node as well as the clipped size
>>>> inside the node. In that case using the same property twice would be
>>>> confusing.
>>>> So in the end I thought that the origin/size properties are more precise
>>>> and applicable for all objects and not only the overlay touchscreen.
>>>> These properties are needed for the buttons anyways and in the future
>>>> more overlay would use the same properties.
>>>
>>> Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
>>> reported to input but they don't move the origin. I'm aligned with the reason
>>> to introduce this function.
>>>
>>> This does beg the question as to whether we need two separate types of children
>>> and related parsing code. Can we not simply have one overlay definition, and
>>> make the decision as to whether we are dealing with a border or virtual button
>>> based on whether 'linux,code' is present?
>>>
>> A single overlay definition would be possible, but in case more objects
>> are added in the future, looking for single properties and then deciding
>> what object it is might get messy pretty fast. You could end up needing
>> a decision tree and the definition in the DT would get more complex.
>>
>> Now the decision tree is straightforward (linux,code -> button), but
>> that might not always be the case. In the current implementation there
>> are well-defined objects and adding a new one will never affect the
>> parsing of the rest.
>> Therefore I would like to keep it more readable and easily extendable.
> 
> As a potential customer of this feature, I'm ultimately looking to describe
> the hardware as succinctly as possible. Currently we have two overlay types,
> a border and button(s). The former will never have linux,code defined, while
> the latter will. From my naive perspective, it seems redundant to define the
> overlay types differently when their properties imply the difference already.
> 
> Ultimately it seems we are simply dealing with generic "segments" scattered
> throughout a larger touch surface. These segments start to look something
> like the following:
> 
> struct touch_segment {
> 	unsigned int x_origin;
> 	unsigned int y_origin;
> 	unsigned int x_size;
> 	unsigned int y_size;
> 	unsigned int code;
> };
> 
> You then have one exported function akin to touchscreen_parse_properties() that
> simply walks the parent device looking for children named "touch-segment-0",
> "touch-segment-1", etc. and parses the five properties, with the fifth (keycode)
> being optional.
> 
> And then, you have one last exported function akin to touchscreen_report_pos()
> that processes the touch coordinates. If the coordinates are in a given segment
> and segment->code == KEY_RESERVED (i.e. linux,code was never given), then this
> function simply passes the shifted coordinates to touchscreen_report_pos().
> 
> If however segment->code != KEY_RESERVED, it calls input_report_key() based on
> whether the coordinates are within the segment. If this simplified solution
> shrinks the code enough, it may even make sense to keep it in touchscreen.c
> which this new feature is so tightly coupled to anyway.
> 
> I'm sure the devil is in the details however, and I understand the value in
> future-proofing. Can you help me understand a potential future case where this
> simplified view would break, and the existing definitions would be better?
> 
> Kind regards,
> Jeff LaBundy

I agree that your approach would reduce the code and then moving this
feature to touchscreen.c would be reasonable. So if in the end that is
the desired solution, I will go for it. But there are some points where
I think the bit of extra code would be worth it.

From a DT perspective, I can imagine some scenarios where a bunch of
segments scattered around would be messy. An example would be a keypad
with let's say N=9 buttons. It could be described easily with a buttons
node and the keys inside. Understanding what the node describes would be
straightforward as well, let alone N being much bigger.
You could argue that the buttons node could have segments inside instead
of buttons, but in the case where a cropped touchscreen is also
described, you would end up with a segment outside the buttons node and
the rest inside. That would reduce the parsing savings. Some labeling
would help in that case, but that would be not as clear as the current
implementation.
There is another point that I will just touch upon because I have no
experience in the matter. I have seen that some keys use the
'linux,input-type' property to define themselves as keys, switches, etc.
If that property or any other that I do not know is necessary for other
implementations, the button object will cover them better than a generic
segment where half of the properties would be meaningless in some
scenarios. Buttons/keys are so ubiquitous that a dedicated object for
them does not look that bad imho.
But as I said, I do not want to make a strong statement here because I
have seen that you maintain several bindings where this properties are
present and I am not the right person to explain that to you... or
actually anyone else out there :)

Talking about the code itself, having a structure for buttons is handy
because you can keep track of the button status (e.g. pressed) and in
the end it is just a child of the base shape that is used for the
overlay touchscreen. The same applies to any function that handles
buttons: they just wrap around the shape functions and add the
button-specific management. So if the parsing is taken aside, the code
does not get much savings from that side and it is again much more
readable and comprehensible.

Thank you for your efforts to improve these patches and the constructive
discussion.

Best regards,
Javier Carrasco
  

Hi Javier,

On Thu, Jun 29, 2023 at 09:53:11AM +0200, Javier Carrasco wrote:
> Hi Jeff,
> 
> On 29.06.23 05:29, Jeff LaBundy wrote:
> > Hi Javier,
> > 
> > On Wed, Jun 28, 2023 at 08:44:51AM +0200, Javier Carrasco wrote:
> > 
> > [...]
> > 
> >>>>>> +static const char *const ts_overlay_names[] = {
> >>>>>> +	[TOUCHSCREEN] = "overlay-touchscreen",
> >>>>>
> >>>>> I'm a little confused why we need new code for this particular function; it's
> >>>>> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> >>>>> can't we keep using those?
> >>>>>
> >>>> According to the bindings, touchscreen-min-x/y define the minimum
> >>>> reported values, but the overlay-touchscreen is actually setting a new
> >>>> origin. Therefore I might be misusing those properties. On the other
> >>>> hand touchscreen-size-x/y would make more sense, but I also considered
> >>>> the case where someone would like to describe the real size of the
> >>>> touchscreen outside of the overlay node as well as the clipped size
> >>>> inside the node. In that case using the same property twice would be
> >>>> confusing.
> >>>> So in the end I thought that the origin/size properties are more precise
> >>>> and applicable for all objects and not only the overlay touchscreen.
> >>>> These properties are needed for the buttons anyways and in the future
> >>>> more overlay would use the same properties.
> >>>
> >>> Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
> >>> reported to input but they don't move the origin. I'm aligned with the reason
> >>> to introduce this function.
> >>>
> >>> This does beg the question as to whether we need two separate types of children
> >>> and related parsing code. Can we not simply have one overlay definition, and
> >>> make the decision as to whether we are dealing with a border or virtual button
> >>> based on whether 'linux,code' is present?
> >>>
> >> A single overlay definition would be possible, but in case more objects
> >> are added in the future, looking for single properties and then deciding
> >> what object it is might get messy pretty fast. You could end up needing
> >> a decision tree and the definition in the DT would get more complex.
> >>
> >> Now the decision tree is straightforward (linux,code -> button), but
> >> that might not always be the case. In the current implementation there
> >> are well-defined objects and adding a new one will never affect the
> >> parsing of the rest.
> >> Therefore I would like to keep it more readable and easily extendable.
> > 
> > As a potential customer of this feature, I'm ultimately looking to describe
> > the hardware as succinctly as possible. Currently we have two overlay types,
> > a border and button(s). The former will never have linux,code defined, while
> > the latter will. From my naive perspective, it seems redundant to define the
> > overlay types differently when their properties imply the difference already.
> > 
> > Ultimately it seems we are simply dealing with generic "segments" scattered
> > throughout a larger touch surface. These segments start to look something
> > like the following:
> > 
> > struct touch_segment {
> > 	unsigned int x_origin;
> > 	unsigned int y_origin;
> > 	unsigned int x_size;
> > 	unsigned int y_size;
> > 	unsigned int code;
> > };
> > 
> > You then have one exported function akin to touchscreen_parse_properties() that
> > simply walks the parent device looking for children named "touch-segment-0",
> > "touch-segment-1", etc. and parses the five properties, with the fifth (keycode)
> > being optional.
> > 
> > And then, you have one last exported function akin to touchscreen_report_pos()
> > that processes the touch coordinates. If the coordinates are in a given segment
> > and segment->code == KEY_RESERVED (i.e. linux,code was never given), then this
> > function simply passes the shifted coordinates to touchscreen_report_pos().
> > 
> > If however segment->code != KEY_RESERVED, it calls input_report_key() based on
> > whether the coordinates are within the segment. If this simplified solution
> > shrinks the code enough, it may even make sense to keep it in touchscreen.c
> > which this new feature is so tightly coupled to anyway.
> > 
> > I'm sure the devil is in the details however, and I understand the value in
> > future-proofing. Can you help me understand a potential future case where this
> > simplified view would break, and the existing definitions would be better?
> > 
> > Kind regards,
> > Jeff LaBundy
> 
> I agree that your approach would reduce the code and then moving this
> feature to touchscreen.c would be reasonable. So if in the end that is
> the desired solution, I will go for it. But there are some points where
> I think the bit of extra code would be worth it.
> 
> From a DT perspective, I can imagine some scenarios where a bunch of
> segments scattered around would be messy. An example would be a keypad
> with let's say N=9 buttons. It could be described easily with a buttons
> node and the keys inside. Understanding what the node describes would be
> straightforward as well, let alone N being much bigger.
> You could argue that the buttons node could have segments inside instead
> of buttons, but in the case where a cropped touchscreen is also
> described, you would end up with a segment outside the buttons node and
> the rest inside. That would reduce the parsing savings. Some labeling
> would help in that case, but that would be not as clear as the current
> implementation.

If we consider your nicely drawn hybrid example with two buttons and a border,
the parent would look something like the following:

touchscreen {
	compatible = "vendor,model";

	overlay-touchscreen {
		x-origin = ...;
		y-origin = ...;
		x-size = ...;
		y-size = ...;
	};

	overlay-buttons {
		button-0 {
			x-origin = ...;
			y-origin = ...;
			x-size = ...;
			y-size = ...;
			linux,code = <KEY_POWER>;
		};

		button-1 {
			x-origin = ...;
			y-origin = ...;
			x-size = ...;
			y-size = ...;
			linux,code = <KEY_INFO>;
		};
	};
};

Is that correct? My first impression was that it seems unnecessary to group
the button-0 and button-1 children under their own 'overlay-buttons' parent,
because there is nothing else inside 'overlay-buttons'. Stated another way,
it seems we are drawing an unnecessary imaginary line around the buttons. It
would be especially confusing for the case where the buttons are scattered
throughout the touch surface and have no logical or physical relationship.

My second impression is that 'overlay-touchscreen' and button-N are nearly
identical save for the 'linux,code' property that can be optional, which makes
it seem like these two should really just be the same node definition. If we
follow my suggestion, the example would collapse as follows:

touchscreen {
        compatible = "vendor,model";

	touch-segment-0 {
		/*
		 * this is the small subset of the entire surface that reports
		 * raw coordinates
		 */
		x-origin = ...;
		y-origin = ...;
		x-size = ...;
		y-size = ...;
	};

	touch-segment-1 {
		x-origin = ...;
		y-origin = ...;
		x-size = ...;
		y-size = ...;
		linux,code = <KEY_POWER>;
	};

	touch-segment-2 {
		x-origin = ...;
		y-origin = ...;
		x-size = ...;
		y-size = ...;
		linux,code = <KEY_INFO>;
	};
};

To me, this seems like a more authentic representation of a monolithic touch
surface. I can't imagine there is not some code savings and simplification to
be had if touch-segment-N can always assume it is a direct descendant of the
parent. In the present implementation, a common property such as 'x-size' may
be one or two generations below the parent depending on the context, which
the code has to keep track of.

> There is another point that I will just touch upon because I have no
> experience in the matter. I have seen that some keys use the
> 'linux,input-type' property to define themselves as keys, switches, etc.
> If that property or any other that I do not know is necessary for other
> implementations, the button object will cover them better than a generic
> segment where half of the properties would be meaningless in some
> scenarios. Buttons/keys are so ubiquitous that a dedicated object for
> them does not look that bad imho.

I think we would simply adopt the same rule I have proposed for 'linux,code',
that is, make these future properties optional and simply omit them in the
case of what is currently defined as 'overlay-touchscreen'. For something
like 'linux,input-type', you would simply declare it as dependening upon
'linux,code' in the binding.

That being said, 'linux,input-type' would not make sense here because we are
simply defining how to interpret momentary touch (touch coordinate or press/
release), so we would never have a switch here. However, nothing in either of
our proposals would prevent it from being used.

> But as I said, I do not want to make a strong statement here because I
> have seen that you maintain several bindings where this properties are
> present and I am not the right person to explain that to you... or
> actually anyone else out there :)

I personally feel my proposal is simpler, but I do not feel strongly about
it. There is nothing functionally incorrect about yours; it merely struck me
as placing some unnecessary burden on the DT author. From my perspective,
"less code is the best code" and we can always add complexity later as unique
use-cases arise.

Since we agree on the majority of the suggestions for v4, maybe a compromise
is to start on those for now, and keep my suggestion in mind as you work
through the non-controversial changes. If you still feel strongly about your
existing structure after that work is complete, then let us keep it.

> 
> Talking about the code itself, having a structure for buttons is handy
> because you can keep track of the button status (e.g. pressed) and in
> the end it is just a child of the base shape that is used for the
> overlay touchscreen. The same applies to any function that handles
> buttons: they just wrap around the shape functions and add the
> button-specific management. So if the parsing is taken aside, the code
> does not get much savings from that side and it is again much more
> readable and comprehensible.

Your module should not be storing button state; that is the job of the input
core. Instead, your module should only be reporting state; the input core
then decides if the state has changed and acts accordingly.

> 
> Thank you for your efforts to improve these patches and the constructive
> discussion.
> 
> Best regards,
> Javier Carrasco

Kind regards,
Jeff LaBundy
  

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
  

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