From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Revert "drm/imagination: Warn or error on unsupported hardware" Date: Sat, 16 May 2026 15:12:53 +1000 Message-ID: In-Reply-To: References: 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:** The revert is clean. It removes: 1. The `pvr_exp_hw_support` module parameter and its `module_param_named`/`= MODULE_PARM_DESC` declarations 2. The `enum pvr_gpu_support_level` type and its three values 3. The `pvr_gpu_support_level()` function with the BVNC-to-support-level ma= pping 4. The `pvr_check_gpu_supported()` function with the support-level gating l= ogic 5. Replaces `return pvr_check_gpu_supported(pvr_dev, gpu_id);` with `return= 0;` All removed code is dead after the change =E2=80=94 no other callers or ref= erences remain. **Observations:** 1. **No `Fixes:` tag:** The patch doesn't include a `Fixes:` tag pointing t= o the original commit `1c21f240fbc1e47b`. While this is a revert (not stric= tly a "fix"), if the intent is for the revert to be backported to stable ke= rnels (since the original commit broke Renesas SoC support), a `Fixes:` tag= would be appropriate to signal that. Without it, stable maintainers won't = pick it up automatically. 2. **Well-documented rationale:** The below-`---` section is thorough, with= 9 references justifying the revert. This is excellent for reviewer context= but won't appear in the final git log (which is the right place for it, si= nce it's background justification rather than core commit message content). 3. **Alternative approach not discussed in commit message body:** The commi= t message body (above the `---`) is very terse =E2=80=94 just "Revert commi= t X, as it stopped the driver from working on various Renesas R-Car SoCs." = It doesn't explain why a revert was chosen over adding the BVNC values. A b= rief note on that design choice would strengthen the commit message for fut= ure readers. 4. **Module parameter removal is an ABI change:** Removing the `exp_hw_supp= ort` module parameter is technically a user-visible ABI change. Any existin= g users who set this parameter in their module configuration (e.g., in `/et= c/modprobe.d/`) would see a warning about an unknown parameter on the next = boot after this patch lands. This is minor but worth noting. 5. **Diff context matches tree:** The removed code at lines 511=E2=80=93580= of the current tree matches the patch's diff context exactly, confirming t= his is a clean revert against the expected base. **Verdict:** The patch is technically sound. The main question is whether t= he drm/imagination maintainers prefer a full revert or would rather have th= e Renesas BVNCs added to the allowlist. Both approaches are valid; the full= revert is simpler and avoids future churn as new SoCs are enabled, but los= es the safety net for truly unknown hardware. --- Generated by Claude Code Patch Reviewer