* [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus
@ 2026-07-23 11:27 Alexandre Hamamdjian via B4 Relay
2026-07-23 11:27 ` [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id Alexandre Hamamdjian via B4 Relay
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-input, linux-kernel, devicetree, Alexandre Hamamdjian,
Teguh Sobirin
The AYANEO Pocket DS drives its FocalTech FT5426 touch controller on a
Qualcomm GENI i2c controller that has no bus recovery and, on this
board, clock-stretches close to the controller timeout. This series
collects the robustness work that helps there, smallest and most
generic first:
1. ignore contacts reported with an out-of-range slot id -- a generic
correctness fix, independent of the board;
2. document, and
3. implement an optional no-regmap-bulk-read mode that reads the touch
frame one register at a time, for controllers whose bus aborts long
transfers;
4. poll while a contact is down to recover a touch release dropped by
a missed edge interrupt.
Patch 1 stands on its own; the rest are opt-in (DT property / chip
behaviour) and inert on healthy hardware. Feedback on whether the bus
mitigations belong here vs. in the i2c controller is welcome.
Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com>
---
Alexandre Hamamdjian (3):
Input: edt-ft5x06 - ignore contacts with an out-of-range slot id
dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property
Input: edt-ft5x06 - poll while a contact is down to recover dropped releases
Teguh Sobirin (1):
Input: edt-ft5x06 - allow reading the touch frame one register at a time
.../bindings/input/touchscreen/edt-ft5x06.yaml | 8 +
drivers/input/touchscreen/edt-ft5x06.c | 200 +++++++++++++++++++--
2 files changed, 196 insertions(+), 12 deletions(-)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260723-b4-ft5426-393d8514e4bf
Best regards,
--
Alexandre Hamamdjian <azkali.limited@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id 2026-07-23 11:27 [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 ` Alexandre Hamamdjian via B4 Relay 2026-07-23 11:40 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property Alexandre Hamamdjian via B4 Relay ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 UTC (permalink / raw) To: Dmitry Torokhov, Henrik Rydberg, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-input, linux-kernel, devicetree, Alexandre Hamamdjian From: Alexandre Hamamdjian <azkali.limited@gmail.com> The per-contact slot id is taken from the top nibble of the third report byte, so it can be any value from 0 to 15. The driver only allocates max_support_points MT slots (2 to 10 depending on the variant), so a report that carries an id at or above that count - be it a genuinely higher-numbered contact or a corrupted byte - is outside the range the input core was told about. input_mt_slot() silently ignores an ABS_MT_SLOT beyond num_slots and leaves the current slot unchanged, so the following input_mt_report_slot_state()/touchscreen_report_pos() pair is applied to whichever slot happened to be selected last, reporting the contact at the wrong position. Skip such entries instead. Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com> --- drivers/input/touchscreen/edt-ft5x06.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index d3b1177185a3..d6c3d033b83d 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -331,6 +331,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) swap(x, y); id = (buf[2] >> 4) & 0x0f; + if (id >= tsdata->max_support_points) + continue; input_mt_slot(tsdata->input, id); if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, -- 2.55.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id 2026-07-23 11:27 ` [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:40 ` sashiko-bot 0 siblings, 0 replies; 9+ messages in thread From: sashiko-bot @ 2026-07-23 11:40 UTC (permalink / raw) To: Alexandre Hamamdjian Cc: conor+dt, devicetree, linux-input, dmitry.torokhov, robh Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] Unbalanced `disable_irq()` calls when suspending and resuming in factory mode lead to a permanently disabled touchscreen. - [High] Stack-allocated buffers are passed to `regmap_bulk_read()` and `i2c_transfer()`, which can cause memory corruption or panics if the underlying I2C host controller driver uses DMA. -- --- Patch [1]: [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id --- commit 9dab47af56724a9e66ce05f01c0e4d726933073c Author: Alexandre Hamamdjian <azkali.limited@gmail.com> Input: edt-ft5x06 - ignore contacts with an out-of-range slot id The per-contact slot id is taken from the top nibble of the third report byte, so it can be any value from 0 to 15. The driver only allocates max_support_points MT slots (2 to 10 depending on the variant), so a report that carries an id at or above that count - be it a genuinely higher-numbered contact or a corrupted byte - is outside the range the input core was told about. input_mt_slot() silently ignores an ABS_MT_SLOT beyond num_slots and leaves the current slot unchanged, so the following input_mt_report_slot_state()/touchscreen_report_pos() pair is applied to whichever slot happened to be selected last, reporting the contact at the wrong position. Skip such entries instead. > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index d3b1177185a3b..d6c3d033b83df 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -331,6 +331,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) > swap(x, y); > > id = (buf[2] >> 4) & 0x0f; > + if (id >= tsdata->max_support_points) > + continue; > > input_mt_slot(tsdata->input, id); > if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, [Severity: High] This isn't a bug introduced by this patch, but I noticed that stack-allocated buffers are being passed to regmap_bulk_read() in this same function: drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_isr() { ... u8 rdbuf[63]; ... error = regmap_bulk_read(tsdata->regmap, tsdata->tdata_cmd, rdbuf, tsdata->tdata_len); ... } Can this lead to memory corruption or kernel panics if the underlying I2C host controller uses DMA without bounce buffering and CONFIG_VMAP_STACK is enabled? I see similar stack allocations passed to i2c_transfer() elsewhere in the driver: drivers/input/touchscreen/edt-ft5x06.c:edt_M06_i2c_read() { ... u8 wbuf[4], rbuf[3]; ... xfer[1].buf = reg_read ? rbuf : val_buf; ret = i2c_transfer(i2c->adapter, xfer, 2); ... } drivers/input/touchscreen/edt-ft5x06.c:edt_M06_i2c_write() { ... u8 wbuf[4]; ... xfer.buf = wbuf; ret = i2c_transfer(i2c->adapter, &xfer, 1); ... } Should these buffers be heap-allocated to avoid violating the kernel DMA API? [Severity: High] This is a pre-existing issue, but there appears to be a regression with unbalanced disable_irq() calls that could leave the touchscreen permanently disabled. When entering factory mode, disable_irq() increments the disable depth: drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_factory_mode() { ... disable_irq(client->irq); ... } If the system then suspends, edt_ft5x06_ts_suspend() unconditionally disables the IRQ again, incrementing the depth a second time: drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_suspend() { ... disable_irq(tsdata->client->irq); ... } During resume, edt_ft5x06_ts_resume() calls enable_irq() once, but then detects tsdata->factory_mode is true and calls edt_ft5x06_factory_mode() again, which disables the IRQ a second time: drivers/input/touchscreen/edt-ft5x06.c:edt_ft5x06_ts_resume() { ... enable_irq(tsdata->client->irq); if (tsdata->factory_mode) ret = edt_ft5x06_factory_mode(tsdata); ... } When the user later switches back to work mode, edt_ft5x06_work_mode() only calls enable_irq() once. Does this leave the IRQ disable depth at 1, keeping the touchscreen completely dead until the module is reloaded or the system is rebooted? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-ft5426-v1-0-d4b4e32be042@gmail.com?part=1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property 2026-07-23 11:27 [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus Alexandre Hamamdjian via B4 Relay 2026-07-23 11:27 ` [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 ` Alexandre Hamamdjian via B4 Relay 2026-07-23 11:35 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time Alexandre Hamamdjian via B4 Relay 2026-07-23 11:27 ` [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases Alexandre Hamamdjian via B4 Relay 3 siblings, 1 reply; 9+ messages in thread From: Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 UTC (permalink / raw) To: Dmitry Torokhov, Henrik Rydberg, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-input, linux-kernel, devicetree, Alexandre Hamamdjian From: Alexandre Hamamdjian <azkali.limited@gmail.com> Document an optional boolean property for boards whose i2c controller cannot reliably sustain the multi-byte block read of a full touch frame. When set, the driver reads the frame one register at a time instead of in a single bulk transfer. Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com> --- .../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml index 68b2f1601654..0d8310a290ea 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml @@ -111,6 +111,14 @@ properties: interrupt-controller: true + no-regmap-bulk-read: + type: boolean + description: + Set this when the i2c controller the touch controller is attached to + cannot reliably sustain the multi-byte block read of a full touch + frame and aborts long transfers. The driver then reads the touch + frame one register at a time instead of in a single bulk transfer. + unevaluatedProperties: false required: -- 2.55.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property 2026-07-23 11:27 ` [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:35 ` sashiko-bot 0 siblings, 0 replies; 9+ messages in thread From: sashiko-bot @ 2026-07-23 11:35 UTC (permalink / raw) To: Alexandre Hamamdjian Cc: robh, dmitry.torokhov, conor+dt, devicetree, linux-input Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `no-regmap-bulk-read` property violates OS-agnostic Device Tree design principles by embedding Linux-specific software terminology and misplacing parent bus limitations into a child node. -- --- Patch [2]: [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property --- commit adbbdf0f113c3cd899e582907d5e795b5a3a7cc7 Author: Alexandre Hamamdjian <azkali.limited@gmail.com> dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property This commit documents an optional boolean property, no-regmap-bulk-read, for boards where the I2C controller cannot reliably sustain multi-byte block reads. It instructs the driver to read the touch frame one register at a time instead of using a bulk transfer. > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml > index 68b2f16016543..0d8310a290ea5 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml > @@ -111,6 +111,14 @@ properties: > > interrupt-controller: true > > + no-regmap-bulk-read: > + type: boolean > + description: > + Set this when the i2c controller the touch controller is attached to > + cannot reliably sustain the multi-byte block read of a full touch > + frame and aborts long transfers. The driver then reads the touch > + frame one register at a time instead of in a single bulk transfer. [Severity: Medium] Does this property violate OS-agnostic Device Tree design principles? The property name uses Linux-specific internal software terminology ("regmap") and the description explicitly dictates driver implementation behavior rather than describing the hardware layout. Additionally, is it appropriate to model a limitation of the parent I2C controller inside the child touch controller's device node? This appears to misplace parent bus limitations into a child node. Should this quirk perhaps be handled at the I2C controller level or expressed in a more hardware-centric, OS-agnostic way? > + > unevaluatedProperties: false > > required: -- Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-ft5426-v1-0-d4b4e32be042@gmail.com?part=2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time 2026-07-23 11:27 [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus Alexandre Hamamdjian via B4 Relay 2026-07-23 11:27 ` [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id Alexandre Hamamdjian via B4 Relay 2026-07-23 11:27 ` [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 ` Alexandre Hamamdjian via B4 Relay 2026-07-23 11:39 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases Alexandre Hamamdjian via B4 Relay 3 siblings, 1 reply; 9+ messages in thread From: Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 UTC (permalink / raw) To: Dmitry Torokhov, Henrik Rydberg, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-input, linux-kernel, devicetree, Alexandre Hamamdjian, Teguh Sobirin From: Teguh Sobirin <teguh@sobir.in> Every touch interrupt reads the whole touch frame in a single i2c block read via regmap_bulk_read(). On some boards the i2c controller the panel is wired to cannot sustain that multi-byte transfer: on the AYANEO Pocket DS the FocalTech FT5426 sits on a marginal Qualcomm GENI bus that intermittently aborts a long read with -EAGAIN or -ETIMEDOUT, and the GENI controller has no bus recovery, so the block read fails on nearly every interrupt and the panel is unusable. Honour the "no-regmap-bulk-read" property. When set, the driver reads the frame one register at a time with a short retry on the transient bus errors, keeping each transfer small enough to complete. Boards on a healthy bus keep using the single bulk transfer and are unaffected. Signed-off-by: Teguh Sobirin <teguh@sobir.in> Co-developed-by: Alexandre Hamamdjian <azkali.limited@gmail.com> Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com> --- drivers/input/touchscreen/edt-ft5x06.c | 37 ++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index d6c3d033b83d..ac61ac44fd64 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -146,6 +146,7 @@ struct edt_ft5x06_ts_data { enum edt_ver version; unsigned int crc_errors; unsigned int header_errors; + bool no_regmap_bulk_read; }; struct edt_i2c_chip_data { @@ -295,6 +296,31 @@ static const struct regmap_config edt_M06_i2c_regmap_config = { .write = edt_M06_i2c_write, }; +static int edt_ft5x06_bulk_read(struct regmap *map, unsigned int start, + void *val, size_t len) +{ + u8 *dst = val; + size_t off; + + for (off = 0; off < len; off++) { + unsigned int v; + int ret, tries; + + for (tries = 0; tries < 3; tries++) { + ret = regmap_read(map, start + off, &v); + if (!ret) + break; + if (ret == -ETIMEDOUT || ret == -EAGAIN) + usleep_range(2000, 4000); + } + if (ret) + return ret; + dst[off] = v; + } + + return 0; +} + static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) { struct edt_ft5x06_ts_data *tsdata = dev_id; @@ -304,8 +330,12 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) int error; memset(rdbuf, 0, sizeof(rdbuf)); - error = regmap_bulk_read(tsdata->regmap, tsdata->tdata_cmd, rdbuf, - tsdata->tdata_len); + if (tsdata->no_regmap_bulk_read) + error = edt_ft5x06_bulk_read(tsdata->regmap, tsdata->tdata_cmd, + rdbuf, tsdata->tdata_len); + else + 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); @@ -1212,6 +1242,9 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client) return error; } + tsdata->no_regmap_bulk_read = + device_property_read_bool(&client->dev, "no-regmap-bulk-read"); + /* * Check which sleep modes we can support. Power-off requires the * reset-pin to ensure correct power-down/power-up behaviour. Start with -- 2.55.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time 2026-07-23 11:27 ` [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:39 ` sashiko-bot 0 siblings, 0 replies; 9+ messages in thread From: sashiko-bot @ 2026-07-23 11:39 UTC (permalink / raw) To: Alexandre Hamamdjian Cc: conor+dt, linux-input, dmitry.torokhov, devicetree, robh Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The no-regmap-bulk-read fallback causes an out-of-bounds memory read and functional breakage on EDT_M06 devices. - [Medium] Immediate redundant retries on permanent bus errors in edt_ft5x06_bulk_read. - [Medium] Incomplete implementation of no-regmap-bulk-read leaves other bulk reads unpatched. - [Low] Subsystem guide violation: variable `ret` used for error codes instead of `error` or `err`. -- --- Patch [3]: [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time --- commit 5f06c007cc93b45a3ff2f9ecba4f1f085c956d6f Author: Teguh Sobirin <teguh@sobir.in> Input: edt-ft5x06 - allow reading the touch frame one register at a time This commit introduces a "no-regmap-bulk-read" property to handle I2C controllers that cannot sustain multi-byte transfers. It implements a fallback that reads the touch frame one register at a time. > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index d6c3d033b83df..ac61ac44fd649 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c [ ... ] > @@ -295,6 +296,31 @@ static const struct regmap_config edt_M06_i2c_regmap_config = { > .write = edt_M06_i2c_write, > }; > > +static int edt_ft5x06_bulk_read(struct regmap *map, unsigned int start, > + void *val, size_t len) > +{ > + u8 *dst = val; > + size_t off; > + > + for (off = 0; off < len; off++) { > + unsigned int v; > + int ret, tries; [Severity: Low] This isn't a bug, but does using 'ret' here violate the input subsystem guidelines? The maintainer prefers using 'error' or 'err' for variables that only hold negative error codes or 0 for success. > + > + for (tries = 0; tries < 3; tries++) { > + ret = regmap_read(map, start + off, &v); [Severity: High] Does this 1-byte read cause an out-of-bounds access and functional breakage on EDT_M06 devices? If the board uses an M06 device with the no-regmap-bulk-read property, this call will split the frame read into 1-byte chunks. However, the custom edt_M06_i2c_read() function expects a larger buffer to validate the header: u8 *buf = (u8 *)val_buf; if (buf[0] != 0xaa || buf[1] != 0xaa || buf[2] != val_size) { Since regmap only provides a 1-byte buffer for a 1-byte read, this will read out of bounds and deterministically fail the header validation. Additionally, subsequent bytes will hit the default case, sending invalid factory commands instead of reading touch data. > + if (!ret) > + break; > + if (ret == -ETIMEDOUT || ret == -EAGAIN) > + usleep_range(2000, 4000); [Severity: Medium] Will this loop immediately issue redundant retries if the error is permanent? If regmap_read() returns a permanent error like -EIO (e.g., due to a NACK), it correctly skips the usleep_range() here. However, since there is no 'else break' for unhandled errors, it will immediately loop and issue the failed transaction again up to 3 times back-to-back. > + } > + if (ret) > + return ret; > + dst[off] = v; > + } > + > + return 0; > +} [ ... ] > @@ -1212,6 +1242,9 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client) > return error; > } > > + tsdata->no_regmap_bulk_read = > + device_property_read_bool(&client->dev, "no-regmap-bulk-read"); > + [Severity: Medium] Is the no-regmap-bulk-read workaround incomplete? While the ISR now uses the fallback, other parts of the driver still use unconditional bulk reads. For example, in edt_ft5x06_ts_identify() called during probe: error = regmap_bulk_read(tsdata->regmap, 0xBB, rdbuf, EDT_NAME_LEN - 1); If the bus truly cannot sustain multi-byte transfers, won't the probe and debugfs features still fail despite this property being set? > /* > * Check which sleep modes we can support. Power-off requires the > * reset-pin to ensure correct power-down/power-up behaviour. Start with -- Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-ft5426-v1-0-d4b4e32be042@gmail.com?part=3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases 2026-07-23 11:27 [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus Alexandre Hamamdjian via B4 Relay ` (2 preceding siblings ...) 2026-07-23 11:27 ` [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 ` Alexandre Hamamdjian via B4 Relay 2026-07-23 11:39 ` sashiko-bot 3 siblings, 1 reply; 9+ messages in thread From: Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:27 UTC (permalink / raw) To: Dmitry Torokhov, Henrik Rydberg, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-input, linux-kernel, devicetree, Alexandre Hamamdjian From: Alexandre Hamamdjian <azkali.limited@gmail.com> The driver is purely interrupt driven: a touch-up is only reported when the controller raises an edge for the release frame and that frame is read successfully. On a marginal i2c bus a read can fail or be dropped, and if the read that carried the release is the one lost there is no further edge to re-read it, so the contact stays held down forever - the pointer sticks mid-drag. This is readily reproducible on the AYANEO Pocket DS, whose FT5426 hangs off an unreliable Qualcomm GENI bus. Stop trusting a single edge to deliver the release. Track which slots are held down in a mask and, while any contact is down, re-read the frame on a short timer. A contact missing from the frame is released only after a few consecutive misses so a lone glitchy read cannot cut a still-present tap or drag; conversely, once the finger is really gone the polled reads stop listing it and it is released regardless of whether an explicit touch-up frame ever arrives. The timer stops as soon as the last contact is released, so an idle panel is still fully interrupt driven. The read path also gains a bounded retry over the transient bus errors and, when reads keep failing for over a second, a reset-line pulse to recover a wedged controller, dropping any held contacts afterwards since the post-reset finger state is unknown. The poll worker is cancelled on suspend and, via a devm action registered before the IRQ, on removal, so it can never touch i2c after the device is powered down or the IRQ freed. Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com> --- drivers/input/touchscreen/edt-ft5x06.c | 173 ++++++++++++++++++++++++++++++--- 1 file changed, 157 insertions(+), 16 deletions(-) diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index ac61ac44fd64..794c0650f5cb 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -19,6 +19,7 @@ #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> +#include <linux/workqueue.h> #include <linux/input.h> #include <linux/input/mt.h> #include <linux/input/touchscreen.h> @@ -147,8 +148,19 @@ struct edt_ft5x06_ts_data { unsigned int crc_errors; unsigned int header_errors; bool no_regmap_bulk_read; + unsigned long last_reset; + unsigned long last_success; + struct delayed_work poll_work; + /* io_lock serialises the frame read between the IRQ and the poll work */ + struct mutex io_lock; + u16 down_mask; + u8 miss[16]; }; +/* poll cadence and release debounce for the poll-while-touched recovery */ +#define EDT_POLL_INTERVAL_MS 15 +#define EDT_RELEASE_MISSES 3 + struct edt_i2c_chip_data { int max_support_points; }; @@ -321,26 +333,83 @@ static int edt_ft5x06_bulk_read(struct regmap *map, unsigned int start, return 0; } -static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) +static void edt_ft5x06_release_all(struct edt_ft5x06_ts_data *tsdata) +{ + int id; + + if (!tsdata->down_mask) + return; + + for (id = 0; id < tsdata->max_support_points; id++) { + if (!(tsdata->down_mask & BIT(id))) + continue; + input_mt_slot(tsdata->input, id); + input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, false); + } + tsdata->down_mask = 0; + memset(tsdata->miss, 0, sizeof(tsdata->miss)); + input_mt_report_pointer_emulation(tsdata->input, true); + input_sync(tsdata->input); +} + +static void edt_ft5x06_fetch_and_report(struct edt_ft5x06_ts_data *tsdata) { - struct edt_ft5x06_ts_data *tsdata = dev_id; struct device *dev = &tsdata->client->dev; + u16 new_mask = 0, released = 0; u8 rdbuf[63]; int i, type, x, y, id; - int error; + int error, tries; memset(rdbuf, 0, sizeof(rdbuf)); - if (tsdata->no_regmap_bulk_read) - error = edt_ft5x06_bulk_read(tsdata->regmap, tsdata->tdata_cmd, - rdbuf, tsdata->tdata_len); - else - error = regmap_bulk_read(tsdata->regmap, tsdata->tdata_cmd, - rdbuf, tsdata->tdata_len); + for (tries = 0; tries < 4; tries++) { + if (tsdata->no_regmap_bulk_read) + error = edt_ft5x06_bulk_read(tsdata->regmap, + tsdata->tdata_cmd, rdbuf, + tsdata->tdata_len); + else + error = regmap_bulk_read(tsdata->regmap, + tsdata->tdata_cmd, rdbuf, + tsdata->tdata_len); + if (!error) + break; + if (error != -EAGAIN && error != -ETIMEDOUT && + error != -EIO && error != -ENXIO) + break; + usleep_range(min(1000U << tries, 4000U), + min(2000U << tries, 8000U)); + } if (error) { dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n", error); - goto out; + /* + * A run of failed reads with no success for over a second means + * the controller is wedged rather than just glitching; pulse the + * reset line to recover it and drop any held contacts, since the + * post-reset finger state is unknown. + */ + if (tsdata->reset_gpio && + time_after(jiffies, tsdata->last_success + HZ) && + time_after(jiffies, tsdata->last_reset + 2 * HZ)) { + tsdata->last_reset = jiffies; + gpiod_set_value_cansleep(tsdata->reset_gpio, 1); + usleep_range(5000, 6000); + gpiod_set_value_cansleep(tsdata->reset_gpio, 0); + msleep(300); + tsdata->last_success = jiffies; + dev_warn_ratelimited(dev, "reset to recover controller\n"); + edt_ft5x06_release_all(tsdata); + } + return; } + tsdata->last_success = jiffies; + + /* + * TD_STATUS holds the active-contact count; a value above the panel + * maximum means the frame is corrupt, so keep the previous state. + */ + if (tsdata->version != EDT_M06 && + (rdbuf[2] & 0x0f) > tsdata->max_support_points) + return; for (i = 0; i < tsdata->max_support_points; i++) { u8 *buf = &rdbuf[i * tsdata->point_len + tsdata->tdata_offset]; @@ -349,10 +418,12 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) /* ignore Reserved events */ if (type == TOUCH_EVENT_RESERVED) continue; - /* M06 sometimes sends bogus coordinates in TOUCH_DOWN */ if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN) continue; + /* releases are derived from the down-mask diff below */ + if (type == TOUCH_EVENT_UP) + continue; x = get_unaligned_be16(buf) & 0x0fff; y = get_unaligned_be16(buf + 2) & 0x0fff; @@ -363,21 +434,75 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) id = (buf[2] >> 4) & 0x0f; if (id >= tsdata->max_support_points) continue; + if (tsdata->prop.max_x && + (x > tsdata->prop.max_x || y > tsdata->prop.max_y)) + continue; input_mt_slot(tsdata->input, id); - if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, - type != TOUCH_EVENT_UP)) - touchscreen_report_pos(tsdata->input, &tsdata->prop, - x, y, true); + input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, true); + touchscreen_report_pos(tsdata->input, &tsdata->prop, x, y, true); + new_mask |= BIT(id); } + /* + * Reconcile held contacts with this frame. A contact absent from the + * frame is released only after EDT_RELEASE_MISSES consecutive misses so + * a single glitchy read cannot cut a still-present tap or drag. + */ + for (id = 0; id < tsdata->max_support_points; id++) { + if (new_mask & BIT(id)) { + tsdata->miss[id] = 0; + continue; + } + if (!(tsdata->down_mask & BIT(id))) + continue; + if (++tsdata->miss[id] >= EDT_RELEASE_MISSES) { + input_mt_slot(tsdata->input, id); + input_mt_report_slot_state(tsdata->input, + MT_TOOL_FINGER, false); + tsdata->miss[id] = 0; + released |= BIT(id); + } + } + tsdata->down_mask = (tsdata->down_mask | new_mask) & ~released; + input_mt_report_pointer_emulation(tsdata->input, true); input_sync(tsdata->input); +} + +static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) +{ + struct edt_ft5x06_ts_data *tsdata = dev_id; + + guard(mutex)(&tsdata->io_lock); + edt_ft5x06_fetch_and_report(tsdata); + if (tsdata->down_mask) + mod_delayed_work(system_wq, &tsdata->poll_work, + msecs_to_jiffies(EDT_POLL_INTERVAL_MS)); -out: return IRQ_HANDLED; } +static void edt_ft5x06_poll_work(struct work_struct *work) +{ + struct edt_ft5x06_ts_data *tsdata = + container_of(to_delayed_work(work), + struct edt_ft5x06_ts_data, poll_work); + + guard(mutex)(&tsdata->io_lock); + edt_ft5x06_fetch_and_report(tsdata); + if (tsdata->down_mask) + mod_delayed_work(system_wq, &tsdata->poll_work, + msecs_to_jiffies(EDT_POLL_INTERVAL_MS)); +} + +static void edt_ft5x06_cancel_poll(void *data) +{ + struct edt_ft5x06_ts_data *tsdata = data; + + cancel_delayed_work_sync(&tsdata->poll_work); +} + struct edt_ft5x06_attribute { struct device_attribute dattr; size_t field_offset; @@ -1244,6 +1369,10 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client) tsdata->no_regmap_bulk_read = device_property_read_bool(&client->dev, "no-regmap-bulk-read"); + tsdata->last_success = jiffies; + tsdata->last_reset = jiffies; + mutex_init(&tsdata->io_lock); + INIT_DELAYED_WORK(&tsdata->poll_work, edt_ft5x06_poll_work); /* * Check which sleep modes we can support. Power-off requires the @@ -1341,6 +1470,15 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client) return error; } + /* + * Registered before the IRQ so it unwinds after the IRQ is freed on + * removal: no edge can re-arm the poll worker once it is cancelled. + */ + error = devm_add_action_or_reset(&client->dev, edt_ft5x06_cancel_poll, + tsdata); + if (error) + return error; + irq_flags = irq_get_trigger_type(client->irq); if (irq_flags == IRQF_TRIGGER_NONE) irq_flags = IRQF_TRIGGER_FALLING; @@ -1383,6 +1521,9 @@ static int edt_ft5x06_ts_suspend(struct device *dev) struct gpio_desc *reset_gpio = tsdata->reset_gpio; int ret; + /* stop the poll worker so it cannot touch i2c after power-down */ + cancel_delayed_work_sync(&tsdata->poll_work); + if (device_may_wakeup(dev)) return 0; -- 2.55.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases 2026-07-23 11:27 ` [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases Alexandre Hamamdjian via B4 Relay @ 2026-07-23 11:39 ` sashiko-bot 0 siblings, 0 replies; 9+ messages in thread From: sashiko-bot @ 2026-07-23 11:39 UTC (permalink / raw) To: Alexandre Hamamdjian Cc: linux-input, devicetree, robh, conor+dt, dmitry.torokhov Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The polling worker can be erroneously re-armed during device suspend, leading to invalid I2C accesses while the device is powered off. - [Medium] A single transient I2C read failure after an idle period immediately triggers a spurious controller reset. - [Medium] The controller reset recovery logic breaks permanently after ~24.8 days of uptime on 32-bit systems due to jiffies wrap-around. -- --- Patch [4]: [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases --- commit 601140d3fc50fbfeaa9e78699c5e29c00098c932 Author: Alexandre Hamamdjian <azkali.limited@gmail.com> Input: edt-ft5x06 - poll while a contact is down to recover dropped releases This commit adds a polling mechanism to recover from dropped release events on unreliable I2C buses. It tracks active contacts and polls the panel while any are down, synthesizing releases if contacts disappear. It also adds a bounded retry for transient bus errors and a controller reset mechanism if reads fail for over a second. > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index ac61ac44fd649..794c0650f5cb3 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c [ ... ] > @@ -321,26 +333,83 @@ static int edt_ft5x06_bulk_read(struct regmap *map, unsigned int start, [ ... ] > +static void edt_ft5x06_fetch_and_report(struct edt_ft5x06_ts_data *tsdata) > { [ ... ] > + for (tries = 0; tries < 4; tries++) { [ ... ] > + } > if (error) { > dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n", > error); > - goto out; > + /* > + * A run of failed reads with no success for over a second means > + * the controller is wedged rather than just glitching; pulse the > + * reset line to recover it and drop any held contacts, since the > + * post-reset finger state is unknown. > + */ > + if (tsdata->reset_gpio && > + time_after(jiffies, tsdata->last_success + HZ) && > + time_after(jiffies, tsdata->last_reset + 2 * HZ)) { [Severity: Medium] Will a single transient I2C read failure after an idle period immediately trigger a spurious controller reset? If the device sits idle for > 1 second, tsdata->last_success becomes stale. When a new touch arrives, if the very first I2C read fails due to a transient bus error, the time_after(jiffies, tsdata->last_success + HZ) check evaluates to true. This bypasses the intended 1-second delay of continuous failures, and immediately pulses the reset line, causing a 300ms stall and dropping the initial touch. [Severity: Medium] Does the controller reset recovery logic break permanently after ~24.8 days of uptime on 32-bit systems due to jiffies wrap-around? If the system runs for more than 24.8 days (exceeding MAX_JIFFY_OFFSET) without the touchscreen wedging, tsdata->last_reset remains at its initialization value. A subsequent I2C bus error triggering the recovery path would cause the time_after(jiffies, tsdata->last_reset + 2 * HZ) check to evaluate to false due to signed integer overflow, bypassing the reset entirely until jiffies wraps around the rest of the 32-bit space. > + tsdata->last_reset = jiffies; > + gpiod_set_value_cansleep(tsdata->reset_gpio, 1); [ ... ] > +static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id) > +{ > + struct edt_ft5x06_ts_data *tsdata = dev_id; > + > + guard(mutex)(&tsdata->io_lock); > + edt_ft5x06_fetch_and_report(tsdata); > + if (tsdata->down_mask) > + mod_delayed_work(system_wq, &tsdata->poll_work, > + msecs_to_jiffies(EDT_POLL_INTERVAL_MS)); > > -out: > return IRQ_HANDLED; > } [ ... ] > @@ -1383,6 +1521,9 @@ static int edt_ft5x06_ts_suspend(struct device *dev) > struct gpio_desc *reset_gpio = tsdata->reset_gpio; > int ret; > > + /* stop the poll worker so it cannot touch i2c after power-down */ > + cancel_delayed_work_sync(&tsdata->poll_work); > + [Severity: High] Can the polling worker be erroneously re-armed during device suspend? In edt_ft5x06_ts_suspend(), cancel_delayed_work_sync() is called to stop the poll worker, but the device's interrupt is not disabled until several operations later (or never disabled in HIBERNATE mode). If an interrupt occurs in this window, the ISR edt_ft5x06_ts_isr() runs, observes a non-zero down_mask, and unconditionally calls mod_delayed_work(). This could cause the worker to execute after the suspend function has disabled regulators and suspended the I2C adapter, potentially leading to I2C access timeouts or hardware hangs. > if (device_may_wakeup(dev)) > return 0; > -- Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-ft5426-v1-0-d4b4e32be042@gmail.com?part=4 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-07-23 11:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-23 11:27 [PATCH 0/4] Input: edt-ft5x06: robustness fixes for a FocalTech FT5426 on a marginal bus Alexandre Hamamdjian via B4 Relay 2026-07-23 11:27 ` [PATCH 1/4] Input: edt-ft5x06 - ignore contacts with an out-of-range slot id Alexandre Hamamdjian via B4 Relay 2026-07-23 11:40 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 2/4] dt-bindings: input: edt-ft5x06 - add no-regmap-bulk-read property Alexandre Hamamdjian via B4 Relay 2026-07-23 11:35 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 3/4] Input: edt-ft5x06 - allow reading the touch frame one register at a time Alexandre Hamamdjian via B4 Relay 2026-07-23 11:39 ` sashiko-bot 2026-07-23 11:27 ` [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases Alexandre Hamamdjian via B4 Relay 2026-07-23 11:39 ` sashiko-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox