Re: [PATCH 2/3] Input: cyttsp5 - init IRQ early to avoid app startup problems

[Hugo Villeneuve]

Hi Marc-Olivier,

On Fri,  2 Oct 2026 14:35:28 -0400
Marc-Olivier Champagne <marc-olivier.champagne@xxxxxxxxxxxxxxxxxxxx>
wrote:

> Sometimes, the driver reports an error when trying to start the
> application.
> 
> After a reset, the bootloader will assert the interrupt pin to indicate
> when it is ready to communicate with the host; on our hardware this
> happens about 3.4 ms after the reset line is released. But since
> cyttsp5_probe() configures the IRQ and then immediately jumps to
> cyttsp5_startup(), there is a chance that the interrupt pin assertion
> can happen at around the same time that we try to deassert it by
> calling cyttsp5_deassert_int().
> 
> Initializing the IRQ while the reset pin is asserted ensures that this
> first interrupt is handled by cyttsp5_handle_irq(), which deasserts the
> interrupt pin by reading the input buffer. Therefore, calling
> cyttsp5_deassert_int() is no longer needed in that case.
> 
> However, when cyttsp5_handle_irq() has processed that first interrupt,
> it has set the cmd_done completion. Because of that, the launch
> application command would complete before its own response has been
> received. Instead of sleeping a fixed 20 ms and hoping the reset
> sentinel has already been received, wait for it explicitly after
> releasing the reset line, so that its completion is consumed before the
> first command is sent.
> 
> Without a reset line, the device may still be powering up if enabling
> the regulators turned it on, or may already be running, and the reset
> sentinel may have been raised before the IRQ was requested. Keep the
> 20 ms delay and the cyttsp5_deassert_int() call in that case, then wait
> for the interrupt handler to finish and drop the completion it may have
> set, so that the sentinel does not complete the first command either
> way.
> 
> This relies on the previous patch, which makes cyttsp5_handle_irq()
> reliably identify the reset sentinel.
> 
> Fixes: 5b0c03e24a06 ("Input: Add driver for Cypress Generation 5 touchscreen")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> Assisted-by: Copilot:claude-fable-5.1
> Signed-off-by: Marc-Olivier Champagne <marc-olivier.champagne@xxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/input/touchscreen/cyttsp5.c | 66 +++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
> index 04e141eceb45..beeb7d5bcd2c 100644
> --- a/drivers/input/touchscreen/cyttsp5.c
> +++ b/drivers/input/touchscreen/cyttsp5.c
> @@ -88,6 +88,7 @@
>  #define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT_MS	3000
>  #define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT_MS	4000
>  #define CY_HID_SET_POWER_TIMEOUT		500
> +#define CY_HID_RESET_SENTINEL_TIMEOUT_MS	1000
>  
>  /* maximum number of concurrent tracks */
>  #define TOUCH_REPORT_SIZE			10
> @@ -760,6 +761,30 @@ static int cyttsp5_deassert_int(struct cyttsp5 *ts)
>  	return -EINVAL;
>  }
>  
> +/*
> + * After a reset the device asserts the interrupt line with a zero-length
> + * report (reset sentinel) once it is ready to communicate. Consume it so
> + * that it does not complete the next command.
> + */
> +static int cyttsp5_wait_reset_sentinel(struct cyttsp5 *ts)
> +{
> +	unsigned long timeout = msecs_to_jiffies(CY_HID_RESET_SENTINEL_TIMEOUT_MS);
> +	int rc;
> +
> +	rc = wait_for_completion_interruptible_timeout(&ts->cmd_done, timeout);
> +	if (rc <= 0) {
> +		dev_err(ts->dev, "Reset sentinel timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (get_unaligned_le16(&ts->response_buf[0]) != 0) {
> +		dev_err(ts->dev, "Unexpected report instead of reset sentinel\n");
> +		return -EPROTO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
>  {
>  	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> @@ -786,12 +811,6 @@ static int cyttsp5_startup(struct cyttsp5 *ts)
>  {
>  	int error;
>  
> -	error = cyttsp5_deassert_int(ts);
> -	if (error) {
> -		dev_err(ts->dev, "Error on deassert int r=%d\n", error);
> -		return -ENODEV;
> -	}
> -
>  	/*
>  	 * Launch the application as the device starts in bootloader mode
>  	 * because of a power-on-reset
> @@ -888,13 +907,6 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
>  		return error;
>  	}
>  
> -	fsleep(10); /* Ensure long-enough reset pulse (minimum 10us). */
> -
> -	gpiod_set_value_cansleep(ts->reset_gpio, 0);
> -
> -	/* Need a delay to have device up */
> -	msleep(20);

My original patch contained only this move of the above
section past the IRQ init. The title of the current commit reflect my
original patch, but no longer this new AI-merged patch, which modifies
two different things in the same patch.

I would suggest that you use the original patch as is, as a
prerequisite for your AI-assisted new patch, to make the changes more
modular and so that they are easier to review.

Thank you for submitting these fixes/improvements!


> -
>  	error = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
>  					  IRQF_ONESHOT, name, ts);
>  	if (error) {
> @@ -902,6 +914,34 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
>  		return error;
>  	}
>  
> +	fsleep(10); /* Ensure long-enough reset pulse (minimum 10us). */
> +
> +	gpiod_set_value_cansleep(ts->reset_gpio, 0);
> +
> +	if (ts->reset_gpio) {
> +		error = cyttsp5_wait_reset_sentinel(ts);
> +		if (error)
> +			return error;
> +	} else {
> +		/*
> +		 * Without a reset line, the device may be powering up or
> +		 * already running. Give it time to come up, then make sure a
> +		 * reset sentinel does not complete the first command: read it
> +		 * if it came before the IRQ was requested, otherwise drop the
> +		 * completion set by the interrupt handler.
> +		 */
> +		msleep(20);
> +
> +		error = cyttsp5_deassert_int(ts);
> +		if (error) {
> +			dev_err(ts->dev, "Error on deassert int r=%d\n", error);
> +			return error;
> +		}
> +
> +		synchronize_irq(irq);
> +		try_wait_for_completion(&ts->cmd_done);
> +	}
> +
>  	error = cyttsp5_startup(ts);
>  	if (error) {
>  		dev_err(ts->dev, "Fail initial startup r=%d\n", error);
> -- 
> 2.34.1
> 


-- 
Hugo Villeneuve