| Message ID | 20240306-ti-sn65dsi83-regulator-imbalance-v1-1-a3cea5f3e5b3@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/bridge: ti-sn65dsi83: Fix enable error path" | expand |
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@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(-) > > 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, >
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; }
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@bootlin.com> --- Many thanks to Alexander for the discussion. --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 - 1 file changed, 1 deletion(-) --- base-commit: a71e4adac20bfe852d269addfef340923ce23a4c change-id: 20240306-ti-sn65dsi83-regulator-imbalance-10e217fd302c Best regards,