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 DRM driver for Solomon SSD16xx e-paper display controllers Date: Tue, 05 May 2026 10:10:10 +1000 Message-ID: In-Reply-To: <20260430183311.2978142-4-devarsht@ti.com> References: <20260430183311.2978142-1-devarsht@ti.com> <20260430183311.2978142-4-devarsht@ti.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is the bulk of the series at ~2550 lines. Detailed review: **RGB888 byte order bug:** ```c + case DRM_FORMAT_RGB888: { + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]); + u8 r = line[x * 3], g = line[x * 3 + 1], b = line[x * 3 + 2]; ``` `DRM_FORMAT_RGB888` is defined as `[23:0] R:G:B little endian`. In little-endian memory, byte 0 is B, byte 1 is G, byte 2 is R. The code has R and B swapped. This affects both `ssd16xx_pixel_luma()` and `ssd16xx_pixel_is_red()`. The luma calculation uses different BT.601 weights for R (299) vs B (114), so grayscale thresholding would be wrong for colourful content. For `ssd16xx_pixel_is_red()`, blue-dominant pixels would be classified as red and vice-versa. **DMA-unsafe stack buffers in SPI transfers:** ```c +static void ssd16xx_send_cmd(struct ssd16xx_panel *panel, u8 cmd, + int *err) +{ + ... + if (panel->dc) { + xfer.tx_buf = &cmd; ``` `cmd` and `data` are stack variables used as SPI tx_buf. While the SPI core handles bounce-buffering for small transfers in most cases, this is fragile and not guaranteed for all SPI controller drivers. Best practice is to use a DMA-safe buffer (e.g., a field in the panel struct, or `kmalloc`'d). Similarly in `ssd16xx_send_data()`. **Properties not stored in connector state:** All custom properties (`rotation`, `refresh_mode`, `color_mode`, etc.) are stored directly on `struct ssd16xx_panel` and modified immediately in `atomic_set_property`: ```c + if (property == panel->rotation_property) { + ... + panel->orientation = val; + panel->reinit_pending = true; + return 0; + } ``` This is not atomic -- if the commit fails validation later, the property has already been applied. The correct pattern is to store pending values in a driver-specific subclass of `drm_connector_state` and apply them in the atomic commit. **Hardcoded array size in validation:** ```c + if (property == panel->border_waveform_init_property) { + if (val >= ARRAY_SIZE(ssd1683_border_waveform_table)) + return -EINVAL; ``` Uses `ssd1683_border_waveform_table` directly instead of `panel->controller_cfg->border_waveform_table`. If another controller is added with a different table size, this validation would be wrong. **`dma_coerce_mask_and_coherent` with 64-bit mask:** ```c + if (!dev->coherent_dma_mask) { + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); ``` `dma_coerce_mask_and_coherent` overrides platform DMA config. For an SPI device, the DMA mask should be inherited from the parent controller. If you must set one, `DMA_BIT_MASK(32)` is more appropriate. But really this should be handled by the SPI subsystem. **Missing locking:** Properties are read and written from multiple code paths (atomic commit, `master_set`, `master_drop`, PM callbacks) without any synchronization beyond the DRM modeset lock. Fields like `init_refresh_pending`, `first_clear_done`, `display_cleared_on_deinit`, `fast_lut_pending`, and `border_waveform_pending` are modified outside the atomic commit path (in `master_set`/`master_drop`) and read inside atomic commit, which is a potential race. **`ssd16xx_plane_destroy` and `ssd16xx_plane_reset` are trivial wrappers:** ```c +static void ssd16xx_plane_destroy(struct drm_plane *plane) +{ + drm_plane_cleanup(plane); +} ``` Just pass `drm_plane_cleanup` and `drm_atomic_helper_plane_reset` directly in the vtable. **Excessive pixel format list:** ```c +static const u32 ssd16xx_formats[] = { + DRM_FORMAT_XRGB8888, + DRM_FORMAT_RGB888, + DRM_FORMAT_RGB565, + DRM_FORMAT_R8, + DRM_FORMAT_NV12, + DRM_FORMAT_NV16, + DRM_FORMAT_YUYV, + DRM_FORMAT_UYVY, + DRM_FORMAT_R1, +}; ``` Supporting 9 pixel formats with hand-written per-pixel conversion loops is unusual for a tiny driver. Consider using `drm_format_helper` infrastructure to convert to a canonical format (e.g., XRGB8888 to R1) and let userspace handle the rest. This would dramatically reduce code and bug surface. **Per-pixel conversion performance:** The conversion loops call `ssd16xx_pixel_is_white()` (and potentially `ssd16xx_pixel_luma()`) per-pixel with a function call per pixel. For a 400x300 display this is 120K function calls per frame. While acceptable for e-paper refresh rates, a batch approach would be cleaner. --- --- Generated by Claude Code Patch Reviewer