Re: [PATCH 2/7] Input: edt-ft5x06 - move the controller bring-up into a helper

Re: [PATCH 2/7] Input: edt-ft5x06 - move the controller bring-up into a helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Input: edt-ft5x06: edt_ft5x06_ts_init_hw() registers device before IRQ
--

commit 5aab9a2f7e5d9f696c6cdfb8f9baf87581af8689
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - move the controller bring-up into a helper

This commit moves the controller reset release, hardware identification,
input device registration, and debugfs setup out of edt_ft5x06_probe()
into a new edt_ft5x06_ts_init_hw() helper function.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 543cc62088c7a..536d633d97a78 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1116,16 +1116,96 @@ static void edt_ft5x06_disable_regulators(void *arg)
>  	regulator_disable(data->iovcc);
>  }
>  
> +/*
> + * Take the controller out of reset, identify it and register the input
> + * device.
> + */
> +static int edt_ft5x06_ts_init_hw(struct edt_ft5x06_ts_data *tsdata)
> +{

[ ... ]

> +	error = input_register_device(input);
> +	if (error)
> +		return error;
> +
> +	edt_ft5x06_ts_prepare_debugfs(tsdata);
> +
> +	return 0;
> +}
> +
>  int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  		     const struct edt_ft5x06_chip_data *chip_data, u16 bustype,
>  		     struct dentry *debugfs)
>  {

[ ... ]

> @@ -1225,78 +1305,22 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,

[ ... ]

> +	error = edt_ft5x06_ts_init_hw(tsdata);
> +	if (error)
>  		return error;
> -	}
>  
>  	irq_flags = irq_get_trigger_type(irq);
>  	if (irq_flags == IRQF_TRIGGER_NONE)
> @@ -1310,12 +1334,6 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  		return error;
>  	}
>  
> -	error = input_register_device(input);
> -	if (error)
> -		return error;
> -
> -	edt_ft5x06_ts_prepare_debugfs(tsdata);
> -
>  	dev_dbg(dev,
>  		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
>  		irq,

[Severity: Medium]
Previously, edt_ft5x06_probe() called devm_request_threaded_irq() before
calling input_register_device() and edt_ft5x06_ts_prepare_debugfs().

With edt_ft5x06_ts_init_hw() now running before devm_request_threaded_irq(),
the input device and debugfs files are published to userspace before the
touchscreen IRQ is requested.

If devm_request_threaded_irq() fails, the input device is announced to
userspace only to be immediately torn down during devres release.

Additionally, because edt_ft5x06_ts_prepare_debugfs() exposes the debugfs
mode file early, writing 1 to it before or during IRQ setup triggers:

edt_ft5x06_debugfs_mode_set()
  -> edt_ft5x06_factory_mode()
       disable_irq(tsdata->irq);
       if (!tsdata->raw_buffer) {
           tsdata->raw_bufsize = tsdata->num_x * tsdata->num_y *
                                 sizeof(u16);
           tsdata->raw_buffer = kzalloc(tsdata->raw_bufsize, GFP_KERNEL);
           ...
       }

Can this cause disable_irq() (or enable_irq() on error or when switching
back in edt_ft5x06_work_mode()) to be called before the IRQ is requested, or
cause irq_startup() inside devm_request_threaded_irq() to re-enable the
interrupt while tsdata->factory_mode is true?

Also, if devm_request_threaded_irq() fails after tsdata->raw_buffer is
allocated, edt_ft5x06_remove() -> edt_ft5x06_ts_teardown_debugfs() is not
run on probe failure. Does this leak tsdata->raw_buffer?

Should devm_request_threaded_irq() be called before edt_ft5x06_ts_init_hw()
(for example, with IRQF_NO_AUTOEN if needed until the hardware is ready), or
should input_register_device() and edt_ft5x06_ts_prepare_debugfs() happen
after the IRQ is requested?

oggler

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140827.290666-1-lukapanio@gmail.com?part=2

Re: [PATCH 6/7] Input: edt-ft5x06 - add SPI transport and FT8722 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: edt-ft5x06 - unaligned DMA rx buffer sharing cacheline with tx
--

commit 6c4cc2125db365d716cb624219a2c7fd67fb674b
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - add SPI transport and FT8722 support

This commit adds an SPI transport driver for FocalTech touchscreen
controllers handled by the edt-ft5x06 driver, along with firmware
download support for the RAM-only FT8722 controller.

> diff --git a/drivers/input/touchscreen/edt-ft5x06-spi.c b/drivers/input/touchscreen/edt-ft5x06-spi.c
> new file mode 100644
> index 0000000000000..d0ae0d4b621dd
> --- /dev/null
> +++ b/drivers/input/touchscreen/edt-ft5x06-spi.c
> @@ -0,0 +1,475 @@

[ ... ]

> +static int fts_spi_xfer(struct spi_device *spi, const u8 *tx, u8 *rx,
> +			size_t len)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = tx,
> +		.rx_buf = rx,
> +		.len = len,
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(spi, &xfer, 1);
> +	fsleep(FTS_SPI_CS_HIGH_US);
> +
> +	return ret;
> +}
> +
> +static int fts_spi_write(struct spi_device *spi, u8 reg, const void *val,
> +			 size_t len)
> +{
> +	size_t xlen = FTS_SPI_HEADER_LEN + (len ? FTS_SPI_DUMMY_LEN + len : 0);
> +	u8 *tx, *rx;
> +	int ret, i;
> +
> +	if (len > U16_MAX)
> +		return -EINVAL;
> +
> +	tx = kzalloc(2 * xlen, GFP_KERNEL);
> +	if (!tx)
> +		return -ENOMEM;
> +	rx = tx + xlen;
> +
> +	tx[0] = reg;
> +	tx[1] = FTS_SPI_CTRL_WRITE;
> +	put_unaligned_be16(len, &tx[2]);
> +	if (len)
> +		memcpy(&tx[FTS_SPI_HEADER_LEN + FTS_SPI_DUMMY_LEN], val, len);
> +
> +	for (i = 0; i < FTS_SPI_RETRIES; i++) {
> +		ret = fts_spi_xfer(spi, tx, rx, xlen);
> +		if (!ret && !(rx[3] & FTS_SPI_STATUS_ERR))
> +			break;
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +
> +	kfree(tx);
> +	return ret;
> +}
> +
> +static int fts_spi_read(struct spi_device *spi, u8 reg, void *val,
> +			size_t len)
> +{
> +	size_t dp = FTS_SPI_HEADER_LEN + FTS_SPI_DUMMY_LEN;
> +	size_t xlen = dp + len + FTS_SPI_CRC_LEN;
> +	u8 *tx, *rx;
> +	int ret, i;
> +
> +	if (!len || len > U16_MAX)
> +		return -EINVAL;
> +
> +	tx = kzalloc(2 * xlen, GFP_KERNEL);
> +	if (!tx)
> +		return -ENOMEM;
> +	rx = tx + xlen;

[Severity: High]
Does placing rx at tx + xlen in fts_spi_write() and fts_spi_read() cause
tx and rx to share a cacheline during full-duplex SPI DMA transfers?

In both functions, a single buffer of size 2 * xlen is allocated, with tx
at offset 0 and rx = tx + xlen. Both buffers are then passed to
fts_spi_xfer() -> spi_sync_transfer() in a single full-duplex
spi_transfer, where the SPI core maps tx as DMA_TO_DEVICE and rx as
DMA_FROM_DEVICE concurrently.

Because xlen is typically not aligned to ARCH_DMA_MINALIGN (for example,
xlen is 4 or 8 for control writes, 10 or 11 for short reads, 71 for
touch report reads in edt_ft5x06_ts_isr(), and 32759 for firmware RAM
writes in fts_boot_write_ram()), rx starts in the middle of the cacheline
where tx ends.

On systems with DMA-capable SPI controllers, this overlapping mapping in
opposite DMA directions triggers DMA-API debug EEXIST warnings and risks
cacheline corruption between tx and rx during cache maintenance on
non-coherent architectures (particularly when xlen is large enough to
bypass swiotlb bouncing under CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC, or
across retries in the FTS_SPI_RETRIES loop), as well as presenting an
unaligned rx_buf pointer to SPI controllers that require aligned buffers.

Could tx and rx be allocated separately, or could the offset of rx be
aligned with ALIGN(xlen, ARCH_DMA_MINALIGN)?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140827.290666-1-lukapanio@gmail.com?part=6

Re: [PATCH 7/7] Input: edt-ft5x06 - support following a DRM panel

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Input: edt-ft5x06 - unbind race between panel_work and remove/devres teardown
- [High] Input: edt-ft5x06 - non-idempotent edt_ft5x06_ts_init_hw() retried in panel_work
- [Medium] Input: edt-ft5x06 - reset_gpio left deasserted on boot failure in panel_work
- [Medium] Input: edt-ft5x06 - missing powered_off check and factory_mode restore for panel follower
- [Medium] Input: edt-ft5x06 - unlocked sysfs reads in model_show() and fw_version_show()
--

commit 8d0f54b6a0e4f2aafd88896b2e7280382657e313
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - support following a DRM panel

This commit adds DRM panel follower support to the edt-ft5x06 touchscreen
driver for TDDI controllers that share power and reset lines with a display
panel. When following a panel, initial hardware probing and subsequent power
transitions are deferred to a workqueue triggered by the panel follower
callbacks.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 7bcc48e07d629..b2fca6c372052 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c

[ ... ]

> @@ -1206,9 +1217,117 @@ static int edt_ft5x06_ts_init_hw(struct edt_ft5x06_ts_data *tsdata)
>  
>  	edt_ft5x06_ts_prepare_debugfs(tsdata);
>  
> +	tsdata->hw_initialized = true;
> +
> +	return 0;
> +}
> +
> +/*
> + * TDDI controllers share their power and reset with the display panel. When
> + * following a panel, power the touch controller down before the panel goes
> + * off and bring it back once the panel is up again.
> + */
> +static void edt_ft5x06_panel_work(struct work_struct *work)
> +{
> +	struct edt_ft5x06_ts_data *tsdata =
> +		container_of(work, struct edt_ft5x06_ts_data, panel_work);
> +	int error;
> +
> +	guard(mutex)(&tsdata->mutex);
> +
> +	if (!tsdata->powered_off)
> +		return;
> +
> +	if (!tsdata->hw_initialized) {
> +		error = edt_ft5x06_ts_init_hw(tsdata);
> +		if (error) {
> +			/* try again the next time the panel comes up */
> +			gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
> +			dev_err(tsdata->dev,
> +				"Failed to initialize the controller: %d\n",
> +				error);
> +			return;
> +		}
> +		goto out;
> +	}

[Severity: High]
Is it safe to retry edt_ft5x06_ts_init_hw() on a subsequent panel prepare
cycle if the first attempt fails?

edt_ft5x06_ts_init_hw() performs several one-shot actions that do not roll
back on error:

1. In edt_ft5x06_ts_init_hw() -> edt_ft5x06_ts_identify(), if an EDT_M06
   chip is detected, it destroys tsdata->regmap and replaces it:

	regmap_exit(tsdata->regmap);
	tsdata->regmap = regmap_init_i2c(client,
					 &edt_M06_i2c_regmap_config);
	if (IS_ERR(tsdata->regmap)) {
		dev_err(tsdata->dev, "regmap allocation failed\n");
		return PTR_ERR(tsdata->regmap);
	}

   If regmap_init_i2c() returns an error pointer, the next retry will pass
   that error pointer into regmap_bulk_read() and dereference it. Even if
   regmap_init_i2c() succeeds and a later step in edt_ft5x06_ts_init_hw()
   fails, the next retry will call edt_ft5x06_ts_identify() using
   edt_M06_i2c_regmap_config, which masks the 0xBB identification register
   to 0x3B and only reads a single byte, breaking M06 identification.

2. edt_ft5x06_ts_init_hw() calls input_register_device(tsdata->input) on the
   input_dev allocated once in edt_ft5x06_probe(). If input_register_device()
   fails, struct input_dev cannot be passed to input_register_device() a
   second time on the next retry.

3. In edt_ft5x06_ts_init_hw(), if tsdata->boot() fails, it calls
   dev_err_probe() from the background panel_work workqueue after
   edt_ft5x06_probe() has already returned.

> +
> +	gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> +
> +	if (tsdata->boot) {
> +		error = tsdata->boot(tsdata->dev, tsdata->reset_gpio);
> +		if (error) {
> +			dev_err(tsdata->dev,
> +				"Failed to boot the controller: %d\n", error);
> +			return;
> +		}
> +	} else {
> +		msleep(300);
> +	}

[Severity: Medium]
Should reset_gpio be re-asserted to 1 if tsdata->boot() fails here, just as
is done in the !tsdata->hw_initialized error path above?

Because tsdata->powered_off remains true on this early return, a subsequent
call to edt_ft5x06_panel_unpreparing() will see tsdata->powered_off == true
and return early without asserting reset_gpio to 1:

	if (tsdata->powered_off)
		return 0;

	disable_irq(tsdata->irq);
	gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
	tsdata->powered_off = true;

That leaves reset_gpio deasserted (0) while the shared TDDI panel power goes
off, and also prevents a 1 -> 0 reset transition on the next panel wake-up.

> +
> +	edt_ft5x06_restore_reg_parameters(tsdata);
> +out:
> +	tsdata->powered_off = false;
> +	enable_irq(tsdata->irq);
> +}

[Severity: Medium]
What happens if tsdata->factory_mode is active across a panel power cycle, or
if the debugfs files are accessed while tsdata->powered_off is true?

In edt_ft5x06_debugfs_mode_set() and edt_ft5x06_debugfs_raw_data_read(),
tsdata->mutex is acquired, but unlike edt_ft5x06_setting_show() and
edt_ft5x06_setting_store(), neither function checks tsdata->powered_off
before accessing the controller over regmap or toggling IRQ state.

In addition, if factory mode was enabled before the panel turned off
(calling disable_irq(tsdata->irq) once and setting tsdata->factory_mode =
true), edt_ft5x06_panel_unpreparing() will call disable_irq(tsdata->irq) a
second time and assert reset_gpio. When edt_ft5x06_panel_work() runs on
resume, the controller comes out of reset in work mode while
tsdata->factory_mode is still true, causing edt_ft5x06_restore_reg_parameters()
on M06 to format writes as factory-mode requests, and leaving tsdata->irq
disabled after the single enable_irq(tsdata->irq) call because
edt_ft5x06_factory_mode() is not called as in edt_ft5x06_ts_resume():

	edt_ft5x06_restore_reg_parameters(tsdata);
	enable_irq(tsdata->irq);

	if (tsdata->factory_mode)
		ret = edt_ft5x06_factory_mode(tsdata);

[ ... ]

> +static void edt_ft5x06_cancel_panel_work(void *data)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = data;
> +
> +	cancel_work_sync(&tsdata->panel_work);
> +}
> +
> +static int edt_ft5x06_register_panel_follower(struct edt_ft5x06_ts_data *tsdata)
> +{
> +	struct device *dev = tsdata->dev;
> +	int error;
> +
> +	INIT_WORK(&tsdata->panel_work, edt_ft5x06_panel_work);
> +	error = devm_add_action_or_reset(dev, edt_ft5x06_cancel_panel_work,
> +					 tsdata);
> +	if (error)
> +		return error;
> +
> +	tsdata->panel_follower.funcs = &edt_ft5x06_panel_follower_funcs;
> +
> +	return devm_drm_panel_add_follower(dev, &tsdata->panel_follower);
> +}

[Severity: High]
Can deferring panel follower removal and cancel_work_sync(&tsdata->panel_work)
to devres actions race with edt_ft5x06_remove() and devres teardown when the
driver is unbound while edt_ft5x06_panel_work() is running?

When unbinding the driver:

1. edt_ft5x06_remove() runs before devres release:

	void edt_ft5x06_remove(struct device *dev)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		edt_ft5x06_ts_teardown_debugfs(tsdata);
	}

   For I2C devices, i2c_device_remove() then immediately calls
   debugfs_remove_recursive(client->debugfs) before devres_release_group().
   Because panel_work has not been cancelled yet, a concurrent execution of
   edt_ft5x06_panel_work() -> edt_ft5x06_ts_init_hw() ->
   edt_ft5x06_ts_prepare_debugfs() can dereference the freed tsdata->debugfs
   dentry and allocate tsdata->raw_buffer after teardown has already run.

2. During devres teardown, remove_nodes() drains dev->devres_head into a
   local todo list before release_nodes() invokes the devm actions for
   drm_panel_remove_follower and edt_ft5x06_cancel_panel_work. If
   edt_ft5x06_panel_work() is mid-flight inside edt_ft5x06_ts_init_hw()
   waiting to finish while edt_ft5x06_cancel_panel_work() blocks in
   cancel_work_sync(), input_register_device(tsdata->input) will append
   devm_input_device_unregister to dev->devres_head after dev->devres_head
   was already drained into todo. release_nodes() will then execute
   devm_input_device_release from todo without having run
   devm_input_device_unregister.

> @@ -1331,14 +1450,24 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  	tsdata->factory_mode = false;
>  	dev_set_drvdata(dev, tsdata);
>  
> -	error = edt_ft5x06_ts_init_hw(tsdata);
> -	if (error)
> -		return error;
> +	/*
> +	 * A controller sharing its power with a panel can only be accessed
> +	 * once the panel is up, so leave it in reset and do the rest of the
> +	 * initialization from the panel follower callback.
> +	 */
> +	tsdata->is_panel_follower = drm_is_panel_follower(dev);
> +	if (!tsdata->is_panel_follower) {
> +		error = edt_ft5x06_ts_init_hw(tsdata);
> +		if (error)
> +			return error;
> +	}

[Severity: Medium]
When tsdata->is_panel_follower is true and edt_ft5x06_ts_init_hw() is
deferred to edt_ft5x06_panel_work(), should model_show() and
fw_version_show() hold tsdata->mutex when reading tsdata->name and
tsdata->fw_version?

	static ssize_t model_show(struct device *dev,
				  struct device_attribute *attr, char *buf)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		return sysfs_emit(buf, "%s\n", tsdata->name);
	}

	static ssize_t fw_version_show(struct device *dev,
				       struct device_attribute *attr, char *buf)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		return sysfs_emit(buf, "%s\n", tsdata->fw_version);
	}

Previously, tsdata->name and tsdata->fw_version were populated synchronously
in edt_ft5x06_probe() before the sysfs attributes in edt_ft5x06_groups were
registered. With hardware initialization deferred to edt_ft5x06_panel_work(),
edt_ft5x06_ts_identify() now writes to tsdata->name and tsdata->fw_version
under tsdata->mutex after the sysfs files are already visible to userspace,
so unlocked reads in model_show() and fw_version_show() can race with
edt_ft5x06_ts_identify().

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140827.290666-1-lukapanio@gmail.com?part=7

Re: [PATCH 6/7] Input: edt-ft5x06 - add SPI transport and FT8722 support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

Hello,

On Sat, Oct 17, 2026 at 04:07:54PM +0200, Luka Panio wrote:
> +#include <linux/mod_devicetable.h>
> [...]
> +#include <linux/spi/spi.h>

<linux/spi/spi.h> already provides both of_device_id and spi_device_id.
So please drop <linux/mod_devicetable.h> which pulls in a lot of
unneeded dependencies.

> [...]
> +static const struct spi_device_id edt_ft5x06_spi_ids[] = {
> +	{ "ft8722-ts", (kernel_ulong_t)&edt_ft8722_data },

Please use a named initializer here.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, edt_ft5x06_spi_ids);

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/7] dt-bindings: input: touchscreen: edt-ft5x06: Add FocalTech FT8722

[Krzysztof Kozlowski]

On Sat, Oct 17, 2026 at 04:07:53PM +0200, Luka Panio wrote:
> The FT8722 is a touch and display driver (TDDI) whose touch controller
> is connected over SPI and runs its firmware from RAM. Document its
> compatible, allow the SPI peripheral properties and add a
> firmware-name property for the firmware to download at power up.
> 
> The "-ts" suffix keeps the compatible distinct from the one of the

Same devices do not have multiple compatibles, so I don't really get the
intention here. What is documented already in the binding? If not touch
then what?

I also do not get why all of the properties are applicable for the
missing 'ts' part.

Best regards,
Krzysztof