Message-ID: <ZZB9x+0eNL8e+xI3@nixie71>
Date: Sat, 30 Dec 2023 14:29:59 -0600
From: Jeff LaBundy <jeff@...undy.com>
To: Javier Carrasco <javier.carrasco@...fvision.net>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Henrik Rydberg <rydberg@...math.org>,
	Bastian Hecht <hechtb@...il.com>,
	Michael Riesch <michael.riesch@...fvision.net>,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay
 handling

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@...fvision.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@...r.kernel.org
>  S:	Maintained
>  F:	drivers/platform/x86/toshiba-wmi.c
>  
> +TOUCH OVERLAY
> +M:	Javier Carrasco <javier.carrasco@...fvision.net>
> +L:	linux-input@...r.kernel.org
> +S:	Maintained
> +F:	drivers/input/touch-overlay.c
> +F:	include/linux/input/touch-overlay.h
> +
>  TPM DEVICE DRIVER
>  M:	Peter Huewe <peterhuewe@....de>
>  M:	Jarkko Sakkinen <jarkko@...nel.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@...fvision.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@...fvision.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

Powered by blists - more mailing lists