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/msm: Centralize the standalone drm device check for GPU Date: Sat, 16 May 2026 12:47:07 +1000 Message-ID: In-Reply-To: <20260513-gmu-sync-state-fix-v1-2-6e33e6aa9b4f@oss.qualcomm.com> References: <20260513-gmu-sync-state-fix-v1-0-6e33e6aa9b4f@oss.qualcomm.com> <20260513-gmu-sync-state-fix-v1-2-6e33e6aa9b4f@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Verdict: Acceptable, but the NULL parameter convention is awkward.** The rename from `msm_gpu_no_components()` to `msm_gpu_use_separate_drm_dev(pdev)` centralizes the `amd,imageon` compatibility check. However, the two call sites in `msm_drv.c` pass `NULL`: ```c + msm_gpu_use_separate_drm_dev(NULL) ? ``` ```c + if (!msm_gpu_use_separate_drm_dev(NULL)) { ``` And the function has a special case for NULL: ```c +bool msm_gpu_use_separate_drm_dev(struct platform_device *pdev) +{ + if (!pdev) + return separate_gpu_kms; + + return of_device_is_compatible(pdev->dev.of_node, "amd,imageon") || separate_gpu_kms; +} ``` Passing `NULL` to skip the imageon check is a code smell. These callers don't have a pdev and really only care about `separate_gpu_kms`. Consider either keeping a no-argument helper for that case, or documenting why the NULL-with-different-semantics pattern is intentional. As-is, a reader has to read the function body to understand that `NULL` means "just check the module param." --- --- Generated by Claude Code Patch Reviewer