Received-SPF: pass (google.com: domain of linux-kernel+bounces-96598-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Subject: Re: [PATCH] Revert "drm/bridge: ti-sn65dsi83: Fix enable error path"
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>, Robert Foss <rfoss@kernel.org>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@gmail.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Luca Ceresoli <luca.ceresoli@bootlin.com>
Date: Fri, 08 Mar 2024 08:42:58 +0100
Message-ID: <3552711.iIbC2pHGDl@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <20240306-ti-sn65dsi83-regulator-imbalance-v1-1-a3cea5f3e5b3@bootlin.com>
References: <20240306-ti-sn65dsi83-regulator-imbalance-v1-1-a3cea5f3e5b3@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"

Hi Luca,

Am Mittwoch, 6. M=E4rz 2024, 13:39:20 CET schrieb Luca Ceresoli:
> This reverts commit 8a91b29f1f50ce7742cdbe5cf11d17f128511f3f.
>=20
> 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:
>=20
>  1. Start video pipeline -> sn65dsi83_atomic_pre_enable -> regulator_enab=
le
>  2. PLL lock fails -> regulator_disable
>  3. Stop video pipeline -> sn65dsi83_atomic_disable -> regulator_disable
>=20
> The reason is clear from the code flow, which looks like this (after
> removing unrelated code):
>=20
>   static void sn65dsi83_atomic_pre_enable()
>   {
>       regulator_enable(ctx->vcc);
>=20
>       if (PLL failed locking) {
>           regulator_disable(ctx->vcc);  <---- added by patch being revert=
ed
>           return;
>       }
>   }
>=20
>   static void sn65dsi83_atomic_disable()
>   {
>       regulator_disable(ctx->vcc);
>   }
>=20
> 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 th=
us
> the regulator would not be disabled.
>=20
> According to the discussion however there is no actual use case for
> removing the module with an active pipeline, except for
> debugging/development.
>=20
> 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 w=
ay
> to support it is calling regulator_disabled() only in .atomic_disable,
> unconditionally, as it was before.
>=20
> 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@bootlin.com>

This is reasonable and explanation is great. Thanks.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
> Many thanks to Alexander for the discussion.
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 -
>  1 file changed, 1 deletion(-)
>=20
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/brid=
ge/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=3D%i\n", ret);
>  		/* On failure, disable PLL again and exit. */
>  		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> -		regulator_disable(ctx->vcc);
>  		return;
>  	}
> =20
>=20
> ---
> base-commit: a71e4adac20bfe852d269addfef340923ce23a4c
> change-id: 20240306-ti-sn65dsi83-regulator-imbalance-10e217fd302c
>=20
> Best regards,
>=20


=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/