[PATCH] drm/bridge: ite-it66121: fix silent probe regression due to reset timing

[PATCH] drm/bridge: ite-it66121: fix silent probe regression due to reset timing

[Luca Ceresoli]

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: stable@vger.kernel.org
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
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);
 }
 
 static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)

---
base-commit: 20b91781216dc693d862368b523e752525172176
change-id: 20261015-drm-ite-it66121-fix-reset-f1765358a866

Best regards,
--  
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/bridge: ite-it66121: fix silent probe regression due to reset timing

[Javier Martinez Canillas]

Luca Ceresoli <luca.ceresoli@bootlin.com> 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: stable@vger.kernel.org
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> 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 <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/bridge: ite-it66121: fix silent probe regression due to reset timing

[Luca Ceresoli]

Hello Javier,

On Thu Oct 15, 2026 at 10:28 AM CEST, Javier Martinez Canillas wrote:
> Luca Ceresoli <luca.ceresoli@bootlin.com> 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: stable@vger.kernel.org
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> ---
>> 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 <javierm@redhat.com>

Thanks for reviewing!

I assume I can keep your R-by with the above comment, let me know if that's
not the case.

Kind regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/bridge: ite-it66121: fix silent probe regression due to reset timing

[Javier Martinez Canillas]

"Luca Ceresoli" <luca.ceresoli@bootlin.com> writes:

> Hello Javier,
>
> On Thu Oct 15, 2026 at 10:28 AM CEST, Javier Martinez Canillas wrote:
>> Luca Ceresoli <luca.ceresoli@bootlin.com> 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: stable@vger.kernel.org
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>>> ---
>>> 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 <javierm@redhat.com>
>
> 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