linux-kernel - Re: [PATCH] Input: edt-ft5x06 - always do msleep(300) during initialization

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y4pCtm4J3HWhYl8/@nixie71>
Date:   Fri, 2 Dec 2022 12:23:50 -0600
From:   Jeff LaBundy <jeff@...undy.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Dario Binacchi <dario.binacchi@...rulasolutions.com>,
        Oliver Graute <oliver.graute@...oconnector.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        broonie@...nel.org
Subject: Re: [PATCH] Input: edt-ft5x06 - always do msleep(300) during
 initialization

+ Mark

Hi Rasmus,

On Fri, Dec 02, 2022 at 11:57:59AM +0100, Rasmus Villemoes wrote:
> We have a board with an FT5446, which is close enough to a
> FT5506 (i.e. it also supports up to 10 touch points and has similar
> register layout) for this driver to work. However, on our board the
> iovcc and vcc regulators are indeed controllable (so not always-on),
> but there is no reset or wakeup gpio hooked up.
> 
> Without a large enough delay between the regulator_enable() calls and
> edt_ft5x06_ts_identify(), the first edt_ft5x06_ts_readwrite() call
> fails with -ENXIO and thus the device fails to probe. So
> unconditionally do an mdelay(300) instead of only when a reset-gpio is
> present.
> 
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>

This is just my $.02, but it does not seem we are on the correct path
here. 300 ms sounds more like bulk capacitor charge time rather than
anything to do with this specific IC; is that a reasonable assumption?

Normally, we want to do the following:

1. Enable regulator
2. Wait for voltage rail to stabilize (RC time constant)
3. Wait for any applicable POR delay (IC datasheet)
4. Deassert reset
5. Wait for any applicable reset delay (IC datasheet)
6. Start communication

Here we are dealing with step (2), which is board dependent. Some may
require more time (larger bulk capacitance), same may require less or
none at all (e.g. voltage rail enabled by default and stable by the
time the kernel starts).

I think the right solution is to introduce a variant of regulator_enable()
which does not return until a delay passes, where that delay is specified
in the regulator's child node. Unless something like this exists?

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 9ac1378610bc..8bafa123083c 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1239,8 +1239,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  	if (tsdata->reset_gpio) {
>  		usleep_range(5000, 6000);
>  		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> -		msleep(300);
>  	}
> +	msleep(300);
>  
>  	input = devm_input_allocate_device(&client->dev);
>  	if (!input) {
> -- 
> 2.37.2
> 

Kind regards,
Jeff LaBundy