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: Tue, 05 May 2026 08:29:50 +1000
Message-ID: <review-patch2-20260504-topic-sm8650-ayaneo-pocket-s2-r63419-v3-2-9f61cf24aebf@linaro.org>
In-Reply-To: <20260504-topic-sm8650-ayaneo-pocket-s2-r63419-v3-2-9f61cf24aebf@linaro.org>
References: <20260504-topic-sm8650-ayaneo-pocket-s2-r63419-v3-0-9f61cf24aebf@linaro.org> <20260504-topic-sm8650-ayaneo-pocket-s2-r63419-v3-2-9f61cf24aebf@linaro.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Issue (medium): Reversed display-on / exit-sleep-mode order in `renesas_r63419_on()`**

```c
static int renesas_r63419_on(struct renesas_r63419_panel *ctx)
{
	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi[0] };
	struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = ctx->dsi[1] };

	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 standard MIPI DCS sequence is exit-sleep-mode first, wait ~120ms, then display-on. The JDI dual-DSI driver (`panel-jdi-lpm102a188a.c`) confirms this ordering at lines 223-234 with an explicit comment: *"We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode_multi() and mipi_dsi_dcs_set_display_on_multi()."*

If the R63419 genuinely requires this reversed order, a comment explaining the deviation would be valuable. Otherwise this looks like a bug.

**Issue (medium): Second DSI link errors silently dropped**

Both `renesas_r63419_on()` and `renesas_r63419_disable()` use two separate `mipi_dsi_multi_context` structs but only return `dsi_ctx.accum_err`:

```c
	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx1);
	...
	return dsi_ctx.accum_err;
```

Errors from operations on `dsi_ctx1` (the second DSI link) are never checked. The kernel provides the `mipi_dsi_dual()` macro (see `include/drm/drm_mipi_dsi.h:486`) that uses a single context and swaps the `.dsi` pointer between calls, ensuring `accum_err` captures failures from both links. Consider using it:

```c
	mipi_dsi_dual(mipi_dsi_dcs_set_display_on_multi,
		      &dsi_ctx, ctx->dsi[0], ctx->dsi[1]);
```

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

```c
static struct panel_desc wt0600_desc = {
```

These are used as read-only match data via `of_device_get_match_data()` and are never modified. Other panel drivers (e.g., `panel-boe-tv101wum-nl6.c`) declare equivalent structs as `static const`. Mark these `const`.

**Nit: Misleading comment on `wt0630_desc`**

```c
	.mode = &wt0600_mode,  /* wt0600 only has different screen size */
```

This reads as "wt0600 only differs in screen size" but should say "wt0630 only has different physical size" since the comment is on the wt0630 descriptor.

**Observation: `prepare` error path vs. `unprepare` ordering**

In `renesas_r63419_prepare()`, the error cleanup path does:
```c
		regulator_bulk_disable(...vcc_supplies);
		regulator_bulk_disable(...vdd_supplies);
		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
```

While `renesas_r63419_unprepare()` does the same order. This is fine functionally, but the convention in many panel drivers is to assert reset before disabling regulators (reverse of the power-on sequence). Not a correctness issue, just a style observation.

---
Generated by Claude Code Patch Reviewer