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: sii9234: use extcon cable detection logic to detect MHL
Date: Thu, 04 Jun 2026 12:19:46 +1000
Message-ID: <review-patch3-20260602-exynos4-sii9234-driver-v4-3-9b2fc38e104a@grimler.se>
In-Reply-To: <20260602-exynos4-sii9234-driver-v4-3-9b2fc38e104a@grimler.se>
References: <20260602-exynos4-sii9234-driver-v4-0-9b2fc38e104a@grimler.se> <20260602-exynos4-sii9234-driver-v4-3-9b2fc38e104a@grimler.se>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Verdict: Acceptable with minor observations.**

**Kconfig dependency (line 539):**
```
depends on EXTCON || !EXTCON
```
This is the standard pattern to express "use extcon if available, compile without it if not" -- `extcon_find_edev_by_node()` and friends have stubs when `CONFIG_EXTCON` is disabled. Note the sii8620 driver uses `select EXTCON` instead, which is a stronger dependency. The `depends on EXTCON || !EXTCON` approach is better here since the driver works fine without extcon in "always on" mode. Good choice.

**`sii9234_extcon_init` (lines 900-934):**
The implementation mirrors sii8620's `sii8620_extcon_init` nearly 1:1, with the improvement of using `dev_err_probe` instead of the manual `-EPROBE_DEFER` check. One observation:

```c
musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
muic = of_get_next_parent(musb);
```

If `of_graph_get_remote_node` returns NULL (no port 1 in DT), `of_get_next_parent(NULL)` returns NULL, so `muic` will be NULL and the function falls through to the "always on" path. This is safe -- same pattern as sii8620.

**Race window in `sii9234_remove` (lines 1003-1017):**

```c
if (ctx->extcon) {
    extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,
                               &ctx->extcon_nb);
    flush_work(&ctx->extcon_wq);
    if (ctx->cable_state > 0)
        sii9234_cable_out(ctx);
} else {
    sii9234_cable_out(ctx);
}
drm_bridge_remove(&ctx->bridge);
```

The ordering is correct: unregister the notifier first (so no new work items get scheduled), then flush any in-flight work, then tear down. This matches the sii8620 pattern. The `drm_bridge_remove` after `cable_out` is also correct.

**Minor observation:** There is a small theoretical race between `flush_work` completing and a final extcon notification sneaking in. Between `extcon_unregister_notifier` returning and the notifier chain lock being fully released, a concurrent `schedule_work` could theoretically be in-flight. However, this is the same pattern as the existing sii8620 driver and hasn't been a problem in practice. Using `cancel_work_sync` instead of `flush_work` would be slightly more robust, as it would also cancel a freshly-queued-but-not-yet-running work item. But since `extcon_unregister_notifier` synchronizes with the notifier chain, this is not a real bug.

**`sii9234_extcon_work` (lines 872-887):**

```c
int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);

if (state == ctx->cable_state)
    return;

ctx->cable_state = state;
```

The `cable_state` field is accessed without locking here, but since this runs from a non-reentrant work queue (system_wq with `schedule_work`, which is serialized for the same work item), and the only other reader is `sii9234_remove` after `flush_work`, this is fine. The `cable_in`/`cable_out` functions internally take `ctx->lock`.

**Probe path (lines 988-998):**

```c
ret = sii9234_extcon_init(ctx);
if (ret < 0)
    return ret;
...
if (!ctx->extcon)
    sii9234_cable_in(ctx);
```

Clean fallback: if no extcon is found, immediately call `cable_in` (original behavior). If extcon is present, cable state will be driven by notifications. One note: there's no initial `extcon_get_state` call after registering the notifier, so if the cable is already plugged in at probe time, the driver relies on a notification arriving. The sii8620 driver also doesn't do an initial poll, so this matches existing behavior. The cable will be detected on next state change. This could be improved in a follow-up but is not a blocker.

**Overall:** This is a well-written series that follows established patterns from the sii8620 driver. The code is correct, has appropriate reviews and testing, and the Kconfig dependency is handled properly. Ready to merge.

---
Generated by Claude Code Patch Reviewer