| Message ID | 20221202105800.653982-1-linux@rasmusvillemoes.dk |
|---|---|
| State | New |
| Headers | show |
| Series | Input: edt-ft5x06 - always do msleep(300) during initialization | expand |
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@rasmusvillemoes.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@labundy.com> Thanks. Dmitry, any chance this could get picked up? I don't see it in next-20221226. Rasmus
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index 9ac1378610bc..8bafa123083c 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -1239,8 +1239,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, if (tsdata->reset_gpio) { usleep_range(5000, 6000); gpiod_set_value_cansleep(tsdata->reset_gpio, 0); - msleep(300); } + msleep(300); input = devm_input_allocate_device(&client->dev); if (!input) {
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@rasmusvillemoes.dk> --- drivers/input/touchscreen/edt-ft5x06.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)