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/tiny: add support for PIXPAPER 4.26 monochrome e-ink panel Date: Thu, 04 Jun 2026 16:48:06 +1000 Message-ID: In-Reply-To: <20260529-bar-v3-2-5c2ac1c751ee@gmail.com> References: <20260529-bar-v3-0-5c2ac1c751ee@gmail.com> <20260529-bar-v3-2-5c2ac1c751ee@gmail.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 **Issues found:** **1. `iosys_map` usage ignores `is_iomem` (moderate)** ```c struct iosys_map map =3D shadow_plane_state->data[0]; const void *vaddr =3D map.vaddr; ``` This directly accesses `map.vaddr` without checking `map.is_iomem`. If the = mapping happens to be I/O memory, `.vaddr` is the wrong union member and th= e subsequent direct pointer dereference in `pixpaper_xrgb8888_to_bw()` woul= d be incorrect. While with `DRM_GEM_SHMEM_HELPER` this is unlikely to produ= ce iomem mappings in practice, the existing pixpaper.c has the same pattern= so this may be an accepted shortcut for tiny drivers. Still, using `iosys_= map_memcpy_from()` or at least an assertion would be more robust. Note: the existing pixpaper.c driver has this same issue, so this is not a = regression, but it would be good practice to address. **2. Unnecessary inner block scope in `pixpaper_plane_atomic_update()` (min= or/style)** ```c if (!plane_state->crtc || !plane_state->fb || !plane_state->visible) return; { struct drm_device *drm =3D &panel->drm; struct drm_framebuffer *fb =3D plane_state->fb; ... ``` The naked `{ ... }` block creates an extra indentation level for no apparen= t reason. The early-return guard above already ensures the rest of the func= tion only executes when valid. Just declaring variables at the function sco= pe (or using C99 mixed declarations) would be cleaner. The existing pixpape= r.c doesn't have this extra block. **3. `pixpaper_crtc_atomic_enable/disable` are empty (minor)** ```c static void pixpaper_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { struct pixpaper_panel *panel =3D to_pixpaper_panel(crtc->dev); struct drm_device *drm =3D &panel->drm; int idx; if (!drm_dev_enter(drm, &idx)) return; drm_dev_exit(idx); } ``` These callbacks acquire and immediately release the `drm_dev` reference wit= hout doing anything between. Either add a `TODO` comment explaining what sh= ould eventually go here (e.g., panel power sequencing), or just leave the c= allbacks as `NULL` if the framework permits. The existing pixpaper.c has ac= tual work in `crtc_atomic_enable` (it calls `pixpaper_panel_hw_init`). For = an e-ink panel, `atomic_enable` seems like the right place to re-init the h= ardware =E2=80=94 consider moving `pixpaper_panel_hw_init()` here instead o= f probe-only. **4. Reset GPIO polarity (minor)** ```c gpiod_set_value_cansleep(panel->reset, 0); msleep(50); gpiod_set_value_cansleep(panel->reset, 1); msleep(50); ``` With `GPIOD_OUT_HIGH` at probe time, then setting to 0/1 =E2=80=94 this ass= erts reset (active-low =3D 0), then deasserts. The DT binding should define= the GPIO as `GPIO_ACTIVE_LOW` if the hardware reset is active-low. If the = binding already does this, then `gpiod_set_value` handles the inversion and= this is correct. But if the panel's reset pin is active-low and the DT doe= sn't flag it, the values here are inverted from what you'd expect. Worth co= nfirming the DT example matches. **5. `pixpaper_drm_driver` should be `const` (minor)** ```c static struct drm_driver pixpaper_drm_driver =3D { ``` This can be declared `static const struct drm_driver` =E2=80=94 the driver = structure is never modified after initialization. Other recent tiny drivers= use `const` here. **6. Framebuffer format check is redundant (very minor)** ```c if (fb->format->format !=3D DRM_FORMAT_XRGB8888) { err.errno_code =3D -EINVAL; drm_err_once(drm, "Unsupported framebuffer format: 0x%08x\n", fb->format->format); goto update_cleanup; } ``` The driver only advertises `DRM_FORMAT_XRGB8888` in `pixpaper_formats[]`, s= o the DRM core will never allow a framebuffer with a different format to re= ach this point. This check is defensive but dead code. Not harmful, but rem= oving it would reduce unnecessary complexity. **7. `NULL` check on `panel` in `pixpaper_remove()` is unnecessary (very mi= nor)** ```c static void pixpaper_remove(struct spi_device *spi) { struct pixpaper_panel *panel =3D spi_get_drvdata(spi); if (!panel) return; ``` The `probe()` function always sets drvdata via `spi_set_drvdata()` (from `d= evm_drm_dev_alloc`), and if probe failed, `remove()` wouldn't be called. Th= e existing pixpaper.c has this same pattern though, so it's an inherited st= yle. **8. MAINTAINERS: file list ordering (nit)** ``` +F: drivers/gpu/drm/tiny/pixpaper-426m.c F: drivers/gpu/drm/tiny/pixpaper.c ``` This is alphabetically correct =E2=80=94 no issue. **Summary for Patch 2:** The driver is well-written and follows existing pa= tterns closely. The main substantive items are: (1) consider making `drm_dr= iver` const, (2) the unnecessary inner block scope in `atomic_update` is od= d, and (3) the empty `crtc_atomic_enable/disable` should either do somethin= g useful (like re-init the panel) or be documented as intentionally empty. = None of these are blockers, but addressing them would improve the code. --- Generated by Claude Code Patch Reviewer