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> That being said, I like Dmitry's idea but realize it's out of scope for this particular issue. > > Rasmus > Kind regards, Jeff LaBundy
