[PATCH v5 0/4] Input: support overlay objects on touchscreens

[PATCH v5 0/4] Input: support overlay objects on touchscreens

[Javier Carrasco]

Some touchscreens are shipped with a physical layer on top of them where
a number of buttons and a resized touchscreen surface might be available.

In order to generate proper key events by overlay buttons and adjust the
touch events to a clipped surface, these patches offer a documented,
device-tree-based solution by means of helper functions.
An implementation for a specific touchscreen driver is also included.

The functions in ts-overlay provide a simple workflow to acquire
physical objects from the device tree, map them into the device driver
structures as overlay objects and generate events according to
the object descriptions.

This feature has been tested with a JT240MHQS-E3 display, which consists
of an st1624 as the base touchscreen and an overlay with two buttons and
a frame that clips its effective surface mounted on it.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Changes in v5:
- touchscreen bindings: move overlay common properties to a $def entry (Rob Herring)
- st1232 bindings: move overlays to the existing example instead of
  making a new one (Rob Herring) 
- Link to v4: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v4-0-5c6c0fc1eed6@wolfvision.net

Changes in v4:
- General: rename "touchscreen" to "touch" to include other consumers.
- PATCH 1/4: move touch-overlay feature to input core.
- PATCH 1/4, 3/4: set key caps and report key events without consumer's
  intervention.
- PATCH 2/4: add missing 'required' field with the required properties.
- Link to v3: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v3-0-b4fb7fc4bab7@wolfvision.net

Changes in v3:
- General: rename "virtobj" and "virtual" to "overlay"
- PATCH 1/4: Make feature bool instead of tristate (selected by
  supported touchscreens)
- Link to v2: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v2-0-f68a6bfe7a0f@wolfvision.net

Changes in v2:
- PATCH 1/4: remove preprocessor directives (the module is selected by
  the drivers that support the feature). Typo in the commit message.
- PATCH 2/4: more detailed documentation. Images and examples were added.
- PATCH 3/4: select ts-virtobj automatically.
- Link to v1: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v1-0-5ae5e81bc264@wolfvision.net

---
Javier Carrasco (4):
      Input: touch-overlay - Add touchscreen overlay object handling
      dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
      Input: st1232 - add touch overlays handling
      dt-bindings: input: touchscreen: st1232: add touch-overlay example

 .../input/touchscreen/sitronix,st1232.yaml         |  28 ++
 .../bindings/input/touchscreen/touchscreen.yaml    | 143 ++++++++
 MAINTAINERS                                        |   7 +
 drivers/input/Makefile                             |   2 +-
 drivers/input/touch-overlay.c                      | 399 +++++++++++++++++++++
 drivers/input/touchscreen/st1232.c                 |  70 +++-
 include/linux/input/touch-overlay.h                |  34 ++
 7 files changed, 668 insertions(+), 15 deletions(-)
---
base-commit: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
change-id: 20230510-feature-ts_virtobj_patch-e267540aae74

Best regards,
-- 
Javier Carrasco <javier.carrasco@wolfvision.net>

[PATCH v5 1/4] Input: touch-overlay - Add touchscreen overlay object handling

[Javier Carrasco]

Some touch devices 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/Makefile              |   2 +-
 drivers/input/touch-overlay.c       | 399 ++++++++++++++++++++++++++++++++++++
 include/linux/input/touch-overlay.h |  34 +++
 4 files changed, 441 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7a7bd8bd80e9..00c03824c3ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21884,6 +21884,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/
 
+TOUCH OVERLAY OBJECTS
+M:	Javier Carrasco <javier.carrasco@wolfvision.net>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/input/touch-overlay.c
+F:	include/linux/input/touch-overlay.h
+
 TTY LAYER AND SERIAL DRIVERS
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 M:	Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index c78753274921..393e9f4d00dc 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -7,7 +7,7 @@
 
 obj-$(CONFIG_INPUT)		+= input-core.o
 input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
-input-core-y += touchscreen.o
+input-core-y += touchscreen.o touch-overlay.o
 
 obj-$(CONFIG_INPUT_FF_MEMLESS)	+= ff-memless.o
 obj-$(CONFIG_INPUT_SPARSEKMAP)	+= sparse-keymap.o
diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
new file mode 100644
index 000000000000..007dbd994474
--- /dev/null
+++ b/drivers/input/touch-overlay.c
@@ -0,0 +1,399 @@
+// 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/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touch-overlay.h>
+#include <linux/module.h>
+#include <linux/property.h>
+
+enum touch_overlay_valid_objects {
+	TOUCH_OVERLAY_TS,
+	TOUCH_OVERLAY_BTN,
+};
+
+static const char *const object_names[] = {
+	[TOUCH_OVERLAY_TS] = "overlay-touchscreen",
+	[TOUCH_OVERLAY_BTN] = "overlay-buttons",
+};
+
+struct touch_overlay_segment {
+	u32 x_origin;
+	u32 y_origin;
+	u32 x_size;
+	u32 y_size;
+};
+
+struct touch_overlay_button {
+	struct touch_overlay_segment segment;
+	u32 key;
+	bool pressed;
+	int slot;
+};
+
+static int touch_overlay_get_segment_props(struct fwnode_handle *segment_node,
+					   struct touch_overlay_segment *segment)
+{
+	int error;
+
+	error = fwnode_property_read_u32(segment_node, "x-origin",
+					 &segment->x_origin);
+	if (error < 0)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "y-origin",
+					 &segment->y_origin);
+	if (error < 0)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "x-size",
+					 &segment->x_size);
+	if (error < 0)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "y-size",
+					 &segment->y_size);
+	if (error < 0)
+		return error;
+
+	return 0;
+}
+
+static int
+touch_overlay_get_button_properties(struct device *dev,
+				    struct fwnode_handle *overlay_node,
+				    struct touch_overlay_button *btn)
+{
+	struct fwnode_handle *btn_node;
+	int error;
+	int j = 0;
+
+	fwnode_for_each_child_node(overlay_node, btn_node) {
+		error = touch_overlay_get_segment_props(btn_node,
+							&btn[j].segment);
+		if (error < 0)
+			goto button_put;
+
+		error = fwnode_property_read_u32(btn_node, "linux,code",
+						 &btn[j].key);
+		if (error < 0)
+			goto button_put;
+
+		dev_dbg(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
+			btn[j].segment.x_origin, btn[j].segment.y_origin,
+			btn[j].segment.x_size, btn[j].segment.y_size, btn[j].key);
+		j++;
+	}
+
+	return 0;
+
+button_put:
+	fwnode_handle_put(btn_node);
+	return error;
+}
+
+static void touch_overlay_set_button_caps(struct touch_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);
+}
+
+static int touch_overlay_count_buttons(struct device *dev)
+{
+	struct fwnode_handle *overlay;
+	struct fwnode_handle *button;
+	int count = 0;
+
+	overlay = device_get_named_child_node(dev,
+					      object_names[TOUCH_OVERLAY_BTN]);
+	if (!overlay)
+		return 0;
+
+	fwnode_for_each_child_node(overlay, button)
+		count++;
+	fwnode_handle_put(overlay);
+
+	return count;
+}
+
+static int touch_overlay_map_touchscreen(struct device *dev,
+					 struct touch_overlay_map *map)
+{
+	struct fwnode_handle *ts_node;
+	int error = 0;
+
+	ts_node = device_get_named_child_node(dev,
+					      object_names[TOUCH_OVERLAY_TS]);
+	if (!ts_node)
+		return 0;
+
+	map->touchscreen =
+		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
+	if (!map->touchscreen) {
+		error = -ENOMEM;
+		goto handle_put;
+	}
+	error = touch_overlay_get_segment_props(ts_node, map->touchscreen);
+	if (error < 0)
+		goto handle_put;
+
+	map->overlay_touchscreen = true;
+	dev_dbg(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);
+
+handle_put:
+	fwnode_handle_put(ts_node);
+
+	return error;
+}
+
+static int touch_overlay_map_buttons(struct device *dev,
+				     struct touch_overlay_map *map)
+{
+	struct fwnode_handle *button;
+	u32 count;
+	int error = 0;
+
+	count = touch_overlay_count_buttons(dev);
+	if (!count)
+		return 0;
+
+	map->buttons = devm_kcalloc(dev, count,
+				    sizeof(*map->buttons), GFP_KERNEL);
+	if (!map->buttons) {
+		error = -ENOMEM;
+		goto map_buttons_ret;
+	}
+	button = device_get_named_child_node(dev,
+					     object_names[TOUCH_OVERLAY_BTN]);
+	if (unlikely(!button)) {
+		error = -ENODEV;
+		goto map_buttons_ret;
+	}
+
+	error = touch_overlay_get_button_properties(dev, button,
+						    map->buttons);
+
+	if (error < 0)
+		goto map_buttons_put;
+
+	map->button_count = count;
+
+map_buttons_put:
+	fwnode_handle_put(button);
+map_buttons_ret:
+	return error;
+}
+
+static bool touch_overlay_defined_objects(struct device *dev)
+{
+	struct fwnode_handle *obj_node;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(object_names); i++) {
+		obj_node = device_get_named_child_node(dev, object_names[i]);
+		if (obj_node) {
+			fwnode_handle_put(obj_node);
+			return true;
+		}
+		fwnode_handle_put(obj_node);
+	}
+
+	return false;
+}
+
+/**
+ * touch_overlay_map_overlay - map overlay objects from the device tree and set
+ * key capabilities if buttons are defined.
+ * @keypad: pointer to the already allocated input_dev.
+ *
+ * Returns a pointer to the object mapping struct.
+ *
+ * If a keypad input device is provided and overlay buttons are defined,
+ * its button capabilities are set accordingly.
+ */
+struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad)
+{
+	struct device *dev = keypad->dev.parent;
+	struct touch_overlay_map *map = NULL;
+	int error;
+
+	if (!touch_overlay_defined_objects(dev))
+		return NULL;
+
+	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
+	if (!map) {
+		error = -ENOMEM;
+		goto map_error;
+	}
+	error = touch_overlay_map_touchscreen(dev, map);
+	if (error < 0)
+		goto map_error;
+
+	error = touch_overlay_map_buttons(dev, map);
+	if (error < 0)
+		goto map_error;
+
+	touch_overlay_set_button_caps(map, keypad);
+
+	return map;
+
+map_error:
+	return ERR_PTR(error);
+}
+EXPORT_SYMBOL(touch_overlay_map_overlay);
+
+/**
+ * touch_overlay_get_touchscreen_abs - get abs size from the overlay node
+ * @map: pointer to the struct that holds the object mapping
+ * @x: horizontal abs
+ * @y: vertical abs
+ *
+ */
+void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map, u16 *x,
+				       u16 *y)
+{
+	*x = map->touchscreen->x_size - 1;
+	*y = map->touchscreen->y_size - 1;
+}
+EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
+
+static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
+					u32 x, u32 y)
+{
+	if (!seg)
+		return false;
+
+	if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
+	    y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
+		return true;
+
+	return false;
+}
+
+/**
+ * touch_overlay_mapped_touchscreen - check if an overlay touchscreen is mapped
+ * @map: pointer to the struct that holds the object mapping
+ *
+ * Returns true if an overlay touchscreen is mapped or false otherwise.
+ */
+bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map)
+{
+	if (!map || !map->overlay_touchscreen)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
+
+/**
+ * touch_overlay_mapped_buttons - check if overlay buttons are mapped
+ * @map: pointer to the struct that holds the object mapping
+ *
+ * Returns true if overlay buttons mapped or false otherwise.
+ */
+bool touch_overlay_mapped_buttons(struct touch_overlay_map *map)
+{
+	if (!map || !map->button_count)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_buttons);
+
+static bool touch_overlay_mt_on_touchscreen(struct touch_overlay_map *map,
+					    u32 *x, u32 *y)
+{
+	bool contact = x && y;
+
+	if (!touch_overlay_mapped_touchscreen(map))
+		return true;
+
+	/* Let the caller handle events with no coordinates (release) */
+	if (!contact)
+		return false;
+
+	if (touch_overlay_segment_event(map->touchscreen, *x, *y)) {
+		*x -= map->touchscreen->x_origin;
+		*y -= map->touchscreen->y_origin;
+		return true;
+	}
+
+	return false;
+}
+
+static bool touch_overlay_button_event(struct input_dev *input,
+				       struct touch_overlay_button *button,
+				       const u32 *x, const u32 *y, u32 slot)
+{
+	bool contact = x && y;
+
+	if (!contact && button->pressed && button->slot == slot) {
+		button->pressed = false;
+		input_report_key(input, button->key, false);
+		input_sync(input);
+		return true;
+	} else if (contact && touch_overlay_segment_event(&button->segment,
+							  *x, *y)) {
+		button->pressed = true;
+		button->slot = slot;
+		input_report_key(input, button->key, true);
+		input_sync(input);
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * touch_overlay_process_event - process input events according to the overlay
+ * mapping. This function acts as a filter to release the calling driver from
+ * the events that are either related to overlay buttons or out of the overlay
+ * touchscreen area if defined.
+ * @map: pointer to the struct that holds the object mapping
+ * @input: pointer to the input device associated to the event
+ * @x: pointer to the x coordinate (NULL if not available - no contact)
+ * @y: pointer to the y coordinate (NULL if not available - no contact)
+ * @slot: slot associated to the event
+ *
+ * Returns true if the event was processed (reported for valid key events
+ * and dropped for events outside the overlay touchscreen area) or false
+ * if the event must be processed by the caller. In that case this function
+ * shifts the (x,y) coordinates to the overlay touchscreen axis if required
+ */
+bool touch_overlay_process_event(struct touch_overlay_map *map,
+				 struct input_dev *input,
+				 u32 *x, u32 *y, u32 slot)
+{
+	int i;
+
+	if (!map)
+		return false;
+
+	/* buttons must be prioritized over overlay touchscreens to account for
+	 * overlappings e.g. a button inside the touchscreen area
+	 */
+	if (touch_overlay_mapped_buttons(map)) {
+		for (i = 0; i < map->button_count; i++) {
+			if (touch_overlay_button_event(input, &map->buttons[i],
+						       x, y, slot))
+				return true;
+		}
+	}
+	/* valid touch events on the overlay touchscreen are left for the client
+	 * to be processed/reported according to its (possibly) unique features
+	 */
+	return !touch_overlay_mt_on_touchscreen(map, x, y);
+}
+EXPORT_SYMBOL(touch_overlay_process_event);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
new file mode 100644
index 000000000000..3e0db813dc34
--- /dev/null
+++ b/include/linux/input/touch-overlay.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
+ */
+
+#ifndef _TOUCH_OVERLAY
+#define _TOUCH_OVERLAY
+
+#include <linux/types.h>
+
+struct input_dev;
+struct device;
+
+struct touch_overlay_map {
+	struct touch_overlay_segment *touchscreen;
+	bool overlay_touchscreen;
+	struct touch_overlay_button *buttons;
+	u32 button_count;
+};
+
+struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad);
+
+void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map,
+				       u16 *x, u16 *y);
+
+bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map);
+
+bool touch_overlay_mapped_buttons(struct touch_overlay_map *map);
+
+bool touch_overlay_process_event(struct touch_overlay_map *map,
+				 struct input_dev *input,
+				 u32 *x, u32 *y, u32 slot);
+
+#endif

-- 
2.39.2

Re: [PATCH v5 1/4] Input: touch-overlay - Add touchscreen overlay object handling

[Jeff LaBundy]

Hi Javier,

Just a few minor comments in addition to my over-arching question in
patch [2/4].

On Tue, Oct 17, 2023 at 01:00:07PM +0200, Javier Carrasco wrote:
> Some touch devices 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.

Normally binding patches precede the code that parses the corresponding
properties, so that the documentation is in the tree by the time the
code is applied.

Typically this only matters for compatible strings, since checkpatch
may complain about undocumented compatible strings when applying a
driver whose binding is not yet merged. That's obviously not the case
here, but I would say let's swap patches 1/2 and 3/4 for consistency.

> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/input/Makefile              |   2 +-
>  drivers/input/touch-overlay.c       | 399 ++++++++++++++++++++++++++++++++++++
>  include/linux/input/touch-overlay.h |  34 +++
>  4 files changed, 441 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..00c03824c3ac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21884,6 +21884,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/
>  
> +TOUCH OVERLAY OBJECTS
> +M:	Javier Carrasco <javier.carrasco@wolfvision.net>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +F:	drivers/input/touch-overlay.c
> +F:	include/linux/input/touch-overlay.h
> +
>  TTY LAYER AND SERIAL DRIVERS
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index c78753274921..393e9f4d00dc 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -7,7 +7,7 @@
>  
>  obj-$(CONFIG_INPUT)		+= input-core.o
>  input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> -input-core-y += touchscreen.o
> +input-core-y += touchscreen.o touch-overlay.o
>  
>  obj-$(CONFIG_INPUT_FF_MEMLESS)	+= ff-memless.o
>  obj-$(CONFIG_INPUT_SPARSEKMAP)	+= sparse-keymap.o
> diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
> new file mode 100644
> index 000000000000..007dbd994474
> --- /dev/null
> +++ b/drivers/input/touch-overlay.c
> @@ -0,0 +1,399 @@
> +// 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/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touch-overlay.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +
> +enum touch_overlay_valid_objects {
> +	TOUCH_OVERLAY_TS,
> +	TOUCH_OVERLAY_BTN,
> +};
> +
> +static const char *const object_names[] = {
> +	[TOUCH_OVERLAY_TS] = "overlay-touchscreen",
> +	[TOUCH_OVERLAY_BTN] = "overlay-buttons",
> +};
> +
> +struct touch_overlay_segment {
> +	u32 x_origin;
> +	u32 y_origin;
> +	u32 x_size;
> +	u32 y_size;
> +};
> +
> +struct touch_overlay_button {
> +	struct touch_overlay_segment segment;
> +	u32 key;
> +	bool pressed;
> +	int slot;
> +};
> +
> +static int touch_overlay_get_segment_props(struct fwnode_handle *segment_node,
> +					   struct touch_overlay_segment *segment)
> +{
> +	int error;
> +
> +	error = fwnode_property_read_u32(segment_node, "x-origin",
> +					 &segment->x_origin);
> +	if (error < 0)

It is sufficient, and much more common in input, to write this as 'if (error)'
as the only nonzero return values are negative anyway.

> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "y-origin",
> +					 &segment->y_origin);
> +	if (error < 0)
> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "x-size",
> +					 &segment->x_size);
> +	if (error < 0)
> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "y-size",
> +					 &segment->y_size);
> +	if (error < 0)
> +		return error;
> +
> +	return 0;
> +}
> +
> +static int
> +touch_overlay_get_button_properties(struct device *dev,
> +				    struct fwnode_handle *overlay_node,
> +				    struct touch_overlay_button *btn)
> +{
> +	struct fwnode_handle *btn_node;
> +	int error;
> +	int j = 0;
> +
> +	fwnode_for_each_child_node(overlay_node, btn_node) {
> +		error = touch_overlay_get_segment_props(btn_node,
> +							&btn[j].segment);
> +		if (error < 0)
> +			goto button_put;
> +
> +		error = fwnode_property_read_u32(btn_node, "linux,code",
> +						 &btn[j].key);
> +		if (error < 0)
> +			goto button_put;
> +
> +		dev_dbg(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
> +			btn[j].segment.x_origin, btn[j].segment.y_origin,
> +			btn[j].segment.x_size, btn[j].segment.y_size, btn[j].key);
> +		j++;
> +	}
> +
> +	return 0;
> +
> +button_put:
> +	fwnode_handle_put(btn_node);
> +	return error;
> +}
> +
> +static void touch_overlay_set_button_caps(struct touch_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);
> +}
> +
> +static int touch_overlay_count_buttons(struct device *dev)
> +{
> +	struct fwnode_handle *overlay;
> +	struct fwnode_handle *button;
> +	int count = 0;
> +
> +	overlay = device_get_named_child_node(dev,
> +					      object_names[TOUCH_OVERLAY_BTN]);
> +	if (!overlay)
> +		return 0;
> +
> +	fwnode_for_each_child_node(overlay, button)
> +		count++;
> +	fwnode_handle_put(overlay);
> +
> +	return count;
> +}
> +
> +static int touch_overlay_map_touchscreen(struct device *dev,
> +					 struct touch_overlay_map *map)
> +{
> +	struct fwnode_handle *ts_node;
> +	int error = 0;
> +
> +	ts_node = device_get_named_child_node(dev,
> +					      object_names[TOUCH_OVERLAY_TS]);
> +	if (!ts_node)
> +		return 0;
> +
> +	map->touchscreen =
> +		devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);

This line break was a bit confusing to read; the choice of line break in
touch_overlay_map_buttons() is much more clear.

> +	if (!map->touchscreen) {
> +		error = -ENOMEM;
> +		goto handle_put;
> +	}
> +	error = touch_overlay_get_segment_props(ts_node, map->touchscreen);
> +	if (error < 0)
> +		goto handle_put;
> +
> +	map->overlay_touchscreen = true;
> +	dev_dbg(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);
> +
> +handle_put:
> +	fwnode_handle_put(ts_node);
> +
> +	return error;
> +}
> +
> +static int touch_overlay_map_buttons(struct device *dev,
> +				     struct touch_overlay_map *map)
> +{
> +	struct fwnode_handle *button;
> +	u32 count;
> +	int error = 0;
> +
> +	count = touch_overlay_count_buttons(dev);
> +	if (!count)
> +		return 0;
> +
> +	map->buttons = devm_kcalloc(dev, count,
> +				    sizeof(*map->buttons), GFP_KERNEL);
> +	if (!map->buttons) {
> +		error = -ENOMEM;
> +		goto map_buttons_ret;
> +	}
> +	button = device_get_named_child_node(dev,
> +					     object_names[TOUCH_OVERLAY_BTN]);
> +	if (unlikely(!button)) {
> +		error = -ENODEV;
> +		goto map_buttons_ret;
> +	}
> +
> +	error = touch_overlay_get_button_properties(dev, button,
> +						    map->buttons);
> +
> +	if (error < 0)
> +		goto map_buttons_put;
> +
> +	map->button_count = count;
> +
> +map_buttons_put:
> +	fwnode_handle_put(button);
> +map_buttons_ret:
> +	return error;
> +}
> +
> +static bool touch_overlay_defined_objects(struct device *dev)
> +{
> +	struct fwnode_handle *obj_node;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(object_names); i++) {
> +		obj_node = device_get_named_child_node(dev, object_names[i]);
> +		if (obj_node) {
> +			fwnode_handle_put(obj_node);
> +			return true;
> +		}
> +		fwnode_handle_put(obj_node);
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * touch_overlay_map_overlay - map overlay objects from the device tree and set
> + * key capabilities if buttons are defined.
> + * @keypad: pointer to the already allocated input_dev.
> + *
> + * Returns a pointer to the object mapping struct.
> + *
> + * If a keypad input device is provided and overlay buttons are defined,
> + * its button capabilities are set accordingly.
> + */
> +struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad)
> +{
> +	struct device *dev = keypad->dev.parent;
> +	struct touch_overlay_map *map = NULL;
> +	int error;
> +
> +	if (!touch_overlay_defined_objects(dev))
> +		return NULL;
> +
> +	map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> +	if (!map) {
> +		error = -ENOMEM;
> +		goto map_error;
> +	}
> +	error = touch_overlay_map_touchscreen(dev, map);
> +	if (error < 0)
> +		goto map_error;
> +
> +	error = touch_overlay_map_buttons(dev, map);
> +	if (error < 0)
> +		goto map_error;
> +
> +	touch_overlay_set_button_caps(map, keypad);
> +
> +	return map;
> +
> +map_error:
> +	return ERR_PTR(error);
> +}
> +EXPORT_SYMBOL(touch_overlay_map_overlay);
> +
> +/**
> + * touch_overlay_get_touchscreen_abs - get abs size from the overlay node
> + * @map: pointer to the struct that holds the object mapping
> + * @x: horizontal abs
> + * @y: vertical abs
> + *
> + */
> +void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map, u16 *x,
> +				       u16 *y)
> +{
> +	*x = map->touchscreen->x_size - 1;
> +	*y = map->touchscreen->y_size - 1;
> +}
> +EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
> +
> +static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
> +					u32 x, u32 y)
> +{
> +	if (!seg)
> +		return false;
> +
> +	if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
> +	    y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * touch_overlay_mapped_touchscreen - check if an overlay touchscreen is mapped
> + * @map: pointer to the struct that holds the object mapping
> + *
> + * Returns true if an overlay touchscreen is mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map)
> +{
> +	if (!map || !map->overlay_touchscreen)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +/**
> + * touch_overlay_mapped_buttons - check if overlay buttons are mapped
> + * @map: pointer to the struct that holds the object mapping
> + *
> + * Returns true if overlay buttons mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_buttons(struct touch_overlay_map *map)
> +{
> +	if (!map || !map->button_count)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_buttons);
> +
> +static bool touch_overlay_mt_on_touchscreen(struct touch_overlay_map *map,
> +					    u32 *x, u32 *y)
> +{
> +	bool contact = x && y;
> +
> +	if (!touch_overlay_mapped_touchscreen(map))
> +		return true;
> +
> +	/* Let the caller handle events with no coordinates (release) */
> +	if (!contact)
> +		return false;
> +
> +	if (touch_overlay_segment_event(map->touchscreen, *x, *y)) {
> +		*x -= map->touchscreen->x_origin;
> +		*y -= map->touchscreen->y_origin;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool touch_overlay_button_event(struct input_dev *input,
> +				       struct touch_overlay_button *button,
> +				       const u32 *x, const u32 *y, u32 slot)
> +{
> +	bool contact = x && y;
> +
> +	if (!contact && button->pressed && button->slot == slot) {
> +		button->pressed = false;
> +		input_report_key(input, button->key, false);
> +		input_sync(input);
> +		return true;
> +	} else if (contact && touch_overlay_segment_event(&button->segment,
> +							  *x, *y)) {
> +		button->pressed = true;
> +		button->slot = slot;
> +		input_report_key(input, button->key, true);
> +		input_sync(input);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * touch_overlay_process_event - process input events according to the overlay
> + * mapping. This function acts as a filter to release the calling driver from
> + * the events that are either related to overlay buttons or out of the overlay
> + * touchscreen area if defined.
> + * @map: pointer to the struct that holds the object mapping
> + * @input: pointer to the input device associated to the event
> + * @x: pointer to the x coordinate (NULL if not available - no contact)
> + * @y: pointer to the y coordinate (NULL if not available - no contact)
> + * @slot: slot associated to the event
> + *
> + * Returns true if the event was processed (reported for valid key events
> + * and dropped for events outside the overlay touchscreen area) or false
> + * if the event must be processed by the caller. In that case this function
> + * shifts the (x,y) coordinates to the overlay touchscreen axis if required
> + */
> +bool touch_overlay_process_event(struct touch_overlay_map *map,
> +				 struct input_dev *input,
> +				 u32 *x, u32 *y, u32 slot)
> +{
> +	int i;
> +
> +	if (!map)
> +		return false;
> +
> +	/* buttons must be prioritized over overlay touchscreens to account for
> +	 * overlappings e.g. a button inside the touchscreen area
> +	 */

Please refer to the kernel coding style guide regarding multi-line comments.

> +	if (touch_overlay_mapped_buttons(map)) {
> +		for (i = 0; i < map->button_count; i++) {
> +			if (touch_overlay_button_event(input, &map->buttons[i],
> +						       x, y, slot))
> +				return true;
> +		}
> +	}
> +	/* valid touch events on the overlay touchscreen are left for the client
> +	 * to be processed/reported according to its (possibly) unique features
> +	 */
> +	return !touch_overlay_mt_on_touchscreen(map, x, y);
> +}
> +EXPORT_SYMBOL(touch_overlay_process_event);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
> diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
> new file mode 100644
> index 000000000000..3e0db813dc34
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TOUCH_OVERLAY
> +#define _TOUCH_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +struct device;
> +
> +struct touch_overlay_map {
> +	struct touch_overlay_segment *touchscreen;
> +	bool overlay_touchscreen;
> +	struct touch_overlay_button *buttons;
> +	u32 button_count;
> +};
> +
> +struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad);
> +
> +void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map,
> +				       u16 *x, u16 *y);
> +
> +bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map);
> +
> +bool touch_overlay_mapped_buttons(struct touch_overlay_map *map);
> +
> +bool touch_overlay_process_event(struct touch_overlay_map *map,
> +				 struct input_dev *input,
> +				 u32 *x, u32 *y, u32 slot);
> +
> +#endif
> 
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy

[PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties

[Javier Carrasco]

The overlay-touchscreen object defines an area within the touchscreen
where touch events are reported and their coordinates get converted to
the overlay origin. This object avoids getting events from areas that
are physically hidden by overlay frames.

For touchscreens where overlay buttons on the touchscreen surface are
provided, the overlay-buttons object contains a node for every button
and the key event that should be reported when pressed.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 .../bindings/input/touchscreen/touchscreen.yaml    | 143 +++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 431c13335c40..5c58eb79ee9a 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -87,6 +87,129 @@ properties:
   touchscreen-y-plate-ohms:
     description: Resistance of the Y-plate in Ohms
 
+  overlay-touchscreen:
+    description: Clipped touchscreen area
+
+      This object can be used to describe a frame that restricts the area
+      within touch events are reported, ignoring the events that occur outside
+      this area. This is of special interest if the touchscreen is shipped
+      with a physical overlay on top of it with a frame that hides some part
+      of the original touchscreen area.
+
+      The x-origin and y-origin properties of this object define the offset of
+      a new origin from where the touchscreen events are referenced.
+      This offset is applied to the events accordingly. The x-size and y-size
+      properties define the size of the overlay-touchscreen (effective area).
+
+      The following example shows the new touchscreen area and the new origin
+      (0',0') for the touch events generated by the device.
+
+                   Touchscreen (full area)
+         ┌────────────────────────────────────────┐
+         │    ┌───────────────────────────────┐   │
+         │    │                               │   │
+         │    ├ y-size                        │   │
+         │    │                               │   │
+         │    │      overlay-touchscreen      │   │
+         │    │                               │   │
+         │    │                               │   │
+         │    │            x-size             │   │
+         │   ┌└──────────────┴────────────────┘   │
+         │(0',0')                                 │
+        ┌└────────────────────────────────────────┘
+      (0,0)
+
+     where (0',0') = (0+x-origin,0+y-origin)
+
+    type: object
+    $ref: '#/$defs/overlay-node'
+    unevaluatedProperties: false
+
+    required:
+      - x-origin
+      - y-origin
+      - x-size
+      - y-size
+
+  overlay-buttons:
+    description: list of nodes defining the buttons on the touchscreen
+
+      This object can be used to describe buttons on the touchscreen area,
+      reporting the touch events on their surface as key events instead of
+      the original touch events.
+
+      This is of special interest if the touchscreen is shipped with a
+      physical overlay on top of it where a number of buttons with some
+      predefined functionality are printed. In that case a specific behavior
+      is expected from those buttons instead of raw touch events.
+
+      The overlay-buttons properties define a per-button area as well as an
+      origin relative to the real touchscreen origin. Touch events within the
+      button area are reported as the key event defined in the linux,code
+      property. Given that the key events do not provide coordinates, the
+      button origin is only used to place the button area on the touchscreen
+      surface. Any event outside the overlay-buttons object is reported as a
+      touch event with no coordinate transformation.
+
+      The following example shows a touchscreen with a single button on it
+
+              Touchscreen (full area)
+        ┌───────────────────────────────────┐
+        │                                   │
+        │                                   │
+        │   ┌─────────┐                     │
+        │   │button 0 │                     │
+        │   │KEY_POWER│                     │
+        │   └─────────┘                     │
+        │                                   │
+        │                                   │
+       ┌└───────────────────────────────────┘
+     (0,0)
+
+      The overlay-buttons object can  be combined with the overlay-touchscreen
+      object as shown in the following example. In that case only the events
+      within the overlay-touchscreen object are reported as touch events.
+
+                  Touchscreen (full area)
+        ┌─────────┬──────────────────────────────┐
+        │         │                              │
+        │         │    ┌───────────────────────┐ │
+        │ button 0│    │                       │ │
+        │KEY_POWER│    │                       │ │
+        │         │    │                       │ │
+        ├─────────┤    │  overlay-touchscreen  │ │
+        │         │    │                       │ │
+        │         │    │                       │ │
+        │ button 1│    │                       │ │
+        │ KEY_INFO│   ┌└───────────────────────┘ │
+        │         │(0',0')                       │
+       ┌└─────────┴──────────────────────────────┘
+     (0,0)
+
+    type: object
+
+    patternProperties:
+      '^button-':
+        type: object
+        description:
+          Each button (key) is represented as a sub-node.
+        $ref: '#/$defs/overlay-node'
+        unevaluatedProperties: false
+
+        properties:
+          label:
+            $ref: /schemas/types.yaml#/definitions/string
+            description: descriptive name of the button
+
+          linux,code: true
+
+        required:
+          - linux,code
+          - x-origin
+          - y-origin
+          - x-size
+          - y-size
+
 dependencies:
   touchscreen-size-x: [ touchscreen-size-y ]
   touchscreen-size-y: [ touchscreen-size-x ]
@@ -94,3 +217,23 @@ dependencies:
   touchscreen-y-mm: [ touchscreen-x-mm ]
 
 additionalProperties: true
+
+$defs:
+  overlay-node:
+    type: object
+    properties:
+      x-origin:
+        description: horizontal origin of the node area
+        $ref: /schemas/types.yaml#/definitions/uint32
+
+      y-origin:
+        description: vertical origin of the node area
+        $ref: /schemas/types.yaml#/definitions/uint32
+
+      x-size:
+        description: horizontal resolution of the node area
+        $ref: /schemas/types.yaml#/definitions/uint32
+
+      y-size:
+        description: vertical resolution of the node area
+        $ref: /schemas/types.yaml#/definitions/uint32

-- 
2.39.2

Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties

[Jeff LaBundy]

Hi Javier,

Thank you for continuing to drive this high-quality work.

On Tue, Oct 17, 2023 at 01:00:08PM +0200, Javier Carrasco wrote:
> The overlay-touchscreen object defines an area within the touchscreen
> where touch events are reported and their coordinates get converted to
> the overlay origin. This object avoids getting events from areas that
> are physically hidden by overlay frames.
> 
> For touchscreens where overlay buttons on the touchscreen surface are
> provided, the overlay-buttons object contains a node for every button
> and the key event that should be reported when pressed.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../bindings/input/touchscreen/touchscreen.yaml    | 143 +++++++++++++++++++++
>  1 file changed, 143 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> index 431c13335c40..5c58eb79ee9a 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> @@ -87,6 +87,129 @@ properties:
>    touchscreen-y-plate-ohms:
>      description: Resistance of the Y-plate in Ohms
>  
> +  overlay-touchscreen:
> +    description: Clipped touchscreen area
> +
> +      This object can be used to describe a frame that restricts the area
> +      within touch events are reported, ignoring the events that occur outside
> +      this area. This is of special interest if the touchscreen is shipped
> +      with a physical overlay on top of it with a frame that hides some part
> +      of the original touchscreen area.
> +
> +      The x-origin and y-origin properties of this object define the offset of
> +      a new origin from where the touchscreen events are referenced.
> +      This offset is applied to the events accordingly. The x-size and y-size
> +      properties define the size of the overlay-touchscreen (effective area).
> +
> +      The following example shows the new touchscreen area and the new origin
> +      (0',0') for the touch events generated by the device.
> +
> +                   Touchscreen (full area)
> +         ┌────────────────────────────────────────┐
> +         │    ┌───────────────────────────────┐   │
> +         │    │                               │   │
> +         │    ├ y-size                        │   │
> +         │    │                               │   │
> +         │    │      overlay-touchscreen      │   │
> +         │    │                               │   │
> +         │    │                               │   │
> +         │    │            x-size             │   │
> +         │   ┌└──────────────┴────────────────┘   │
> +         │(0',0')                                 │
> +        ┌└────────────────────────────────────────┘
> +      (0,0)
> +
> +     where (0',0') = (0+x-origin,0+y-origin)
> +
> +    type: object
> +    $ref: '#/$defs/overlay-node'
> +    unevaluatedProperties: false
> +
> +    required:
> +      - x-origin
> +      - y-origin
> +      - x-size
> +      - y-size
> +
> +  overlay-buttons:
> +    description: list of nodes defining the buttons on the touchscreen
> +
> +      This object can be used to describe buttons on the touchscreen area,
> +      reporting the touch events on their surface as key events instead of
> +      the original touch events.
> +
> +      This is of special interest if the touchscreen is shipped with a
> +      physical overlay on top of it where a number of buttons with some
> +      predefined functionality are printed. In that case a specific behavior
> +      is expected from those buttons instead of raw touch events.
> +
> +      The overlay-buttons properties define a per-button area as well as an
> +      origin relative to the real touchscreen origin. Touch events within the
> +      button area are reported as the key event defined in the linux,code
> +      property. Given that the key events do not provide coordinates, the
> +      button origin is only used to place the button area on the touchscreen
> +      surface. Any event outside the overlay-buttons object is reported as a
> +      touch event with no coordinate transformation.
> +
> +      The following example shows a touchscreen with a single button on it
> +
> +              Touchscreen (full area)
> +        ┌───────────────────────────────────┐
> +        │                                   │
> +        │                                   │
> +        │   ┌─────────┐                     │
> +        │   │button 0 │                     │
> +        │   │KEY_POWER│                     │
> +        │   └─────────┘                     │
> +        │                                   │
> +        │                                   │
> +       ┌└───────────────────────────────────┘
> +     (0,0)
> +
> +      The overlay-buttons object can  be combined with the overlay-touchscreen
> +      object as shown in the following example. In that case only the events
> +      within the overlay-touchscreen object are reported as touch events.
> +
> +                  Touchscreen (full area)
> +        ┌─────────┬──────────────────────────────┐
> +        │         │                              │
> +        │         │    ┌───────────────────────┐ │
> +        │ button 0│    │                       │ │
> +        │KEY_POWER│    │                       │ │
> +        │         │    │                       │ │
> +        ├─────────┤    │  overlay-touchscreen  │ │
> +        │         │    │                       │ │
> +        │         │    │                       │ │
> +        │ button 1│    │                       │ │
> +        │ KEY_INFO│   ┌└───────────────────────┘ │
> +        │         │(0',0')                       │
> +       ┌└─────────┴──────────────────────────────┘
> +     (0,0)
> +
> +    type: object

I am still confused why the buttons need to live under an 'overlay-buttons'
parent node, which seems like an imaginary boundary. In my view, the touch
surface comprises the following types of rectangular areas:

1. A touchscreen, wherein granular coordinates and pressure are reported.
2. A momentary button, wherein pressure is quantized into a binary value
   (press or release), and coordinates are ignored.

Any contact that falls outside of (1) and (2) is presumed to be part of a
border or matting, and is hence ignored.

Areas (1) and (2) exist in the same "plane", so why can they not reside
under the same parent node? The following seems much more representative
of the actual hardware we intend to describe in the device tree:

	touchscreen {
		compatible = "...";
		reg = <...>;

		/* raw coordinates reported here */
		touch-area-1 {
			x-origin = <...>;
			y-origin = <...>;
			x-size = <...>;
			y-size = <...>;
		};

		/* a button */
		touch-area-2a {
			x-origin = <...>;
			y-origin = <...>;
			x-size = <...>;
			y-size = <...>;
			linux,code = <KEY_POWER>;
		};

		/* another button */
		touch-area-2b {
			x-origin = <...>;
			y-origin = <...>;
			x-size = <...>;
			y-size = <...>;
			linux,code = <KEY_INFO>;
		};
	};

With this method, the driver merely stores a list head. The parsing code
then walks the client device node; for each touch* child encountered, it
allocates memory for a structure of five members, and adds it to the list.

The event handling code then simply iterates through the list and checks
if the coordinates reported by the hardware fall within each rectangle. If
so, and the keycode in the list element is equal to KEY_RESERVED (zero),
we assume the rectangle is of type (1); the coordinates are passed to
touchscreen_report_pos() and the pressure is reported as well.

If the keycode is not equal to KEY_RESERVED (e.g. KEY_POWER), we assume
the rectangle is of type (2); input_report_key() is called instead and
the coordinates are ignored altogether.

Instead, the existing implementation has two separate structures, one of
which inherits the other. Its corresponding properties are then parsed in
a separate function to account for the fact that the two structures exist
at different layers in the heirarchy.

My argument is that all nodes can be parsed in the same way, without having
to understand whether they represent case (1) or (2). The existing method
also has to count the nodes; this would not be necessary with a linked list.

Can you help me understand why the buttons must be "wrapped" in their own
node? As I mention in v2, this isn't a deal breaker necessarily, but I'd
like to understand the reasoning behind what I interpret as additional
complexity, and a degree of separation from a natural description of the
touch surface.

The only reason I can think to insert the 'overlay-buttons' parent is in
case we want to specify a property within this node that applies to all
buttons, but this does not exist in the current implementation, nor do I
see a compelling reason to do so in the future.

> +
> +    patternProperties:
> +      '^button-':
> +        type: object
> +        description:
> +          Each button (key) is represented as a sub-node.
> +        $ref: '#/$defs/overlay-node'
> +        unevaluatedProperties: false
> +
> +        properties:
> +          label:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            description: descriptive name of the button
> +
> +          linux,code: true
> +
> +        required:
> +          - linux,code
> +          - x-origin
> +          - y-origin
> +          - x-size
> +          - y-size
> +
>  dependencies:
>    touchscreen-size-x: [ touchscreen-size-y ]
>    touchscreen-size-y: [ touchscreen-size-x ]
> @@ -94,3 +217,23 @@ dependencies:
>    touchscreen-y-mm: [ touchscreen-x-mm ]
>  
>  additionalProperties: true
> +
> +$defs:
> +  overlay-node:
> +    type: object
> +    properties:
> +      x-origin:
> +        description: horizontal origin of the node area
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +      y-origin:
> +        description: vertical origin of the node area
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +      x-size:
> +        description: horizontal resolution of the node area
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +      y-size:
> +        description: vertical resolution of the node area
> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties

[Javier Carrasco]

Hi Jeff,

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

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

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

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

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

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

Thanks again for your thorough review and enriching feedback.

Best regards,
Javier Carrasco

Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties

[Javier Carrasco]

Hi Jeff,

On 26.10.23 16:46, Jeff LaBundy wrote:
> Hi Javier,
> 
> Thank you for continuing to drive this high-quality work.
> 
> On Tue, Oct 17, 2023 at 01:00:08PM +0200, Javier Carrasco wrote:
>> The overlay-touchscreen object defines an area within the touchscreen
>> where touch events are reported and their coordinates get converted to
>> the overlay origin. This object avoids getting events from areas that
>> are physically hidden by overlay frames.
>>
>> For touchscreens where overlay buttons on the touchscreen surface are
>> provided, the overlay-buttons object contains a node for every button
>> and the key event that should be reported when pressed.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>>  .../bindings/input/touchscreen/touchscreen.yaml    | 143 +++++++++++++++++++++
>>  1 file changed, 143 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>> index 431c13335c40..5c58eb79ee9a 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>> @@ -87,6 +87,129 @@ properties:
>>    touchscreen-y-plate-ohms:
>>      description: Resistance of the Y-plate in Ohms
>>  
>> +  overlay-touchscreen:
>> +    description: Clipped touchscreen area
>> +
>> +      This object can be used to describe a frame that restricts the area
>> +      within touch events are reported, ignoring the events that occur outside
>> +      this area. This is of special interest if the touchscreen is shipped
>> +      with a physical overlay on top of it with a frame that hides some part
>> +      of the original touchscreen area.
>> +
>> +      The x-origin and y-origin properties of this object define the offset of
>> +      a new origin from where the touchscreen events are referenced.
>> +      This offset is applied to the events accordingly. The x-size and y-size
>> +      properties define the size of the overlay-touchscreen (effective area).
>> +
>> +      The following example shows the new touchscreen area and the new origin
>> +      (0',0') for the touch events generated by the device.
>> +
>> +                   Touchscreen (full area)
>> +         ┌────────────────────────────────────────┐
>> +         │    ┌───────────────────────────────┐   │
>> +         │    │                               │   │
>> +         │    ├ y-size                        │   │
>> +         │    │                               │   │
>> +         │    │      overlay-touchscreen      │   │
>> +         │    │                               │   │
>> +         │    │                               │   │
>> +         │    │            x-size             │   │
>> +         │   ┌└──────────────┴────────────────┘   │
>> +         │(0',0')                                 │
>> +        ┌└────────────────────────────────────────┘
>> +      (0,0)
>> +
>> +     where (0',0') = (0+x-origin,0+y-origin)
>> +
>> +    type: object
>> +    $ref: '#/$defs/overlay-node'
>> +    unevaluatedProperties: false
>> +
>> +    required:
>> +      - x-origin
>> +      - y-origin
>> +      - x-size
>> +      - y-size
>> +
>> +  overlay-buttons:
>> +    description: list of nodes defining the buttons on the touchscreen
>> +
>> +      This object can be used to describe buttons on the touchscreen area,
>> +      reporting the touch events on their surface as key events instead of
>> +      the original touch events.
>> +
>> +      This is of special interest if the touchscreen is shipped with a
>> +      physical overlay on top of it where a number of buttons with some
>> +      predefined functionality are printed. In that case a specific behavior
>> +      is expected from those buttons instead of raw touch events.
>> +
>> +      The overlay-buttons properties define a per-button area as well as an
>> +      origin relative to the real touchscreen origin. Touch events within the
>> +      button area are reported as the key event defined in the linux,code
>> +      property. Given that the key events do not provide coordinates, the
>> +      button origin is only used to place the button area on the touchscreen
>> +      surface. Any event outside the overlay-buttons object is reported as a
>> +      touch event with no coordinate transformation.
>> +
>> +      The following example shows a touchscreen with a single button on it
>> +
>> +              Touchscreen (full area)
>> +        ┌───────────────────────────────────┐
>> +        │                                   │
>> +        │                                   │
>> +        │   ┌─────────┐                     │
>> +        │   │button 0 │                     │
>> +        │   │KEY_POWER│                     │
>> +        │   └─────────┘                     │
>> +        │                                   │
>> +        │                                   │
>> +       ┌└───────────────────────────────────┘
>> +     (0,0)
>> +
>> +      The overlay-buttons object can  be combined with the overlay-touchscreen
>> +      object as shown in the following example. In that case only the events
>> +      within the overlay-touchscreen object are reported as touch events.
>> +
>> +                  Touchscreen (full area)
>> +        ┌─────────┬──────────────────────────────┐
>> +        │         │                              │
>> +        │         │    ┌───────────────────────┐ │
>> +        │ button 0│    │                       │ │
>> +        │KEY_POWER│    │                       │ │
>> +        │         │    │                       │ │
>> +        ├─────────┤    │  overlay-touchscreen  │ │
>> +        │         │    │                       │ │
>> +        │         │    │                       │ │
>> +        │ button 1│    │                       │ │
>> +        │ KEY_INFO│   ┌└───────────────────────┘ │
>> +        │         │(0',0')                       │
>> +       ┌└─────────┴──────────────────────────────┘
>> +     (0,0)
>> +
>> +    type: object
> 
> I am still confused why the buttons need to live under an 'overlay-buttons'
> parent node, which seems like an imaginary boundary. In my view, the touch
> surface comprises the following types of rectangular areas:
> 
> 1. A touchscreen, wherein granular coordinates and pressure are reported.
> 2. A momentary button, wherein pressure is quantized into a binary value
>    (press or release), and coordinates are ignored.
> 
> Any contact that falls outside of (1) and (2) is presumed to be part of a
> border or matting, and is hence ignored.
> 
> Areas (1) and (2) exist in the same "plane", so why can they not reside
> under the same parent node? The following seems much more representative
> of the actual hardware we intend to describe in the device tree:
> 
> 	touchscreen {
> 		compatible = "...";
> 		reg = <...>;
> 
> 		/* raw coordinates reported here */
> 		touch-area-1 {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 		};
> 
> 		/* a button */
> 		touch-area-2a {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 			linux,code = <KEY_POWER>;
> 		};
> 
> 		/* another button */
> 		touch-area-2b {
> 			x-origin = <...>;
> 			y-origin = <...>;
> 			x-size = <...>;
> 			y-size = <...>;
> 			linux,code = <KEY_INFO>;
> 		};
> 	};
> 
Now that I am working on the approach you suggested, I see that some
things can get slightly more complicated. I still think that it is worth
a try, but I would like to discuss a couple of points.

The node parsing is not that simple anymore because the touch-area nodes
are only surrounded by the touchscreen node. Theoretically they could be
even be defined with other properties in between. The current approach
only needs to find the overlay-buttons parent and iterate over all the
inner nodes(simply by calling device_get_named_child_node() and
fwnode_for_each_child_node() the parsing is achieved in two lines +
error checking). So maybe even if we opt for the single-object approach,
an overlay node to group all the touch-areas could simplify the parsing.
Or did you have a different approach in mind? Your example would turn
into this one:

	touchscreen {
		compatible = "...";
		reg = <...>;

		touch-overlay {
			/* raw coordinates reported here */
			touch-area-1 {
				x-origin = <...>;
				y-origin = <...>;
				x-size = <...>;
				y-size = <...>;
			};

			/* a button */
			touch-area-2a {
				x-origin = <...>;
				y-origin = <...>;
				x-size = <...>;
				y-size = <...>;
				linux,code = <KEY_POWER>;
			};

			/* another button */
			touch-area-2b {
				x-origin = <...>;
				y-origin = <...>;
				x-size = <...>;
				y-size = <...>;
				linux,code = <KEY_INFO>;
			};
		};
	};
In my opinion it looks cleaner as well because you are defining a
physical object: the overlay.

> With this method, the driver merely stores a list head. The parsing code
> then walks the client device node; for each touch* child encountered, it
> allocates memory for a structure of five members, and adds it to the list.
> 
The button objects do not only store the keycode, but also the slot and
if they are pressed or not. I could allocate memory for these members as
well, but maybe an additional struct with the button-specific members
set to NULL for the touch areas with keycode = KEY_RESERVED would make
sense. I don't know if that's adding too much overhead for two members
though.

> The event handling code then simply iterates through the list and checks
> if the coordinates reported by the hardware fall within each rectangle. If
> so, and the keycode in the list element is equal to KEY_RESERVED (zero),
> we assume the rectangle is of type (1); the coordinates are passed to
> touchscreen_report_pos() and the pressure is reported as well.

There is another case to consider that might make the iteration less
optimal, but I don't think it will be critical.

A button could be defined inside an overlay-touchscreen (no keycode)
area. Given that the other way round (a touchscreen inside a button)
does not make much sense, the buttons have a higher priority.

Let's take your example: imagine that your third area
is a button inside the first one. We have to iterate through the whole
list until we are sure we checked if there are buttons in the given
position, but keeping in mind that the first object already has the
right coordinates to handle the touch event. Your approach even allows
for multiple no-key areas and we do not know if there are buttons when
we iterate (there could be none).
Therefore some iterations could be unnecessary, but this is probably an
edge case that would cost at most a couple of extra iterations compared
to a two-list approach.

I will keep on working on the next version with a single list while we
clarify these points, so maybe we can save an iteration.
> Kind regards,
> Jeff LaBundy

Best regards,
Javier Carrasco

Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties

[Jeff LaBundy]

Hi Javier,

I am so sorry for the delayed response.

On Thu, Nov 23, 2023 at 08:48:56PM +0100, Javier Carrasco wrote:
> Hi Jeff,
> 
> On 26.10.23 16:46, Jeff LaBundy wrote:
> > Hi Javier,
> > 
> > Thank you for continuing to drive this high-quality work.
> > 
> > On Tue, Oct 17, 2023 at 01:00:08PM +0200, Javier Carrasco wrote:
> >> The overlay-touchscreen object defines an area within the touchscreen
> >> where touch events are reported and their coordinates get converted to
> >> the overlay origin. This object avoids getting events from areas that
> >> are physically hidden by overlay frames.
> >>
> >> For touchscreens where overlay buttons on the touchscreen surface are
> >> provided, the overlay-buttons object contains a node for every button
> >> and the key event that should be reported when pressed.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> >> ---
> >>  .../bindings/input/touchscreen/touchscreen.yaml    | 143 +++++++++++++++++++++
> >>  1 file changed, 143 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> >> index 431c13335c40..5c58eb79ee9a 100644
> >> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> >> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> >> @@ -87,6 +87,129 @@ properties:
> >>    touchscreen-y-plate-ohms:
> >>      description: Resistance of the Y-plate in Ohms
> >>  
> >> +  overlay-touchscreen:
> >> +    description: Clipped touchscreen area
> >> +
> >> +      This object can be used to describe a frame that restricts the area
> >> +      within touch events are reported, ignoring the events that occur outside
> >> +      this area. This is of special interest if the touchscreen is shipped
> >> +      with a physical overlay on top of it with a frame that hides some part
> >> +      of the original touchscreen area.
> >> +
> >> +      The x-origin and y-origin properties of this object define the offset of
> >> +      a new origin from where the touchscreen events are referenced.
> >> +      This offset is applied to the events accordingly. The x-size and y-size
> >> +      properties define the size of the overlay-touchscreen (effective area).
> >> +
> >> +      The following example shows the new touchscreen area and the new origin
> >> +      (0',0') for the touch events generated by the device.
> >> +
> >> +                   Touchscreen (full area)
> >> +         ┌────────────────────────────────────────┐
> >> +         │    ┌───────────────────────────────┐   │
> >> +         │    │                               │   │
> >> +         │    ├ y-size                        │   │
> >> +         │    │                               │   │
> >> +         │    │      overlay-touchscreen      │   │
> >> +         │    │                               │   │
> >> +         │    │                               │   │
> >> +         │    │            x-size             │   │
> >> +         │   ┌└──────────────┴────────────────┘   │
> >> +         │(0',0')                                 │
> >> +        ┌└────────────────────────────────────────┘
> >> +      (0,0)
> >> +
> >> +     where (0',0') = (0+x-origin,0+y-origin)
> >> +
> >> +    type: object
> >> +    $ref: '#/$defs/overlay-node'
> >> +    unevaluatedProperties: false
> >> +
> >> +    required:
> >> +      - x-origin
> >> +      - y-origin
> >> +      - x-size
> >> +      - y-size
> >> +
> >> +  overlay-buttons:
> >> +    description: list of nodes defining the buttons on the touchscreen
> >> +
> >> +      This object can be used to describe buttons on the touchscreen area,
> >> +      reporting the touch events on their surface as key events instead of
> >> +      the original touch events.
> >> +
> >> +      This is of special interest if the touchscreen is shipped with a
> >> +      physical overlay on top of it where a number of buttons with some
> >> +      predefined functionality are printed. In that case a specific behavior
> >> +      is expected from those buttons instead of raw touch events.
> >> +
> >> +      The overlay-buttons properties define a per-button area as well as an
> >> +      origin relative to the real touchscreen origin. Touch events within the
> >> +      button area are reported as the key event defined in the linux,code
> >> +      property. Given that the key events do not provide coordinates, the
> >> +      button origin is only used to place the button area on the touchscreen
> >> +      surface. Any event outside the overlay-buttons object is reported as a
> >> +      touch event with no coordinate transformation.
> >> +
> >> +      The following example shows a touchscreen with a single button on it
> >> +
> >> +              Touchscreen (full area)
> >> +        ┌───────────────────────────────────┐
> >> +        │                                   │
> >> +        │                                   │
> >> +        │   ┌─────────┐                     │
> >> +        │   │button 0 │                     │
> >> +        │   │KEY_POWER│                     │
> >> +        │   └─────────┘                     │
> >> +        │                                   │
> >> +        │                                   │
> >> +       ┌└───────────────────────────────────┘
> >> +     (0,0)
> >> +
> >> +      The overlay-buttons object can  be combined with the overlay-touchscreen
> >> +      object as shown in the following example. In that case only the events
> >> +      within the overlay-touchscreen object are reported as touch events.
> >> +
> >> +                  Touchscreen (full area)
> >> +        ┌─────────┬──────────────────────────────┐
> >> +        │         │                              │
> >> +        │         │    ┌───────────────────────┐ │
> >> +        │ button 0│    │                       │ │
> >> +        │KEY_POWER│    │                       │ │
> >> +        │         │    │                       │ │
> >> +        ├─────────┤    │  overlay-touchscreen  │ │
> >> +        │         │    │                       │ │
> >> +        │         │    │                       │ │
> >> +        │ button 1│    │                       │ │
> >> +        │ KEY_INFO│   ┌└───────────────────────┘ │
> >> +        │         │(0',0')                       │
> >> +       ┌└─────────┴──────────────────────────────┘
> >> +     (0,0)
> >> +
> >> +    type: object
> > 
> > I am still confused why the buttons need to live under an 'overlay-buttons'
> > parent node, which seems like an imaginary boundary. In my view, the touch
> > surface comprises the following types of rectangular areas:
> > 
> > 1. A touchscreen, wherein granular coordinates and pressure are reported.
> > 2. A momentary button, wherein pressure is quantized into a binary value
> >    (press or release), and coordinates are ignored.
> > 
> > Any contact that falls outside of (1) and (2) is presumed to be part of a
> > border or matting, and is hence ignored.
> > 
> > Areas (1) and (2) exist in the same "plane", so why can they not reside
> > under the same parent node? The following seems much more representative
> > of the actual hardware we intend to describe in the device tree:
> > 
> > 	touchscreen {
> > 		compatible = "...";
> > 		reg = <...>;
> > 
> > 		/* raw coordinates reported here */
> > 		touch-area-1 {
> > 			x-origin = <...>;
> > 			y-origin = <...>;
> > 			x-size = <...>;
> > 			y-size = <...>;
> > 		};
> > 
> > 		/* a button */
> > 		touch-area-2a {
> > 			x-origin = <...>;
> > 			y-origin = <...>;
> > 			x-size = <...>;
> > 			y-size = <...>;
> > 			linux,code = <KEY_POWER>;
> > 		};
> > 
> > 		/* another button */
> > 		touch-area-2b {
> > 			x-origin = <...>;
> > 			y-origin = <...>;
> > 			x-size = <...>;
> > 			y-size = <...>;
> > 			linux,code = <KEY_INFO>;
> > 		};
> > 	};
> > 
> Now that I am working on the approach you suggested, I see that some
> things can get slightly more complicated. I still think that it is worth
> a try, but I would like to discuss a couple of points.
> 
> The node parsing is not that simple anymore because the touch-area nodes
> are only surrounded by the touchscreen node. Theoretically they could be
> even be defined with other properties in between. The current approach
> only needs to find the overlay-buttons parent and iterate over all the
> inner nodes(simply by calling device_get_named_child_node() and
> fwnode_for_each_child_node() the parsing is achieved in two lines +
> error checking). So maybe even if we opt for the single-object approach,
> an overlay node to group all the touch-areas could simplify the parsing.
> Or did you have a different approach in mind? Your example would turn
> into this one:
> 
> 	touchscreen {
> 		compatible = "...";
> 		reg = <...>;
> 
> 		touch-overlay {
> 			/* raw coordinates reported here */
> 			touch-area-1 {
> 				x-origin = <...>;
> 				y-origin = <...>;
> 				x-size = <...>;
> 				y-size = <...>;
> 			};
> 
> 			/* a button */
> 			touch-area-2a {
> 				x-origin = <...>;
> 				y-origin = <...>;
> 				x-size = <...>;
> 				y-size = <...>;
> 				linux,code = <KEY_POWER>;
> 			};
> 
> 			/* another button */
> 			touch-area-2b {
> 				x-origin = <...>;
> 				y-origin = <...>;
> 				x-size = <...>;
> 				y-size = <...>;
> 				linux,code = <KEY_INFO>;
> 			};
> 		};
> 	};
> In my opinion it looks cleaner as well because you are defining a
> physical object: the overlay.

I like this idea. My original example assumes one would include any node
that contains some magic substring (e.g. "touch") in the node name, but
thinking about this more, that may be a bit presumptive. It seems safer
to wrap all of the children in one newly introduced node as you have done
here.

> 
> > With this method, the driver merely stores a list head. The parsing code
> > then walks the client device node; for each touch* child encountered, it
> > allocates memory for a structure of five members, and adds it to the list.
> > 
> The button objects do not only store the keycode, but also the slot and
> if they are pressed or not. I could allocate memory for these members as
> well, but maybe an additional struct with the button-specific members
> set to NULL for the touch areas with keycode = KEY_RESERVED would make
> sense. I don't know if that's adding too much overhead for two members
> though.

It's still not clear to me why your code is responsible for storing button
state; that's the job of the input subsystem. Your code is only responsible
for reporting instantaneous state after you are told something interesting
happened (e.g. interrupt). The input core is responsible for determining
whether the most recently reported state is different than the last.

> 
> > The event handling code then simply iterates through the list and checks
> > if the coordinates reported by the hardware fall within each rectangle. If
> > so, and the keycode in the list element is equal to KEY_RESERVED (zero),
> > we assume the rectangle is of type (1); the coordinates are passed to
> > touchscreen_report_pos() and the pressure is reported as well.
> 
> There is another case to consider that might make the iteration less
> optimal, but I don't think it will be critical.
> 
> A button could be defined inside an overlay-touchscreen (no keycode)
> area. Given that the other way round (a touchscreen inside a button)
> does not make much sense, the buttons have a higher priority.
> 
> Let's take your example: imagine that your third area
> is a button inside the first one. We have to iterate through the whole
> list until we are sure we checked if there are buttons in the given
> position, but keeping in mind that the first object already has the
> right coordinates to handle the touch event. Your approach even allows
> for multiple no-key areas and we do not know if there are buttons when
> we iterate (there could be none).
> Therefore some iterations could be unnecessary, but this is probably an
> edge case that would cost at most a couple of extra iterations compared
> to a two-list approach.

I think we need to model the overlay as having only two dimensions, with
nothing "on top of" or "inside" anything else. For this case of a button
inside a touch surface, with the latter making a square doughnut shape of
sorts, the 'touch-overlay' node would have five children: two tall
rectangles (left and right), two shorter rectanges (above and below the
button), and then finally a button in the center.

Stated another way, the 'touch-overlay' node shall support an infinite
number of infinitesimally small rectangles which comprise the entire touch
surface. It shall be possible for a contact to be in zero rectangles, but
impossible for any contact to be in more than one rectangle. I appreciate
that the devil is in the details, but here we are defining the interface,
independent of the implementation.

> 
> I will keep on working on the next version with a single list while we
> clarify these points, so maybe we can save an iteration.

I see there is a v6 now; I'll take a look at that next. Thanks again for
the productive discussion!

> > Kind regards,
> > Jeff LaBundy
> 
> Best regards,
> Javier Carrasco

Kind regards,
Jeff LaBundy

[PATCH v5 3/4] Input: st1232 - add touch overlays handling

[Javier Carrasco]

Use touch-overlay to support overlay objects such as buttons and resized
frames defined in the device tree.

If an overlay-touchscreen is provided, ignore touch events outside of
the area defined by its properties. Otherwise, transform the event
coordinates to the overlay-touchscreen x and y-axis if required.

If buttons are provided, register an additional device to report key
events separately. A key event will be generated if the coordinates
of a touch event are within the area defined by the button properties.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/input/touchscreen/st1232.c | 70 ++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 6475084aee1b..d22f7d57f468 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -22,6 +22,7 @@
 #include <linux/pm_qos.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/input/touch-overlay.h>
 
 #define ST1232_TS_NAME	"st1232-ts"
 #define ST1633_TS_NAME	"st1633-ts"
@@ -56,6 +57,8 @@ struct st1232_ts_data {
 	struct touchscreen_properties prop;
 	struct dev_pm_qos_request low_latency_req;
 	struct gpio_desc *reset_gpio;
+	struct input_dev *overlay_keypad;
+	struct touch_overlay_map *overlay_map;
 	const struct st_chip_info *chip_info;
 	int read_buf_len;
 	u8 *read_buf;
@@ -130,6 +133,7 @@ static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
 static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
 {
 	struct input_dev *input = ts->input_dev;
+	struct input_dev *keypad = ts->overlay_keypad;
 	struct input_mt_pos pos[ST_TS_MAX_FINGERS];
 	u8 z[ST_TS_MAX_FINGERS];
 	int slots[ST_TS_MAX_FINGERS];
@@ -138,14 +142,20 @@ static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
 
 	for (i = 0; i < ts->chip_info->max_fingers; i++) {
 		u8 *buf = &ts->read_buf[i * 4];
+		bool contact = buf[0] & BIT(7);
+		unsigned int x, y;
 
-		if (buf[0] & BIT(7)) {
-			unsigned int x = ((buf[0] & 0x70) << 4) | buf[1];
-			unsigned int y = ((buf[0] & 0x07) << 8) | buf[2];
-
-			touchscreen_set_mt_pos(&pos[n_contacts],
-					       &ts->prop, x, y);
+		if (contact) {
+			x = ((buf[0] & 0x70) << 4) | buf[1];
+			y = ((buf[0] & 0x07) << 8) | buf[2];
+		}
+		if (touch_overlay_process_event(ts->overlay_map, keypad,
+						contact ? &x : NULL,
+						contact ? &y : NULL, i))
+			continue;
 
+		if (contact) {
+			touchscreen_set_mt_pos(&pos[n_contacts], &ts->prop, x, y);
 			/* st1232 includes a z-axis / touch strength */
 			if (ts->chip_info->have_z)
 				z[n_contacts] = ts->read_buf[i + 6];
@@ -226,6 +236,7 @@ static int st1232_ts_probe(struct i2c_client *client)
 	const struct st_chip_info *match;
 	struct st1232_ts_data *ts;
 	struct input_dev *input_dev;
+	struct input_dev *overlay_keypad;
 	u16 max_x, max_y;
 	int error;
 
@@ -263,8 +274,13 @@ static int st1232_ts_probe(struct i2c_client *client)
 	if (!input_dev)
 		return -ENOMEM;
 
+	overlay_keypad = devm_input_allocate_device(&client->dev);
+	if (!overlay_keypad)
+		return -ENOMEM;
+
 	ts->client = client;
 	ts->input_dev = input_dev;
+	ts->overlay_keypad = overlay_keypad;
 
 	ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL,
 						 GPIOD_OUT_HIGH);
@@ -286,24 +302,37 @@ static int st1232_ts_probe(struct i2c_client *client)
 
 	input_dev->name = "st1232-touchscreen";
 	input_dev->id.bustype = BUS_I2C;
+	overlay_keypad->name = "st1232-keypad";
+	overlay_keypad->id.bustype = BUS_I2C;
 
 	/* Wait until device is ready */
 	error = st1232_ts_wait_ready(ts);
 	if (error)
 		return error;
 
-	/* Read resolution from the chip */
-	error = st1232_ts_read_resolution(ts, &max_x, &max_y);
-	if (error) {
-		dev_err(&client->dev,
-			"Failed to read resolution: %d\n", error);
-		return error;
-	}
-
 	if (ts->chip_info->have_z)
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
 				     ts->chip_info->max_area, 0, 0);
 
+	/* map overlay objects if defined in the device tree */
+	ts->overlay_map = touch_overlay_map_overlay(ts->overlay_keypad);
+	if (IS_ERR(ts->overlay_map))
+		return PTR_ERR(ts->overlay_map);
+
+	if (touch_overlay_mapped_touchscreen(ts->overlay_map)) {
+		/* Read resolution from the overlay touchscreen if defined */
+		touch_overlay_get_touchscreen_abs(ts->overlay_map,
+						  &max_x, &max_y);
+	} else {
+		/* Read resolution from the chip */
+		error = st1232_ts_read_resolution(ts, &max_x, &max_y);
+		if (error) {
+			dev_err(&client->dev,
+				"Failed to read resolution: %d\n", error);
+			return error;
+		}
+	}
+
 	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
 			     0, max_x, 0, 0);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -335,6 +364,19 @@ static int st1232_ts_probe(struct i2c_client *client)
 		return error;
 	}
 
+	/* Register keypad input device if overlay buttons were defined */
+	if (touch_overlay_mapped_buttons(ts->overlay_map)) {
+		error = input_register_device(ts->overlay_keypad);
+		if (error) {
+			dev_err(&client->dev,
+				"Unable to register %s input device\n",
+				overlay_keypad->name);
+			return error;
+		}
+	} else {
+		input_free_device(ts->overlay_keypad);
+	}
+
 	i2c_set_clientdata(client, ts);
 
 	return 0;

-- 
2.39.2

[PATCH v5 4/4] dt-bindings: input: touchscreen: st1232: add touch-overlay example

[Javier Carrasco]

The st1232 driver supports the overlay-touchscreen and overlay-buttons
objects defined in the generic touchscreen bindings and implemented in
the touch-overlay module.

Add nodes for an overlay touchscreen and overlay buttons to the existing
example.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 .../input/touchscreen/sitronix,st1232.yaml         | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
index 1d8ca19fd37a..f33fc0113a67 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
@@ -37,6 +37,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/input/linux-event-codes.h>
     i2c {
             #address-cells = <1>;
             #size-cells = <0>;
@@ -46,5 +47,32 @@ examples:
                     reg = <0x55>;
                     interrupts = <2 0>;
                     gpios = <&gpio1 166 0>;
+
+                    overlay-touchscreen {
+                            x-origin = <0>;
+                            x-size = <240>;
+                            y-origin = <40>;
+                            y-size = <280>;
+                    };
+
+                    overlay-buttons {
+                            button-light {
+                                    label = "Camera light";
+                                    linux,code = <KEY_LIGHTS_TOGGLE>;
+                                    x-origin = <40>;
+                                    x-size = <40>;
+                                    y-origin = <0>;
+                                    y-size = <40>;
+                            };
+
+                            button-power {
+                                    label = "Power";
+                                    linux,code = <KEY_POWER>;
+                                    x-origin = <160>;
+                                    x-size = <40>;
+                                    y-origin = <0>;
+                                    y-size = <40>;
+                            };
+                    };
             };
     };

-- 
2.39.2