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: Thu, 12 Mar 2026 07:03:37 +1000 Message-ID: In-Reply-To: <20260311101438.365997-4-tzimmermann@suse.de> References: <20260311101438.365997-1-tzimmermann@suse.de> <20260311101438.365997-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 Provides new standalone atomic helper functions (`drm_mipi_dbi_*` prefix) a= nd initializer macros (`DRM_MIPI_DBI_PLANE_FUNCS`, `DRM_MIPI_DBI_CRTC_FUNCS= `, etc.) for drivers to use with regular atomic modesetting. **Bug =E2=80=94 uninitialized variable:** In `drm_mipi_dbi_plane_helper_ato= mic_check()` (mbox line ~1106): ```c struct drm_crtc_state *new_crtc_state; if (new_plane_state->crtc) new_crtc_state =3D drm_atomic_get_new_crtc_state(state, new_plane_state= ->crtc); ret =3D drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state= , ...); ``` When `new_plane_state->crtc` is NULL (plane being disabled), `new_crtc_stat= e` is **used uninitialized**. While `drm_atomic_helper_check_plane_state()`= does handle `crtc_state =3D=3D NULL`, it will receive stack garbage rather= than NULL. This must be: ```c struct drm_crtc_state *new_crtc_state =3D NULL; ``` **Minor:** The tail of the same function is redundant: ```c if (ret) return ret; else if (!new_plane_state->visible) return 0; return 0; ``` This could simply be `return ret;`. The `else if` branch suggests future ch= ecks were intended but not yet added. **Minor:** Moving `drm_gem_atomic_helper.h`, `drm_gem_framebuffer_helper.h`= , `drm_probe_helper.h`, and `drm_atomic_state_helper.h` into `drm_mipi_dbi.= h` (to support the initializer macros) adds transitive header weight to all= includers. This is a common trade-off for macro-based initializers. ### PATCH 4 (04/16): drm/hx8357d: Use regular atomic helpers Standard mechanical conversion. Introduces `struct hx8357d_device` wrapping= `mipi_dbi_dev` with embedded plane/crtc/encoder/connector. Converts `yx240= qv29_enable` to `hx8357d_crtc_helper_atomic_enable`. Probe open-codes the p= ipeline setup using the new `DRM_MIPI_DBI_*` macros. No issues found. ### PATCH 5 (05/16): drm/ili9163: Use regular atomic helpers Identical structural conversion as hx8357d. No issues found. ### PATCH 6 (06/16): drm/ili9225: Use regular atomic helpers More complex conversion since ili9225 has custom update and disable callbac= ks. Uses `DRM_GEM_SHADOW_PLANE_HELPER_FUNCS` instead of `DRM_MIPI_DBI_PLANE= _HELPER_FUNCS` since it provides its own `atomic_update`. The old `!pipe->c= rtc.state->active` guard is correctly replaced with `!plane_state->fb` (sta= ndard pattern for regular atomic helpers where the framework manages CRTC-a= ctive gating). No issues found. ### PATCH 7 (07/16): drm/ili9341: Use regular atomic helpers Standard mechanical conversion, same pattern as hx8357d/ili9163. No issues found. ### PATCH 8 (08/16): drm/ili9486: Use regular atomic helpers Standard mechanical conversion. Minor improvement: renames old `waveshare_e= nable` to `ili9486_crtc_helper_atomic_enable` for consistency. No issues found. ### PATCH 9 (09/16): drm/mi0283qt: Use regular atomic helpers Standard mechanical conversion. No issues found. ### PATCH 10 (10/16): drm/panel-mipi-dbi: Use regular atomic helpers Slightly different from others because the pixel format is determined dynam= ically at probe time. Correctly passes the runtime `formats[]` array to `dr= m_universal_plane_init()` while using `formats[0]` (native format) for `drm= _mipi_dbi_dev_init()` buffer sizing. No issues found. ### PATCH 11 (11/16): drm/st7586: Use regular atomic helpers Complex conversion like ili9225 =E2=80=94 custom update, enable, and disabl= e. Uses `DRM_FORMAT_XRGB8888` only (this driver does internal 2bpp grayscal= e conversion). Sets `preferred_depth =3D 24` to match XRGB8888. **Minor style inconsistency:** `st7586_plane_helper_atomic_update` uses `if= (drm_dev_enter(...)) { ... }` while ili9225's equivalent uses the early-re= turn pattern. Both are correct but inconsistent. ### PATCH 12 (12/16): drm/st7735r: Rename struct st7735r_priv to struct st7= 735r_device Purely cosmetic rename to follow DRM naming conventions. Adds `to_st7735r_d= evice()` upcast helper. No issues found. ### PATCH 13 (13/16): drm/st7735r: Rename priv variable to st7735r Purely cosmetic rename of local variable `priv` =E2=86=92 `st7735r`. No issues found. ### PATCH 14 (14/16): drm/st7735r: Use regular atomic helpers Standard mechanical conversion for st7735r. Uses the `DRM_MIPI_DBI_*` macro= s. Only custom callback is `st7735r_crtc_helper_atomic_enable`. No issues found. ### PATCH 15 (15/16): drm/mipi-dbi: Remove simple-display helpers from mipi= -dbi Large deletion patch removing all now-unused simple-display-pipe wrapper fu= nctions (`mipi_dbi_pipe_update`, `mipi_dbi_pipe_disable`, `mipi_dbi_enable_= flush`, `mipi_dbi_dev_init_with_formats`, `mipi_dbi_dev_init`, etc.), the `= DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS` macro, and the `pipe`/`connector` f= ields from `struct mipi_dbi_dev`. No issues found. Clean removal of dead code. ### PATCH 16 (16/16): drm/simple-kms: Deprecate simple-kms helpers Strips all kerneldoc from `drm_simple_kms_helper.c`/`.h`, removes the "Simp= le KMS Helper Reference" from the GPU documentation, and adds TODO items fo= r converting remaining users. **Three typos:** - Commit message (line 5016): `"voluteers"` =E2=86=92 `"volunteers"` - TODO item (line 5105): `"fucntions"` =E2=86=92 `"functions"` - Header deprecation comment (lines 5306-5307): ``` Simple KMS helpers are deprected in favor of regular atomic helpers. Do n= ot use the min new code. ``` Should be: `"deprecated"` and `"Do not use them in new code."` --- Generated by Claude Code Patch Reviewer