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: samsung-dsim: use DSIM interrupt to wait for PLL stability Date: Wed, 25 Mar 2026 08:14:57 +1000 Message-ID: In-Reply-To: <20260322-exynos-dsim-fixes-v2-2-0069c9e1d9bf@disroot.org> References: <20260322-exynos-dsim-fixes-v2-0-0069c9e1d9bf@disroot.org> <20260322-exynos-dsim-fixes-v2-2-0069c9e1d9bf@disroot.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: Has bugs that need fixing.** **Bug 1 (Critical): Inverted `wait_for_completion_timeout` check** At lines 868-871 of the patched file: ```c if (wait_for_completion_timeout(&dsi->pll_stabilized, usecs_to_jiffies(timeout))) { dev_err(dsi->dev, "PLL failed to stabilize\n"); return 0; } ``` `wait_for_completion_timeout()` returns **0 on timeout** and **>0 on succes= s**. The condition is inverted =E2=80=94 the error path runs on success, an= d the success path runs on timeout. This should be: ```c if (!wait_for_completion_timeout(...)) { ``` **Bug 2 (Critical): PLL_STABLE interrupt is masked** Looking at the interrupt handler at line 1596-1602, after `DSIM_INT_SW_RST_= RELEASE`, the interrupt mask is set to block everything except `DSIM_INT_RX= _DONE`, `DSIM_INT_SFR_FIFO_EMPTY`, `DSIM_INT_SFR_HDR_FIFO_EMPTY`, `DSIM_INT= _RX_ECC_ERR`, and `DSIM_INT_SW_RST_RELEASE`. Notably, `DSIM_INT_PLL_STABLE`= is **not** in this unmasked set =E2=80=94 it remains masked. This means th= e PLL_STABLE interrupt will never be delivered after reset, and `wait_for_c= ompletion_timeout` will always time out. The mask at line 1597 needs to also include `DSIM_INT_PLL_STABLE`: ```c unsigned long mask =3D ~(DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY | DSIM_INT_SFR_HDR_FIFO_EMPTY | DSIM_INT_RX_ECC_ERR | DSIM_INT_SW_RST_RELEASE | DSIM_INT_PLL_STABLE); ``` (Note: since bug 1 inverts the sense, on current hardware these two bugs ma= y accidentally cancel out =E2=80=94 the masked interrupt causes a timeout, = which the inverted check treats as success. But that's fragile and wrong.) **Minor: Typo in comment** Line 855: `Calcutate` should be `Calculate`. **Minor: Assumption about clk_data[0] being bus clock** The comment says "It is assumed that the bus clock is the first clock in th= e provided bulk clock data." This holds for all current variants (exynos3 h= as `bus_clk`, exynos4 has `bus_clk`, exynos5433 has `bus_clk`, exynos7870 h= as `bus`), but for the IMX variants the first clock is also `bus_clk`. Stil= l, this is fragile =E2=80=94 if a future driver data entry reorders clocks,= this silently breaks. Consider whether a named lookup would be more robust= , though this is a minor concern. **Minor: `usecs_to_jiffies` with small values** With a typical HZ=3D250, `usecs_to_jiffies(100)` returns 1 jiffy (4ms), whi= ch is already much larger than 100=C2=B5s. The "gracious lower bound of 100= microseconds" described in the commit message is somewhat misleading since= the actual wait granularity is jiffies. This isn't a bug but worth being a= ware of. --- Generated by Claude Code Patch Reviewer