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 ?
In any case, the patch makes sense to me:
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
