Message-ID: <Zox_cVYsErrLu4Mq@google.com>
Date: Mon, 8 Jul 2024 17:08:17 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Javier Carrasco <javier.carrasco@...fvision.net>
Cc: Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bastian Hecht <hechtb@...il.com>,
	Michael Riesch <michael.riesch@...fvision.net>,
	linux-input@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jeff LaBundy <jeff@...undy.com>
Subject: Re: [PATCH v10 2/4] Input: touch-overlay - Add touchscreen overlay
 handling

Hi Javier,

On Wed, Jun 26, 2024 at 11:56:14AM +0200, Javier Carrasco wrote:
> Some touch devices provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.

Thank you for your work. I think it is pretty much ready to be merged,
just a few small comments:

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

So if we have overlays and also want to invert/swap axis then the
overlays should be processed first and only then
touchscreen_set_mt_pos() or touchscreen_report_pos() should be called?

But then it will not work if we need help frm the input core to assign
slots in cases when touch controller does not implement [reliable]
contact tracing/identification.

I think this all needs to be clarified.

> 
> 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.
> 
> Reviewed-by: Jeff LaBundy <jeff@...undy.com>
> Signed-off-by: Javier Carrasco <javier.carrasco@...fvision.net>

> +int touch_overlay_map(struct list_head *list, struct input_dev *input)
> +{
> +	struct fwnode_handle *overlay, *fw_segment;
> +	struct device *dev = input->dev.parent;
> +	struct touch_overlay_segment *segment;
> +	int error = 0;
> +
> +	overlay = device_get_named_child_node(dev, "touch-overlay");

We can annotate this as

	struct fwnode_handle *overlay __free(fwnode_handle) = 
		device_get_named_child_node(dev, "touch-overlay");

> +	if (!overlay)
> +		return 0;
> +
> +	fwnode_for_each_available_child_node(overlay, fw_segment) {
> +		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> +		if (!segment) {
> +			fwnode_handle_put(fw_segment);
> +			error = -ENOMEM;

return -ENOMEM;

> +			break;
> +		}
> +		error = touch_overlay_get_segment(fw_segment, segment, input);
> +		if (error) {
> +			fwnode_handle_put(fw_segment);

return error;

> +			break;
> +		}
> +		list_add_tail(&segment->list, list);
> +	}
> +	fwnode_handle_put(overlay);

Drop.

> +
> +	return error;

return 0;

> +}
> +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->key) {
> +			*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;

This is a static function in the module, we are not passing NULL
segments to it ever. Such tests should be done on API boundary.

> +
> +	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->key)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +static bool touch_overlay_event_on_ts(struct list_head *list, u32 *x, u32 *y)
> +{
> +	struct touch_overlay_segment *segment;
> +	struct list_head *ptr;
> +	bool valid_touch = true;
> +
> +	if (!x || !y)
> +		return false;
> +
> +	list_for_each(ptr, list) {
> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
> +		if (segment->key)
> +			continue;
> +
> +		if (touch_overlay_segment_event(segment, *x, *y)) {
> +			*x -= segment->x_origin;
> +			*y -= segment->y_origin;
> +			return true;
> +		}
> +		/* ignore touch events outside the defined area */
> +		valid_touch = false;
> +	}
> +
> +	return valid_touch;
> +}
> +
> +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 (segment->slot == slot && segment->pressed) {
> +		/* button release */
> +		if (!contact) {
> +			segment->pressed = false;
> +			input_report_key(input, segment->key, false);
> +			input_sync(input);

Do we really need to emit sync here? Can we require/rely on the driver
calling us to emit input_sync() once it's done processing current
frame/packet?

> +			return true;
> +		}
> +
> +		/* sliding out of the button releases it */
> +		if (!touch_overlay_segment_event(segment, *x, *y)) {
> +			segment->pressed = false;
> +			input_report_key(input, segment->key, false);
> +			input_sync(input);
> +			/* keep available for a possible touch event */
> +			return false;
> +		}
> +		/* ignore sliding on the button while pressed */
> +		return true;
> +	} else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
> +		segment->pressed = true;
> +		segment->slot = slot;
> +		input_report_key(input, segment->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)

Would it be better to have a separate argument communicating slot state
(contact/no contact)?

> + * @slot: slot associated to the event

What if we are not dealing with an MT device? Can we say that they
should use slot 0 or maybe -1?

> + *
> + * 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->key &&
> +		    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_event_on_ts(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..cef05c46000d
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,22 @@
> +/* 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 *input);
> +
> +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_process_event(struct list_head *list, struct input_dev *input,
> +				 u32 *x, u32 *y, u32 slot);
> +
> +#endif
> 
> -- 
> 2.40.1
> 

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists