From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
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 18:37:56 +1000
Message-ID: <review-patch2-20260522-topic-sm8650-ayaneo-pocket-s2-r63419-v6-2-16edddda9951@linaro.org>
In-Reply-To: <20260522-topic-sm8650-ayaneo-pocket-s2-r63419-v6-2-16edddda9951@linaro.org>
References: <20260522-topic-sm8650-ayaneo-pocket-s2-r63419-v6-0-16edddda9951@linaro.org> <20260522-topic-sm8650-ayaneo-pocket-s2-r63419-v6-2-16edddda9951@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

**Overall: Good, minor issues only.**

**Issue 1 (minor): `wt0600_desc` and `wt0630_desc` should be `const`**

```c
static struct panel_desc wt0600_desc =3D {
```
```c
static struct panel_desc wt0630_desc =3D {
```

These are only ever read via `of_device_get_match_data()` and never modifie=
d. Every other panel driver in the tree declares its descriptor structs as =
`static const`. These should be:

```c
static const struct panel_desc wt0600_desc =3D {
```

**Issue 2 (minor): Dead `dsi_info` field in `struct panel_desc`**

```c
struct panel_desc {
	const struct drm_display_mode *mode;
	unsigned int lanes;
	unsigned long mode_flags;
	enum mipi_dsi_pixel_format format;
	const struct mipi_dsi_device_info dsi_info;
};
```

The `dsi_info` field is declared in the struct but never initialized in eit=
her `wt0600_desc` or `wt0630_desc`, and never referenced anywhere. The prob=
e function creates its own `struct mipi_dsi_device_info info =3D { }` on th=
e stack instead. This appears to be a leftover from an earlier revision. It=
 should be removed.

**Issue 3 (observation, not a bug): Display-on before exit-sleep ordering**

```c
static int renesas_r63419_on(struct renesas_r63419_panel *ctx)
{
	struct mipi_dsi_multi_context dsi_ctx =3D { 0 };

	dsi_link_switch(ctx, &dsi_ctx, 0);
	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
	dsi_link_switch(ctx, &dsi_ctx, 1);
	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
	mipi_dsi_msleep(&dsi_ctx, 150);

	dsi_link_switch(ctx, &dsi_ctx, 0);
	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
	dsi_link_switch(ctx, &dsi_ctx, 1);
	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
	mipi_dsi_msleep(&dsi_ctx, 50);

	return dsi_ctx.accum_err;
}
```

This sends `SET_DISPLAY_ON` before `EXIT_SLEEP_MODE`, which is the opposite=
 of the typical MIPI DCS sequence (where the panel is brought out of sleep =
first, then display is enabled). If this is what the R63419 vendor spec req=
uires, it's fine =E2=80=94 but it's worth a double-check since it's unusual=
. Most panel drivers do `exit_sleep =E2=86=92 delay =E2=86=92 display_on`.

**No other issues.** The rest of the driver is solid:

- The `dsi_link_switch` helper pattern for sharing a single `mipi_dsi_multi=
_context` across both DSI links is a clean approach for error accumulation.
- Regulator error handling in `renesas_r63419_prepare()` correctly disables=
 VDD supplies when VCC enable fails.
- The power-off sequence in `unprepare` correctly reverses the power-on ord=
er (VCC then VDD).
- Reset GPIO polarity (`GPIOD_OUT_HIGH` =E2=86=92 active/asserted at probe,=
 deasserted with `0` in prepare) is correct for active-low reset.
- Use of `devm_regulator_bulk_get_const()` with separate VDD and VCC arrays=
 matches the need for sequenced power-on.
- `prepare_prev_first =3D true` is appropriate for DSI panels that need the=
 host initialized before the panel.
- All devm APIs are used correctly, making a `remove` function unnecessary.

---
Generated by Claude Code Patch Reviewer