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
