Claude review: drm: renesas: rzg2l_mipi_dsi: Move rzg2l_mipi_dsi_set_display_timing()

Claude review: drm: renesas: rzg2l_mipi_dsi: Move rzg2l_mipi_dsi_set_display_timing()

[Claude Code Review Bot]

Patch Review

This moves `rzg2l_mipi_dsi_set_display_timing()` from `atomic_pre_enable` to `atomic_enable`, placing it after `rzg2l_mipi_dsi_start_hs_clock()`. The hardware manual requires display timings to be set after HS clock is started.

**Issue: Silent dropping of error handling for rzg2l_mipi_dsi_startup()**

In the original code:
```c
	ret = rzg2l_mipi_dsi_startup(dsi, mode);
	if (ret < 0)
		return;
```

The patch changes this to:
```c
	rzg2l_mipi_dsi_startup(dsi, mode);
```

`rzg2l_mipi_dsi_startup()` returns an int and can fail (e.g., `pm_runtime_resume_and_get()`, `dphy_init()`, `dphy_conf_clks()` can all return errors). Silently ignoring the return value means that if startup fails, the driver will proceed to `atomic_enable` and attempt to start the HS clock on uninitialized hardware. The commit message mentions "Drop the unused ret variable" but `ret` was being *used* for error checking — it just couldn't propagate the error since the bridge callback returns void. Still, the early return prevented further damage. This should be preserved:

```c
	if (rzg2l_mipi_dsi_startup(dsi, mode) < 0)
		return;
```

**Duplicated connector/crtc/mode lookup:** The mode lookup code is now duplicated in both `atomic_pre_enable` and `atomic_enable`. Consider caching the mode pointer in `struct rzg2l_mipi_dsi` during `atomic_pre_enable` to avoid the duplication, though this is a minor style point.

Otherwise the fix itself is correct per the hardware manual requirements.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 0/3] Improvements on RZ/G2L MIPI DSI driver

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

Hi All,

Enhance the RZ/G2L MIPI DSI driver based on section "34.4.2.1 Reset" of
the RZ/G2L hardware manual Rev.1.50 May 2025. According to this section,
it is required to wait >= 1 msec after deasserting the CMN_RSTB signal,
and writing to DSI PHY timing registers and LINK registers should be done
before deasserting CMN_RSTB.
Additionally, the hardware manual suggests display timing settings should
be done after the HS clock is started.

v2->v3:
 * Merged patch#2 and patch#3 to avoid breakage.
 * Moved the patch from patch#4 to patch#2.
 * Added fixes tag for patch#2.
 * Updated commit description for patch#2 and patch#3.
v1->v2:
 * Updated commit header and description
 * Moved the code from rzg2l_mipi_dsi_dphy_init() to rzg2l_mipi_dsi_startup()
 * Moved the check before calling reset_control_deassert(), so that it will be
   skipped for RZ/V2H SoC
 * Added fixes patch for moving rzg2l_mipi_dsi_set_display_timing()
 * Added fixes patch for assert of CMN_RSTB signal

Biju Das (3):
  drm: renesas: rzg2l_mipi_dsi: Move rzg2l_mipi_dsi_set_display_timing()
  drm: renesas: rzg2l_mipi_dsi: Increase reset deassertion delay
  drm: renesas: rzg2l_mipi_dsi: Fix deassert/assert of CMN_RSTB signal

 .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c    | 34 +++++++++++--------
 1 file changed, 19 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] drm: renesas: rzg2l_mipi_dsi: Move rzg2l_mipi_dsi_set_display_timing()

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

The RZ/G2L hardware manual (Rev. 1.50, May 2025), Section 34.4.2.1,
requires display timings to be set after the HS clock is started. Move
rzg2l_mipi_dsi_set_display_timing() from
rzg2l_mipi_dsi_atomic_pre_enable() to rzg2l_mipi_dsi_atomic_enable(),
placing it after rzg2l_mipi_dsi_start_hs_clock(). Drop the unused ret
variable from rzg2l_mipi_dsi_atomic_pre_enable().

Fixes: 5ce16c169a4c ("drm: renesas: rz-du: Add atomic_pre_enable")
Fixes: 7a043f978ed1 ("drm: rcar-du: Add RZ/G2L DSI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v2:
 * New patch
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
index a87a301326c7..ff95cb9a7de5 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
@@ -1025,29 +1025,33 @@ static void rzg2l_mipi_dsi_atomic_pre_enable(struct drm_bridge *bridge,
 	const struct drm_display_mode *mode;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
-	int ret;
 
 	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
 	mode = &drm_atomic_get_new_crtc_state(state, crtc)->adjusted_mode;
 
-	ret = rzg2l_mipi_dsi_startup(dsi, mode);
-	if (ret < 0)
-		return;
-
-	rzg2l_mipi_dsi_set_display_timing(dsi, mode);
+	rzg2l_mipi_dsi_startup(dsi, mode);
 }
 
 static void rzg2l_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
 					 struct drm_atomic_state *state)
 {
 	struct rzg2l_mipi_dsi *dsi = bridge_to_rzg2l_mipi_dsi(bridge);
+	const struct drm_display_mode *mode;
+	struct drm_connector *connector;
+	struct drm_crtc *crtc;
 	int ret;
 
 	ret = rzg2l_mipi_dsi_start_hs_clock(dsi);
 	if (ret < 0)
 		goto err_stop;
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+	mode = &drm_atomic_get_new_crtc_state(state, crtc)->adjusted_mode;
+
+	rzg2l_mipi_dsi_set_display_timing(dsi, mode);
+
 	ret = rzg2l_mipi_dsi_start_video(dsi);
 	if (ret < 0)
 		goto err_stop_clock;
-- 
2.43.0

[PATCH v3 2/3] drm: renesas: rzg2l_mipi_dsi: Increase reset deassertion delay

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

The RZ/G2L hardware manual (Rev. 1.50, May 2025), Section 34.4.2.1,
requires waiting at least 1 msec after deasserting the CMN_RSTB signal
before the DSI-Tx module is ready. Increase the delay from 1 usec to
1 msec by replacing udelay(1) with fsleep(1000) for RZ/G2L SoCs.

Fixes: 7a043f978ed1 ("drm: rcar-du: Add RZ/G2L DSI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Moved the patch from patch#4 to patch#2.
 * Added fixes tag.
 * Updated commit description.
v1->v2:
 * Updated commit header and description.
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
index ff95cb9a7de5..9d9f77d8f949 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
@@ -528,7 +528,7 @@ static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
 	if (ret < 0)
 		return ret;
 
-	udelay(1);
+	fsleep(1000);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v3 3/3] drm: renesas: rzg2l_mipi_dsi: Fix deassert/assert of CMN_RSTB signal

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

The RZ/G2L hardware manual (Rev. 1.50, May 2025), Section 34.4.2.1,
requires deasserting the CMN_RSTB signal after setting the Link registers.
Move the reset_control_deassert() call from rzg2l_mipi_dsi_dphy_init() to
rzg2l_mipi_dsi_startup(), placing it after the Link register writes. This
reset signal is optional for RZ/V2H SoCs, so add a NULL check. Drop the
unused ret variable from rzg2l_mipi_dsi_dphy_init().

The CMN_RSTB signal is not required for reading PHY registers in the
probe. Move reset_control_assert() from rzg2l_mipi_dsi_dphy_exit() to
rzg2l_mipi_dsi_stop(), placing it before the dphy_exit() call. Since this
reset signal is optional for RZ/V2H, the call is a no-op on that SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Merged patch#2 and patch#3 to avoid breakage.
 * Updated commit description
v1->v2:
 * Updated commit header and description
 * Moved the code from rzg2l_mipi_dsi_dphy_init() to rzg2l_mipi_dsi_startup()
 * Moved the check before calling reset_control_deassert(), so that it will be
   skipped for RZ/V2H SoC
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
index 9d9f77d8f949..715872130780 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
@@ -484,7 +484,6 @@ static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
 	u32 dphytim1;
 	u32 dphytim2;
 	u32 dphytim3;
-	int ret;
 
 	/* All DSI global operation timings are set with recommended setting */
 	for (i = 0; i < ARRAY_SIZE(rzg2l_mipi_dsi_global_timings); ++i) {
@@ -524,12 +523,6 @@ static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
 	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYTIM2, dphytim2);
 	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYTIM3, dphytim3);
 
-	ret = reset_control_deassert(dsi->rstc);
-	if (ret < 0)
-		return ret;
-
-	fsleep(1000);
-
 	return 0;
 }
 
@@ -541,8 +534,6 @@ static void rzg2l_mipi_dsi_dphy_exit(struct rzg2l_mipi_dsi *dsi)
 
 	dphyctrl0 &= ~(DSIDPHYCTRL0_EN_LDO1200 | DSIDPHYCTRL0_EN_BGR);
 	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYCTRL0, dphyctrl0);
-
-	reset_control_assert(dsi->rstc);
 }
 
 static int rzg2l_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long mode_freq,
@@ -811,6 +802,14 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 	FIELD_MODIFY(DSISETR_MRPSZ, &dsisetr, RZG2L_DCS_BUF_SIZE);
 	rzg2l_mipi_dsi_link_write(dsi, DSISETR, dsisetr);
 
+	if (dsi->rstc) {
+		ret = reset_control_deassert(dsi->rstc);
+		if (ret < 0)
+			goto err_phy;
+
+		fsleep(1000);
+	}
+
 	return 0;
 
 err_phy:
@@ -822,6 +821,7 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 
 static void rzg2l_mipi_dsi_stop(struct rzg2l_mipi_dsi *dsi)
 {
+	reset_control_assert(dsi->rstc);
 	dsi->info->dphy_exit(dsi);
 	pm_runtime_put(dsi->dev);
 }
-- 
2.43.0

Claude review: Improvements on RZ/G2L MIPI DSI driver

[Claude Code Review Bot]

Overall Series Review

Subject: Improvements on RZ/G2L MIPI DSI driver
Author: Biju <biju.das.au@gmail.com>
Patches: 4
Reviewed: 2026-03-31T17:20:05.256307

---

This is a well-structured 3-patch series that brings the RZ/G2L MIPI DSI driver into conformance with the hardware manual (Rev. 1.50, Section 34.4.2.1). The changes are:

1. Move display timing setup to after HS clock start
2. Fix the reset deassertion delay from 1 usec to 1 msec
3. Move CMN_RSTB deassert/assert to the correct points in the startup/stop sequence

The series is logically ordered and the patches are appropriately separated. Patches 1 and 2 carry Fixes tags and Cc: stable, which is appropriate. The overall approach is sound and the code changes are small and targeted.

**One concern spans patches 1 and 3**: the return value of `rzg2l_mipi_dsi_startup()` is silently dropped in `rzg2l_mipi_dsi_atomic_pre_enable()` after patch 1. This was already the case before patch 1 (the original code had `if (ret < 0) return;` which just returned void), but patch 1 makes it more explicit by removing the check entirely. This isn't a regression introduced by this series, but it's worth noting.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm: renesas: rzg2l_mipi_dsi: Move rzg2l_mipi_dsi_set_display_timing()

[Claude Code Review Bot]

Patch Review

**Purpose**: Move `rzg2l_mipi_dsi_set_display_timing()` from `atomic_pre_enable` to `atomic_enable`, placing it after `rzg2l_mipi_dsi_start_hs_clock()` per the hardware manual requirement.

**Review**:

The move itself is correct — display timings are now set after HS clock start in `atomic_enable`, which matches the hardware manual.

Minor observations:

1. **Dropped error handling on `rzg2l_mipi_dsi_startup()`**: The original code had:
   ```c
   ret = rzg2l_mipi_dsi_startup(dsi, mode);
   if (ret < 0)
       return;
   ```
   After the patch, it's simply:
   ```c
   rzg2l_mipi_dsi_startup(dsi, mode);
   ```
   Since `atomic_pre_enable` returns void, the old code just silently returned on error anyway (no error propagation). Dropping the check is arguably cleaner, but if `rzg2l_mipi_dsi_startup()` fails, `atomic_enable` will still proceed to call `rzg2l_mipi_dsi_start_hs_clock()` on an uninitialized PHY. Consider at minimum logging an error, or saving the error state so `atomic_enable` can bail out early. This is a pre-existing issue, not introduced by this patch.

2. **Duplicated connector/crtc/mode lookup**: The mode is looked up in both `atomic_pre_enable` and `atomic_enable` now. This is unavoidable given the bridge callback split, so it's fine, but the mode could alternatively be cached in `struct rzg2l_mipi_dsi` during `pre_enable` to avoid the repetition. Not a blocker.

3. The Fixes tags look correct, targeting the commits that introduced `atomic_pre_enable` and the original driver.

**Verdict**: Looks good. The hardware manual motivation is clear and the change is correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: renesas: rzg2l_mipi_dsi: Increase reset deassertion delay

[Claude Code Review Bot]

Patch Review

**Purpose**: Replace `udelay(1)` with `fsleep(1000)` — increasing from 1 usec to 1 msec per the hardware manual requirement of >= 1 msec after CMN_RSTB deassertion.

**Review**:

This is a straightforward and clearly correct fix. The original `udelay(1)` was three orders of magnitude too short per the hardware manual.

1. **Use of `fsleep(1000)` is appropriate**: `fsleep()` automatically selects between `udelay`, `usleep_range`, and `msleep` based on the duration. For 1000 usec, it will use `usleep_range()`, which is correct for a non-atomic context (this is called from the bridge enable path, which can sleep).

2. The Fixes tag and stable Cc are appropriate — this was a real hardware requirement being violated.

**Verdict**: Clean one-liner fix. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: renesas: rzg2l_mipi_dsi: Fix deassert/assert of CMN_RSTB signal

[Claude Code Review Bot]

Patch Review

**Purpose**: Move `reset_control_deassert()` from `dphy_init` to after Link register writes in `startup()`, and move `reset_control_assert()` from `dphy_exit` to `stop()`, per the hardware manual requirement that CMN_RSTB deassertion happens after Link register configuration.

**Review**:

1. **`reset_control_deassert` moved correctly**: It now sits at the end of `rzg2l_mipi_dsi_startup()` after all the Link register writes (TXSETR, CLSTPTSETR, LPTRNSTSETR, DSISETR), which matches the hardware manual sequence. The `fsleep(1000)` from patch 2 follows it correctly.

2. **NULL check for `dsi->rstc`**: The guard `if (dsi->rstc)` in `rzg2l_mipi_dsi_startup()` is correct since `rstc` is obtained via `devm_reset_control_get_optional_exclusive()` and can be NULL for RZ/V2H.

3. **`reset_control_assert` in `rzg2l_mipi_dsi_stop()`**: The unconditional call:
   ```c
   reset_control_assert(dsi->rstc);
   ```
   is safe because `reset_control_assert(NULL)` is a no-op in the kernel reset framework. However, there's an **asymmetry** with the deassert path which does check `if (dsi->rstc)`. For consistency and clarity, it might be better to also wrap the assert in `if (dsi->rstc)`. This is a style nit, not a bug.

4. **Assert before dphy_exit**: The assert is placed before `dphy_exit()` in `stop()`, which is the reverse of the startup order (dphy_init then deassert). This is correct teardown ordering.

5. **Error path**: If `reset_control_deassert()` fails, the code jumps to `err_phy` which calls `dphy_exit()` and `pm_runtime_put()`. This is correct cleanup.

6. **Missing Fixes tag**: Unlike patches 1 and 2, this patch does not carry a Fixes tag or Cc: stable. If the incorrect ordering of CMN_RSTB relative to Link register writes is a hardware manual violation that could cause real issues, this arguably deserves a Fixes tag too. The cover letter implies this is also a correctness fix based on the same hardware manual section.

**Verdict**: Correct and well-structured. Minor style nit on the assert/deassert symmetry, and the missing Fixes tag should be considered.

---

**Summary**: The series is in good shape. The hardware manual references are clear, the changes are well-motivated, and the code is correct. The main actionable items are:

- Consider adding a Fixes tag to patch 3 if the CMN_RSTB ordering was causing real hardware issues
- Consider adding `if (dsi->rstc)` guard around `reset_control_assert()` in patch 3 for symmetry with the deassert path (cosmetic only, not a bug)

---
Generated by Claude Code Patch Reviewer