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: novatek-nt36672a: Inline panel init sequences Date: Mon, 18 May 2026 15:58:44 +1000 Message-ID: In-Reply-To: <20260518034252.16337-1-chintanlike@gmail.com> References: <20260518034252.16337-1-chintanlike@gmail.com> <20260518034252.16337-1-chintanlike@gmail.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 **Positive aspects:** - Correct use of `mipi_dsi_dcs_write_var_seq_multi()` for loop bodies where= the command byte is a runtime variable. The `_var_` variant uses stack-all= ocated `const u8 d[]` instead of `static const u8 d[]`, which is necessary = when the initializer contains non-constant expressions: ```c for (reg =3D 0x02; reg <=3D 0x10; reg++) mipi_dsi_dcs_write_var_seq_multi(dsi_ctx, reg, 0x40); ``` - All 12 loop compressions faithfully represent the original contiguous reg= ister ranges with no registers added or dropped. Spot-checked ranges like `= 0x5B=E2=80=930x6F` (value 0x50, 21 registers), `0x8B=E2=80=930x9F` (value 0= x80, 21 registers), and `0xE5=E2=80=930xF6` (value 0x40, 18 registers) all = match the original table entries exactly. - The `u8 reg` loop variable is safe =E2=80=94 the maximum loop bound is `0= xF6` (246), so the post-increment never wraps around 255. - NULL checks on function pointers in `prepare`/`unprepare` are good defens= ive practice, even though the single panel descriptor always provides all t= hree. - The `on_cmds_2` (5 entries) and `off_cmds` (4 entries) tables are convert= ed 1:1 with no loops, which is correct since those sequences have no repeat= ing patterns. **Minor nits (non-blocking):** 1. **Naming: `off_init` is slightly misleading.** The function runs during = `unprepare` (display shutdown), not initialization. The original field was = `off_cmds` which was more semantically neutral. Consider `off_cmds_fn` or `= unprepare_cmds` or just `off` to avoid the implication that "init" means st= artup: ```c void (*off_init)(struct mipi_dsi_multi_context *dsi_ctx); ``` This is a style nit =E2=80=94 the commit message and context make the in= tent clear. 2. **Gaps between individual writes could also use brief comments.** The or= iginal data tables made the register gaps (0x3C, 0x3E, 0xA1, 0xA3=E2=80=93A= 5, etc.) visible implicitly through the numbering. In the new inline form, = a reader might wonder whether those gaps were intentional or accidental. A = comment like `/* 0x3C, 0x3E intentionally skipped */` at the relevant gaps = could help, but this is entirely optional. 3. **Commit message mentions `mipi_dsi_dcs_write_var_seq_multi()` but doesn= 't explain why it's needed instead of `mipi_dsi_dcs_write_seq_multi()`.** A= one-liner noting "the `_var_` variant is required when the command byte co= mes from a variable, as the `_seq_` variant uses `static const` arrays" wou= ld help future readers of the git log understand the distinction. **Verdict:** The patch is correct and ready to merge. The refactoring is fa= ithful, the macro usage is appropriate, and the code is cleaner as a result= . The nits above are optional improvements. --- Generated by Claude Code Patch Reviewer