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@xxxxxxxxxxx>
> Signed-off-by: Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx>
> +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@xxxxxxxxxxxxxx>
> + */
> +
> +#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
