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-edp: Change BOE NV140WUM-N64 timings Date: Fri, 27 Feb 2026 12:30:41 +1000 Message-ID: In-Reply-To: <20260226083235.2176689-1-zhouhaikun5@huaqin.corp-partner.google.com> References: <20260226083235.2176689-1-zhouhaikun5@huaqin.corp-partner.google.com> <20260226083235.2176689-1-zhouhaikun5@huaqin.corp-partner.google.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 **Commit message:** The commit message says: > This screen timing requires a backlight off time of more than 100ms > from the end of the data stream to avoid screen flickering and red > light caused by screen material limitations. This is a bit vague. It would be helpful to clarify: - Whether this is a `.disable` delay (T9 timing per the eDP spec), which it= is based on the code. The commit message says "backlight off time" but the= `.disable` field is documented in the struct as *"Time for panel to turn t= he display off (panel-off to backlight-off, T9-min in eDP timing diagrams)"= *. The terminology should be more precise =E2=80=94 this is a T9 minimum de= lay, not a backlight-off time per se. - How the issue was observed (which platform/board), which would help maint= ainers evaluate the change. **Code changes =E2=80=94 new delay struct:** ```c +static const struct panel_delay delay_200_500_e200_d100 =3D { + .hpd_absent =3D 200, + .unprepare =3D 500, + .enable =3D 200, + .disable =3D 100, +}; ``` This correctly follows the established naming convention (`delay_[hpd_absen= t]_[unprepare]_e[enable]_d[disable]`), and the field values are correct. Th= ere are already `_d10`, `_d50`, and `_d200` variants, so `_d100` fits in na= turally. **Minor issue =E2=80=94 placement:** The new struct is inserted at line 193= 8 (after `delay_200_500_e200`), directly before `delay_200_500_e200_d200`. = Looking at the existing code in the drm-next tree, the `_d` variants are or= dered as `_d200` (line 1791), `_d10` (line 1798), `_d50` (line 1805). The p= atch places `_d100` between `delay_200_500_e200` and `delay_200_500_e200_d2= 00`. Since the existing `_d` variants aren't numerically ordered either, th= is placement is acceptable, though ideally all the `_d` variants would be g= rouped together. **Code change =E2=80=94 panel entry update:** ```c - EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cf6, &delay_200_500_e200, "NV140WUM-N64= "), + EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cf6, &delay_200_500_e200_d100, "NV140WU= M-N64"), ``` This is straightforward and correct =E2=80=94 swapping the delay struct poi= nter for the NV140WUM-N64 panel entry. **Functional assessment:** The `.disable` field is used in `panel_edp_disab= le()` where it triggers an `msleep()` call. Previously this panel had `.dis= able` implicitly zero (no delay). Adding a 100ms delay is a conservative, l= ow-risk change. The 100ms value seems reasonable for a T9-min requirement, = and similar delays exist for other panels (10ms, 50ms, 200ms). **Nits:** - The patch is missing a `Cc: stable` tag if this is fixing a visible user-= facing issue (flickering). If this has always been broken, it may warrant b= ackporting. - It would be good to have a `Tested-by:` tag from someone who verified the= fix on actual hardware. **Verdict:** The patch looks functionally correct and safe. The commit mess= age could be improved with more precise eDP timing terminology and platform= context, but the code change itself is clean and follows existing conventi= ons. --- Generated by Claude Code Patch Reviewer