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@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...).
> 
> 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@xxxxxxxxxxx>

Thanks.

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

Rasmus