Claude review: drm/mgag200: validate pixel clock against PLL range in mode_valid

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

Patch Review

**Correctness of the framework plumbing (mgag200_drv.h + mgag200_mode.c):**

The new optional callback in `mgag200_device_funcs` is well-placed — it's added before `pixpllc_atomic_check`, which is the logical ordering. The dispatch in `mgag200_crtc_helper_mode_valid` correctly checks for NULL before calling:

```c
if (funcs->pixpllc_mode_valid) {
    status = funcs->pixpllc_mode_valid(crtc, mode);
    if (status != MODE_OK)
        return status;
}
```

This is clean and correctly returns early on failure.

**G200EH mode_valid — verified correct:**

Constants and loop ranges match `mgag200_g200eh_pixpllc_atomic_check` exactly:
- `vcomax=800000, vcomin=400000, pllreffreq=33333`
- `testp: 16→1 (>>= 1)`, `testm: 1..<33`, `testn: 17..<257`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**G200EH3 mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=3000000, vcomin=1500000, pllreffreq=25000`
- `testm: 150→6 (--)`, `testn: 120→60 (--)`
- Formula: `(pllreffreq * testn) / testm` ✓
- Correctly omits `testp` from computation (it's a constant 0 in atomic_check, only used as `p = testp + 1` when saving)

Minor: The `atomic_check` has `if (delta == 0) break;` early exits inside both loops; the `mode_valid` omits them. This is functionally correct (just slightly less efficient at mode enumeration time) but is an unnecessary inconsistency with the G200ER `mode_valid`, which does include the early breaks.

**G200ER mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=1488000, vcomin=1056000, pllreffreq=48000`
- `m_div_val[] = { 1, 2, 4, 8 }` ✓
- `testr: 0..<4`, `testn: 5..<129`, `testm: 3→0 (--)`, `testo: 5..<33`
- Formula: `vco = pllreffreq * (testn + 1) / (testr + 1)`, `computed = vco / (m_div_val[testm] * (testo + 1))` ✓
- Correctly includes the `delta == 0` early break optimization

**G200EV mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=550000, vcomin=150000, pllreffreq=50000`
- `testp: 16→1 (--)`, `testn: 1..<257`, `testm: 1..<17`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**G200EW3 mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=800000, vcomin=400000, pllreffreq=25000`
- `testp: 1..<8`, `testp2: 1..<8`, `testm: 1..<26`, `testn: 32..<2048`
- Guard: `testp < testp2` → continue ✓
- VCO check: `clock * testp * testp2` against bounds ✓
- Formula: `(pllreffreq * testn) / (testm * testp * testp2)` ✓

**G200WB mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=550000, vcomin=150000, pllreffreq=48000`
- `testp: 1..<9`, `testm: 1..<17`, `testn: 1..<151`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**Tolerance choice:**

All six `mode_valid` functions use `permitteddelta = clock * 5 / 1000` (0.5%). The commit message states this matches G200SE's existing enforcement. This is a reasonable tolerance — if the best achievable clock is off by more than 0.5%, the mode genuinely cannot be synthesized acceptably. This is more conservative than what the `atomic_check` does today (accepts any delta, however large), which is the right direction.

**Summary of suggestions:**

1. Add a `Fixes:` tag and `Cc: stable@vger.kernel.org`
2. Consider factoring the search loop into a shared helper to eliminate ~200 lines of duplicated logic and prevent future drift between `mode_valid` and `atomic_check`
3. Minor style nit: the `delta == 0` early-break optimization is present in G200ER's `mode_valid` but absent from G200EH3's, despite both atomic_check counterparts having it — should be consistent one way or the other

---
Generated by Claude Code Patch Reviewer