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/
