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: Wed, 01 Apr 2026 07:39:20 +1000
Message-ID: <review-patch4-20260331-ws-lcd-v2-4-a1add63b6eb6@oss.qualcomm.com>
In-Reply-To: <20260331-ws-lcd-v2-4-a1add63b6eb6@oss.qualcomm.com>
References: <20260331-ws-lcd-v2-0-a1add63b6eb6@oss.qualcomm.com> <20260331-ws-lcd-v2-4-a1add63b6eb6@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the bulk of the series, adding 15 panel definitions. Several observations:

**1. Missing trailing commas in OF table entries.** Every new `.data` line is missing a trailing comma, while the existing entries all have them:

```c
	}, {
		.compatible = "waveshare,2.8inch-panel",
		.data = &waveshare_28_lcd_panel    /* <-- missing comma */
	}, {
```

Compare with existing style:
```c
	}, {
		.compatible = "waveshare,13.3inch-panel",
		.data = &waveshare_133inch,        /* <-- has comma */
	}, {
```

All 15 new entries are missing the trailing comma. While this compiles fine (the comma is optional before `}`), the existing code consistently uses trailing commas and this should match for style consistency.

**2. Forward declaration of `waveshare_80_lcd_c_mode`.** The patch uses:

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

...with the actual definition appearing later. This works in C but is unusual for this file. It could be avoided by simply reordering the definitions so `waveshare_80_lcd_c_mode` and `waveshare_80_lcd_c_panel` appear before `waveshare_70_lcd_e_panel`. Since the panels are ordered numerically, moving 8.0" before 7.0"-E would be a minor ordering inconsistency, but the forward declaration is more surprising than a slightly out-of-order definition. Either approach is acceptable; just noting the trade-off.

**3. Mode reuse pattern.** Several panels share display modes:
- `waveshare_70_lcd_c_panel` reuses `waveshare_50_lcd_c_mode` (both 1024x600)
- `waveshare_70_lcd_e_panel` reuses `waveshare_80_lcd_c_mode` (both 1280x800)
- `waveshare_101_lcd_c_panel` reuses `waveshare_80_lcd_c_mode` (both 1280x800)

This is fine and follows existing practice in panel-simple.c where panels with identical timings share mode structs.

**4. Clock values.** The `waveshare_34_lcd_c_mode` has `.clock = 50000` for an 800x800 panel with htotal=958, vtotal=828. That gives a refresh rate of 50000000/(958*828) = ~63 Hz, which seems reasonable. The `.clock = 83333` entries (e.g., for 720x1280 @ 83333000/(1000*1340) = ~62 Hz) also check out. The timings appear plausible.

**5. Pixel format note.** The commit message honestly states: "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 worth keeping in mind -- the LVDS panels all use `MEDIA_BUS_FMT_RGB888_1X7X4_SPWG` (JEIDA mapping), which is a reasonable default but may need correction if specific panels actually use VESA mapping (`MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA` -- though SPWG and JEIDA are effectively the same thing in this context). Since the bridge handles the conversion, this shouldn't matter in practice.

**Summary for patch 4:** Add trailing commas to all 15 OF table entries. Consider eliminating the forward declaration by reordering definitions.

---
Generated by Claude Code Patch Reviewer