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: Move rzg2l_mipi_dsi_set_display_timing()
Date: Tue, 31 Mar 2026 17:20:05 +1000
Message-ID: <review-patch1-20260330104450.128512-2-biju.das.jz@bp.renesas.com>
In-Reply-To: <20260330104450.128512-2-biju.das.jz@bp.renesas.com>
References: <20260330104450.128512-1-biju.das.jz@bp.renesas.com> <20260330104450.128512-2-biju.das.jz@bp.renesas.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

**Review**:

The move itself is correct =E2=80=94 display timings are now set after HS c=
lock start in `atomic_enable`, which matches the hardware manual.

Minor observations:

1. **Dropped error handling on `rzg2l_mipi_dsi_startup()`**: The original c=
ode had:
   ```c
   ret =3D 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 retur=
ned on error anyway (no error propagation). Dropping the check is arguably =
cleaner, but if `rzg2l_mipi_dsi_startup()` fails, `atomic_enable` will stil=
l 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 `atomi=
c_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 c=
ached in `struct rzg2l_mipi_dsi` during `pre_enable` to avoid the repetitio=
n. Not a blocker.

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

**Verdict**: Looks good. The hardware manual motivation is clear and the ch=
ange is correct.

---
Generated by Claude Code Patch Reviewer