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/vblank: Add drm_crtc_vblank_is_off() helper Date: Fri, 27 Feb 2026 14:40:52 +1000 Message-ID: In-Reply-To: <20260224212639.390768-2-sunpeng.li@amd.com> References: <20260224212639.390768-1-sunpeng.li@amd.com> <20260224212639.390768-2-sunpeng.li@amd.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 **Generally clean.** The refactoring correctly replaces the `drm_crtc_vblan= k_get()` + check return value pattern with a direct state query, removing t= he need for a balancing `drm_crtc_vblank_put()`. The condition in `drm_crtc_vblank_is_off()`: ```c guard(spinlock_irqsave)(&crtc->dev->vbl_lock); return vblank->inmodeset && !vblank->enabled; ``` correctly captures the "vblank_off was called" state =E2=80=94 `inmodeset` = is set in `drm_crtc_vblank_off()` when it bumps the refcount, and `enabled`= is cleared by `drm_vblank_disable_and_save()`. This is semantically what t= he original code was checking. **Minor issues:** 1. The function is declared in `drm_internal.h` but uses `EXPORT_SYMBOL`. T= his is needed because `drm_atomic_helper.c` compiles into a separate module= (`drm_kms_helper`), but it's a slightly awkward pattern =E2=80=94 an expor= ted symbol with no public header declaration. Consider whether a public hea= der (e.g., `drm_vblank.h`) would be more appropriate if other drivers/helpe= rs might eventually need this. 2. The kdoc says "Return False otherwise" with a capital 'F' =E2=80=94 shou= ld be lowercase "false" for consistency. --- Generated by Claude Code Patch Reviewer