From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/panel: simple: add Waveshare LCD panels
Date: Tue, 31 Mar 2026 17:17:01 +1000
Message-ID: <review-patch4-20260330-ws-lcd-v1-4-309834a435c0@oss.qualcomm.com>
In-Reply-To: <20260330-ws-lcd-v1-4-309834a435c0@oss.qualcomm.com>
References: <20260330-ws-lcd-v1-0-309834a435c0@oss.qualcomm.com> <20260330-ws-lcd-v1-4-309834a435c0@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the main patch adding 15 panel definitions (~381 lines). A few issu=
es:

**1. Missing trailing commas in OF match table (style issue):**

Every new entry is missing a trailing comma after `.data`:

```c
	}, {
		.compatible =3D "waveshare,2.8inch-panel",
		.data =3D &waveshare_28_lcd_panel
	}, {
```

The existing code uses trailing commas consistently (e.g., `.data =3D &wave=
share_133inch,`). All 15 new entries should have trailing commas added for =
consistency with the rest of the file.

**2. Forward declaration of `waveshare_80_lcd_c_mode`:**

```c
static const struct drm_display_mode waveshare_80_lcd_c_mode;
static const struct panel_desc waveshare_70_lcd_e_panel =3D {
	.modes =3D &waveshare_80_lcd_c_mode,
	...
};
```

The actual definition of `waveshare_80_lcd_c_mode` comes ~60 lines later. W=
hile this is valid C (tentative definition completed by the later actual de=
finition), it's an unusual pattern in panel-simple.c. It would be cleaner t=
o simply reorder the definitions so `waveshare_80_lcd_c_mode` is defined be=
fore `waveshare_70_lcd_e_panel`, just as `waveshare_50_lcd_c_mode` is defin=
ed before `waveshare_70_lcd_c_panel` which also shares a mode.

**3. Mode reuse is good but should be documented:**

Several panels share modes (same resolution/timings, different physical siz=
es):
- `waveshare_70_lcd_c_panel` reuses `waveshare_50_lcd_c_mode` (1024x600)
- `waveshare_70_lcd_e_panel` reuses `waveshare_80_lcd_c_mode` (1280x800)
- `waveshare_101_lcd_c_panel` reuses `waveshare_80_lcd_c_mode` (1280x800)

This is fine and avoids duplication, but the forward declaration for one of=
 them (point 2) should be avoided by reordering.

**4. Minor commit message typo:** "serie" should be "series":
```
Waveshare have a serie of DSI panel kits
```

**5. Pixel format caveat:** The commit message honestly notes:
```
Note, since the panels are hidden behind the bridges which are not being
programmed by the kernel, I could not confirm the pixel format for the
panels.
```

This is good transparency but worth flagging =E2=80=94 the DPI panels use `=
MEDIA_BUS_FMT_RGB888_1X24` and the LVDS panels use `MEDIA_BUS_FMT_RGB888_1X=
7X4_SPWG`. If any panel actually uses a different LVDS mapping (e.g., JEIDA=
), it would produce incorrect colors. Since the bridge firmware handles the=
 conversion, this may not matter in practice, but it's worth confirming wit=
h hardware if possible.

---
Generated by Claude Code Patch Reviewer