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: renesas: rzg2l_mipi_dsi: Fix deassert/assert of CMN_RSTB signal
Date: Tue, 31 Mar 2026 17:20:06 +1000
Message-ID: <review-patch3-20260330104450.128512-4-biju.das.jz@bp.renesas.com>
In-Reply-To: <20260330104450.128512-4-biju.das.jz@bp.renesas.com>
References: <20260330104450.128512-1-biju.das.jz@bp.renesas.com> <20260330104450.128512-4-biju.das.jz@bp.renesas.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

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