From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: ti-sn65dsi83: Move PLL initialization to atomic_enable Date: Tue, 05 May 2026 10:20:12 +1000 Message-ID: In-Reply-To: <20260430143542.2279584-1-paul.geurts@prodrive-technologies.com> References: <20260430143542.2279584-1-paul.geurts@prodrive-technologies.com> <20260430143542.2279584-1-paul.geurts@prodrive-technologies.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Concept: Good.** The rationale is clear and the fix is appropriate for th= e problem described. **Issue 1 =E2=80=94 `devm_add_action` moved before PLL enable, creating a c= leanup asymmetry on PLL failure.** In the original code, `devm_add_action(ctx->dev, sn65dsi83_release_resource= s, ctx)` was placed at the `err_add_action` label =E2=80=94 meaning it was = called both on the success path and the PLL-failure path. The `sn65dsi83_re= lease_resources()` function handles tearing down the enable GPIO and disabl= ing 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 co= rrect =E2=80=94 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 co= de 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), bu= t there's no explicit resource cleanup triggered. The question is whether `= sn65dsi83_atomic_disable()` will still be called by the DRM core after a fa= iled `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** st= ill called on the teardown path even if enable had issues, so this is likel= y acceptable. But this is a subtle behavioral dependency that the commit me= ssage should document. **Issue 2 =E2=80=94 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 w= on't be registered, and when `sn65dsi83_atomic_disable()` later calls `devm= _release_action()`, it won't find the action =E2=80=94 meaning the regulato= r and GPIO won't be released. This was also not checked in the original cod= e (same pre-existing bug), so it's not a regression introduced by this patc= h, but it would be good to fix while the code is being reworked. Using `dev= m_add_action_or_reset()` would be the safer alternative, as it calls the re= lease function on failure. **Issue 3 (minor) =E2=80=94 Commit message could be more precise.** The commit message says "Enable and lock the PLL in enable instead of pre_e= nable" which is accurate, but it would benefit from explicitly mentioning t= hat the soft reset (`REG_RC_RESET_SOFT_RESET` write) is also moved, since i= t'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 =3D 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 onl= y uses `ctx`, `pval`, and `ret`, all of which are declared in the new `atom= ic_enable`). The register configuration that *must* precede PLL lock (clock= source, divider, lane config, format registers, timing registers) all rema= in 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 =E2=86=92 soft reset =E2=86=92 clear errors =E2=86=92 check statu= s =E2=86=92 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 i= n `atomic_enable` is safe w.r.t. resource cleanup, and (2) optionally handl= ing the `devm_add_action` return value. Neither is a showstopper, but item = (1) deserves a response from the author confirming DRM core guarantees `ato= mic_disable` will be called. --- Generated by Claude Code Patch Reviewer