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@xxxxxxxxxxxxxxxxxx>
> 
> 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