Claude review: drm/tiny: add support for PIXPAPER 4.26 monochrome e-ink panel

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

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 <drm/drm_fbdev_shmem.h>
+#include <drm/drm_gem_shmem_helper.h>
...
+	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 = to_pixpaper_panel(crtc->dev);
+	struct drm_device *drm = &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`. The existing pixpaper.c sends `POWER_ON` in enable and `POWER_OFF`/`DEEP_SLEEP` in disable. For an e-ink panel, at minimum a power-on/reset should happen in enable. If the panel truly requires no power sequencing, these callbacks 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-cycled, which means suspend/resume or DPMS off/on won't work correctly.

**Missing `.shutdown` callback**

```c
+static struct spi_driver pixpaper_spi_driver = {
+	.driver = {
+		.name = "pixpaper-426m",
+		.of_match_table = pixpaper_dt_ids,
+	},
+	.id_table = pixpaper_ids,
+	.probe = pixpaper_probe,
+	.remove = 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 by one extra tab level mid-function:

```c
+		drm_mode_destroy(connector->dev, mode);
+		return 0;
+		}                          // ← wrong indent: closing brace at 2 tabs
+
+		drm_mode_probed_add(connector, mode);   // ← 2 tabs, should be 1
+
+		connector->display_info.width_mm  = PIXPAPER_WIDTH_MM;  // ← 2 tabs
+		connector->display_info.height_mm = PIXPAPER_HEIGHT_MM; // ← 2 tabs
+
+	return 1;
```

The closing brace of the `if` block and the subsequent statements are indented at the wrong level. This code is syntactically valid but misleading — 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 remaining 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 declaration.

**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 = &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 simply be declared at the top of the function, or the inner block should be extracted into a helper function.

**`pixpaper_xrgb8888_to_bw()` uses `uint8_t`/`uint32_t` instead of kernel-style `u8`/`u32`**

```c
+	const uint8_t *src_base = src;
+	uint8_t *dst_pixels = 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 be consistent.

**`pixpaper_xrgb8888_to_bw()` has redundant NULL checks**

```c
+	if (dst == NULL || src == NULL)
+		return;
```

The caller `pixpaper_prepare_buffer()` already ensures `vaddr` is valid and `dst` was just allocated with `kzalloc()` (returning NULL is handled by the 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_mode_status`, not `int`. While these are compatible in practice, the return type should be `enum drm_mode_status` for correctness and to match the existing `pixpaper_mode_valid()` function return type.

**`pixpaper_drm_driver` should be `const`**

```c
+static struct drm_driver pixpaper_drm_driver = {
```

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 — 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 typically takes 2**

```c
+static const u8 pixpaper_init_ram_x_window[] = {
+	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., divided by 8). If this is a different controller, this might be correct, but 4 bytes with the raw pixel width looks suspicious. Similarly, the X RAM counter (0x4E) gets 2 bytes where typically 1 is expected. Verify against the actual panel controller datasheet.

**`pixpaper_remove()` — unnecessary NULL check**

```c
+static void pixpaper_remove(struct spi_device *spi)
+{
+	struct pixpaper_panel *panel = spi_get_drvdata(spi);
+
+	if (!panel)
+		return;
```

The `spi_get_drvdata()` cannot return NULL after a successful probe that called `spi_set_drvdata()`. This check is unnecessary.

**Reset polarity concern**: The probe does `devm_gpiod_get(dev, "reset", GPIOD_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. Typically you'd want to pulse reset low (assert), then bring it high (de-assert) to release the panel from reset. This depends on whether `GPIOD_OUT_HIGH` means "active" or "logic high" — with `GPIO_ACTIVE_HIGH` in the DT binding, `gpiod_set_value(1)` means assert-active which is logic high. Make 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 should use `GPIO_ACTIVE_LOW` and the code should set value 1 to assert, 0 to de-assert.

---
Generated by Claude Code Patch Reviewer