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: imx8mp-hdmi-tx-connector-fixup: add an hdmi-connector when missing using a DT overlay at boot time
Date: Sun, 12 Apr 2026 13:37:35 +1000
Message-ID: <review-patch8-20260407-drm-lcdif-dbanc-v4-8-247a16e61ef9@bootlin.com>
In-Reply-To: <20260407-drm-lcdif-dbanc-v4-8-247a16e61ef9@bootlin.com>
References: <20260407-drm-lcdif-dbanc-v4-0-247a16e61ef9@bootlin.com> <20260407-drm-lcdif-dbanc-v4-8-247a16e61ef9@bootlin.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Bug in error handling:**

```c
+	err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &ovcs_id, NULL);
+	if (err)
+		err = of_overlay_remove(&ovcs_id);
+
+	return err;
```

This logic is inverted. When `of_overlay_fdt_apply()` **fails** (returns nonzero `err`), the code tries to call `of_overlay_remove()` on the failed overlay. When it **succeeds** (returns 0), the code skips `of_overlay_remove()` and returns 0, which is the intended behavior -- but the error path is wrong. If `of_overlay_fdt_apply()` fails, the overlay was not applied, so calling `of_overlay_remove()` on a potentially uninitialized/invalid `ovcs_id` is incorrect and could cause undefined behavior.

The correct code should be:

```c
	err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &ovcs_id, NULL);
	if (err)
		pr_err("Failed to apply HDMI connector fixup overlay: %d\n", err);

	return err;
```

Or simply `return of_overlay_fdt_apply(...)`.

**Minor nits in commit message**: "device tre" should be "device tree", "isntantiating" should be "instantiating", "abd" should be "and".

**Kconfig**: The `select OF_OVERLAY` and `select DRM_DISPLAY_CONNECTOR` are added to `DRM_IMX8MP_DW_HDMI_BRIDGE` rather than to `DRM_IMX8MP_DW_HDMI_BRIDGE_CONNECTOR_FIXUP`. This means that even if the fixup config is disabled, the parent config still forces `OF_OVERLAY` and `DRM_DISPLAY_CONNECTOR` to be selected. The `OF_OVERLAY` select should probably be in the fixup config instead, since only the fixup needs overlay support. `DRM_DISPLAY_CONNECTOR` is arguably needed for the output_port=1 mode (patch 10) regardless of the fixup, but that's a judgment call.

**Hardcoded DT paths**: The use of hardcoded paths like `/soc@0/bus@32c00000/hdmi@32fd8000` is somewhat fragile but is a known pattern for early init fixups like this (similar to the tilcdc precedent cited). This is acceptable.

---

---
Generated by Claude Code Patch Reviewer