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/rockchip: dsi: Add dphy_get_timing support for multiple PHY types
Date: Thu, 04 Jun 2026 11:57:42 +1000
Message-ID: <review-patch2-20260603033532.164-3-kernel@airkyi.com>
In-Reply-To: <20260603033532.164-3-kernel@airkyi.com>
References: <20260603033532.164-1-kernel@airkyi.com> <20260603033532.164-3-kernel@airkyi.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: BUG =E2=80=94 NULL pointer dereference on RK3506**

The design is sound: the monolithic `dw_mipi_dsi_phy_get_timing()` is refac=
tored into per-DPHY-type callbacks (`dw_mipi_dsi_phy_rk3288_get_timing` for=
 the table-based RK3288/RK3399 DPHY, and `dw_mipi_dsi_phy_px30_get_timing` =
with fixed values for PX30 and successors). The dispatch function unconditi=
onally dereferences the callback:

```c
static int
dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps,
			   struct dw_mipi_dsi_dphy_timing *timing)
{
	struct dw_mipi_dsi_rockchip *dsi =3D priv_data;

	return dsi->cdata->dphy_get_timing(dsi, lane_mbps, timing);
}
```

**`rk3506_chip_data` does not set `.dphy_get_timing`**, so `dsi->cdata->dph=
y_get_timing` is NULL on RK3506. This will cause a kernel oops. Looking at =
the applied tree at line 1716-1727:

```c
static const struct rockchip_dw_dsi_chip_data rk3506_chip_data[] =3D {
	{
		...
		.max_data_lanes =3D 2,
		.max_bit_rate_per_lane =3D 1500000000UL,
		/* .dphy_get_timing is missing! */
	},
	{ /* sentinel */ }
};
```

Every other chip_data entry has `.dphy_get_timing` set. RK3506 should presu=
mably use `dw_mipi_dsi_phy_px30_get_timing` given it's a newer SoC, consist=
ent with the pattern used for RK3128, RK3368, RK3568, and RV1126.

**Other notes on this patch:**

The forward declaration `struct dw_mipi_dsi_rockchip;` before `struct rockc=
hip_dw_dsi_chip_data` is necessary and correctly placed.

The PX30 fixed timing values (`clk_lp2hs=3D0x40, clk_hs2lp=3D0x40, data_lp2=
hs=3D0x10, data_hs2lp=3D0x14`) are just magic numbers with no explanation o=
f where they come from =E2=80=94 a brief comment referencing the specific h=
ardware documentation or downstream source would help reviewers, but the co=
mmit message does mention "fixed timing configuration" for PX30, so this is=
 minor.

---

---
Generated by Claude Code Patch Reviewer