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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <YvWLwpQemH1Rm95r@google.com>
Date:   Thu, 11 Aug 2022 16:07:46 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Benjamin Rood <benjaminjrood@...il.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dan MacDonald <dmacdonald@...bellmedical.com>
Subject: Re: 0001-Input-pixcir_i2c_ts-lengthen-reset-pulse-to-touchscr

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@...il.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@...bellmedical.com> for helping to discover and fix these
> issues.
> 
> Signed-off-by: Benjamin Rood <benjaminjrood@...il.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