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

[Claude Code Review Bot <claude-review@example.com>]

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