Claude review: drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

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

Patch Review

**Verdict: Has critical bugs — 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 (remaining jiffies) on success** (`kernel/sched/completion.c:166`). This condition is inverted — 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 = 80000` and assuming a ~100 MHz bus clock, `timeout = max(80000/100, 100) = 800` microseconds. For other variants the values are even smaller (e.g., `PLL_TIMER = 500` → ~100 us floor).

`usecs_to_jiffies(800)` returns **0** at any standard `HZ` value (100, 250, 1000), because one jiffy is always >= 1 ms. With a zero timeout, `wait_for_completion_timeout` returns immediately without waiting, making this a no-op.

Combined with Bug 1, the code "works" by accident: the PLL hasn't stabilized yet (returns 0 → treated as success), and the PLL presumably finishes stabilizing shortly after. But **no actual wait occurs**, which defeats the entire purpose of the patch.

The timeout should use a more appropriate scale. Consider `fsleep()` + manual completion check, or use `msecs_to_jiffies()` with a millisecond-scale timeout, 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 minimum (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 = ~(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 remains masked (bit set = masked). If `samsung_dsim_set_pll` is called after 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 mask 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" → "Calculate"

#### Issue 5 (Minor): Assumption about clk_data[0]

```c
fin = 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 the provided bulk clock data." This is true for `exynos7870_clk_bulk_data` (where `[0]` is `"bus"`), but other variants have different first clocks (e.g., `exynos3_clk_bulk_data[0]` is `"bus_clk"`). The assumption should be validated or the clock should be looked up by name rather than index.

---
Generated by Claude Code Patch Reviewer