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: Tue, 03 Mar 2026 13:09:10 +1000
Message-ID: <review-patch2-20260302-v6-18-topic-imx93-parallel-display-v10-2-634fe2778c7a@pengutronix.de>
In-Reply-To: <20260302-v6-18-topic-imx93-parallel-display-v10-2-634fe2778c7a@pengutronix.de>
References: <20260302-v6-18-topic-imx93-parallel-display-v10-0-634fe2778c7a@pengutronix.de> <20260302-v6-18-topic-imx93-parallel-display-v10-2-634fe2778c7a@pengutronix.de>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Subject:** `[PATCH v10 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support`

This is the main driver patch. Several issues:

**Bug: `kmalloc_obj` used with dereference of uninitialized pointer**

```c
input_fmts = kmalloc_obj(*input_fmts, GFP_KERNEL);
```

`kmalloc_obj` takes a `VAR_OR_TYPE` as its first argument and uses `typeof(VAR_OR_TYPE)` internally to determine the allocation size. Here `*input_fmts` is a dereference of the `u32 *input_fmts` pointer, which at this point is uninitialized. While `kmalloc_obj` uses only `typeof()` (so the value is never actually read at runtime - the compiler extracts the type at compile time), dereferencing an uninitialized pointer in the source expression is arguably UB by the letter of the C standard, even inside `typeof()`. More practically, this is confusing to read. The more idiomatic form would be:

```c
input_fmts = kmalloc_obj(u32, GFP_KERNEL);
```

or simply using `kmalloc(sizeof(*input_fmts), GFP_KERNEL)` / `kmalloc(sizeof(u32), GFP_KERNEL)` which is the traditional pattern used throughout the DRM bridge subsystem.

**Missing `default` in switch statement in `atomic_enable`:**

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

If the format is something unexpected, `val` is used uninitialized in the `regmap_update_bits()` call below. The `atomic_check` should prevent this, but defensive programming would add a `default:` case (perhaps with a `dev_warn` + `return`). At minimum, initialize `val` to a sane default (e.g., `FORMAT_RGB888_TO_RGB888`).

**`imx93_pdfc_bridge_attach` uses `bridge->encoder` instead of the `encoder` parameter:**

```c
static int
imx93_pdfc_bridge_attach(struct drm_bridge *bridge, struct drm_encoder *encoder,
			 enum drm_bridge_attach_flags flags)
{
	return drm_bridge_attach(bridge->encoder, bridge->next_bridge, bridge, flags);
}
```

The function receives `encoder` as a parameter but passes `bridge->encoder` to `drm_bridge_attach()`. Looking at other drivers in the same directory (e.g., `imx8mp-hdmi-pvi.c`), the convention is to pass the `encoder` parameter directly:

```c
return drm_bridge_attach(encoder, pvi->bridge.next_bridge, bridge, flags);
```

While `bridge->encoder` should be set by the time `attach` is called, using the function parameter `encoder` is the correct and idiomatic pattern. This should be changed.

**Missing `nxp,imx91-pdfc` in the of_device_id table:**

The DT binding in patch 1 documents `nxp,imx91-pdfc` as a valid compatible string, but the driver only matches:
```c
static const struct of_device_id imx93_pdfc_dt_ids[] = {
	{ .compatible = "nxp,imx93-pdfc", },
	{ /* sentinel */ }
};
```

If i.MX91 uses the same register layout (as stated in the cover letter), a second entry `{ .compatible = "nxp,imx91-pdfc", }` should be added. Without it, the driver won't probe on i.MX91-based device trees using the `nxp,imx91-pdfc` compatible.

**Kconfig: Missing `COMPILE_TEST` dependency:**

```
config DRM_IMX93_PARALLEL_DISP_FMT_CONVERTER
	tristate "NXP i.MX91/i.MX93 parallel display format converter"
	depends on OF
```

Other entries in this Kconfig are inside an `if ARCH_MXC || COMPILE_TEST` block, so this inherits that guard. However, the `depends on OF` is redundant since `ARCH_MXC` already selects `OF`. Not a blocker, just a nit consistent with how other entries in the file are structured.

**Verdict:** Needs fixes for the `bridge->encoder` vs `encoder` parameter issue and the missing `nxp,imx91-pdfc` compatible. The `kmalloc_obj(*input_fmts, ...)` usage and missing `default` in the switch are worth addressing too.

---

---
Generated by Claude Code Patch Reviewer