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

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

Patch Review

**Issues found:**

**1. `iosys_map` usage ignores `is_iomem` (moderate)**

```c
struct iosys_map map = shadow_plane_state->data[0];
const void *vaddr = map.vaddr;
```

This directly accesses `map.vaddr` without checking `map.is_iomem`. If the mapping happens to be I/O memory, `.vaddr` is the wrong union member and the subsequent direct pointer dereference in `pixpaper_xrgb8888_to_bw()` would be incorrect. While with `DRM_GEM_SHMEM_HELPER` this is unlikely to produce iomem mappings in practice, the existing pixpaper.c has the same pattern so this may be an accepted shortcut for tiny drivers. Still, using `iosys_map_memcpy_from()` or at least an assertion would be more robust.

Note: the existing pixpaper.c driver has this same issue, so this is not a regression, but it would be good practice to address.

**2. Unnecessary inner block scope in `pixpaper_plane_atomic_update()` (minor/style)**

```c
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 naked `{ ... }` block creates an extra indentation level for no apparent reason. The early-return guard above already ensures the rest of the function only executes when valid. Just declaring variables at the function scope (or using C99 mixed declarations) would be cleaner. The existing pixpaper.c doesn't have this extra block.

**3. `pixpaper_crtc_atomic_enable/disable` are empty (minor)**

```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);
}
```

These callbacks acquire and immediately release the `drm_dev` reference without doing anything between. Either add a `TODO` comment explaining what should eventually go here (e.g., panel power sequencing), or just leave the callbacks as `NULL` if the framework permits. The existing pixpaper.c has actual work in `crtc_atomic_enable` (it calls `pixpaper_panel_hw_init`). For an e-ink panel, `atomic_enable` seems like the right place to re-init the hardware — consider moving `pixpaper_panel_hw_init()` here instead of probe-only.

**4. Reset GPIO polarity (minor)**

```c
gpiod_set_value_cansleep(panel->reset, 0);
msleep(50);
gpiod_set_value_cansleep(panel->reset, 1);
msleep(50);
```

With `GPIOD_OUT_HIGH` at probe time, then setting to 0/1 — this asserts reset (active-low = 0), then deasserts. The DT binding should define the GPIO as `GPIO_ACTIVE_LOW` if the hardware reset is active-low. If the binding already does this, then `gpiod_set_value` handles the inversion and this is correct. But if the panel's reset pin is active-low and the DT doesn't flag it, the values here are inverted from what you'd expect. Worth confirming the DT example matches.

**5. `pixpaper_drm_driver` should be `const` (minor)**

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

This can be declared `static const struct drm_driver` — the driver structure is never modified after initialization. Other recent tiny drivers use `const` here.

**6. Framebuffer format check is redundant (very minor)**

```c
if (fb->format->format != DRM_FORMAT_XRGB8888) {
    err.errno_code = -EINVAL;
    drm_err_once(drm, "Unsupported framebuffer format: 0x%08x\n",
                fb->format->format);
    goto update_cleanup;
}
```

The driver only advertises `DRM_FORMAT_XRGB8888` in `pixpaper_formats[]`, so the DRM core will never allow a framebuffer with a different format to reach this point. This check is defensive but dead code. Not harmful, but removing it would reduce unnecessary complexity.

**7. `NULL` check on `panel` in `pixpaper_remove()` is unnecessary (very minor)**

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

    if (!panel)
        return;
```

The `probe()` function always sets drvdata via `spi_set_drvdata()` (from `devm_drm_dev_alloc`), and if probe failed, `remove()` wouldn't be called. The existing pixpaper.c has this same pattern though, so it's an inherited style.

**8. MAINTAINERS: file list ordering (nit)**

```
+F:	drivers/gpu/drm/tiny/pixpaper-426m.c
 F:	drivers/gpu/drm/tiny/pixpaper.c
```

This is alphabetically correct — no issue.

**Summary for Patch 2:** The driver is well-written and follows existing patterns closely. The main substantive items are: (1) consider making `drm_driver` const, (2) the unnecessary inner block scope in `atomic_update` is odd, and (3) the empty `crtc_atomic_enable/disable` should either do something useful (like re-init the panel) or be documented as intentionally empty. None of these are blockers, but addressing them would improve the code.

---
Generated by Claude Code Patch Reviewer