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: validate pixel clock against PLL range in mode_valid Date: Thu, 04 Jun 2026 12:48:08 +1000 Message-ID: In-Reply-To: <1780402238.22b24b6f590c@berkoc.com> References: <1780402238.22b24b6f590c@berkoc.com> <1780402238.22b24b6f590c@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 of the framework plumbing (mgag200_drv.h + mgag200_mode.c):** The new optional callback in `mgag200_device_funcs` is well-placed =E2=80= =94 it's added before `pixpllc_atomic_check`, which is the logical ordering= . The dispatch in `mgag200_crtc_helper_mode_valid` correctly checks for NUL= L before calling: ```c if (funcs->pixpllc_mode_valid) { status =3D funcs->pixpllc_mode_valid(crtc, mode); if (status !=3D MODE_OK) return status; } ``` This is clean and correctly returns early on failure. **G200EH mode_valid =E2=80=94 verified correct:** Constants and loop ranges match `mgag200_g200eh_pixpllc_atomic_check` exact= ly: - `vcomax=3D800000, vcomin=3D400000, pllreffreq=3D33333` - `testp: 16=E2=86=921 (>>=3D 1)`, `testm: 1..<33`, `testn: 17..<257` - Formula: `(pllreffreq * testn) / (testm * testp)` =E2=9C=93 **G200EH3 mode_valid =E2=80=94 verified correct:** Constants and ranges match: - `vcomax=3D3000000, vcomin=3D1500000, pllreffreq=3D25000` - `testm: 150=E2=86=926 (--)`, `testn: 120=E2=86=9260 (--)` - Formula: `(pllreffreq * testn) / testm` =E2=9C=93 - Correctly omits `testp` from computation (it's a constant 0 in atomic_che= ck, only used as `p =3D testp + 1` when saving) Minor: The `atomic_check` has `if (delta =3D=3D 0) break;` early exits insi= de both loops; the `mode_valid` omits them. This is functionally correct (j= ust slightly less efficient at mode enumeration time) but is an unnecessary= inconsistency with the G200ER `mode_valid`, which does include the early b= reaks. **G200ER mode_valid =E2=80=94 verified correct:** Constants and ranges match: - `vcomax=3D1488000, vcomin=3D1056000, pllreffreq=3D48000` - `m_div_val[] =3D { 1, 2, 4, 8 }` =E2=9C=93 - `testr: 0..<4`, `testn: 5..<129`, `testm: 3=E2=86=920 (--)`, `testo: 5..<= 33` - Formula: `vco =3D pllreffreq * (testn + 1) / (testr + 1)`, `computed =3D = vco / (m_div_val[testm] * (testo + 1))` =E2=9C=93 - Correctly includes the `delta =3D=3D 0` early break optimization **G200EV mode_valid =E2=80=94 verified correct:** Constants and ranges match: - `vcomax=3D550000, vcomin=3D150000, pllreffreq=3D50000` - `testp: 16=E2=86=921 (--)`, `testn: 1..<257`, `testm: 1..<17` - Formula: `(pllreffreq * testn) / (testm * testp)` =E2=9C=93 **G200EW3 mode_valid =E2=80=94 verified correct:** Constants and ranges match: - `vcomax=3D800000, vcomin=3D400000, pllreffreq=3D25000` - `testp: 1..<8`, `testp2: 1..<8`, `testm: 1..<26`, `testn: 32..<2048` - Guard: `testp < testp2` =E2=86=92 continue =E2=9C=93 - VCO check: `clock * testp * testp2` against bounds =E2=9C=93 - Formula: `(pllreffreq * testn) / (testm * testp * testp2)` =E2=9C=93 **G200WB mode_valid =E2=80=94 verified correct:** Constants and ranges match: - `vcomax=3D550000, vcomin=3D150000, pllreffreq=3D48000` - `testp: 1..<9`, `testm: 1..<17`, `testn: 1..<151` - Formula: `(pllreffreq * testn) / (testm * testp)` =E2=9C=93 **Tolerance choice:** All six `mode_valid` functions use `permitteddelta =3D clock * 5 / 1000` (0= .5%). The commit message states this matches G200SE's existing enforcement.= This is a reasonable tolerance =E2=80=94 if the best achievable clock is o= ff 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 ~20= 0 lines of duplicated logic and prevent future drift between `mode_valid` a= nd `atomic_check` 3. Minor style nit: the `delta =3D=3D 0` early-break optimization is presen= t in G200ER's `mode_valid` but absent from G200EH3's, despite both atomic_c= heck counterparts having it =E2=80=94 should be consistent one way or the o= ther --- Generated by Claude Code Patch Reviewer