From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panel: simple: Add timings for Raspberry Pi 7" panel Date: Thu, 23 Apr 2026 09:39:22 +1000 Message-ID: In-Reply-To: <20260420-rpi-7inch-v1-2-e68d5c9c44bc@ideasonboard.com> References: <20260420-rpi-7inch-v1-0-e68d5c9c44bc@ideasonboard.com> <20260420-rpi-7inch-v1-2-e68d5c9c44bc@ideasonboard.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 The commit message is excellent =E2=80=94 it documents the strict back-porc= h requirements, the connector type rationale, and the hardware topology cle= arly. **Timing analysis:** The timing definition looks correct for an 800x480 panel. The typical pixel= clock of 30MHz gives approximately 60fps: - htotal =3D 800 + 72 + 32 + 46 =3D 950 - vtotal =3D 480 + 21 + 2 + 23 =3D 526 - 30MHz / (950 * 526) =3D ~60.0 Hz The fixed back-porch values (hbp=3D46, vbp=3D23 with min=3Dtyp=3Dmax) match= the commit message's claim that these must be exact. The wide pixel clock = range (10=E2=80=9350MHz) allows flexible refresh rate selection, which is r= easonable for a panel behind a DSI-to-DPI bridge where the host controller = determines the actual rate. **Observation =E2=80=94 similarity to rocktech_rk070er9427:** The existing `rocktech_rk070er9427` panel at line 4274 shares strikingly si= milar parameters: same resolution (800x480), same physical size (154x86mm),= identical fixed back-porches (hbp=3D46, vbp=3D23), and overlapping front-p= orch/pixel-clock ranges. The main differences are bpc (6 vs 8), bus_format = (RGB666_1X18 vs RGB888_1X24), and the new panel's bus_flags/sync flags. Thi= s might suggest Rocktech is one of the undisclosed panel vendors =E2=80=94 = not a problem for the patch, but an interesting data point. **bus_flags usage:** The patch uses `DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE` and `DRM_BUS_FLAG_PIXDATA_= DRIVE_POSEDGE` in the `panel_desc.bus_flags`. These are from the transmitte= r's perspective (DRIVE), not the receiver/panel perspective (SAMPLE). Since= `DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE` =3D=3D `DRM_BUS_FLAG_PIXDATA_SAMPLE_N= EGEDGE` (both BIT(2)), and similarly for sync, these will pass the DPI vali= dation checks at line 664 without warnings. This is consistent with how several other panels in the file define their b= us_flags (e.g., lines 1644, 1851, 2123-2124), so this is fine. However, giv= en the comment in the timing struct: ```c /* Note: the rest of the flags are defined below in bus_flags */ ``` It's worth noting that `DISPLAY_FLAGS_VSYNC_HIGH | DISPLAY_FLAGS_HSYNC_HIGH= ` in the timing struct defines sync polarity (which sync level is active), = while `DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE` in bus_flags defines the clock edge= on which sync is driven =E2=80=94 these are orthogonal, so no conflict. Th= e comment might mislead a reader into thinking the bus_flags are an alterna= tive location for the *same* kind of flags, when they're actually complemen= tary. This is a very minor nit. **of_device_id table:** The entry is correctly placed in alphabetical order between `qishenglong,go= pher2b-lcd` and `raystar,rff500f-awh-dnn` at the updated line 5519. **No blocking issues.** --- Generated by Claude Code Patch Reviewer