Re: 0001-Input-pixcir_i2c_ts-lengthen-reset-pulse-to-touchscr

Re: 0001-Input-pixcir_i2c_ts-lengthen-reset-pulse-to-touchscr

[Dmitry Torokhov]

Hi Benjamin,

On Thu, Aug 11, 2022 at 02:33:13PM -0400, Benjamin Rood wrote:
> From bd53d80e1966320f8d6f2775337043cbac2d1b6e Mon Sep 17 00:00:00 2001
> From: Benjamin Rood <benjaminjrood@gmail.com>
> Date: Thu, 11 Aug 2022 14:18:16 -0400
> Subject: [PATCH] Input: pixcir_i2c_ts - lengthen reset pulse to touchscreen
>  controller
> 
> This patch adjust the reset pulse in the pixcir_i2c_ts driver to address
> the following issues:
> 
> 1. Not driving the reset signal HIGH for a long enough period, resulting
> in a "shark fin" reset signal.
> 2. Not waiting long enough after assering the reset signal to issue the
> first I2C transaction, which results in a NACK because the touchscreen
> controller was not ready to communicate.  This typically results in the
> touchscreen controller not enumerating as an input device.
> 
> The changes included in this patch address the above two issues by:
> 
> 1. Configuring the delay after driving the reset signal HIGH to use
> usleep_range(500, 1000) to allow the reset signal to reach a full logic
> HIGH voltage for a maximum period of 1ms.  This is overkill as the
> datasheet specs 80ns as the minimum duration of the reset pulse, so by
> overshooting the timing by quite a lot, it gives the driving chip enough
> time to drive a logic HIGH to assert the reset.

I guess 1 msec is not that bad...

> 2. Configuring the delay after de-asserting the reset signal to be a
> duration of 1s before issuing the first I2C transaction.  This allows
> the touchscreen controller to fully boot which avoids a NACK an a
> missing input device.

but 1 sec is insanely long, especially for driver that is not marked for
async probe. I wonder, since (I assume) the time to get out of reset
state is not specified in the datasheet, if we could not retry I2C xfers
with a small delays between them instead of waiting for whole second.

How long does it actually take for the controller to exit the reset
state on your system?

> 
> Kudos also should be given to my colleage Dan MacDonald
> <dmacdonald@curbellmedical.com> for helping to discover and fix these
> issues.
> 
> Signed-off-by: Benjamin Rood <benjaminjrood@gmail.com>
> ---
>  drivers/input/touchscreen/pixcir_i2c_ts.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index dc148b4bed74..324e41886dea 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -222,10 +222,10 @@ static void pixcir_reset(struct pixcir_i2c_ts_data *tsdata)
>  {
>   if (!IS_ERR_OR_NULL(tsdata->gpio_reset)) {
>   gpiod_set_value_cansleep(tsdata->gpio_reset, 1);
> - ndelay(100); /* datasheet section 1.2.3 says 80ns min. */
> + usleep_range(500, 1000); /* datasheet section 1.2.3 says 80ns min. */
>   gpiod_set_value_cansleep(tsdata->gpio_reset, 0);
>   /* wait for controller ready. 100ms guess. */
> - msleep(100);
> + msleep(1000);

Please note that the patch is whitespace-damaged. Did you paste it into
gmail UI?

>   }
>  }
>  
> -- 
> 2.25.1

Thanks.

-- 
Dmitry

[PATCH v2] Input: pixcir_i2c_ts - lengthen reset pulse to touchscreen controller

[Benjamin Rood]

From: Benjamin Rood <benjaminjrood@gmail.com>

This patch adjust the reset pulse in the pixcir_i2c_ts driver to address
the following issue:

1. Not driving the reset signal HIGH for a long enough period, resulting
in a "shark fin" reset signal.  Some microcontrollers do not have "fast"
internal pullups, which may result in a "shark fin" signal if the delay
between asserting and releasing the signal is too short.  Even though as
noted in the driver code the duration of 80ns is the minimum duration to
assert the reset signal, a host microcontroller using an internal
pull-up to assert the reset signal may not drive the signal high enough
with this delay to effectively put the controller into reset.

We encountered this exact scenario with an NXP i.MX8M Plus directly
connected to the RST I/O of the pixcir controller, with a resulting
non-operative touchscreen on a cold-boot sequence.

The change included in this patch addresses the issue by:

1. Configuring the delay after driving the reset signal HIGH to use
usleep_range(500, 1000) to allow the reset signal to reach a full logic
HIGH voltage for a maximum period of 1ms.  This is overkill as the
datasheet specs 80ns as the minimum duration of the reset pulse, so by
overshooting the timing by quite a lot, it gives the driving chip enough
time to drive a logic HIGH to assert the reset.

Kudos also should be given to my colleage Dan MacDonald
<dmacdonald@curbellmedical.com> for helping to discover and fix these
issues.

Signed-off-by: Benjamin Rood <benjaminjrood@gmail.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index dc148b4bed74..4af4b9016c94 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -222,7 +222,7 @@ static void pixcir_reset(struct pixcir_i2c_ts_data *tsdata)
 {
 	if (!IS_ERR_OR_NULL(tsdata->gpio_reset)) {
 		gpiod_set_value_cansleep(tsdata->gpio_reset, 1);
-		ndelay(100);	/* datasheet section 1.2.3 says 80ns min. */
+		usleep_range(500, 1000);	/* datasheet section 1.2.3 says 80ns min. */
 		gpiod_set_value_cansleep(tsdata->gpio_reset, 0);
 		/* wait for controller ready. 100ms guess. */
 		msleep(100);
-- 
2.25.1