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/mipi-dbi: Provide callbacks for atomic interfaces Date: Sun, 22 Mar 2026 04:15:33 +1000 Message-ID: In-Reply-To: <20260319160110.109610-4-tzimmermann@suse.de> References: <20260319160110.109610-1-tzimmermann@suse.de> <20260319160110.109610-4-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the core infrastructure patch. The new helper functions and macros = are well-designed. Minor issues: 1. **Documentation typos** in `drm_mipi_dbi_connector_helper_get_modes()`: - `"Sets the connecto rmodes"` =E2=86=92 should be `"Sets the connector = modes"` - `"get_mods callback"` =E2=86=92 should be `"get_modes callback"` - The cross-reference to `drm_gem_destroy_shadow_plane_state()` seems ir= relevant to a connector get_modes function =E2=80=94 this looks like a copy= -paste error. 2. **Redundant return path** in `drm_mipi_dbi_plane_helper_atomic_check()`: ```c if (ret) return ret; else if (!new_plane_state->visible) return 0; =20 return 0; ``` The `else if` branch and the final `return 0` are equivalent. This could= simply be `return ret;` after the check call, but it's harmless and may be= intentional to leave a hook point for future additions. 3. **Documentation typo** in `drm_mipi_dbi_crtc_helper_atomic_check()`: `"ensures that the primary plane as been set up"` =E2=86=92 should be `"= has been set up"`. 4. The `drm_mipi_dbi_plane_helper_atomic_update()` function drops the `WARN= _ON(!fb)` that existed in the original `mipi_dbi_pipe_update()`. The new co= de silently returns on `!fb`, which is fine since the atomic framework guar= antees `fb` is set when the plane is visible, but worth noting. 5. The `mipi_dbi_pipe_disable()` wrapper now calls `drm_mipi_dbi_crtc_helpe= r_atomic_disable(&pipe->crtc, pipe->crtc.state->state)` =E2=80=94 passing `= pipe->crtc.state->state` is correct here since the simple-pipe disable call= back is called from within the atomic commit, so state is valid. However, t= he new `drm_mipi_dbi_crtc_helper_atomic_disable()` doesn't actually use the= `state` parameter, so this is fine regardless. --- Generated by Claude Code Patch Reviewer