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/bridge: imx: Add i.MX93 parallel display format configuration support
Date: Wed, 04 Mar 2026 07:36:19 +1000
Message-ID: <review-patch2-20260303-v6-18-topic-imx93-parallel-display-v11-2-1b03733c8461@pengutronix.de>
In-Reply-To: <20260303-v6-18-topic-imx93-parallel-display-v11-2-1b03733c8461@pengutronix.de>
References: <20260303-v6-18-topic-imx93-parallel-display-v11-0-1b03733c8461@pengutronix.de> <20260303-v6-18-topic-imx93-parallel-display-v11-2-1b03733c8461@pengutronix.de>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the main driver patch. Several observations:

**1. Potential uninitialized variable `val` (minor):**

```c
unsigned int val;

bridge_state =3D drm_atomic_get_new_bridge_state(state, bridge);

switch (bridge_state->output_bus_cfg.format) {
case MEDIA_BUS_FMT_RGB888_1X24:
case MEDIA_BUS_FMT_FIXED:
	val =3D FORMAT_RGB888_TO_RGB888;
	...
	break;
case MEDIA_BUS_FMT_RGB666_1X18:
	val =3D FORMAT_RGB888_TO_RGB666;
	break;
case MEDIA_BUS_FMT_RGB565_1X16:
	val =3D FORMAT_RGB565_TO_RGB565;
	break;
}

regmap_update_bits(pdfc->regmap, IMX93_DISPLAY_MUX_REG, mask, val);
```

The switch has no `default` case. While `atomic_check` should prevent an un=
supported format from reaching this point, the compiler will likely warn ab=
out `val` being potentially used uninitialized. Adding a `default: return;`=
 (or initializing `val =3D FORMAT_RGB888_TO_RGB888;`) would silence the war=
ning and be more defensive.

**2. Missing `nxp,imx91-pdfc` in of_device_id:**

```c
static const struct of_device_id imx93_pdfc_dt_ids[] =3D {
	{ .compatible =3D "nxp,imx93-pdfc", },
	{ /* sentinel */ }
};
```

As noted above, the binding defines `nxp,imx91-pdfc` but the driver doesn't=
 match it. Since the cover letter explicitly states "I added the support fo=
r both," adding `{ .compatible =3D "nxp,imx91-pdfc", }` here would be consi=
stent.

**3. `kmalloc_obj` usage looks correct:**

```c
input_fmts =3D kmalloc_obj(*input_fmts);
```

`*input_fmts` dereferences to `u32`, so this allocates `sizeof(u32)` with d=
efault GFP flags. Correct for a single-element array.

**4. `bus-width` of 16 is accepted but not fully handled:**

The dt-binding allows `bus-width` of 16, 18, or 24. The driver only handles=
 `phy_bus_width =3D=3D 18` specially (in `atomic_enable` to select RGB888-t=
o-RGB666 truncation). If `bus-width` is set to 16 in DT, `pdfc->phy_bus_wid=
th` will be 16 but the driver doesn't act on it at all =E2=80=94 it won't a=
ffect the format selection when the output format is `MEDIA_BUS_FMT_RGB888_=
1X24`. This may be intentional (the downstream bridge's format negotiation =
handles it via `MEDIA_BUS_FMT_RGB565_1X16`), but it's worth confirming.

**5. Kconfig dependency:**

The driver uses `syscon_node_to_regmap()` which comes from `MFD_SYSCON`. Th=
ere's no explicit `select MFD_SYSCON` or `depends on MFD_SYSCON` in the Kco=
nfig entry. This typically works because the parent blk-ctrl driver already=
 selects it, but an explicit dependency would be safer for build-testing sc=
enarios (where `COMPILE_TEST` could enable this driver without the blk-ctrl=
).

**6. Bridge funcs struct naming:**

```c
static const struct drm_bridge_funcs funcs =3D {
```

A generic name like `funcs` works but `imx93_pdfc_bridge_funcs` would be mo=
re consistent with other drivers in the same directory and easier to find i=
n debugging/stack traces.

**7. Overall driver structure is clean.** The use of `devm_drm_bridge_alloc=
`, `dev_err_probe`, `syscon_node_to_regmap` from the parent, and the bridge=
 format negotiation callbacks all follow modern kernel patterns correctly.

---
Generated by Claude Code Patch Reviewer