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: add support for the Renesas R63419 based dual-DSI video mode Display Panels Date: Mon, 25 May 2026 20:36:17 +1000 Message-ID: In-Reply-To: <20260521-topic-sm8650-ayaneo-pocket-s2-r63419-v5-2-dd5700299390@linaro.org> References: <20260521-topic-sm8650-ayaneo-pocket-s2-r63419-v5-0-dd5700299390@linaro.org> <20260521-topic-sm8650-ayaneo-pocket-s2-r63419-v5-2-dd5700299390@linaro.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug: `mipi_dsi_multi_context` not initialized** In both `renesas_r63419_on()` and `renesas_r63419_disable()`, the `dsi_ctx`= is declared on the stack without an initializer: ```c static int renesas_r63419_on(struct renesas_r63419_panel *ctx) { struct mipi_dsi_multi_context dsi_ctx; dsi_link_switch(ctx, &dsi_ctx, 0); ``` and: ```c static int renesas_r63419_disable(struct drm_panel *panel) { struct renesas_r63419_panel *ctx =3D to_renesas_r63419_panel(panel); struct mipi_dsi_multi_context dsi_ctx; dsi_link_switch(ctx, &dsi_ctx, 0); ``` The `dsi_link_switch()` helper only sets `dsi_ctx->dsi` =E2=80=94 it does n= ot initialize `accum_err`. The `mipi_dsi_multi_context` documentation expli= citly states `accum_err` must be "Init to 0", and all `_multi` functions ar= e no-ops if `accum_err` is non-zero. Without `CONFIG_INIT_STACK_ALL_ZERO`, = `accum_err` contains indeterminate stack data, and if it happens to be non-= zero, the entire init/disable sequence silently does nothing. Fix: either zero-initialize the struct (`struct mipi_dsi_multi_context dsi_= ctx =3D { };`) or use designated initializers (`=3D { .dsi =3D ctx->dsi[0] = }`). **Style: `panel_desc` structs should be `const`** ```c static struct panel_desc wt0600_desc =3D { ``` and ```c static struct panel_desc wt0630_desc =3D { ``` These are never modified and are used as read-only match data. They should = be `static const struct panel_desc`. **Observation: DCS command ordering** The `renesas_r63419_on()` function sends `set_display_on` before `exit_slee= p_mode`: ```c mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); /* ... */ mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); ``` The standard MIPI DCS sequence is sleep-out first, then display-on (per MIP= I DCS spec section 9.15/9.28). This is likely intentional per the R63419 ve= ndor datasheet =E2=80=94 but since it's unusual, a brief comment noting thi= s is the vendor-specified sequence would prevent future "fix" attempts. **Observation: `dsi_link_switch` pattern vs established dual-DSI patterns** The existing in-tree dual-DSI driver (`panel-jdi-lpm102a188a.c`) uses two s= eparate `mipi_dsi_multi_context` variables, each properly initialized with = `=3D { .dsi =3D ... }`. The `dsi_link_switch` pattern introduced here is a = reasonable idea for reducing boilerplate when both links share error accumu= lation, but it's novel. If it becomes a common pattern, it might be worth p= romoting to a helper in the DSI core, but for now a single driver introduci= ng it is fine =E2=80=94 once the initialization bug above is fixed. **Minor: regulator bulk get lacks `dev_err_probe` wrapping** ```c ret =3D devm_regulator_bulk_get_const(&dsi->dev, ARRAY_SIZE(renesas_r63419_vdd_supplies), renesas_r63419_vdd_supplies, &ctx->vdd_supplies); if (ret < 0) return ret; ``` Other error paths in probe use `dev_err_probe()` consistently. These two re= gulator calls return bare `ret`. While the regulator subsystem may log inte= rnally, using `dev_err_probe()` would be consistent with the rest of the fu= nction and properly handles `-EPROBE_DEFER` logging. **Everything else looks good:** - `devm_drm_panel_alloc` / `devm_drm_panel_add` / `devm_mipi_dsi_attach` us= age is correct and eliminates the need for a `remove` callback. - The dual-mode clock calculation (`2 * (720 + 100 + 8 + 40) * (2560 + 15 += 2 + 8) * 60 / 1000`) is consistent across both modes, with only physical d= imensions differing. - The `prepare_prev_first =3D true` setting is appropriate for a panel that= needs the CRTC pipeline prepared before the panel powers on. - GPIO is requested as `GPIOD_OUT_HIGH` (reset asserted at probe), then dea= sserted in `prepare` =E2=80=94 correct active-low reset handling. - OF match table properly maps board-specific compatibles to per-panel desc= riptors. --- Generated by Claude Code Patch Reviewer