[PATCH RFC 0/3] Input - Tap to wake for FT5452

[PATCH RFC 0/3] Input - Tap to wake for FT5452

[David Heidelberg via B4 Relay]

Sending RFC just to validate if this would be the correct approach to
implementing the tap toi wake functionality

I've tested this patch, it does wake up the phone.
The second assessment is let device shell to react to double-tap input
device when phone is only in screen-off mode, thus the second input
device to bind to (when touchscreen is in the runtime sleep).

Feel free to skip detailed review for RFC v1, I'll polish the code for
upcoming v2 submission, after design discussion and approval.

Thanks

Signed-off-by: David Heidelberg <david@ixit.cz>
---
David Heidelberg (3):
      dt-bindings: input: touchscreen: edt-ft5x06: Add wakeup gestures
      Input: edt-ft5x06 - Add wakeup gesture support for FT5452
      arm64: dts: qcom: sdm845-shift-axolotl: Wake the system on a touchscreen double tap

 .../bindings/input/touchscreen/edt-ft5x06.yaml     |  37 ++++
 arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts  |   6 +
 drivers/input/touchscreen/edt-ft5x06.c             | 205 ++++++++++++++++++++-
 3 files changed, 244 insertions(+), 4 deletions(-)
---
base-commit: 2637bdec0535a2044cd830eba9a79363651d9043
change-id: 20261104-ft5x06-double-tap-8022020de323

Best regards,
--  
David Heidelberg <david@ixit.cz>

[PATCH RFC 1/3] dt-bindings: input: touchscreen: edt-ft5x06: Add wakeup gestures

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

The FocalTech controllers have a gesture mode in which the firmware
keeps detecting a configurable set of gestures and signals them on the
interrupt line, so they can wake the system. Describe the gestures as
event child nodes carrying the key code to report, like the Azoteq
IQS7211 binding does, so that the mapping is the board's choice and
more gestures can be added later without changing the driver
interface.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 .../bindings/input/touchscreen/edt-ft5x06.yaml     | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
index 68b2f16016543..bfc540665a2d2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
@@ -106,16 +106,35 @@ properties:
   threshold:
     description: Allows setting the  "click"-threshold in the range from 0 to 255.
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 0
     maximum: 255
 
   interrupt-controller: true
 
+patternProperties:
+  "^event-(tap-double|swipe-(up|down|left|right))$":
+    type: object
+    $ref: ../input.yaml#
+    description:
+      A gesture the controller keeps detecting in its gesture mode, reported
+      as a key on a separate input device. Gesture mode is entered when the
+      touchscreen input device is closed or the system suspends, provided
+      the device may wake up the system. Only controllers whose firmware
+      implements the gesture registers support these nodes.
+
+    properties:
+      linux,code: true
+
+    required:
+      - linux,code
+
+    additionalProperties: false
+
 unevaluatedProperties: false
 
 required:
   - compatible
   - reg
   - interrupts
 
 examples:
@@ -130,9 +149,27 @@ examples:
         reg = <0x38>;
         interrupt-parent = <&gpio2>;
         interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
         reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
         wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
       };
     };
 
+  - |
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      touchscreen@38 {
+        compatible = "focaltech,ft5452";
+        reg = <0x38>;
+        interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
+        wakeup-source;
+
+        event-tap-double {
+          linux,code = <KEY_WAKEUP>;
+        };
+      };
+    };
+
 ...

-- 
2.55.0

Re: [PATCH RFC 1/3] dt-bindings: input: touchscreen: edt-ft5x06: Add wakeup gestures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: input: edt-ft5x06: event-* nodes encode OS policy in DT
--

commit 6f5aa6f1554c8cb1ae88e2ea6e589e4befad85e1
Author: David Heidelberg <david@ixit.cz>

dt-bindings: input: touchscreen: edt-ft5x06: Add wakeup gestures

This patch adds event child nodes to the edt-ft5x06 touchscreen binding
to describe firmware wakeup gestures and map them to Linux input keycodes.

> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
> index 68b2f16016543..bfc540665a2d2 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
> @@ -111,6 +111,25 @@ properties:
>  
>    interrupt-controller: true
>  
> +patternProperties:
> +  "^event-(tap-double|swipe-(up|down|left|right))$":
> +    type: object
> +    $ref: ../input.yaml#
> +    description:
> +      A gesture the controller keeps detecting in its gesture mode, reported
> +      as a key on a separate input device. Gesture mode is entered when the
> +      touchscreen input device is closed or the system suspends, provided
> +      the device may wake up the system. Only controllers whose firmware
> +      implements the gesture registers support these nodes.
> +
> +    properties:
> +      linux,code: true
> +
> +    required:
> +      - linux,code
> +
> +    additionalProperties: false

[Severity: Medium]
Do these event-(tap-double|swipe-(up|down|left|right)) child nodes encode
OS/userspace policy and Linux driver details rather than describing the
hardware?

Unlike physical buttons, printed touch-overlay segments (touch-overlay in
touchscreen.yaml), or trackpad controllers with per-gesture hardware
sensing and timing tuning registers (such as azoteq,iqs7211.yaml, which
configures hardware duration, distance, angle, and threshold registers per
gesture), a touchscreen panel has no board-specific physical wiring or
markings that determine whether double-tap or directional swipe wakeup
gestures exist or what keycode they map to.

Whether the touchscreen interrupt can wake the system is already described
by the standard wakeup-source property, and whether the controller
supports gesture detection is determined by the controller compatible and
firmware. Choosing which wakeup gestures are enabled during screen-off or
system suspend and which Linux keycode (such as KEY_WAKEUP) they emit is
runtime OS/userspace policy rather than static hardware description.

Furthermore, the description documents Linux driver implementation details
(reporting the gesture on a separate input device and entering gesture
mode when the touchscreen input device is closed or the system suspends),
whereas Documentation/devicetree/bindings/writing-bindings.rst specifies
that bindings should describe hardware rather than OS or device driver
behavior.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-ft5x06-double-tap-v1-0-5a1bd80449cc@ixit.cz?part=1

[PATCH RFC 2/3] Input: edt-ft5x06 - Add wakeup gesture support for FT5452

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

The FocalTech firmware has a gesture mode in which the controller keeps
looking for the gestures enabled in its mask register and raises the
interrupt when one is seen. Build the mask from the event child nodes
in the device tree and report each detected gesture as the key code the
node carries, on a separate "edt-ft5x06 gestures" input device.

The separate device is what lets the gesture reach userspace when the
touchscreen input device is closed, which is also the moment the
gesture mode is entered: a compositor that closes the touchscreen when
it blanks the display, or inhibits it through the "inhibited"
attribute, can so keep only the gesture device open and turn the screen
back on from a double tap, which is what the vendor kernels do through
their display notifiers. Opening the touchscreen again leaves gesture
mode.

The mode is also entered on system suspend when the device is allowed
to wake the system, in place of powering the controller off, and the
interrupt line is left to the PM core as the wake source.

Boards relying on the current wakeup-source behaviour, where the
controller keeps scanning and any touch wakes the system, are
unaffected: without event nodes nothing changes, and the nodes are only
read for chips known to implement the gesture registers, for now the
FT5452.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/input/touchscreen/edt-ft5x06.c | 205 ++++++++++++++++++++++++++++++++-
 1 file changed, 201 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index e69d64c3db2dd..fc810d68409cf 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -58,33 +58,58 @@
 #define EV_REGISTER_OFFSET_X		0x46
 
 #define NO_REGISTER			0xff
 
 #define WORK_REGISTER_OPMODE		0x3c
 #define FACTORY_REGISTER_OPMODE		0x01
 #define PMOD_REGISTER_OPMODE		0xa5
 
+#define GESTURE_REGISTER_EN		0xd0
+#define GESTURE_REGISTER_MASK		0xd1
+#define GESTURE_REGISTER_OUTPUT		0xd3
+
+/*
+ * The gesture ID reported in GESTURE_REGISTER_OUTPUT also gives the position
+ * of the gesture's bit in GESTURE_REGISTER_MASK.
+ */
+#define GESTURE_ID_FIRST		0x20
+
 #define TOUCH_EVENT_DOWN		0x00
 #define TOUCH_EVENT_UP			0x01
 #define TOUCH_EVENT_ON			0x02
 #define TOUCH_EVENT_RESERVED		0x03
 
 #define EDT_NAME_LEN			23
+#define EDT_GESTURE_DEV_NAME		"edt-ft5x06 gestures"
 #define EDT_SWITCH_MODE_RETRIES		10
 #define EDT_SWITCH_MODE_DELAY		5 /* msec */
 #define EDT_RAW_DATA_RETRIES		100
 #define EDT_RAW_DATA_DELAY		1000 /* usec */
 
 #define EDT_DEFAULT_NUM_X		1024
 #define EDT_DEFAULT_NUM_Y		1024
 
 #define M06_REG_CMD(factory) ((factory) ? 0xf3 : 0xfc)
 #define M06_REG_ADDR(factory, addr) ((factory) ? (addr) & 0x7f : (addr) & 0x3f)
 
+/* FocalTech gestures: DT event node and the gesture ID */
+struct edt_ft5x06_gesture {
+	const char *node;
+	u8 id;
+};
+
+static const struct edt_ft5x06_gesture edt_ft5x06_gestures[] = {
+	{ "event-swipe-left",	0x20 },
+	{ "event-swipe-right",	0x21 },
+	{ "event-swipe-up",	0x22 },
+	{ "event-swipe-down",	0x23 },
+	{ "event-tap-double",	0x24 },
+};
+
 enum edt_pmode {
 	EDT_PMODE_NOT_SUPPORTED,
 	EDT_PMODE_HIBERNATE,
 	EDT_PMODE_POWEROFF,
 };
 
 enum edt_ver {
 	EDT_M06,
@@ -103,16 +128,17 @@ struct edt_reg_addr {
 	int reg_offset_y;
 	int reg_num_x;
 	int reg_num_y;
 };
 
 struct edt_ft5x06_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input;
+	struct input_dev *gesture_input;
 	struct touchscreen_properties prop;
 	u16 num_x;
 	u16 num_y;
 	struct regulator *vcc;
 	struct regulator *iovcc;
 
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *wake_gpio;
@@ -121,40 +147,44 @@ struct edt_ft5x06_ts_data {
 
 #if defined(CONFIG_DEBUG_FS)
 	u8 *raw_buffer;
 	size_t raw_bufsize;
 #endif
 
 	struct mutex mutex;
 	bool factory_mode;
+	bool in_gesture_mode;
 	enum edt_pmode suspend_mode;
 	int threshold;
 	int gain;
 	int offset;
 	int offset_x;
 	int offset_y;
 	int report_rate;
 	int max_support_points;
 	int point_len;
+	unsigned int gesture_code[ARRAY_SIZE(edt_ft5x06_gestures)];
+	u8 gesture_mask;
 	u8 tdata_cmd;
 	int tdata_len;
 	int tdata_offset;
 
 	char name[EDT_NAME_LEN];
 	char fw_version[EDT_NAME_LEN];
 
 	struct edt_reg_addr reg_addr;
 	enum edt_ver version;
 	unsigned int crc_errors;
 	unsigned int header_errors;
 };
 
 struct edt_i2c_chip_data {
 	int  max_support_points;
+	bool has_wake_gestures;
 };
 
 static const struct regmap_config edt_ft5x06_i2c_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 };
 
 static bool edt_ft5x06_ts_check_crc(struct edt_ft5x06_ts_data *tsdata,
@@ -290,24 +320,58 @@ static int edt_M06_i2c_write(void *context, const void *data, size_t count)
 
 static const struct regmap_config edt_M06_i2c_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.read = edt_M06_i2c_read,
 	.write = edt_M06_i2c_write,
 };
 
+static void edt_ft5x06_ts_report_gesture(struct edt_ft5x06_ts_data *tsdata)
+{
+	struct device *dev = &tsdata->client->dev;
+	unsigned int code = KEY_RESERVED;
+	unsigned int id;
+	int err;
+
+	err = regmap_read(tsdata->regmap, GESTURE_REGISTER_OUTPUT, &id);
+	if (err) {
+		dev_err_ratelimited(dev, "Unable to fetch gesture, error: %d\n",
+				    err);
+		return;
+	}
+
+	for (int i = 0; i < ARRAY_SIZE(edt_ft5x06_gestures); i++)
+		if (edt_ft5x06_gestures[i].id == id)
+			code = tsdata->gesture_code[i];
+
+	if (code == KEY_RESERVED) {
+		dev_dbg(dev, "unhandled gesture %#x\n", id);
+		return;
+	}
+
+	input_report_key(tsdata->gesture_input, code, 1);
+	input_sync(tsdata->gesture_input);
+	input_report_key(tsdata->gesture_input, code, 0);
+	input_sync(tsdata->gesture_input);
+}
+
 static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 {
 	struct edt_ft5x06_ts_data *tsdata = dev_id;
 	struct device *dev = &tsdata->client->dev;
 	u8 rdbuf[63];
 	int i, type, x, y, id;
 	int error;
 
+	if (tsdata->in_gesture_mode) {
+		edt_ft5x06_ts_report_gesture(tsdata);
+		return IRQ_HANDLED;
+	}
+
 	memset(rdbuf, 0, sizeof(rdbuf));
 	error = regmap_bulk_read(tsdata->regmap, tsdata->tdata_cmd, rdbuf,
 				 tsdata->tdata_len);
 	if (error) {
 		dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n",
 				    error);
 		goto out;
 	}
@@ -1108,16 +1172,120 @@ static void edt_ft5x06_exit_regmap(void *arg)
 static void edt_ft5x06_disable_regulators(void *arg)
 {
 	struct edt_ft5x06_ts_data *data = arg;
 
 	regulator_disable(data->vcc);
 	regulator_disable(data->iovcc);
 }
 
+static int edt_ft5x06_ts_set_gesture_mode(struct edt_ft5x06_ts_data *tsdata,
+					  bool enable)
+{
+	int err;
+
+	if (enable) {
+		err = regmap_write(tsdata->regmap, GESTURE_REGISTER_MASK,
+				   tsdata->gesture_mask);
+		if (err)
+			return err;
+	} else {
+		/* let the IRQ thread read the gesture that woke us first */
+		synchronize_irq(tsdata->client->irq);
+	}
+
+	err = regmap_write(tsdata->regmap, GESTURE_REGISTER_EN, enable);
+	if (err)
+		return err;
+
+	tsdata->in_gesture_mode = enable;
+
+	return 0;
+}
+
+/*
+ * The touchscreen is opened and closed by userspace, either by opening and
+ * closing its event device or through the "inhibited" attribute. Closing it
+ * is the moment to start looking for the wakeup gestures.
+ */
+static int edt_ft5x06_ts_open(struct input_dev *input)
+{
+	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
+
+	if (!tsdata->in_gesture_mode)
+		return 0;
+
+	return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
+}
+
+static void edt_ft5x06_ts_close(struct input_dev *input)
+{
+	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
+	struct device *dev = &tsdata->client->dev;
+	int err;
+
+	if (!device_may_wakeup(dev))
+		return;
+
+	err = edt_ft5x06_ts_set_gesture_mode(tsdata, true);
+	if (err)
+		dev_warn(dev, "Failed to enter gesture mode: %d\n", err);
+}
+
+/*
+ * The gestures detected in gesture mode are reported on their own input
+ * device, so that they still reach userspace when the touchscreen input
+ * device has been closed.
+ */
+static int edt_ft5x06_ts_setup_gestures(struct edt_ft5x06_ts_data *tsdata)
+{
+	struct device *dev = &tsdata->client->dev;
+	struct input_dev *input = NULL;
+	struct fwnode_handle *child;
+	int err;
+
+	for (int i = 0; i < ARRAY_SIZE(edt_ft5x06_gestures); i++) {
+		const struct edt_ft5x06_gesture *gesture = &edt_ft5x06_gestures[i];
+		unsigned int code;
+
+		child = device_get_named_child_node(dev, gesture->node);
+		if (!child)
+			continue;
+
+		err = fwnode_property_read_u32(child, "linux,code", &code);
+		fwnode_handle_put(child);
+		if (err)
+			return dev_err_probe(dev, err, "%s: failed to read linux,code\n",
+					     gesture->node);
+
+		if (code == KEY_RESERVED)
+			continue;
+
+		if (!input) {
+			input = devm_input_allocate_device(dev);
+			if (!input)
+				return -ENOMEM;
+
+			input->name = EDT_GESTURE_DEV_NAME;
+			input->id.bustype = BUS_I2C;
+		}
+
+		input_set_capability(input, EV_KEY, code);
+		tsdata->gesture_code[i] = code;
+		tsdata->gesture_mask |= BIT(gesture->id - GESTURE_ID_FIRST);
+	}
+
+	if (!input)
+		return 0;
+
+	tsdata->gesture_input = input;
+
+	return input_register_device(input);
+}
+
 static int edt_ft5x06_ts_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	const struct edt_i2c_chip_data *chip_data;
 	struct edt_ft5x06_ts_data *tsdata;
 	unsigned int val;
 	struct input_dev *input;
 	unsigned long irq_flags;
@@ -1295,16 +1463,27 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client)
 
 	input_set_abs_params(input, ABS_MT_POSITION_X,
 			     0, tsdata->num_x * 64 - 1, 0, 0);
 	input_set_abs_params(input, ABS_MT_POSITION_Y,
 			     0, tsdata->num_y * 64 - 1, 0, 0);
 
 	touchscreen_parse_properties(input, true, &tsdata->prop);
 
+	if (chip_data->has_wake_gestures) {
+		error = edt_ft5x06_ts_setup_gestures(tsdata);
+		if (error)
+			return error;
+
+		if (tsdata->gesture_input) {
+			input->open = edt_ft5x06_ts_open;
+			input->close = edt_ft5x06_ts_close;
+		}
+	}
+
 	error = input_mt_init_slots(input, tsdata->max_support_points,
 				    INPUT_MT_DIRECT);
 	if (error) {
 		dev_err(&client->dev, "Unable to init MT slots.\n");
 		return error;
 	}
 
 	irq_flags = irq_get_trigger_type(client->irq);
@@ -1315,16 +1494,18 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client)
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  NULL, edt_ft5x06_ts_isr, irq_flags,
 					  client->name, tsdata);
 	if (error) {
 		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
 		return error;
 	}
 
+	input_set_drvdata(input, tsdata);
+
 	error = input_register_device(input);
 	if (error)
 		return error;
 
 	edt_ft5x06_ts_prepare_debugfs(tsdata);
 
 	dev_dbg(&client->dev,
 		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
@@ -1344,18 +1525,23 @@ static void edt_ft5x06_ts_remove(struct i2c_client *client)
 
 static int edt_ft5x06_ts_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 	struct gpio_desc *reset_gpio = tsdata->reset_gpio;
 	int ret;
 
-	if (device_may_wakeup(dev))
-		return 0;
+	if (device_may_wakeup(dev)) {
+		/* the IRQ stays enabled, the PM core arms it as the wake source */
+		if (!tsdata->gesture_input || tsdata->in_gesture_mode)
+			return 0;
+
+		return edt_ft5x06_ts_set_gesture_mode(tsdata, true);
+	}
 
 	if (tsdata->suspend_mode == EDT_PMODE_NOT_SUPPORTED)
 		return 0;
 
 	/* Enter hibernate mode. */
 	ret = regmap_write(tsdata->regmap, PMOD_REGISTER_OPMODE,
 			   PMOD_REGISTER_HIBERNATE);
 	if (ret)
@@ -1386,18 +1572,28 @@ static int edt_ft5x06_ts_suspend(struct device *dev)
 }
 
 static int edt_ft5x06_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 	int ret = 0;
 
-	if (device_may_wakeup(dev))
-		return 0;
+	if (device_may_wakeup(dev)) {
+		if (!tsdata->in_gesture_mode)
+			return 0;
+
+		guard(mutex)(&tsdata->input->mutex);
+
+		/* stays armed until the input device is opened again */
+		if (!input_device_enabled(tsdata->input))
+			return 0;
+
+		return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
+	}
 
 	if (tsdata->suspend_mode == EDT_PMODE_NOT_SUPPORTED)
 		return 0;
 
 	if (tsdata->suspend_mode == EDT_PMODE_POWEROFF) {
 		struct gpio_desc *reset_gpio = tsdata->reset_gpio;
 
 		/*
@@ -1455,16 +1651,17 @@ static const struct edt_i2c_chip_data edt_ft5x06_data = {
 };
 
 static const struct edt_i2c_chip_data edt_ft3518_data = {
 	.max_support_points = 10,
 };
 
 static const struct edt_i2c_chip_data edt_ft5452_data = {
 	.max_support_points = 5,
+	.has_wake_gestures = true,
 };
 
 static const struct edt_i2c_chip_data edt_ft5506_data = {
 	.max_support_points = 10,
 };
 
 static const struct edt_i2c_chip_data edt_ft6236_data = {
 	.max_support_points = 2,

-- 
2.55.0

Re: [PATCH RFC 2/3] Input: edt-ft5x06 - Add wakeup gesture support for FT5452

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Input: edt-ft5x06 - in_gesture_mode desync across non-wakeup suspend/resume
- [Medium] Input: edt-ft5x06 - IRQ race in edt_ft5x06_ts_set_gesture_mode()
- [Medium] Input: edt-ft5x06: missing input->mutex serialization in suspend and resume
- [Low] Input: edt-ft5x06 - use __free(fwnode_handle) and explicit error returns
--

commit 3e641160614ae8cff0e8f98270a632d8f526b6a7
Author: David Heidelberg <david@ixit.cz>

Input: edt-ft5x06 - Add wakeup gesture support for FT5452

This commit adds wakeup gesture support for the FocalTech FT5452 touchscreen
controller by parsing event child nodes from the device tree and reporting
detected gestures as key events on a separate input device. Gesture mode is
entered when the touchscreen input device is closed or during system suspend
when wakeup is enabled.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index e69d64c3db2dd..fc810d68409cf 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
[ ... ]
> @@ -303,6 +362,11 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  	int i, type, x, y, id;
>  	int error;
>  
> +	if (tsdata->in_gesture_mode) {
> +		edt_ft5x06_ts_report_gesture(tsdata);
> +		return IRQ_HANDLED;
> +	}
> +
[ ... ]
> @@ -1113,6 +1177,110 @@ static void edt_ft5x06_disable_regulators(void *arg)
>  	regulator_disable(data->iovcc);
>  }
>  
> +static int edt_ft5x06_ts_set_gesture_mode(struct edt_ft5x06_ts_data *tsdata,
> +					  bool enable)
> +{
> +	int err;
> +
> +	if (enable) {\
> +		err = regmap_write(tsdata->regmap, GESTURE_REGISTER_MASK,
> +				   tsdata->gesture_mask);
> +		if (err)
> +			return err;
> +	} else {
> +		/* let the IRQ thread read the gesture that woke us first */
> +		synchronize_irq(tsdata->client->irq);
> +	}
> +
> +	err = regmap_write(tsdata->regmap, GESTURE_REGISTER_EN, enable);
> +	if (err)
> +		return err;
> +
> +	tsdata->in_gesture_mode = enable;
> +
> +	return 0;
> +}

[Severity: Medium]
Can edt_ft5x06_ts_set_gesture_mode() race with the threaded interrupt handler
edt_ft5x06_ts_isr() while client->irq remains enabled?

When enable is true, synchronize_irq() is not called before switching modes.
An already-triggered touch interrupt in edt_ft5x06_ts_isr() can observe
tsdata->in_gesture_mode as true after GESTURE_REGISTER_EN is written and read
GESTURE_REGISTER_OUTPUT instead of touch coordinates. Conversely, if a
gesture interrupt fires right after the sleeping regmap_write() to
GESTURE_REGISTER_EN completes on the I2C bus, edt_ft5x06_ts_isr() can run
before tsdata->in_gesture_mode = enable is stored and read normal touch data
instead of GESTURE_REGISTER_OUTPUT.

When enable is false, synchronize_irq() is called before writing 0 to
GESTURE_REGISTER_EN while the interrupt line is still enabled. Once the
controller receives GESTURE_REGISTER_EN = 0 over I2C, it resumes normal touch
scanning before regmap_write() returns to clear tsdata->in_gesture_mode, so a
touch interrupt in that window will see tsdata->in_gesture_mode as true in
edt_ft5x06_ts_isr() and call edt_ft5x06_ts_report_gesture().

> +
> +/*
> + * The touchscreen is opened and closed by userspace, either by opening and
> + * closing its event device or through the "inhibited" attribute. Closing it
> + * is the moment to start looking for the wakeup gestures.
> + */
> +static int edt_ft5x06_ts_open(struct input_dev *input)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
> +
> +	if (!tsdata->in_gesture_mode)
> +		return 0;
> +
> +	return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
> +}
> +
> +static void edt_ft5x06_ts_close(struct input_dev *input)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
> +	struct device *dev = &tsdata->client->dev;
> +	int err;
> +
> +	if (!device_may_wakeup(dev))
> +		return;
> +
> +	err = edt_ft5x06_ts_set_gesture_mode(tsdata, true);
> +	if (err)
> +		dev_warn(dev, "Failed to enter gesture mode: %d\n", err);
> +}
> +
> +/*
> + * The gestures detected in gesture mode are reported on their own input
> + * device, so that they still reach userspace when the touchscreen input
> + * device has been closed.
> + */
> +static int edt_ft5x06_ts_setup_gestures(struct edt_ft5x06_ts_data *tsdata)
> +{
> +	struct device *dev = &tsdata->client->dev;
> +	struct input_dev *input = NULL;
> +	struct fwnode_handle *child;
> +	int err;
> +
> +	for (int i = 0; i < ARRAY_SIZE(edt_ft5x06_gestures); i++) {
> +		const struct edt_ft5x06_gesture *gesture = &edt_ft5x06_gestures[i];
> +		unsigned int code;
> +
> +		child = device_get_named_child_node(dev, gesture->node);
> +		if (!child)
> +			continue;
> +
> +		err = fwnode_property_read_u32(child, "linux,code", &code);
> +		fwnode_handle_put(child);
> +		if (err)
> +			return dev_err_probe(dev, err, "%s: failed to read linux,code\n",
> +					     gesture->node);
> +
> +		if (code == KEY_RESERVED)
> +			continue;
> +
> +		if (!input) {
> +			input = devm_input_allocate_device(dev);
> +			if (!input)
> +				return -ENOMEM;
> +
> +			input->name = EDT_GESTURE_DEV_NAME;
> +			input->id.bustype = BUS_I2C;
> +		}
> +
> +		input_set_capability(input, EV_KEY, code);
> +		tsdata->gesture_code[i] = code;
> +		tsdata->gesture_mask |= BIT(gesture->id - GESTURE_ID_FIRST);
> +	}
> +
> +	if (!input)
> +		return 0;
> +
> +	tsdata->gesture_input = input;
> +
> +	return input_register_device(input);
> +}

[Severity: Low]
This isn't a bug, but in edt_ft5x06_ts_setup_gestures(), could child be
declared inside the loop using __free(fwnode_handle):

	struct fwnode_handle *child __free(fwnode_handle) =
		device_get_named_child_node(dev, gesture->node);

instead of calling fwnode_handle_put(child) manually?

Also, in both edt_ft5x06_ts_setup_gestures() and edt_ft5x06_ts_resume(),
which have multiple failure points, could the final calls to
input_register_device(input) and edt_ft5x06_ts_set_gesture_mode(tsdata, false)
check err and explicitly return 0 on success to match input subsystem style
conventions?

[ ... ]
> @@ -1349,8 +1530,13 @@ static int edt_ft5x06_ts_suspend(struct device *dev)
>  	struct gpio_desc *reset_gpio = tsdata->reset_gpio;
>  	int ret;
>  
> -	if (device_may_wakeup(dev))
> -		return 0;
> +	if (device_may_wakeup(dev)) {
> +		/* the IRQ stays enabled, the PM core arms it as the wake source */
> +		if (!tsdata->gesture_input || tsdata->in_gesture_mode)
> +			return 0;
> +
> +		return edt_ft5x06_ts_set_gesture_mode(tsdata, true);
> +	}
>  
>  	if (tsdata->suspend_mode == EDT_PMODE_NOT_SUPPORTED)
>  		return 0;
> @@ -1391,8 +1577,18 @@ static int edt_ft5x06_ts_resume(struct device *dev)
>  	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
>  	int ret = 0;
>  
> -	if (device_may_wakeup(dev))
> -		return 0;
> +	if (device_may_wakeup(dev)) {
> +		if (!tsdata->in_gesture_mode)
> +			return 0;
> +
> +		guard(mutex)(&tsdata->input->mutex);
> +
> +		/* stays armed until the input device is opened again */
> +		if (!input_device_enabled(tsdata->input))
> +			return 0;
> +
> +		return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
> +	}

[Severity: Medium]
What happens if the touchscreen input device is closed while wakeup is
enabled, and userspace then disables wakeup via sysfs before suspending?

1. edt_ft5x06_ts_close() calls edt_ft5x06_ts_set_gesture_mode(tsdata, true),
   which sets tsdata->in_gesture_mode = true.
2. Userspace disables wakeup via power/wakeup while the input device remains
   closed, so device_may_wakeup(dev) becomes false.
3. On system suspend, edt_ft5x06_ts_suspend() takes the non-wakeup path and
   hibernates or powers off the controller (asserting reset_gpio and disabling
   vcc/iovcc) without clearing tsdata->in_gesture_mode.
4. On resume, edt_ft5x06_ts_resume() takes the non-wakeup path and resets or
   wakes the controller back into normal touch mode while
   tsdata->in_gesture_mode remains true.

While the input device remains closed, any touch interrupt will be handled by
edt_ft5x06_ts_isr() as a gesture via edt_ft5x06_ts_report_gesture(). In
addition, if wakeup is later re-enabled before a subsequent suspend while the
device is still closed, edt_ft5x06_ts_suspend() will see
tsdata->in_gesture_mode == true and return 0 without programming
GESTURE_REGISTER_MASK or GESTURE_REGISTER_EN.

[Severity: Medium]
Should edt_ft5x06_ts_suspend() and the initial tsdata->in_gesture_mode check
in edt_ft5x06_ts_resume() also be serialized under tsdata->input->mutex?

edt_ft5x06_ts_open() and edt_ft5x06_ts_close() are called under
tsdata->input->mutex from input_open_device(), input_close_device(),
input_inhibit_device(), and input_uninhibit_device(). However,
edt_ft5x06_ts_suspend() reads tsdata->in_gesture_mode and calls
edt_ft5x06_ts_set_gesture_mode(tsdata, true) without holding
tsdata->input->mutex, and edt_ft5x06_ts_resume() checks
!tsdata->in_gesture_mode before acquiring guard(mutex)(&tsdata->input->mutex).

If a deferred file release via delayed_fput_work (evdev_release() ->
evdev_close_device() -> input_close_device()) or an in-kernel input handler
runs concurrently with edt_ft5x06_ts_suspend() or edt_ft5x06_ts_resume(), can
concurrent calls to edt_ft5x06_ts_set_gesture_mode() interleave I2C writes to
GESTURE_REGISTER_MASK and GESTURE_REGISTER_EN and leave
tsdata->in_gesture_mode out of sync with the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-ft5x06-double-tap-v1-0-5a1bd80449cc@ixit.cz?part=2

[PATCH RFC 3/3] arm64: dts: qcom: sdm845-shift-axolotl: Wake the system on a touchscreen double tap

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

The touch interrupt on GPIO 125 is routed to the PDC, so the panel can
wake the phone. Report the controller's double tap gesture as
KEY_WAKEUP.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
index 1b0eaeafbd517..e4eef60271c55 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
@@ -504,16 +504,22 @@ touchscreen@38 {
 		iovcc-supply = <&vreg_l14a_1p88>;
 
 		pinctrl-0 = <&ts_int_active &ts_reset_active>;
 		pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
 		pinctrl-names = "default", "sleep";
 
 		touchscreen-size-x = <1080>;
 		touchscreen-size-y = <2160>;
+
+		wakeup-source;
+
+		event-tap-double {
+			linux,code = <KEY_WAKEUP>;
+		};
 	};
 };
 
 &i2c10 {
 	/* SMB1355@0x0C */
 };
 
 &i2c11 {

-- 
2.55.0

Re: [PATCH RFC 3/3] arm64: dts: qcom: sdm845-shift-axolotl: Wake the system on a touchscreen double tap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: sdm845-shift-axolotl: touchscreen wakeup gesture fails with -EACCES
- [Medium] Input: edt-ft5x06: stale in_gesture_mode across non-wakeup suspend/resume
--

commit b6f2bc4fb2c1b25270ce05595064b9f93a030624
Author: David Heidelberg <david@ixit.cz>

arm64: dts: qcom: sdm845-shift-axolotl: Wake the system on a touchscreen double tap

This commit enables wakeup-source and configures the double-tap gesture to
report KEY_WAKEUP on the FocalTech FT5452 touchscreen controller in
sdm845-shift-axolotl.dts.

> diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> index 1b0eaeafbd517..e4eef60271c55 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> @@ -509,6 +509,12 @@ touchscreen@38 {
>  
>  		touchscreen-size-x = <1080>;
>  		touchscreen-size-y = <2160>;
> +
> +		wakeup-source;

[Severity: Medium]
Can tsdata->in_gesture_mode in drivers/input/touchscreen/edt-ft5x06.c get
out of sync with the hardware state across a non-wakeup suspend and resume
cycle when wakeup-source is enabled here?

When the touchscreen input device is closed (or inhibited) while
device_may_wakeup(dev) is true, edt_ft5x06_ts_close() calls
edt_ft5x06_ts_set_gesture_mode(tsdata, true), which programs
GESTURE_REGISTER_MASK and GESTURE_REGISTER_EN and sets
tsdata->in_gesture_mode = true:

drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_close() {
	...
	err = edt_ft5x06_ts_set_gesture_mode(tsdata, true);
	...
}

If userspace then disables wakeup via power/wakeup before system suspend,
edt_ft5x06_ts_suspend() sees device_may_wakeup(dev) is false and falls
through to hibernate or power off the controller (asserting reset_gpio and
disabling the vcc and iovcc regulators in EDT_PMODE_POWEROFF mode) without
clearing tsdata->in_gesture_mode:

drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_suspend() {
	if (device_may_wakeup(dev)) {
		/* the IRQ stays enabled, the PM core arms it as the wake source */
		if (!tsdata->gesture_input || tsdata->in_gesture_mode)
			return 0;

		return edt_ft5x06_ts_set_gesture_mode(tsdata, true);
	}
	...
}

On resume, edt_ft5x06_ts_resume() also takes the !device_may_wakeup(dev)
path, powering the controller back on and resetting it into normal touch
mode while leaving tsdata->in_gesture_mode set to true:

drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_resume() {
	if (device_may_wakeup(dev)) {
		if (!tsdata->in_gesture_mode)
			return 0;
		...
	}
	...
}

While the input device remains closed, wouldn't any touch interrupt cause
edt_ft5x06_ts_isr() to take the tsdata->in_gesture_mode branch and call
edt_ft5x06_ts_report_gesture() even though GESTURE_REGISTER_EN was reset to
0 in hardware, and wouldn't a subsequent wakeup-enabled suspend skip
re-programming gesture mode because tsdata->in_gesture_mode is still true?

> +
> +		event-tap-double {
> +			linux,code = <KEY_WAKEUP>;
> +		};

[Severity: High]
When a double-tap gesture on touchscreen@38 wakes the system from suspend,
does reading GESTURE_REGISTER_OUTPUT in edt_ft5x06_ts_report_gesture() fail
with -EACCES and drop the KEY_WAKEUP event because the parent GENI I2C
controller still has runtime PM disabled when the wakeup IRQ is retriggered?

On sdm845-shift-axolotl.dts, touchscreen@38 is a child of i2c5
(qcom,geni-i2c, handled by drivers/i2c/busses/i2c-qcom-geni.c). During
suspend, edt_ft5x06_ts_suspend() leaves tsdata->client->irq enabled when
device_may_wakeup(dev) is true:

drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_suspend() {
	if (device_may_wakeup(dev)) {
		/* the IRQ stays enabled, the PM core arms it as the wake source */
		if (!tsdata->gesture_input || tsdata->in_gesture_mode)
			return 0;

		return edt_ft5x06_ts_set_gesture_mode(tsdata, true);
	}
	...
}

In dpm_suspend_late(), device_suspend_late() calls pm_runtime_disable() on
the parent GENI I2C controller (gi2c->se.dev), setting power.disable_depth
to 1. In dpm_suspend_noirq(), suspend_device_irqs() -> suspend_device_irq()
sees the wakeup IRQ is still enabled, so it sets IRQD_WAKEUP_ARMED without
setting IRQD_IRQ_ENABLED_ON_SUSPEND. Next, geni_i2c_suspend_noirq() calls
pm_runtime_force_suspend(gi2c->se.dev), incrementing power.disable_depth to
2 and leaving gi2c->se.dev in RPM_SUSPENDED with power.needs_force_resume =
false.

When the double-tap gesture fires on GPIO 125 during suspend,
irq_pm_handle_wakeup() clears IRQD_WAKEUP_ARMED, marks the IRQ
IRQS_SUSPENDED | IRQS_PENDING, and increments desc->depth to 1.

During dpm_resume_noirq(), dpm_noirq_resume_devices() runs
geni_i2c_resume_noirq():

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_resume_noirq() {
	ret = pm_runtime_force_resume(dev);
	if (ret)
		return ret;

	i2c_mark_adapter_resumed(&gi2c->adap);
	return 0;
}

Because needs_force_resume is false and runtime_status is RPM_SUSPENDED,
pm_runtime_force_resume() does not resume gi2c->se.dev and only decrements
power.disable_depth from 2 to 1.

Still inside dpm_resume_noirq(), before dpm_resume_early() runs
device_resume_early() to re-enable runtime PM on gi2c->se.dev,
resume_device_irqs() -> resume_irq() -> __enable_irq() -> irq_startup() ->
check_irq_resend() runs for tsdata->client->irq, which immediately
retriggers the pending interrupt and wakes the threaded handler
edt_ft5x06_ts_isr() -> edt_ft5x06_ts_report_gesture():

drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_report_gesture() {
	...
	err = regmap_read(tsdata->regmap, GESTURE_REGISTER_OUTPUT, &id);
	if (err) {
		dev_err_ratelimited(dev, "Unable to fetch gesture, error: %d\n",
				    err);
		return;
	}
	...
}

When regmap_read() -> geni_i2c_xfer() -> pm_runtime_resume_and_get() ->
rpm_resume() runs before device_resume_early() on gi2c->se.dev, doesn't
rpm_resume() fail with -EACCES because gi2c->se.dev is RPM_SUSPENDED with
power.disable_depth == 1, causing the wakeup gesture to be lost unless
edt-ft5x06 disables tsdata->client->irq in edt_ft5x06_ts_suspend() and
re-enables it in edt_ft5x06_ts_resume()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-ft5x06-double-tap-v1-0-5a1bd80449cc@ixit.cz?part=3