Re: [PATCH] Revert "drm/bridge: ti-sn65dsi83: Fix enable error path"

[Alexander Stein]

Hi Luca,

Am Mittwoch, 6. März 2024, 13:39:20 CET schrieb Luca Ceresoli:
> This reverts commit 8a91b29f1f50ce7742cdbe5cf11d17f128511f3f.
> 
> The regulator_disable() added by the original commit solves one kind of
> regulator imbalance but adds another one as it allows the regulator to be
> disabled one more time than it is enabled in the following scenario:
> 
>  1. Start video pipeline -> sn65dsi83_atomic_pre_enable -> regulator_enable
>  2. PLL lock fails -> regulator_disable
>  3. Stop video pipeline -> sn65dsi83_atomic_disable -> regulator_disable
> 
> The reason is clear from the code flow, which looks like this (after
> removing unrelated code):
> 
>   static void sn65dsi83_atomic_pre_enable()
>   {
>       regulator_enable(ctx->vcc);
> 
>       if (PLL failed locking) {
>           regulator_disable(ctx->vcc);  <---- added by patch being reverted
>           return;
>       }
>   }
> 
>   static void sn65dsi83_atomic_disable()
>   {
>       regulator_disable(ctx->vcc);
>   }
> 
> The use case for introducing the additional regulator_disable() was
> removing the module for debugging (see link below for the discussion). If
> the module is removed after a .atomic_pre_enable, i.e. with an active
> pipeline from the DRM point of view, .atomic_disable is not called and thus
> the regulator would not be disabled.
> 
> According to the discussion however there is no actual use case for
> removing the module with an active pipeline, except for
> debugging/development.
> 
> On the other hand, the occurrence of a PLL lock failure is possible due to
> any physical reason (e.g. a temporary hardware failure for electrical
> reasons) so handling it gracefully should be supported. As there is no way
> for .atomic[_pre]_enable to report an error to the core, the only clean way
> to support it is calling regulator_disabled() only in .atomic_disable,
> unconditionally, as it was before.
> 
> Link: https://lore.kernel.org/all/15244220.uLZWGnKmhe@steina-w/
> Fixes: 8a91b29f1f50 ("drm/bridge: ti-sn65dsi83: Fix enable error path")
> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>

This is reasonable and explanation is great. Thanks.
Reviewed-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>

> ---
> Many thanks to Alexander for the discussion.
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index e3501608aef9..12fb22d4cd23 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -499,7 +499,6 @@ printk(KERN_ERR "%s: LVDS in fallback (24/SPWG)\n", __func__);
>  		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
>  		/* On failure, disable PLL again and exit. */
>  		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> -		regulator_disable(ctx->vcc);
>  		return;
>  	}
>  
> 
> ---
> base-commit: a71e4adac20bfe852d269addfef340923ce23a4c
> change-id: 20240306-ti-sn65dsi83-regulator-imbalance-10e217fd302c
> 
> Best regards,
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/