Claude review: drm/tiny: Add DRM driver for Solomon SSD16xx e-paper display controllers

[Claude Code Review Bot <claude-review@example.com>]

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