[PATCH] drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable

[PATCH] drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable

[Paul Geurts]

Some MIPI DSI encoder drivers, like the Samsung DSI controller found in
i.MX8M Mini, enable the DSI clock on encoder enable. The sn65dsi83
pre_enable is executed before the DSIM encoder enable. As the DSI input
clock is the PLL input clock, the PLL cannot be locked in pre_enable.

Enable and lock the PLL in enable instead of pre_enable, to make sure
the input clock is available.

Fixes: dd9e329af723 ("drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec")
Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 35 +++++++++++++--------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 17a885244e1e..018e524c473c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -525,7 +525,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	struct drm_crtc *crtc;
 	bool lvds_format_24bpp;
 	bool lvds_format_jeida;
-	unsigned int pval;
 	__le16 le16val;
 	u16 val;
 	int ret;
@@ -670,6 +669,21 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		     mode->vsync_start - mode->vdisplay);
 	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
 
+	devm_add_action(ctx->dev, sn65dsi83_release_resources, ctx);
+err_exit:
+	drm_bridge_exit(idx);
+}
+
+static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
+				    struct drm_atomic_state *state)
+{
+	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+	unsigned int pval;
+	int idx, ret;
+
+	if (!drm_bridge_enter(bridge, &idx))
+		return;
+
 	/* Enable PLL */
 	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
 	usleep_range(3000, 4000);
@@ -680,7 +694,7 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		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);
-		goto err_add_action;
+		goto out;
 	}
 
 	/* Trigger reset after CSR register update. */
@@ -689,22 +703,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	/* Wait for 10ms after soft reset as specified in datasheet */
 	usleep_range(10000, 12000);
 
-err_add_action:
-	devm_add_action(ctx->dev, sn65dsi83_release_resources, ctx);
-err_exit:
-	drm_bridge_exit(idx);
-}
-
-static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
-				    struct drm_atomic_state *state)
-{
-	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
-	unsigned int pval;
-	int idx;
-
-	if (!drm_bridge_enter(bridge, &idx))
-		return;
-
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
@@ -724,6 +722,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 		sn65dsi83_monitor_start(ctx);
 	}
 
+out:
 	drm_bridge_exit(idx);
 }
 
-- 
2.39.2

Claude review: drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable
Author: Paul Geurts <paul.geurts@prodrive-technologies.com>
Patches: 1
Reviewed: 2026-05-05T10:20:12.571409

---

This is a single-patch series that moves PLL initialization (enable, lock poll, soft reset) from `sn65dsi83_atomic_pre_enable()` into `sn65dsi83_atomic_enable()`. The motivation is sound: some DSI encoder drivers (e.g., Samsung DSIM on i.MX8M Mini) only enable the DSI clock during their own `.enable` callback, which runs *after* the bridge's `.pre_enable`. Since the SN65DSI83's PLL takes its input from the DSI link clock, trying to lock the PLL in `.pre_enable` will fail when the clock isn't yet available.

The patch correctly identifies the Fixes tag for commit dd9e329af723, which moved PLL init *into* pre_enable. The code movement itself is straightforward and the resulting structure is clean.

**However, there are two issues with the error handling that need attention.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable

[Claude Code Review Bot]

Patch Review

**Concept: Good.** The rationale is clear and the fix is appropriate for the problem described.

**Issue 1 — `devm_add_action` moved before PLL enable, creating a cleanup asymmetry on PLL failure.**

In the original code, `devm_add_action(ctx->dev, sn65dsi83_release_resources, ctx)` was placed at the `err_add_action` label — meaning it was called both on the success path and the PLL-failure path. The `sn65dsi83_release_resources()` function handles tearing down the enable GPIO and disabling the regulator, which were set up earlier in `pre_enable`.

In the new code, `devm_add_action` is called unconditionally at the end of `sn65dsi83_atomic_pre_enable()` (line 672 of the patched file), which is correct — resources acquired in pre_enable need cleanup registration regardless of what happens later in enable.

However, when PLL lock fails in the new `sn65dsi83_atomic_enable()`, the code jumps to `out:` and simply calls `drm_bridge_exit(idx)`. The IRQ/monitor error-clearing code and IRQ enable code are skipped (which is correct), but there's no explicit resource cleanup triggered. The question is whether `sn65dsi83_atomic_disable()` will still be called by the DRM core after a failed `atomic_enable`. If it is, then `devm_release_action` at line 738 will handle cleanup. If it isn't, the resources will leak until device removal.

Looking at the DRM core behavior: in most cases, `atomic_disable` **is** still called on the teardown path even if enable had issues, so this is likely acceptable. But this is a subtle behavioral dependency that the commit message should document.

**Issue 2 — Return value of `devm_add_action` is not checked.**

```c
devm_add_action(ctx->dev, sn65dsi83_release_resources, ctx);
```

`devm_add_action()` can return `-ENOMEM`. If it fails, the cleanup action won't be registered, and when `sn65dsi83_atomic_disable()` later calls `devm_release_action()`, it won't find the action — meaning the regulator and GPIO won't be released. This was also not checked in the original code (same pre-existing bug), so it's not a regression introduced by this patch, but it would be good to fix while the code is being reworked. Using `devm_add_action_or_reset()` would be the safer alternative, as it calls the release function on failure.

**Issue 3 (minor) — Commit message could be more precise.**

The commit message says "Enable and lock the PLL in enable instead of pre_enable" which is accurate, but it would benefit from explicitly mentioning that the soft reset (`REG_RC_RESET_SOFT_RESET` write) is also moved, since it's logically coupled to PLL initialization and is part of the moved block.

**Code correctness of the move itself: Good.** The block of code moved is:

```c
/* Enable PLL */
regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
usleep_range(3000, 4000);
ret = regmap_read_poll_timeout(ctx->regmap, REG_RC_LVDS_PLL, pval, ...);
...
/* Trigger reset after CSR register update. */
regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
usleep_range(10000, 12000);
```

This block has no dependencies on local variables from `pre_enable` (it only uses `ctx`, `pval`, and `ret`, all of which are declared in the new `atomic_enable`). The register configuration that *must* precede PLL lock (clock source, divider, lane config, format registers, timing registers) all remain in `pre_enable`. This ordering is correct per the datasheet flow.

**The error-clearing and IRQ setup code that was already in `atomic_enable` remains in place after the moved PLL block**, which is the right ordering: PLL lock → soft reset → clear errors → check status → enable IRQ/monitoring.

**Summary:** The core fix is correct and addresses a real problem. The two items to address are (1) confirming/documenting that the PLL-failure path in `atomic_enable` is safe w.r.t. resource cleanup, and (2) optionally handling the `devm_add_action` return value. Neither is a showstopper, but item (1) deserves a response from the author confirming DRM core guarantees `atomic_disable` will be called.

---
Generated by Claude Code Patch Reviewer