"Luca Ceresoli" <[email protected]> writes: > Hello Javier, > > On Thu Oct 15, 2026 at 10:28 AM CEST, Javier Martinez Canillas wrote: >> Luca Ceresoli <[email protected]> writes: >> >> Hello Luca, >> >>> Commit e02b5262fd28 ("drm/bridge: it66121: acquire reset GPIO in probe") >>> rightfully added code to make the reset pin really work instead of being a >>> no-op, but that change introduced a regression making probe fail most of >>> the times, at least on some hardware. >>> >>> On a BeagleBone Black rev.D the IT66122 silently fails probing >90% of the >>> times. This is because: >>> >>> 1. the regmap_read() calls just after it66121_hw_reset() to read the ID >>> registers fail >>> 2. there is no error checking for these regmap_read(), execution >>> continues silently >>> 3. this results in not matching any of the IDs from it66xx_chip_info >>> 4. and this results in silently returning -ENODEV >>> >>> Empirical tests showed a small delay after de-asserting reset fixes >>> probing. Likely the device needs some ramp-up time after reset. No such >>> requirement is mentioned in the docs I could find, so the amount of delay >>> was chosen by testing: 10 ms worked in 100% of 25 test runs, so setting a >>> 20 ms delay should provide a safe margin. >>> >>> Fixes: e02b5262fd28 ("drm/bridge: it66121: acquire reset GPIO in probe") >>> Cc: [email protected] >>> Signed-off-by: Luca Ceresoli <[email protected]> >>> --- >>> FYI For items 2 and 4 above, I'm sending separate patches to make those >>> errors not silent. Those aren't fixes, so they would be for drm-misc-next, >>> while this patch is for drm-misc-fixes. >>> --- >>> drivers/gpu/drm/bridge/ite-it66121.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c >>> b/drivers/gpu/drm/bridge/ite-it66121.c >>> index 51fc68ffdf8f..692a85e1e2e8 100644 >>> --- a/drivers/gpu/drm/bridge/ite-it66121.c >>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >>> @@ -356,6 +356,7 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) >>> gpiod_set_value(ctx->gpio_reset, 1); >>> msleep(20); >>> gpiod_set_value(ctx->gpio_reset, 0); >>> + msleep(20); >> >> Maybe adding a comment here mentioning that this value was found by >> testing and is not mentioned in the IT66122 documentation ? > > Sure, I'll add in v2: > > /* Post-reset delay not specified, value found by testing */ > >> In any case, the patch makes sense to me: >> >> Reviewed-by: Javier Martinez Canillas <[email protected]> > > Thanks for reviewing! >
You are welcome. > I assume I can keep your R-by with the above comment, let me know if that's > not the case. > That's correct, yes. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
