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

[PATCH v6 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, this series offers a documented,
device-tree-based solution by means of helper functions.
An implementation for a specific touchscreen driver is also included.

The functions in touch-overlay provide a simple workflow to acquire
physical objects from the device tree, map them into a list 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 v6:
- General: use a single list to manage a single type of object.
- General: swap patches to have bindings preceding the code.
- touch-overlay.c: minor code-sytle fixes.
- Link to v5: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v5-0-ff6b5c4db693@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):
      dt-bindings: touchscreen: add touch-overlay property
      Input: touch-overlay - Add touchscreen overlay handling
      dt-bindings: input: touchscreen: st1232: add touch-overlay example
      Input: st1232 - add touch overlays handling

 .../input/touchscreen/sitronix,st1232.yaml         |  29 +++
 .../bindings/input/touchscreen/touchscreen.yaml    | 119 +++++++++
 MAINTAINERS                                        |   7 +
 drivers/input/Makefile                             |   2 +-
 drivers/input/touch-overlay.c                      | 283 +++++++++++++++++++++
 drivers/input/touchscreen/st1232.c                 |  71 +++++-
 include/linux/input/touch-overlay.h                |  24 ++
 7 files changed, 520 insertions(+), 15 deletions(-)
---
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
change-id: 20230510-feature-ts_virtobj_patch-e267540aae74

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

[PATCH v6 1/4] dt-bindings: touchscreen: add touch-overlay property

[Javier Carrasco]

The touch-overlay encompasses a number of touch areas that define a
clipped touchscreen area and/or buttons with a specific functionality.

A clipped touchscreen area avoids getting events from regions that are
physically hidden by overlay frames.

For touchscreens with printed overlay buttons, sub-nodes with a suitable
key code can be defined to report key events instead of the original
touch events.

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

diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 431c13335c40..d5ac90667bef 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -87,6 +87,125 @@ properties:
   touchscreen-y-plate-ohms:
     description: Resistance of the Y-plate in Ohms
 
+  touch-overlay:
+    description: list of nodes defining segments (touch areas) on the
+      touchscreen.
+
+      This object can be used to describe a series of segments to
+      restrict the region within touch events are reported or buttons
+      with a specific functionality.
+
+      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. Printed buttons on that overlay are also a typical
+      use case.
+
+      A new touchscreen area is defined as a sub-node without a key code. If a
+      key code is defined in the sub-node, it will be interpreted as a button.
+
+      The x-origin and y-origin properties of a touchscreen area 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 touchscreen effective area.
+
+      The following example shows a new touchscreen area with the new origin
+      (0',0') for the touch events generated by the device.
+
+                   Touchscreen (full area)
+         ┌────────────────────────────────────────┐
+         │    ┌───────────────────────────────┐   │
+         │    │                               │   │
+         │    ├ y-size                        │   │
+         │    │                               │   │
+         │    │       touchscreen area        │   │
+         │    │         (no key code)         │   │
+         │    │                               │   │
+         │    │            x-size             │   │
+         │   ┌└──────────────┴────────────────┘   │
+         │(0',0')                                 │
+        ┌└────────────────────────────────────────┘
+      (0,0)
+
+     where (0',0') = (0+x-origin,0+y-origin)
+
+      Sub-nodes with key codes report the touch events on their surface as key
+      events instead.
+
+      The following example shows a touchscreen with a single button on it.
+
+              Touchscreen (full area)
+        ┌───────────────────────────────────┐
+        │                                   │
+        │                                   │
+        │   ┌─────────┐                     │
+        │   │button 0 │                     │
+        │   │KEY_POWER│                     │
+        │   └─────────┘                     │
+        │                                   │
+        │                                   │
+       ┌└───────────────────────────────────┘
+     (0,0)
+
+      Segments defining buttons and clipped toushcreen areas can be combined
+      as shown in the following example.
+      In that case only the events within the touchscreen area are reported
+      as touch events. Events within the button areas report their associated
+      key code. Any events outside the defined areas are ignored.
+
+                  Touchscreen (full area)
+        ┌─────────┬──────────────────────────────┐
+        │         │                              │
+        │         │    ┌───────────────────────┐ │
+        │ button 0│    │                       │ │
+        │KEY_POWER│    │                       │ │
+        │         │    │                       │ │
+        ├─────────┤    │   touchscreen area    │ │
+        │         │    │     (no key code)     │ │
+        │         │    │                       │ │
+        │ button 1│    │                       │ │
+        │ KEY_INFO│   ┌└───────────────────────┘ │
+        │         │(0',0')                       │
+       ┌└─────────┴──────────────────────────────┘
+     (0,0)
+
+    type: object
+
+    patternProperties:
+      '^segment-':
+        type: object
+        description:
+          Each segment is represented as a sub-node.
+        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
+
+          label:
+            $ref: /schemas/types.yaml#/definitions/string
+            description: descriptive name of the button
+
+          linux,code: true
+
+        required:
+          - x-origin
+          - y-origin
+          - x-size
+          - y-size
+
+        unevaluatedProperties: false
+
 dependencies:
   touchscreen-size-x: [ touchscreen-size-y ]
   touchscreen-size-y: [ touchscreen-size-x ]

-- 
2.39.2

Re: [PATCH v6 1/4] dt-bindings: touchscreen: add touch-overlay property

[Jeff LaBundy]

Hi Javier,

This is excellent, just one small comment below.

On Wed, Dec 20, 2023 at 09:39:43AM +0100, Javier Carrasco wrote:
> The touch-overlay encompasses a number of touch areas that define a
> clipped touchscreen area and/or buttons with a specific functionality.
> 
> A clipped touchscreen area avoids getting events from regions that are
> physically hidden by overlay frames.
> 
> For touchscreens with printed overlay buttons, sub-nodes with a suitable
> key code can be defined to report key events instead of the original
> touch events.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../bindings/input/touchscreen/touchscreen.yaml    | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> index 431c13335c40..d5ac90667bef 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> @@ -87,6 +87,125 @@ properties:
>    touchscreen-y-plate-ohms:
>      description: Resistance of the Y-plate in Ohms
>  
> +  touch-overlay:
> +    description: list of nodes defining segments (touch areas) on the
> +      touchscreen.
> +
> +      This object can be used to describe a series of segments to
> +      restrict the region within touch events are reported or buttons
> +      with a specific functionality.
> +
> +      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. Printed buttons on that overlay are also a typical
> +      use case.
> +
> +      A new touchscreen area is defined as a sub-node without a key code. If a
> +      key code is defined in the sub-node, it will be interpreted as a button.
> +
> +      The x-origin and y-origin properties of a touchscreen area 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 touchscreen effective area.
> +
> +      The following example shows a new touchscreen area with the new origin
> +      (0',0') for the touch events generated by the device.
> +
> +                   Touchscreen (full area)
> +         ┌────────────────────────────────────────┐
> +         │    ┌───────────────────────────────┐   │
> +         │    │                               │   │
> +         │    ├ y-size                        │   │
> +         │    │                               │   │
> +         │    │       touchscreen area        │   │
> +         │    │         (no key code)         │   │
> +         │    │                               │   │
> +         │    │            x-size             │   │
> +         │   ┌└──────────────┴────────────────┘   │
> +         │(0',0')                                 │
> +        ┌└────────────────────────────────────────┘
> +      (0,0)
> +
> +     where (0',0') = (0+x-origin,0+y-origin)
> +
> +      Sub-nodes with key codes report the touch events on their surface as key
> +      events instead.
> +
> +      The following example shows a touchscreen with a single button on it.
> +
> +              Touchscreen (full area)
> +        ┌───────────────────────────────────┐
> +        │                                   │
> +        │                                   │
> +        │   ┌─────────┐                     │
> +        │   │button 0 │                     │
> +        │   │KEY_POWER│                     │
> +        │   └─────────┘                     │
> +        │                                   │
> +        │                                   │
> +       ┌└───────────────────────────────────┘
> +     (0,0)
> +
> +      Segments defining buttons and clipped toushcreen areas can be combined
> +      as shown in the following example.
> +      In that case only the events within the touchscreen area are reported
> +      as touch events. Events within the button areas report their associated
> +      key code. Any events outside the defined areas are ignored.
> +
> +                  Touchscreen (full area)
> +        ┌─────────┬──────────────────────────────┐
> +        │         │                              │
> +        │         │    ┌───────────────────────┐ │
> +        │ button 0│    │                       │ │
> +        │KEY_POWER│    │                       │ │
> +        │         │    │                       │ │
> +        ├─────────┤    │   touchscreen area    │ │
> +        │         │    │     (no key code)     │ │
> +        │         │    │                       │ │
> +        │ button 1│    │                       │ │
> +        │ KEY_INFO│   ┌└───────────────────────┘ │
> +        │         │(0',0')                       │
> +       ┌└─────────┴──────────────────────────────┘
> +     (0,0)
> +
> +    type: object
> +
> +    patternProperties:
> +      '^segment-':
> +        type: object
> +        description:
> +          Each segment is represented as a sub-node.
> +        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
> +
> +          label:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            description: descriptive name of the button

Please consider replacing the word "button" with "segment"; as we see in
patch [3/4], the label can describe a touch surface and not just a button.

> +
> +          linux,code: true
> +
> +        required:
> +          - x-origin
> +          - y-origin
> +          - x-size
> +          - y-size
> +
> +        unevaluatedProperties: false
> +
>  dependencies:
>    touchscreen-size-x: [ touchscreen-size-y ]
>    touchscreen-size-y: [ touchscreen-size-x ]
> 
> -- 
> 2.39.2
> 
> 

With that changed, feel free to add: 

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

Obviously we need Dmitry and the binding maintainers to review as well; I am
merely expressing my own agreement as a future customer of this function.

Kind regards,
Jeff LaBundy

Re: [PATCH v6 1/4] dt-bindings: touchscreen: add touch-overlay property

[Rob Herring]

On Wed, Dec 20, 2023 at 09:39:43AM +0100, Javier Carrasco wrote:
> The touch-overlay encompasses a number of touch areas that define a
> clipped touchscreen area and/or buttons with a specific functionality.
> 
> A clipped touchscreen area avoids getting events from regions that are
> physically hidden by overlay frames.
> 
> For touchscreens with printed overlay buttons, sub-nodes with a suitable
> key code can be defined to report key events instead of the original
> touch events.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../bindings/input/touchscreen/touchscreen.yaml    | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay 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.

Note that the current implementation allows for multiple definitions
of touchscreen areas (regions that report touch events), but only the
first one will be used for the touchscreen device that the consumers
typically provide.
Should the need for multiple touchscreen areas arise, additional
touchscreen devices would be required at the consumer side.
There is no limitation in the number of touch areas defined as buttons.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 MAINTAINERS                         |   7 +
 drivers/input/Makefile              |   2 +-
 drivers/input/touch-overlay.c       | 283 ++++++++++++++++++++++++++++++++++++
 include/linux/input/touch-overlay.h |  24 +++
 4 files changed, 315 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 97f51d5ec1cf..3218d8694735 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22031,6 +22031,13 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/toshiba-wmi.c
 
+TOUCH OVERLAY
+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
+
 TPM DEVICE DRIVER
 M:	Peter Huewe <peterhuewe@gmx.de>
 M:	Jarkko Sakkinen <jarkko@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..0a0646ceb755
--- /dev/null
+++ b/drivers/input/touch-overlay.c
@@ -0,0 +1,283 @@
+// 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/list.h>
+#include <linux/module.h>
+#include <linux/property.h>
+
+struct button {
+	u32 key;
+	bool pressed;
+	int slot;
+};
+
+struct touch_overlay_segment {
+	struct list_head list;
+	u32 x_origin;
+	u32 y_origin;
+	u32 x_size;
+	u32 y_size;
+	struct button *btn;
+};
+
+static int touch_overlay_get_segment(struct fwnode_handle *segment_node,
+				     struct touch_overlay_segment *segment,
+				     struct input_dev *keypad,
+				     struct device *dev)
+{
+	int error;
+
+	error = fwnode_property_read_u32(segment_node, "x-origin",
+					 &segment->x_origin);
+	if (error)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "y-origin",
+					 &segment->y_origin);
+	if (error)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "x-size",
+					 &segment->x_size);
+	if (error)
+		return error;
+
+	error = fwnode_property_read_u32(segment_node, "y-size",
+					 &segment->y_size);
+	if (error)
+		return error;
+
+	if (fwnode_property_present(segment_node, "linux,code")) {
+		segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
+					    GFP_KERNEL);
+		if (!segment->btn)
+			return -ENOMEM;
+
+		error = fwnode_property_read_u32(segment_node, "linux,code",
+						 &segment->btn->key);
+		if (error)
+			return error;
+
+		input_set_capability(keypad, EV_KEY, segment->btn->key);
+	}
+
+	return 0;
+}
+
+/**
+ * touch_overlay_map - map overlay objects from the device tree and set
+ * key capabilities if buttons are defined.
+ * @list: pointer to the list that will hold the segments
+ * @keypad: pointer to the already allocated input_dev
+ *
+ * Returns 0 on success and error number otherwise.
+ *
+ * If a keypad input device is provided and overlay buttons are defined,
+ * its button capabilities are set accordingly.
+ */
+int touch_overlay_map(struct list_head *list, struct input_dev *keypad)
+{
+	struct fwnode_handle *overlay, *fw_segment;
+	struct device *dev = keypad->dev.parent;
+	struct touch_overlay_segment *segment;
+	int error;
+
+	overlay = device_get_named_child_node(dev, "touch-overlay");
+	if (!overlay)
+		return 0;
+
+	fwnode_for_each_child_node(overlay, fw_segment) {
+		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
+		if (!segment) {
+			error = -ENOMEM;
+			goto put_overlay;
+		}
+		error = touch_overlay_get_segment(fw_segment, segment, keypad,
+						  dev);
+		if (error)
+			goto put_overlay;
+
+		list_add_tail(&segment->list, list);
+	}
+
+put_overlay:
+	fwnode_handle_put(overlay);
+	return error;
+}
+EXPORT_SYMBOL(touch_overlay_map);
+
+/**
+ * touch_overlay_get_touchscreen_abs - get abs size from the touchscreen area.
+ * @list: pointer to the list that holds the segments
+ * @x: horizontal abs
+ * @y: vertical abs
+ */
+void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y)
+{
+	struct touch_overlay_segment *segment;
+	struct list_head *ptr;
+
+	list_for_each(ptr, list) {
+		segment = list_entry(ptr, struct touch_overlay_segment, list);
+		if (!segment->btn) {
+			*x = segment->x_size - 1;
+			*y = segment->y_size - 1;
+			break;
+		}
+	}
+}
+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 a touchscreen area is mapped
+ * @list: pointer to the list that holds the segments
+ *
+ * Returns true if a touchscreen area is mapped or false otherwise.
+ */
+bool touch_overlay_mapped_touchscreen(struct list_head *list)
+{
+	struct touch_overlay_segment *segment;
+	struct list_head *ptr;
+
+	list_for_each(ptr, list) {
+		segment = list_entry(ptr, struct touch_overlay_segment, list);
+		if (!segment->btn)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
+
+/**
+ * touch_overlay_mapped_buttons - check if overlay buttons are mapped
+ * @list: pointer to the list that holds the segments
+ *
+ * Returns true if overlay buttons mapped or false otherwise.
+ */
+bool touch_overlay_mapped_buttons(struct list_head *list)
+{
+	struct touch_overlay_segment *segment;
+	struct list_head *ptr;
+
+	list_for_each(ptr, list) {
+		segment = list_entry(ptr, struct touch_overlay_segment, list);
+		if (segment->btn)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_buttons);
+
+static bool touch_overlay_mt_on_touchscreen(struct list_head *list,
+					    u32 *x, u32 *y)
+{
+	struct touch_overlay_segment *segment;
+	bool contact = x && y;
+	struct list_head *ptr;
+
+	/* Let the caller handle events with no coordinates (release) */
+	if (!contact)
+		return false;
+
+	list_for_each(ptr, list) {
+		segment = list_entry(ptr, struct touch_overlay_segment, list);
+		if (!segment->btn &&
+		    touch_overlay_segment_event(segment, *x, *y)) {
+			*x -= segment->x_origin;
+			*y -= segment->y_origin;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static bool touch_overlay_button_event(struct input_dev *input,
+				       struct touch_overlay_segment *segment,
+				       const u32 *x, const u32 *y, u32 slot)
+{
+	bool contact = x && y;
+
+	if (!contact && segment->btn->pressed && segment->btn->slot == slot) {
+		segment->btn->pressed = false;
+		input_report_key(input, segment->btn->key, false);
+		input_sync(input);
+		return true;
+	} else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
+		segment->btn->pressed = true;
+		segment->btn->slot = slot;
+		input_report_key(input, segment->btn->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.
+ * @list: pointer to the list that holds the segments
+ * @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 list_head *list,
+				 struct input_dev *input,
+				 u32 *x, u32 *y, u32 slot)
+{
+	struct touch_overlay_segment *segment;
+	struct list_head *ptr;
+
+	/*
+	 * buttons must be prioritized over overlay touchscreens to account for
+	 * overlappings e.g. a button inside the touchscreen area.
+	 */
+	list_for_each(ptr, list) {
+		segment = list_entry(ptr, struct touch_overlay_segment, list);
+		if (segment->btn &&
+		    touch_overlay_button_event(input, segment, 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(list, 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..df974eb46dd4
--- /dev/null
+++ b/include/linux/input/touch-overlay.h
@@ -0,0 +1,24 @@
+/* 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;
+
+int touch_overlay_map(struct list_head *list, struct input_dev *keypad);
+
+void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y);
+
+bool touch_overlay_mapped_touchscreen(struct list_head *list);
+
+bool touch_overlay_mapped_buttons(struct list_head *list);
+
+bool touch_overlay_process_event(struct list_head *list, struct input_dev *input,
+				 u32 *x, u32 *y, u32 slot);
+
+#endif

-- 
2.39.2

Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling

[Jeff LaBundy]

Hi Javier,

Truly excellent work; I'm glad to see this keep moving forward, and the
new linked list approach looks well-suited to this application.

On Wed, Dec 20, 2023 at 09:39:44AM +0100, 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.
> 
> Note that the current implementation allows for multiple definitions
> of touchscreen areas (regions that report touch events), but only the
> first one will be used for the touchscreen device that the consumers
> typically provide.
> Should the need for multiple touchscreen areas arise, additional
> touchscreen devices would be required at the consumer side.
> There is no limitation in the number of touch areas defined as buttons.

Having reviewed this version in detail, it's clear how the implementation
imposes this restriction. However, it's not clear why we have to have this
restriction straight out of the gate; it also breaks the "square doughnut"
example we discussed in v5, where a button resides inside a touch surface
which is split into four discrete rectangles that report X/Y coordinates
relative to the same origin.

From my naive point of view, a driver should merely pass a contact's ID
(for HW that can track contacts), coordinates, and pressure. Your helper(s)
are then responisble for iterating over the list, determining the segment
in which the coordinates fall, and then reporting the event by way of
input_report_abs() or input_report_key() based on whether or not a keycode
is defined.

I think the problem with that approach is that touchscreen drivers only
report coordinates when the pressure is nonzero. The process of dropping
a contact, i.e. button release for some segments, happens inside input-mt
by virtue of the driver calling input_mt_sync_frame().

It makes sense now why you are duplicating the contact tracking to a degree
here. Therefore, it's starting to look more and more like the overlay segment
handling needs to move into the input-mt core, where much of the information
you need already exists.

If we look at input_mt_report_pointer_emulation(), the concept of button
release is already happening; all we really want to do here is gently
expand the core to understand that some ranges of coordinates are simply
quantized to a keycode with binary pressure (i.e. press/release).

In addition to removing duplicate code as well as the restriction of supporting
only one X/Y surface, moving overlay support into the input-mt core would
remove the need to modify each touchscreen driver one at a time with what
are largely the same nontrivial changes. If we think about it more, the
touchscreen controller itself is not changing, so the driver really shouldn't
have to change much either.

Stated another way, I think it's a better design pattern if we let drivers
continue to do their job of merely lobbing hardware state to the input
subsytem via input_mt_slot(), touchscreen_report_pos() and input_mt_sync_frame(),
then leave it to the input subsystem alone to iterate over the list and
determine whether some coordinates must be handled differently.

The main drawback to this approach is that the overlay buttons would need
to go back to being part of the touchscreen input device as in v1, as opposed
to giving the driver the flexibility of splitting the buttons and X/Y surfaces
into two separate input devices.

When we first discussed this with Peter, we agreed that splitting them into two
input devices grants the most flexibility, in case user space opts to inhibit
one but not the other, etc. However since the buttons and X/Y surfaces are all
part of the same physical substrate, it seems the chances of user space being
interested in one but not the other are low.

Furthermore, folding the buttons and X/Y surfaces back into the same input
device would remove the need for each touchscreen driver to preemptively
allocate a second input device, but then remove it later as in patch [4/4]
in case the helpers did not find any buttons.

What are your thoughts on evolving the approach in this way? It's obviously
another big change and carries some risk to the core, so I'm curious to hear
Dmitry's and others' thoughts as well. I appreciate that you've been iterating
on this for some time, and good is not the enemy of great; therefore, maybe
a compromise is to move forward with the current approach in support of the
hardware you have today, then work it into the input-mt core over time. But
it would be nice to avoid ripping up participating touchscreen drivers twice.

Thank you for your patience and continued effort. In the meantime, please note
some minor comments that are independent of this architectural decision.

> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/input/Makefile              |   2 +-
>  drivers/input/touch-overlay.c       | 283 ++++++++++++++++++++++++++++++++++++
>  include/linux/input/touch-overlay.h |  24 +++
>  4 files changed, 315 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97f51d5ec1cf..3218d8694735 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22031,6 +22031,13 @@ L:	platform-driver-x86@vger.kernel.org
>  S:	Maintained
>  F:	drivers/platform/x86/toshiba-wmi.c
>  
> +TOUCH OVERLAY
> +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
> +
>  TPM DEVICE DRIVER
>  M:	Peter Huewe <peterhuewe@gmx.de>
>  M:	Jarkko Sakkinen <jarkko@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..0a0646ceb755
> --- /dev/null
> +++ b/drivers/input/touch-overlay.c
> @@ -0,0 +1,283 @@
> +// 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/list.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +
> +struct button {
> +	u32 key;
> +	bool pressed;
> +	int slot;
> +};
> +
> +struct touch_overlay_segment {
> +	struct list_head list;
> +	u32 x_origin;
> +	u32 y_origin;
> +	u32 x_size;
> +	u32 y_size;
> +	struct button *btn;

I think you can simply declare the struct itself here as opposed to a pointer to
one; this would avoid a second call to devm_kzalloc().

> +};
> +
> +static int touch_overlay_get_segment(struct fwnode_handle *segment_node,
> +				     struct touch_overlay_segment *segment,
> +				     struct input_dev *keypad,
> +				     struct device *dev)
> +{
> +	int error;
> +
> +	error = fwnode_property_read_u32(segment_node, "x-origin",
> +					 &segment->x_origin);
> +	if (error)
> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "y-origin",
> +					 &segment->y_origin);
> +	if (error)
> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "x-size",
> +					 &segment->x_size);
> +	if (error)
> +		return error;
> +
> +	error = fwnode_property_read_u32(segment_node, "y-size",
> +					 &segment->y_size);
> +	if (error)
> +		return error;
> +
> +	if (fwnode_property_present(segment_node, "linux,code")) {

Instead of walking the device tree twice by calling fwnode_property_present()
followed by fwnode_property_read_u32(), you can simply check whether the
latter returns -EINVAL, which indicates the optional property was absent.

> +		segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
> +					    GFP_KERNEL);
> +		if (!segment->btn)
> +			return -ENOMEM;
> +
> +		error = fwnode_property_read_u32(segment_node, "linux,code",
> +						 &segment->btn->key);
> +		if (error)
> +			return error;
> +
> +		input_set_capability(keypad, EV_KEY, segment->btn->key);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * touch_overlay_map - map overlay objects from the device tree and set
> + * key capabilities if buttons are defined.
> + * @list: pointer to the list that will hold the segments
> + * @keypad: pointer to the already allocated input_dev
> + *
> + * Returns 0 on success and error number otherwise.
> + *
> + * If a keypad input device is provided and overlay buttons are defined,
> + * its button capabilities are set accordingly.
> + */
> +int touch_overlay_map(struct list_head *list, struct input_dev *keypad)
> +{
> +	struct fwnode_handle *overlay, *fw_segment;
> +	struct device *dev = keypad->dev.parent;
> +	struct touch_overlay_segment *segment;
> +	int error;
> +
> +	overlay = device_get_named_child_node(dev, "touch-overlay");
> +	if (!overlay)
> +		return 0;
> +
> +	fwnode_for_each_child_node(overlay, fw_segment) {
> +		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> +		if (!segment) {
> +			error = -ENOMEM;
> +			goto put_overlay;

For this and the below case where you exit the loop early in case of an
error, you must call fwnode_handle_put(fw_segment) manually. The reference
count is handled automatically only when the loop iterates and terminates
naturally.

Since nothing else happens between the loop and the 'put_overlay' label,
you can also replace the goto with a break and remove the label altogether.

> +		}
> +		error = touch_overlay_get_segment(fw_segment, segment, keypad,
> +						  dev);
> +		if (error)
> +			goto put_overlay;
> +
> +		list_add_tail(&segment->list, list);
> +	}
> +
> +put_overlay:
> +	fwnode_handle_put(overlay);
> +	return error;
> +}
> +EXPORT_SYMBOL(touch_overlay_map);
> +
> +/**
> + * touch_overlay_get_touchscreen_abs - get abs size from the touchscreen area.
> + * @list: pointer to the list that holds the segments
> + * @x: horizontal abs
> + * @y: vertical abs
> + */
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y)
> +{
> +	struct touch_overlay_segment *segment;
> +	struct list_head *ptr;
> +
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (!segment->btn) {
> +			*x = segment->x_size - 1;
> +			*y = segment->y_size - 1;
> +			break;
> +		}
> +	}
> +}
> +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 a touchscreen area is mapped
> + * @list: pointer to the list that holds the segments
> + *
> + * Returns true if a touchscreen area is mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_touchscreen(struct list_head *list)
> +{
> +	struct touch_overlay_segment *segment;
> +	struct list_head *ptr;
> +
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (!segment->btn)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +/**
> + * touch_overlay_mapped_buttons - check if overlay buttons are mapped
> + * @list: pointer to the list that holds the segments
> + *
> + * Returns true if overlay buttons mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_buttons(struct list_head *list)
> +{
> +	struct touch_overlay_segment *segment;
> +	struct list_head *ptr;
> +
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (segment->btn)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_buttons);
> +
> +static bool touch_overlay_mt_on_touchscreen(struct list_head *list,
> +					    u32 *x, u32 *y)
> +{
> +	struct touch_overlay_segment *segment;
> +	bool contact = x && y;
> +	struct list_head *ptr;
> +
> +	/* Let the caller handle events with no coordinates (release) */
> +	if (!contact)
> +		return false;
> +
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (!segment->btn &&
> +		    touch_overlay_segment_event(segment, *x, *y)) {
> +			*x -= segment->x_origin;
> +			*y -= segment->y_origin;
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +static bool touch_overlay_button_event(struct input_dev *input,
> +				       struct touch_overlay_segment *segment,
> +				       const u32 *x, const u32 *y, u32 slot)
> +{
> +	bool contact = x && y;
> +
> +	if (!contact && segment->btn->pressed && segment->btn->slot == slot) {
> +		segment->btn->pressed = false;
> +		input_report_key(input, segment->btn->key, false);
> +		input_sync(input);
> +		return true;
> +	} else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
> +		segment->btn->pressed = true;
> +		segment->btn->slot = slot;
> +		input_report_key(input, segment->btn->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.
> + * @list: pointer to the list that holds the segments
> + * @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 list_head *list,
> +				 struct input_dev *input,
> +				 u32 *x, u32 *y, u32 slot)
> +{
> +	struct touch_overlay_segment *segment;
> +	struct list_head *ptr;
> +
> +	/*
> +	 * buttons must be prioritized over overlay touchscreens to account for
> +	 * overlappings e.g. a button inside the touchscreen area.
> +	 */
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (segment->btn &&
> +		    touch_overlay_button_event(input, segment, 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(list, 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..df974eb46dd4
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,24 @@
> +/* 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;
> +
> +int touch_overlay_map(struct list_head *list, struct input_dev *keypad);
> +
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y);
> +
> +bool touch_overlay_mapped_touchscreen(struct list_head *list);
> +
> +bool touch_overlay_mapped_buttons(struct list_head *list);
> +
> +bool touch_overlay_process_event(struct list_head *list, struct input_dev *input,
> +				 u32 *x, u32 *y, u32 slot);
> +
> +#endif
> 
> -- 
> 2.39.2
> 
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling

[Javier Carrasco]

Hi Jeff,

On 30.12.23 21:29, Jeff LaBundy wrote:
> Having reviewed this version in detail, it's clear how the implementation
> imposes this restriction. However, it's not clear why we have to have this
> restriction straight out of the gate; it also breaks the "square doughnut"
> example we discussed in v5, where a button resides inside a touch surface
> which is split into four discrete rectangles that report X/Y coordinates
> relative to the same origin.
> 
> From my naive point of view, a driver should merely pass a contact's ID
> (for HW that can track contacts), coordinates, and pressure. Your helper(s)
> are then responisble for iterating over the list, determining the segment
> in which the coordinates fall, and then reporting the event by way of
> input_report_abs() or input_report_key() based on whether or not a keycode
> is defined.
> 
> I think the problem with that approach is that touchscreen drivers only
> report coordinates when the pressure is nonzero. The process of dropping
> a contact, i.e. button release for some segments, happens inside input-mt
> by virtue of the driver calling input_mt_sync_frame().
> 
> It makes sense now why you are duplicating the contact tracking to a degree
> here. Therefore, it's starting to look more and more like the overlay segment
> handling needs to move into the input-mt core, where much of the information
> you need already exists.
> 
> If we look at input_mt_report_pointer_emulation(), the concept of button
> release is already happening; all we really want to do here is gently
> expand the core to understand that some ranges of coordinates are simply
> quantized to a keycode with binary pressure (i.e. press/release).
> 
> In addition to removing duplicate code as well as the restriction of supporting
> only one X/Y surface, moving overlay support into the input-mt core would
> remove the need to modify each touchscreen driver one at a time with what
> are largely the same nontrivial changes. If we think about it more, the
> touchscreen controller itself is not changing, so the driver really shouldn't
> have to change much either.
> 
> Stated another way, I think it's a better design pattern if we let drivers
> continue to do their job of merely lobbing hardware state to the input
> subsytem via input_mt_slot(), touchscreen_report_pos() and input_mt_sync_frame(),
> then leave it to the input subsystem alone to iterate over the list and
> determine whether some coordinates must be handled differently.
> 
> The main drawback to this approach is that the overlay buttons would need
> to go back to being part of the touchscreen input device as in v1, as opposed
> to giving the driver the flexibility of splitting the buttons and X/Y surfaces
> into two separate input devices.
> 
> When we first discussed this with Peter, we agreed that splitting them into two
> input devices grants the most flexibility, in case user space opts to inhibit
> one but not the other, etc. However since the buttons and X/Y surfaces are all
> part of the same physical substrate, it seems the chances of user space being
> interested in one but not the other are low.
> 
> Furthermore, folding the buttons and X/Y surfaces back into the same input
> device would remove the need for each touchscreen driver to preemptively
> allocate a second input device, but then remove it later as in patch [4/4]
> in case the helpers did not find any buttons.
> 
> What are your thoughts on evolving the approach in this way? It's obviously
> another big change and carries some risk to the core, so I'm curious to hear
> Dmitry's and others' thoughts as well. I appreciate that you've been iterating
> on this for some time, and good is not the enemy of great; therefore, maybe
> a compromise is to move forward with the current approach in support of the
> hardware you have today, then work it into the input-mt core over time. But
> it would be nice to avoid ripping up participating touchscreen drivers twice.
> 
> Thank you for your patience and continued effort. In the meantime, please note
> some minor comments that are independent of this architectural decision.
> 

Thanks again for your thorough reviews and proposals to improve the code.

I am basically open to any solution that improves the quality of the
feature. If I get you right, moving everything to the input-mt core
would hide the overlay stuff from the device drivers (which sounds good)
and a bit of code could be simplified by using the existing infrastructure.

On the other hand, adding this feature to the input-mt core right away
increases the risk of breaking things that many users need. We are
already using this feature in some prototypes since v1 without any issue
so far, but it would be great if it could be tested under different
circumstances (hardware, configurations, etc.) before it goes into the
core, wouldn't it?

I would also like to know what more experienced people think about it,
if we should go all out and add it to the input-mt core now or as you
also suggested, move forward with the current approach and let it
"mature" first. The cost of that would be modifying device driver code
twice, but I suppose that not so many drivers will add this feature in
the next kernel iterations... maybe you? it sounds like that you might
have a use case for it :)

>> +struct button {
>> +	u32 key;
>> +	bool pressed;
>> +	int slot;
>> +};
>> +
>> +struct touch_overlay_segment {
>> +	struct list_head list;
>> +	u32 x_origin;
>> +	u32 y_origin;
>> +	u32 x_size;
>> +	u32 y_size;
>> +	struct button *btn;
> 
> I think you can simply declare the struct itself here as opposed to a pointer to
> one; this would avoid a second call to devm_kzalloc().
> 

That was the other option I mentioned in my reply and I am fine with the
modification you propose.

>> +	if (fwnode_property_present(segment_node, "linux,code")) {
> 
> Instead of walking the device tree twice by calling fwnode_property_present()
> followed by fwnode_property_read_u32(), you can simply check whether the
> latter returns -EINVAL, which indicates the optional property was absent.
> 

Ack.

>> +		segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
>> +					    GFP_KERNEL);
>> +		if (!segment->btn)
>> +			return -ENOMEM;
>> +


>> +	fwnode_for_each_child_node(overlay, fw_segment) {
>> +		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
>> +		if (!segment) {
>> +			error = -ENOMEM;
>> +			goto put_overlay;
> 
> For this and the below case where you exit the loop early in case of an
> error, you must call fwnode_handle_put(fw_segment) manually. The reference
> count is handled automatically only when the loop iterates and terminates
> naturally.
> 
> Since nothing else happens between the loop and the 'put_overlay' label,
> you can also replace the goto with a break and remove the label altogether.
> 

Ack.

> 
> Kind regards,
> Jeff LaBundy


Thanks and best regards,
Javier Carrasco

Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling

[Jeff LaBundy]

Hi Javier,

On Fri, Jan 05, 2024 at 08:21:25PM +0100, Javier Carrasco wrote:
> Hi Jeff,
> 
> On 30.12.23 21:29, Jeff LaBundy wrote:
> > Having reviewed this version in detail, it's clear how the implementation
> > imposes this restriction. However, it's not clear why we have to have this
> > restriction straight out of the gate; it also breaks the "square doughnut"
> > example we discussed in v5, where a button resides inside a touch surface
> > which is split into four discrete rectangles that report X/Y coordinates
> > relative to the same origin.
> > 
> > From my naive point of view, a driver should merely pass a contact's ID
> > (for HW that can track contacts), coordinates, and pressure. Your helper(s)
> > are then responisble for iterating over the list, determining the segment
> > in which the coordinates fall, and then reporting the event by way of
> > input_report_abs() or input_report_key() based on whether or not a keycode
> > is defined.
> > 
> > I think the problem with that approach is that touchscreen drivers only
> > report coordinates when the pressure is nonzero. The process of dropping
> > a contact, i.e. button release for some segments, happens inside input-mt
> > by virtue of the driver calling input_mt_sync_frame().
> > 
> > It makes sense now why you are duplicating the contact tracking to a degree
> > here. Therefore, it's starting to look more and more like the overlay segment
> > handling needs to move into the input-mt core, where much of the information
> > you need already exists.
> > 
> > If we look at input_mt_report_pointer_emulation(), the concept of button
> > release is already happening; all we really want to do here is gently
> > expand the core to understand that some ranges of coordinates are simply
> > quantized to a keycode with binary pressure (i.e. press/release).
> > 
> > In addition to removing duplicate code as well as the restriction of supporting
> > only one X/Y surface, moving overlay support into the input-mt core would
> > remove the need to modify each touchscreen driver one at a time with what
> > are largely the same nontrivial changes. If we think about it more, the
> > touchscreen controller itself is not changing, so the driver really shouldn't
> > have to change much either.
> > 
> > Stated another way, I think it's a better design pattern if we let drivers
> > continue to do their job of merely lobbing hardware state to the input
> > subsytem via input_mt_slot(), touchscreen_report_pos() and input_mt_sync_frame(),
> > then leave it to the input subsystem alone to iterate over the list and
> > determine whether some coordinates must be handled differently.
> > 
> > The main drawback to this approach is that the overlay buttons would need
> > to go back to being part of the touchscreen input device as in v1, as opposed
> > to giving the driver the flexibility of splitting the buttons and X/Y surfaces
> > into two separate input devices.
> > 
> > When we first discussed this with Peter, we agreed that splitting them into two
> > input devices grants the most flexibility, in case user space opts to inhibit
> > one but not the other, etc. However since the buttons and X/Y surfaces are all
> > part of the same physical substrate, it seems the chances of user space being
> > interested in one but not the other are low.
> > 
> > Furthermore, folding the buttons and X/Y surfaces back into the same input
> > device would remove the need for each touchscreen driver to preemptively
> > allocate a second input device, but then remove it later as in patch [4/4]
> > in case the helpers did not find any buttons.
> > 
> > What are your thoughts on evolving the approach in this way? It's obviously
> > another big change and carries some risk to the core, so I'm curious to hear
> > Dmitry's and others' thoughts as well. I appreciate that you've been iterating
> > on this for some time, and good is not the enemy of great; therefore, maybe
> > a compromise is to move forward with the current approach in support of the
> > hardware you have today, then work it into the input-mt core over time. But
> > it would be nice to avoid ripping up participating touchscreen drivers twice.
> > 
> > Thank you for your patience and continued effort. In the meantime, please note
> > some minor comments that are independent of this architectural decision.
> > 
> 
> Thanks again for your thorough reviews and proposals to improve the code.
> 
> I am basically open to any solution that improves the quality of the
> feature. If I get you right, moving everything to the input-mt core
> would hide the overlay stuff from the device drivers (which sounds good)
> and a bit of code could be simplified by using the existing infrastructure.

Yes, that is the idea.

> 
> On the other hand, adding this feature to the input-mt core right away
> increases the risk of breaking things that many users need. We are
> already using this feature in some prototypes since v1 without any issue
> so far, but it would be great if it could be tested under different
> circumstances (hardware, configurations, etc.) before it goes into the
> core, wouldn't it?

I agree with you. Thinking about this more, immediately introducing this
feature to the core is a relatively high risk that would be shared by all
users. I like your idea of introducing a preliminary version first before
making heavy-handed changes. That's the beauty of helper functions; they
only impact users who explicitly opt in.

> 
> I would also like to know what more experienced people think about it,
> if we should go all out and add it to the input-mt core now or as you
> also suggested, move forward with the current approach and let it
> "mature" first. The cost of that would be modifying device driver code
> twice, but I suppose that not so many drivers will add this feature in
> the next kernel iterations... maybe you? it sounds like that you might
> have a use case for it :)

I don't have an immediate use case, but I've been looking at this from
the perspective of a future customer of it. Maybe the right path forward
is as follows:

1. Stick with the same general architecture of v6 and its "limitations",
   which in practice are unlikely to be encountered. I imagine the overlay
   layout you have been using would be the most common use case.
2. Make the handful of small changes that have been suggested thus far.
3. Consider updating patch [4/4] to combine the touchscreen and buttons
   into the same input device as you had in v1. This sets a little simpler
   precedent for the first user of these helpers. If later these helpers
   do get absorbed into the core, thereby forcing a single input device,
   the st1232 would continue to appear the same to user space.

Does this seem reasonable?

> 
> >> +struct button {
> >> +	u32 key;
> >> +	bool pressed;
> >> +	int slot;
> >> +};
> >> +
> >> +struct touch_overlay_segment {
> >> +	struct list_head list;
> >> +	u32 x_origin;
> >> +	u32 y_origin;
> >> +	u32 x_size;
> >> +	u32 y_size;
> >> +	struct button *btn;
> > 
> > I think you can simply declare the struct itself here as opposed to a pointer to
> > one; this would avoid a second call to devm_kzalloc().
> > 
> 
> That was the other option I mentioned in my reply and I am fine with the
> modification you propose.
> 
> >> +	if (fwnode_property_present(segment_node, "linux,code")) {
> > 
> > Instead of walking the device tree twice by calling fwnode_property_present()
> > followed by fwnode_property_read_u32(), you can simply check whether the
> > latter returns -EINVAL, which indicates the optional property was absent.
> > 
> 
> Ack.
> 
> >> +		segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
> >> +					    GFP_KERNEL);
> >> +		if (!segment->btn)
> >> +			return -ENOMEM;
> >> +
> 
> 
> >> +	fwnode_for_each_child_node(overlay, fw_segment) {
> >> +		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> >> +		if (!segment) {
> >> +			error = -ENOMEM;
> >> +			goto put_overlay;
> > 
> > For this and the below case where you exit the loop early in case of an
> > error, you must call fwnode_handle_put(fw_segment) manually. The reference
> > count is handled automatically only when the loop iterates and terminates
> > naturally.
> > 
> > Since nothing else happens between the loop and the 'put_overlay' label,
> > you can also replace the goto with a break and remove the label altogether.
> > 
> 
> Ack.
> 
> > 
> > Kind regards,
> > Jeff LaBundy
> 
> 
> Thanks and best regards,
> Javier Carrasco

Kind regards,
Jeff LaBundy

Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling

[Javier Carrasco]

On 11.01.24 14:55, Jeff LaBundy wrote:
> Hi Javier,
> 
> I agree with you. Thinking about this more, immediately introducing this
> feature to the core is a relatively high risk that would be shared by all
> users. I like your idea of introducing a preliminary version first before
> making heavy-handed changes. That's the beauty of helper functions; they
> only impact users who explicitly opt in.
> 
> I don't have an immediate use case, but I've been looking at this from
> the perspective of a future customer of it. Maybe the right path forward
> is as follows:
> 
> 1. Stick with the same general architecture of v6 and its "limitations",
>    which in practice are unlikely to be encountered. I imagine the overlay
>    layout you have been using would be the most common use case.
> 2. Make the handful of small changes that have been suggested thus far.
> 3. Consider updating patch [4/4] to combine the touchscreen and buttons
>    into the same input device as you had in v1. This sets a little simpler
>    precedent for the first user of these helpers. If later these helpers
>    do get absorbed into the core, thereby forcing a single input device,
>    the st1232 would continue to appear the same to user space.
> 
> Does this seem reasonable?
> 

It seems reasonable, so I will go for that approach in v7: single input
device and less modifications in the consumer drivers.

Best regards,
Javier Carrasco

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

[Javier Carrasco]

The touch-overaly feature adds support for segments (touch areas) on the
touchscreen surface that represent overlays with clipped touchscreen
areas and printed buttons.

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

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

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
index 1d8ca19fd37a..e7ee7a0d74c4 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,33 @@ examples:
                     reg = <0x55>;
                     interrupts = <2 0>;
                     gpios = <&gpio1 166 0>;
+
+                    touch-overlay {
+                            segment-0 {
+                                    label = "Touchscreen";
+                                    x-origin = <0>;
+                                    x-size = <240>;
+                                    y-origin = <40>;
+                                    y-size = <280>;
+                            };
+
+                            segment-1a {
+                                    label = "Camera light";
+                                    linux,code = <KEY_LIGHTS_TOGGLE>;
+                                    x-origin = <40>;
+                                    x-size = <40>;
+                                    y-origin = <0>;
+                                    y-size = <40>;
+                            };
+
+                            segment-2a {
+                                    label = "Power";
+                                    linux,code = <KEY_POWER>;
+                                    x-origin = <160>;
+                                    x-size = <40>;
+                                    y-origin = <0>;
+                                    y-size = <40>;
+                            };
+                    };
             };
     };

-- 
2.39.2

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

[Rob Herring]

On Wed, Dec 20, 2023 at 09:39:45AM +0100, Javier Carrasco wrote:
> The touch-overaly feature adds support for segments (touch areas) on the
> touchscreen surface that represent overlays with clipped touchscreen
> areas and printed buttons.
> 
> Add nodes for a clipped touchscreen and overlay buttons to the existing
> example.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../input/touchscreen/sitronix,st1232.yaml         | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Javier Carrasco]

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

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 | 71 ++++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 6475084aee1b..5684cf04bfa1 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,7 +57,9 @@ 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;
 	const struct st_chip_info *chip_info;
+	struct list_head touch_overlay_list;
 	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->touch_overlay_list, 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,38 @@ 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 */
+	INIT_LIST_HEAD(&ts->touch_overlay_list);
+	error = touch_overlay_map(&ts->touch_overlay_list, ts->overlay_keypad);
+	if (error)
+		return error;
+
+	if (touch_overlay_mapped_touchscreen(&ts->touch_overlay_list)) {
+		/* Read resolution from the overlay touchscreen if defined */
+		touch_overlay_get_touchscreen_abs(&ts->touch_overlay_list,
+						  &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 +365,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->touch_overlay_list)) {
+		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