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: Wed, 27 May 2026 15:04:41 +1000 Message-ID: In-Reply-To: <20260526-bar-v2-2-c66df9a840c4@gmail.com> References: <20260526-bar-v2-0-c66df9a840c4@gmail.com> <20260526-bar-v2-2-c66df9a840c4@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: **Issue 1 (Bug): `pixpaper_mode_config_valid` return type mismatch** ```c static int pixpaper_mode_config_valid(struct drm_device *dev, const struct drm_display_mode *mode) { return pixpaper_mode_valid(mode); } ``` The `drm_mode_config_funcs.mode_valid` callback must return `enum drm_mode_= status`, but this function is declared as returning `int`. While it works d= ue to implicit conversion, it's wrong and may trigger compiler warnings. Th= e existing `pixpaper.c` doesn't have a `mode_config_funcs.mode_valid` at al= l =E2=80=94 it only has the `crtc_helper_funcs.mode_valid`. Consider whethe= r this extra layer is actually needed; if the CRTC already validates the mo= de, the mode_config-level callback is redundant. **Issue 2 (Bug): Indentation error in `pixpaper_connector_get_modes`** ```c if (drm_mode_validate_size(mode, connector->dev->mode_config.max_width, connector->dev->mode_config.max_height) !=3D MODE_OK) { ... drm_mode_destroy(connector->dev, mode); return 0; } drm_mode_probed_add(connector, mode); connector->display_info.width_mm =3D PIXPAPER_WIDTH_MM; connector->display_info.height_mm =3D PIXPAPER_HEIGHT_MM; return 1; ``` The closing brace of the `if` block and the three lines after it (`drm_mode= _probed_add`, width_mm, height_mm assignments) are indented with extra tabs= , making the code look like it's inside the `if` block when it's actually o= utside. This is a style issue that will confuse readers and likely trip che= ckpatch. **Issue 3 (Design): Massive code duplication with `pixpaper.c`** The new driver duplicates substantial infrastructure from the existing `pix= paper.c`: - `struct pixpaper_error_ctx` and the error-propagation pattern - `pixpaper_wait_busy` / busy polling logic - `pixpaper_spi_write`, `pixpaper_send_cmd`, `pixpaper_send_data` - The entire DRM pipeline setup (plane/crtc/encoder/connector init) - `probe()` / `remove()` flows are nearly identical - `pixpaper_connector_get_modes()` pattern Given that both panels share the same SPI + GPIO interface (reset, busy, dc= ) and the same DRM pipeline structure, these should be a single driver with= panel-specific data (resolution, init sequences, pixel format conversion) = selected by compatible string. This is the standard approach in `drm/tiny` = drivers that support multiple panel variants (e.g., see how `ili9341.c` or = the mipi-dbi panels handle it). **Issue 4 (Questionable): Reset polarity** ```c panel->reset =3D devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); ... gpiod_set_value_cansleep(panel->reset, 0); msleep(50); gpiod_set_value_cansleep(panel->reset, 1); msleep(50); ``` With `GPIOD_OUT_HIGH`, the GPIO starts asserted (high). Then the init seque= nce sets it to 0, waits, sets it to 1. For a typical active-low reset, you'= d want `GPIOD_OUT_LOW` (assert reset on probe), then deassert (set to 1) to= bring the panel out of reset. The current code starts with reset deasserte= d (high), then briefly asserts it (low=E2=86=92high), which is a valid rese= t pulse =E2=80=94 but `GPIOD_OUT_HIGH` at probe time means the GPIO is imme= diately driven high before the reset sequence runs, potentially leaving the= panel in an undefined state if it was already powered up. The existing `pi= xpaper.c` does the same thing, so this is inherited behavior, but it's wort= h noting. **Issue 5 (Cleanup): 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 `atomic_enable` and `atomic_disable` acquire and immediately release `= drm_dev_enter/exit` without doing anything in between. This is dead code. T= he existing `pixpaper.c` actually sends power-on/power-off commands in thes= e callbacks. Either implement proper panel power management here (send appr= opriate commands for this panel's power state), or remove these callbacks e= ntirely and leave them NULL in the helper funcs. **Issue 6 (Style): Unnecessary inner brace 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; struct drm_framebuffer *fb =3D plane_state->fb; ... } } ``` The inner `{ }` block serves no purpose =E2=80=94 the variables declared in= side could be moved to function scope. This appears to be a leftover from d= evelopment. Just declare the variables at the top of the function. **Issue 7 (Minor): Forward declaration of `pixpaper_xrgb8888_to_bw`** ```c static void pixpaper_xrgb8888_to_bw(const void *src, void *dst, u32 height, u32 width, u32 src_pitch, u32 dst_pitch); ``` This forward declaration exists because the function is defined later in th= e file. Reorder the function definitions to eliminate the need for the forw= ard declaration =E2=80=94 kernel style generally prefers defining functions= before use. **Issue 8 (Minor): `pixpaper_xrgb8888_to_bw` uses `uint8_t`/`uint32_t` mixe= d with `u8`/`u32`** ```c const uint8_t *src_base =3D src; uint8_t *dst_pixels =3D dst; ... uint8_t r, g, b; u8 bit; u32 bit_pos =3D x % 8; ... uint32_t gray_val; uint32_t pixel; ``` Kernel code should consistently use `u8`, `u16`, `u32`, `u64` rather than m= ixing with `uint8_t`/`uint32_t`. The existing driver also has this issue bu= t new code should be clean. **Issue 9 (Minor): DMA mask setup may be unnecessary with SHMEM helper** ```c if (!dev->dma_mask) dev->dma_mask =3D &dev->coherent_dma_mask; ret =3D dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); ``` The driver selects `DRM_GEM_SHMEM_HELPER` which doesn't require DMA. This D= MA mask setup was inherited from the existing driver but may be unnecessary= for a pure-SPI driver using shmem GEM objects. SPI transfers are done via = `spi_write()` which handles its own DMA if needed. **Issue 10 (Minor): `drm_drv.h` should be `const` for `pixpaper_drm_driver`= ** ```c static struct drm_driver pixpaper_drm_driver =3D { ``` This should be `static const struct drm_driver` since it's never modified. = The existing `pixpaper.c` has this same issue, but new drivers should get i= t right. --- Generated by Claude Code Patch Reviewer