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:

**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 due to implicit conversion, it's wrong and may trigger compiler warnings. The existing `pixpaper.c` doesn't have a `mode_config_funcs.mode_valid` at all — it only has the `crtc_helper_funcs.mode_valid`. Consider whether this extra layer is actually needed; if the CRTC already validates the mode, 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) != MODE_OK) {
		...
		drm_mode_destroy(connector->dev, mode);
		return 0;
		}

		drm_mode_probed_add(connector, mode);

		connector->display_info.width_mm  = PIXPAPER_WIDTH_MM;
		connector->display_info.height_mm = 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 outside. This is a style issue that will confuse readers and likely trip checkpatch.

**Issue 3 (Design): Massive code duplication with `pixpaper.c`**

The new driver duplicates substantial infrastructure from the existing `pixpaper.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 = 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 sequence 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 deasserted (high), then briefly asserts it (low→high), which is a valid reset pulse — but `GPIOD_OUT_HIGH` at probe time means the GPIO is immediately driven high before the reset sequence runs, potentially leaving the panel in an undefined state if it was already powered up. The existing `pixpaper.c` does the same thing, so this is inherited behavior, but it's worth 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 = 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 `atomic_enable` and `atomic_disable` acquire and immediately release `drm_dev_enter/exit` without doing anything in between. This is dead code. The existing `pixpaper.c` actually sends power-on/power-off commands in these callbacks. Either implement proper panel power management here (send appropriate commands for this panel's power state), or remove these callbacks entirely 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 = &panel->drm;
		struct drm_framebuffer *fb = plane_state->fb;
		...
	}
}
```

The inner `{ }` block serves no purpose — the variables declared inside could be moved to function scope. This appears to be a leftover from development. 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 the file. Reorder the function definitions to eliminate the need for the forward declaration — kernel style generally prefers defining functions before use.

**Issue 8 (Minor): `pixpaper_xrgb8888_to_bw` uses `uint8_t`/`uint32_t` mixed with `u8`/`u32`**

```c
	const uint8_t *src_base = src;
	uint8_t *dst_pixels = dst;
	...
	uint8_t r, g, b;
	u8 bit;
	u32 bit_pos = x % 8;
	...
	uint32_t gray_val;
	uint32_t pixel;
```

Kernel code should consistently use `u8`, `u16`, `u32`, `u64` rather than mixing with `uint8_t`/`uint32_t`. The existing driver also has this issue but 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 = &dev->coherent_dma_mask;
ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
```

The driver selects `DRM_GEM_SHMEM_HELPER` which doesn't require DMA. This DMA 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 = {
```

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 it right.

---
Generated by Claude Code Patch Reviewer