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: Tue, 05 May 2026 10:27:00 +1000 Message-ID: In-Reply-To: <20260430-topic-sm8650-ayaneo-pocket-s2-r63419-v2-2-91ac10453d0c@linaro.org> References: <20260430-topic-sm8650-ayaneo-pocket-s2-r63419-v2-0-91ac10453d0c@linaro.org> <20260430-topic-sm8650-ayaneo-pocket-s2-r63419-v2-2-91ac10453d0c@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 **Signed-off-by order:** The patch has `From: KancyJoe` indicating KancyJoe= is the author, but the Signed-off-by chain is: ``` Signed-off-by: Neil Armstrong Signed-off-by: KancyJoe ``` Per kernel conventions, the author's Signed-off-by should come first in the= chain. This should be: ``` Signed-off-by: KancyJoe Signed-off-by: Neil Armstrong ``` **Critical: MIPI DCS command ordering is wrong in `renesas_r63419_on()`:** ```c mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); mipi_dsi_dcs_set_display_on_multi(&dsi_ctx1); mipi_dsi_msleep(&dsi_ctx, 150); mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx1); mipi_dsi_msleep(&dsi_ctx, 50); ``` The MIPI DCS specification requires `EXIT_SLEEP_MODE` **first**, followed b= y a delay (typically 120ms), then `SET_DISPLAY_ON`. Every other panel drive= r in the kernel tree follows this order. This is reversed here and will lik= ely cause initialization failures or display corruption. The correct sequen= ce would be: ```c mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx1); mipi_dsi_msleep(&dsi_ctx, 120); mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); mipi_dsi_dcs_set_display_on_multi(&dsi_ctx1); ``` **Second DSI context error never checked:** In `renesas_r63419_on()`, two s= eparate `mipi_dsi_multi_context` structs are used (`dsi_ctx` and `dsi_ctx1`= ), but only `dsi_ctx.accum_err` is returned. Errors on the second DSI link = (`dsi_ctx1.accum_err`) are silently discarded. The same issue exists in `re= nesas_r63419_disable()`: ```c return dsi_ctx.accum_err; ``` This should also check `dsi_ctx1.accum_err`, e.g.: ```c return dsi_ctx.accum_err ?: dsi_ctx1.accum_err; ``` Alternatively, look at how `panel-sharp-lq079l1sx01.c` handles this =E2=80= =94 it uses a single context and helper functions that send to both DSI lin= ks in one call via `mipi_dsi_dual_dcs_write_seq_multi()`. **Should use `devm_mipi_dsi_attach()` instead of `mipi_dsi_attach()`:** The= probe function uses manual `mipi_dsi_attach()`: ```c ret =3D mipi_dsi_attach(ctx->dsi[i]); if (ret < 0) { drm_panel_remove(&ctx->panel); return dev_err_probe(dev, ret, "Failed to attach DSI device %d\n", i); } ``` If the second DSI attach (`i=3D1`) fails, the first DSI (`i=3D0`) was alrea= dy attached but is never detached. Using `devm_mipi_dsi_attach()` (as `pane= l-sharp-lq079l1sx01.c` does) would handle cleanup automatically. With `devm= _mipi_dsi_attach()`, the `remove` callback would also simplify to just `drm= _panel_remove()`, since detach is handled by devres. **Panel descriptors should be `const`:** ```c static struct panel_desc wt0600_desc =3D { ... static struct panel_desc wt0630_desc =3D { ``` These are never modified and should be `static const struct panel_desc`. Th= e `desc` member pointer in the panel struct is already `const struct panel_= desc *desc`, so the types are compatible. **Unprepare reset/power ordering:** In `renesas_r63419_unprepare()`, the re= set GPIO is asserted *after* disabling regulators: ```c regulator_bulk_disable(..., ctx->vcc_supplies); regulator_bulk_disable(..., ctx->vdd_supplies); gpiod_set_value_cansleep(ctx->reset_gpio, 1); ``` The typical and safer convention (followed by most panel drivers) is to ass= ert reset *before* cutting power, to ensure the panel IC enters a known sta= te while it still has power. The prepare error path does the same thing =E2= =80=94 disables regulators first, then asserts reset. Consider reversing to= : assert reset, then disable VCC, then disable VDD. **Missing `#include` for `of_graph.h` may not be needed:** The driver inclu= des `` for `of_graph_get_remote_node()`. This is fine, bu= t worth noting that some newer drivers use different patterns. Not a blocke= r. **`width_mm`/`height_mm` set on mode struct:** In `renesas_r63419_get_modes= ()`: ```c mode->width_mm =3D ctx->desc->width_mm; mode->height_mm =3D ctx->desc->height_mm; ``` While these fields still exist in `struct drm_display_mode`, the canonical = location is `connector->display_info.width_mm/height_mm` (which this driver= also sets, so the mode fields are redundant). Minor nit. **Otherwise well-structured:** The driver follows established patterns =E2= =80=94 `devm_drm_panel_alloc`, `devm_regulator_bulk_get_const`, `devm_mipi_= dsi_device_register_full`, proper Kconfig dependencies, alphabetical Makefi= le ordering, and correct `prepare_prev_first =3D true` for dual-DSI. The mo= de clock computation is correctly doubled for the dual-DSI configuration. --- Generated by Claude Code Patch Reviewer