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

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

[Rasmus Villemoes]

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@rasmusvillemoes.dk>
---
 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

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

[Jeff LaBundy]

+ 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@rasmusvillemoes.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

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

[Dmitry Torokhov]

Hi Jeff,

On Fri, Dec 02, 2022 at 12:23:50PM -0600, Jeff LaBundy wrote:
> + 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@rasmusvillemoes.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?

regulator_enable() (via regulator_do_enable() already does this:


	/* Allow the regulator to ramp; it would be useful to extend
	 * this for bulk operations so that the regulators can ramp
	 * together.
	 */
	trace_regulator_enable_delay(rdev_get_name(rdev));
	...

but I wonder if here we are still dealing with some form of 5 even in
the absence of reset gpio being actually wired up (how is the chip's
reset pin actually wired in this system)?

It would be good if we had something like regulator_get_enabled_time()
to know actual time when regulator was enabled (and for always on or
stub regulators we might use the boot time as "on" time), so that we do
not wait unnecessarily in case the regulator was turned on long time ago
or was never turned off...

Thanks.

-- 
Dmitry

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

[Mark Brown]

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

On Fri, Dec 02, 2022 at 12:23:50PM -0600, Jeff LaBundy wrote:

> 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?

regulator_enable() does not return until the regulator is ready,
if it returns earlier that is a bug in the driver for the
regulator or how it is configured on the board.

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

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

[Rasmus Villemoes]

On 02/12/2022 19.23, Jeff LaBundy wrote:
> + 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@rasmusvillemoes.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), 

Nope, we are really essentially dealing with step 5, even if there's no
reset gpio that we've flipped around. The data sheet says to wait 200 ms
(and I don't know why the driver does 300, perhaps there's some other
chip in the family with that value, or perhaps it was just a
belt-and-suspenders choice) after releasing reset. It's just that
"releasing reset" is, in my case, effectively happens at the same time
as the regulators are enabled.

I also played around with some smaller values. As I wrote, with no
delay, I would get -ENXIO, but with both 50 and 100, the chip would
"respond", but the values were essentially garbage (and not reproducible
from one boot to the next). So even if it's a rather long time, it most
definitely is a hard requirement to wait that long - perhaps we could
make it 200, but I'd rather not reduce that time when I don't know if
other variants have that 300 as a requirement.

Even if we could interrogate the regulator and ask it if "are you
actually always-on", I'd rather not make the delay conditional on that;
we cannot know if it has been on for 300+ ms, and since the device does
respond, but not correctly, we could end up with probing and
initializing the device, but in a wrong state. That's a recipe for
impossible debugging (add a single printk somewhere earlier and the
timing changes so that suddenly it gets initialized correctly...).

Rasmus

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

[Jeff LaBundy]

Hi Rasmus,

On Mon, Dec 05, 2022 at 09:59:08AM +0100, Rasmus Villemoes wrote:
> On 02/12/2022 19.23, Jeff LaBundy wrote:
> > + 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@rasmusvillemoes.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), 
> 
> Nope, we are really essentially dealing with step 5, even if there's no
> reset gpio that we've flipped around. The data sheet says to wait 200 ms
> (and I don't know why the driver does 300, perhaps there's some other
> chip in the family with that value, or perhaps it was just a
> belt-and-suspenders choice) after releasing reset. It's just that
> "releasing reset" is, in my case, effectively happens at the same time
> as the regulators are enabled.
> 
> I also played around with some smaller values. As I wrote, with no
> delay, I would get -ENXIO, but with both 50 and 100, the chip would
> "respond", but the values were essentially garbage (and not reproducible
> from one boot to the next). So even if it's a rather long time, it most
> definitely is a hard requirement to wait that long - perhaps we could
> make it 200, but I'd rather not reduce that time when I don't know if
> other variants have that 300 as a requirement.
> 
> Even if we could interrogate the regulator and ask it if "are you
> actually always-on", I'd rather not make the delay conditional on that;
> we cannot know if it has been on for 300+ ms, and since the device does
> respond, but not correctly, we could end up with probing and
> initializing the device, but in a wrong state. That's a recipe for
> impossible debugging (add a single printk somewhere earlier and the
> timing changes so that suddenly it gets initialized correctly...).

Thank you for these additional details, especially with my having taken
us on a tangent :) Perhaps the controller requires so much time because
it is loading firmware internally. Based on this information, the patch
seems reasonable to me.

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

That being said, I like Dmitry's idea but realize it's out of scope for
this particular issue.

> 
> Rasmus
> 

Kind regards,
Jeff LaBundy

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

[Rasmus Villemoes]

On 06/12/2022 04.00, Jeff LaBundy wrote:
> Hi Rasmus,
> 
> On Mon, Dec 05, 2022 at 09:59:08AM +0100, Rasmus Villemoes wrote:
>> On 02/12/2022 19.23, Jeff LaBundy wrote:
>>> + 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@rasmusvillemoes.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), 
>>
>> Nope, we are really essentially dealing with step 5, even if there's no
>> reset gpio that we've flipped around. The data sheet says to wait 200 ms
>> (and I don't know why the driver does 300, perhaps there's some other
>> chip in the family with that value, or perhaps it was just a
>> belt-and-suspenders choice) after releasing reset. It's just that
>> "releasing reset" is, in my case, effectively happens at the same time
>> as the regulators are enabled.
>>
>> I also played around with some smaller values. As I wrote, with no
>> delay, I would get -ENXIO, but with both 50 and 100, the chip would
>> "respond", but the values were essentially garbage (and not reproducible
>> from one boot to the next). So even if it's a rather long time, it most
>> definitely is a hard requirement to wait that long - perhaps we could
>> make it 200, but I'd rather not reduce that time when I don't know if
>> other variants have that 300 as a requirement.
>>
>> Even if we could interrogate the regulator and ask it if "are you
>> actually always-on", I'd rather not make the delay conditional on that;
>> we cannot know if it has been on for 300+ ms, and since the device does
>> respond, but not correctly, we could end up with probing and
>> initializing the device, but in a wrong state. That's a recipe for
>> impossible debugging (add a single printk somewhere earlier and the
>> timing changes so that suddenly it gets initialized correctly...).
> 
> Thank you for these additional details, especially with my having taken
> us on a tangent :) Perhaps the controller requires so much time because
> it is loading firmware internally. Based on this information, the patch
> seems reasonable to me.
> 
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>

Thanks.

Dmitry, any chance this could get picked up? I don't see it in
next-20221226.

Rasmus