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