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, 07 May 2026 13:55:15 +1000 Message-ID: In-Reply-To: <20260506-bar-v1-2-12195406f4ef@gmail.com> References: <20260506-bar-v1-0-12195406f4ef@gmail.com> <20260506-bar-v1-2-12195406f4ef@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 This is the main driver patch. Several issues: **Critical: Kconfig/driver GEM helper mismatch** The Kconfig entry selects `DRM_GEM_DMA_HELPER`: ```c +config DRM_PIXPAPER_426M + select DRM_GEM_DMA_HELPER ``` But the driver code uses SHMEM-based GEM helpers: ```c +#include +#include ... + DRM_GEM_SHMEM_DRIVER_OPS, + DRM_FBDEV_SHMEM_DRIVER_OPS, ``` The existing `pixpaper.c` driver correctly selects `DRM_GEM_SHMEM_HELPER`. = This should be `select DRM_GEM_SHMEM_HELPER` (and needs `depends on MMU` to= match the existing driver, since SHMEM requires MMU). **Empty CRTC enable/disable callbacks** ```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); +} ``` Both enable and disable do nothing beyond `drm_dev_enter`/`drm_dev_exit`. T= he existing pixpaper.c sends `POWER_ON` in enable and `POWER_OFF`/`DEEP_SLE= EP` in disable. For an e-ink panel, at minimum a power-on/reset should happ= en in enable. If the panel truly requires no power sequencing, these callba= cks could simply be removed (set to NULL), but that seems unlikely for an e= -ink panel. As-is, the panel is initialized in `probe` and never power-cycl= ed, which means suspend/resume or DPMS off/on won't work correctly. **Missing `.shutdown` callback** ```c +static struct spi_driver pixpaper_spi_driver =3D { + .driver =3D { + .name =3D "pixpaper-426m", + .of_match_table =3D pixpaper_dt_ids, + }, + .id_table =3D pixpaper_ids, + .probe =3D pixpaper_probe, + .remove =3D pixpaper_remove, +}; ``` Other tiny SPI drivers (repaper.c, sharp-memory.c, the existing pixpaper.c)= provide a `.shutdown` callback that calls `drm_atomic_helper_shutdown()`. = Without it, a system reboot may not cleanly shut down the display. Add: ```c static void pixpaper_shutdown(struct spi_device *spi) { drm_atomic_helper_shutdown(&spi_get_drvdata(spi)->drm); } ``` **Indentation error in `pixpaper_connector_get_modes()`** Starting after the `drm_mode_validate_size` check, the indentation shifts b= y one extra tab level mid-function: ```c + drm_mode_destroy(connector->dev, mode); + return 0; + } // =E2=86=90 wrong indent: closing brace at 2= tabs + + drm_mode_probed_add(connector, mode); // =E2=86=90 2 tabs, should be 1 + + connector->display_info.width_mm =3D PIXPAPER_WIDTH_MM; // =E2=86=90 2= tabs + connector->display_info.height_mm =3D PIXPAPER_HEIGHT_MM; // =E2=86=90 2= tabs + + return 1; ``` The closing brace of the `if` block and the subsequent statements are inden= ted at the wrong level. This code is syntactically valid but misleading =E2= =80=94 the `drm_mode_probed_add` and `display_info` assignments are inside = the wrong visual scope. The closing `}` should be at 1-tab level, and the r= emaining statements at 1-tab level too. **Unnecessary forward declaration** ```c +static void pixpaper_xrgb8888_to_bw(const void *src, void *dst, u32 height, + u32 width, u32 src_pitch, u32 dst_pitch); ``` The function is defined later at line ~393 in the new file. Simply move the= definition before `pixpaper_prepare_buffer()` to avoid the forward declara= tion. **Unusual bare block scope in `pixpaper_plane_atomic_update()`** ```c +static void pixpaper_plane_atomic_update(struct drm_plane *plane, + struct drm_atomic_state *state) +{ + ... + if (!plane_state->crtc || !plane_state->fb || !plane_state->visible) + return; + + { + struct drm_device *drm =3D &panel->drm; + ... ``` This bare `{ }` block to introduce new variable declarations is unusual in = kernel code and makes the function harder to read. The variables should sim= ply be declared at the top of the function, or the inner block should be ex= tracted into a helper function. **`pixpaper_xrgb8888_to_bw()` uses `uint8_t`/`uint32_t` instead of kernel-s= tyle `u8`/`u32`** ```c + const uint8_t *src_base =3D src; + uint8_t *dst_pixels =3D dst; + ... + uint8_t r, g, b; + uint32_t gray_val; + uint32_t pixel; ``` Kernel code prefers `u8`, `u16`, `u32`, `u64` over the `uint*_t` types. The= rest of the driver uses the kernel types correctly; this function should b= e consistent. **`pixpaper_xrgb8888_to_bw()` has redundant NULL checks** ```c + if (dst =3D=3D NULL || src =3D=3D NULL) + return; ``` The caller `pixpaper_prepare_buffer()` already ensures `vaddr` is valid and= `dst` was just allocated with `kzalloc()` (returning NULL is handled by th= e caller). These NULL checks are unnecessary. **`pixpaper_mode_config_valid()` signature mismatch** ```c +static int pixpaper_mode_config_valid(struct drm_device *dev, + const struct drm_display_mode *mode) +{ + return pixpaper_mode_valid(mode); +} ``` The `.mode_valid` callback in `drm_mode_config_funcs` returns `enum drm_mod= e_status`, not `int`. While these are compatible in practice, the return ty= pe should be `enum drm_mode_status` for correctness and to match the existi= ng `pixpaper_mode_valid()` function return type. **`pixpaper_drm_driver` should be `const`** ```c +static struct drm_driver pixpaper_drm_driver =3D { ``` This should be `static const struct drm_driver` as it's never modified. The= existing pixpaper.c has this same issue, but new code should get it right. **`MODULE_IMPORT_NS("DMA_BUF")` present but no DMA buffer operations** ```c +MODULE_IMPORT_NS("DMA_BUF"); ``` The driver uses SHMEM helpers, not DMA. Check whether this namespace import= is actually needed =E2=80=94 it may pull in if required by SHMEM helpers, = but it's worth verifying this isn't a leftover from a DMA-based iteration. **Minor: `pixpaper_init_ram_x_window` sends 4 bytes but command 0x44 typica= lly takes 2** ```c +static const u8 pixpaper_init_ram_x_window[] =3D { + PIXPAPER_RAM_START_ADDR, + PIXPAPER_RAM_START_ADDR, + (PIXPAPER_WIDTH - 1) & 0xff, + (PIXPAPER_WIDTH - 1) >> 8, +}; ``` For SSD1681/SSD16xx-family controllers, command 0x44 (Set RAM X Start/End) = typically takes 2 bytes (start and end X address in byte units, i.e., divid= ed by 8). If this is a different controller, this might be correct, but 4 b= ytes with the raw pixel width looks suspicious. Similarly, the X RAM counte= r (0x4E) gets 2 bytes where typically 1 is expected. Verify against the act= ual panel controller datasheet. **`pixpaper_remove()` =E2=80=94 unnecessary NULL check** ```c +static void pixpaper_remove(struct spi_device *spi) +{ + struct pixpaper_panel *panel =3D spi_get_drvdata(spi); + + if (!panel) + return; ``` The `spi_get_drvdata()` cannot return NULL after a successful probe that ca= lled `spi_set_drvdata()`. This check is unnecessary. **Reset polarity concern**: The probe does `devm_gpiod_get(dev, "reset", GP= IOD_OUT_HIGH)` meaning the reset line starts asserted. Then `pixpaper_panel= _hw_init()` does: ```c + gpiod_set_value_cansleep(panel->reset, 0); // de-assert + msleep(50); + gpiod_set_value_cansleep(panel->reset, 1); // assert + msleep(50); ``` This asserts reset again after the init sequence, which seems backwards. Ty= pically you'd want to pulse reset low (assert), then bring it high (de-asse= rt) to release the panel from reset. This depends on whether `GPIOD_OUT_HIG= H` means "active" or "logic high" =E2=80=94 with `GPIO_ACTIVE_HIGH` in the = DT binding, `gpiod_set_value(1)` means assert-active which is logic high. M= ake sure the panel's reset is active-low at the hardware level and that the= DT `GPIO_ACTIVE_HIGH` flag is correct; if reset is active-low, the DT shou= ld use `GPIO_ACTIVE_LOW` and the code should set value 1 to assert, 0 to de= -assert. --- Generated by Claude Code Patch Reviewer