| Message ID | 20230510-feature-ts_virtobj_patch-v6-2-d8a605975153@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: support overlay objects on touchscreens | expand |
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
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
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
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
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
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(-)