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: Sat, 16 May 2026 08:42:29 +1000 Message-ID: In-Reply-To: <20260516-exynos-dsim-fixes-v2-2-db9bf96ae641@disroot.org> References: <20260516-exynos-dsim-fixes-v2-0-db9bf96ae641@disroot.org> <20260516-exynos-dsim-fixes-v2-2-db9bf96ae641@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 critical bugs =E2=80=94 needs respin.** #### Bug 1 (Critical): Inverted `wait_for_completion_timeout` return value ```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 **positive (re= maining jiffies) on success** (`kernel/sched/completion.c:166`). This condi= tion is inverted =E2=80=94 it prints the error and returns failure when the= PLL *successfully* stabilizes, and silently continues when it *times out*. The correct check should be: ```c if (!wait_for_completion_timeout(&dsi->pll_stabilized, usecs_to_jiffies(timeout))) { ``` #### Bug 2 (Critical): Timeout rounds to zero jiffies The computed timeout is in microseconds. For Exynos 7870, `PLL_TIMER =3D 80= 000` and assuming a ~100 MHz bus clock, `timeout =3D max(80000/100, 100) = =3D 800` microseconds. For other variants the values are even smaller (e.g.= , `PLL_TIMER =3D 500` =E2=86=92 ~100 us floor). `usecs_to_jiffies(800)` returns **0** at any standard `HZ` value (100, 250,= 1000), because one jiffy is always >=3D 1 ms. With a zero timeout, `wait_f= or_completion_timeout` returns immediately without waiting, making this a n= o-op. Combined with Bug 1, the code "works" by accident: the PLL hasn't stabilize= d yet (returns 0 =E2=86=92 treated as success), and the PLL presumably fini= shes stabilizing shortly after. But **no actual wait occurs**, which defeat= s the entire purpose of the patch. The timeout should use a more appropriate scale. Consider `fsleep()` + manu= al completion check, or use `msecs_to_jiffies()` with a millisecond-scale t= imeout, or add 1 jiffy to ensure a non-zero wait: ```c usecs_to_jiffies(timeout) + 1 ``` or convert the timeout to a reasonable millisecond value with a sane minimu= m (e.g., 1ms). #### Issue 3 (Moderate): PLL_STABLE interrupt may be masked After `DSIM_INT_SW_RST_RELEASE`, the IRQ handler sets the interrupt mask to: ```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` (`BIT(31)`) is **not** in this unmasked set, so it re= mains masked (bit set =3D masked). If `samsung_dsim_set_pll` is called afte= r this mask is applied, the PLL_STABLE interrupt will never reach the CPU, = and the completion will never fire. The mask should include `DSIM_INT_PLL_STABLE` in the unmasked set, or the m= ask should be explicitly updated before calling `samsung_dsim_set_pll`. #### Issue 4 (Minor): Typo in comment ```c * bus clock rate. Calcutate the timeout delay on-the-fly here. ``` "Calcutate" =E2=86=92 "Calculate" #### Issue 5 (Minor): Assumption about clk_data[0] ```c fin =3D clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ; ``` The comment says "It is assumed that the bus clock is the first clock in th= e provided bulk clock data." This is true for `exynos7870_clk_bulk_data` (w= here `[0]` is `"bus"`), but other variants have different first clocks (e.g= ., `exynos3_clk_bulk_data[0]` is `"bus_clk"`). The assumption should be val= idated or the clock should be looked up by name rather than index. --- Generated by Claude Code Patch Reviewer