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/mgag200: reject pixel clocks outside PLL range Date: Mon, 18 May 2026 16:11:33 +1000 Message-ID: In-Reply-To: <28e476e896dafdd31518dc5360ab7766@berkoc.com> References: <28e476e896dafdd31518dc5360ab7766@berkoc.com> <28e476e896dafdd31518dc5360ab7766@berkoc.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good.** The check is placed at exactly the right spot =E2=80= =94 after the search loop exhausts all candidates and before the results ar= e written to `pixpllc->m/n/p/s`. The tolerance formula `clock * 5 / 1000` (= 0.5% of the requested clock) matches the g200se reference at `mgag200_g200s= e.c:140` and `mgag200_g200se.c:232`. Verified across all six files that the= insertion point is between the end of the search loop and the `pixpllc->m = =3D m` assignment. **Why the bug matters:** In `atomic_update`, the register values are derive= d as e.g. `pixpllcm =3D pixpllc->m - 1` (`mgag200_g200eh.c:112`). With `m = =3D 0`, this underflows to `0xffffffff` (truncated to a `u8`), producing co= rrupted PLL register writes. **Style concern =E2=80=94 `pr_warn` vs `drm_warn`:** The patch uses bare `p= r_warn("PLL delta too large\n")` to match the g200se reference. However, th= e rest of the driver consistently uses `drm_err`/`drm_warn`/`drm_dbg` (whic= h prepend device identification). A `struct drm_device *` is available in `= atomic_check` via `crtc->dev`. This is a pre-existing issue in g200se too, = but since six new call sites are being added, it would be worth upgrading t= o: ```c drm_warn(crtc->dev, "PLL delta too large\n"); ``` across all seven sites (the six new ones plus the existing g200se). This ma= kes log messages attributable to a specific device, which matters on multi-= GPU or multi-BMC systems. **Minor functional observation:** The `pr_warn` fires during `atomic_check`= , which is a validation path =E2=80=94 userspace probing supported modes wi= ll trigger this message for any unsupported clock. This could be chatty. A = `drm_dbg_kms` might be more appropriate, since the `-EINVAL` return already= communicates the rejection to the DRM core. The g200se code has the same i= ssue, so this is more of a follow-up suggestion than a blocker for this pat= ch. **No logic bugs found.** The patch is a straightforward, correct backport o= f the existing g200se pattern to the six files that were missing it. The in= teger arithmetic is safe for the clock ranges these PLLs support (VCO maxim= a are all under 1.6 GHz =3D 1,600,000 kHz, so `clock * 5` won't overflow a = `long`). **Verdict:** The patch fixes a real bug with the right approach. Suggest sw= itching `pr_warn` to `drm_dbg_kms(crtc->dev, ...)` (or at minimum `drm_warn= (crtc->dev, ...)`), but the logic itself is correct and ready. --- Generated by Claude Code Patch Reviewer