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/rcar-du: dsc: Add rudimentary Renesas R-Car V4H DSC driver
Date: Sat, 16 May 2026 09:46:52 +1000
Message-ID: <review-patch3-20260515-rcar-du-dsc-v2-3-f6b9240a1240@ideasonboard.com>
In-Reply-To: <20260515-rcar-du-dsc-v2-3-f6b9240a1240@ideasonboard.com>
References: <20260515-rcar-du-dsc-v2-0-f6b9240a1240@ideasonboard.com> <20260515-rcar-du-dsc-v2-3-f6b9240a1240@ideasonboard.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

**Status: Needs minor fixes**

**Error handling in atomic_enable:**

```c
+static void rcar_dsc_atomic_enable(struct drm_bridge *bridge,
+				   struct drm_atomic_state *state)
+{
+	struct rcar_dsc *dsc =3D bridge_to_rcar_dsc(bridge);
+
+	WARN_ON(clk_prepare_enable(dsc->clk));
+	WARN_ON(reset_control_deassert(dsc->rst));
+}
```

If `clk_prepare_enable()` fails, the code still proceeds to deassert the re=
set. The reset deassert will also likely fail or leave things in an inconsi=
stent state. Consider returning early on clock failure:

```c
if (WARN_ON(clk_prepare_enable(dsc->clk)))
    return;
WARN_ON(reset_control_deassert(dsc->rst));
```

**Bridge reference for next_bridge:**

```c
+	dsc->bridge.next_bridge =3D of_drm_find_and_get_bridge(remote);
+	of_node_put(remote);
+	if (!dsc->bridge.next_bridge)
+		return -EPROBE_DEFER;
```

The use of `of_drm_find_and_get_bridge()` with direct assignment to `bridge=
.next_bridge` is correct =E2=80=94 the bridge core will put the reference o=
n bridge destruction. Using `devm_drm_of_get_bridge()` here would cause a d=
ouble-put. Good.

However, returning `-EPROBE_DEFER` unconditionally for NULL means a DT misc=
onfiguration (e.g. output port pointing to a non-bridge node) would cause i=
nfinite probe deferral. This is a common pattern in DRM drivers though, so =
it's acceptable, just worth being aware of.

**Unused mmio mapping:**

```c
+	dsc->mmio =3D devm_platform_ioremap_resource(pdev, 0);
```

The `mmio` is mapped but never read or written since the driver only operat=
es in bypass mode. This is fine for correctness (it validates the resource =
exists), but could also be deferred to when DSC encoding support is added. =
Not a blocking issue.

**Kconfig structure:**

The two-config pattern (`DRM_RCAR_USE_DSC` bool + `DRM_RCAR_DSC` def_trista=
te) matches the existing pattern used by LVDS and MIPI DSI in this director=
y. The `select RESET_CONTROLLER` is appropriate.

---

---
Generated by Claude Code Patch Reviewer