Message-ID: <Y46wMrS7iN6yBsBf@nixie71>
Date:   Mon, 5 Dec 2022 21:00:02 -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

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@...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), 
> 
> 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@...undy.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

Powered by blists - more mailing lists