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

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

Overall Series Review

Subject: drm/mgag200: validate pixel clock against PLL range in mode_valid
Author: Berkant Koc <me@berkoc.com>
Patches: 1
Reviewed: 2026-06-04T12:48:08.407892

---

This is a single well-structured patch (v2) that addresses a real bug: when a requested pixel clock falls outside a model's VCO window, the PLL search loop in `pixpllc_atomic_check` finds no candidate, leaving `m = n = p = s = 0`. The subsequent `atomic_update` then computes `pixpllc->m - 1` (unsigned underflow), programming garbage into the PLL registers.

The approach — filtering at `mode_valid` time via an optional per-model callback — is sound and follows Thomas Zimmermann's suggestion. It catches bad modes before they reach `atomic_check`, so userspace never sees modes the PLL cannot synthesize.

I verified all six `mode_valid` functions against their `atomic_check` counterparts in the current tree. The VCO constants (`vcomax`, `vcomin`, `pllreffreq`), loop ranges, computation formulas, and VCO guard conditions all match exactly across all six models (G200EH, G200EH3, G200ER, G200EV, G200EW3, G200WB). The bug description in the commit message is accurate — each `atomic_check` initializes `m = n = p = s = 0` and returns 0 unconditionally.

**Main concerns:**

1. **Significant code duplication** — Each `mode_valid` duplicates the entire search loop from its `atomic_check` counterpart (just without saving `m/n/p/s`). This creates a maintenance risk: if VCO constants or loop ranges are updated in `atomic_check`, the `mode_valid` copy must be updated in lockstep. Consider whether the search loop could be factored into a shared helper that `mode_valid` calls (checking "was a valid candidate found?") and `atomic_check` also calls (saving the best candidate). This would eliminate ~200 lines of near-duplicate code.

2. **Missing `Fixes:` tag** — The commit message describes a genuine bug with real symptoms (corrupted output clock, unsigned underflow). A `Fixes:` tag referencing the commit that introduced the unconditional `return 0` in these `atomic_check` functions would help stable kernel maintainers identify this for backporting. A `Cc: stable@vger.kernel.org` would also be appropriate.

3. **No runtime testing** — The commit message honestly notes "Tested with static analysis ... runtime confirmation on physical Matrox hardware welcome." For a display driver change, runtime testing on at least one affected model would significantly increase confidence.

**The patch is correct as-is.** The duplication concern is about long-term maintainability, not correctness. Whether the maintainer prefers the factored or duplicated approach is a judgment call.

---

---
Generated by Claude Code Patch Reviewer