Claude review: drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling and high TMDS clock ratio support

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the main feature patch and the most complex. Several aspects to examine:

**Struct additions:**
```c
	struct drm_connector *curr_conn;
	struct delayed_work scramb_work;
	bool scramb_enabled;
```

**Concurrency concern with `curr_conn`:** The `curr_conn` pointer is set in `atomic_enable` and cleared in `atomic_disable`. It's used without locking in the delayed work item (`dw_hdmi_qp_scramb_work`) and in `detect_ctx`. There's a potential race:

1. `scramb_work` fires and reads `hdmi->curr_conn`
2. Concurrently, `atomic_disable` runs and sets `hdmi->curr_conn = NULL`
3. The work dereferences a NULL or stale pointer

The `cancel_delayed_work_sync()` in `dw_hdmi_qp_disable_scramb()` should prevent this because it's called before `hdmi->curr_conn = NULL`:

```c
	WRITE_ONCE(hdmi->scramb_enabled, false);
	cancel_delayed_work_sync(&hdmi->scramb_work);
	...
	// (in atomic_disable, after disable_scramb returns)
	hdmi->curr_conn = NULL;
```

The `WRITE_ONCE(false)` + `cancel_delayed_work_sync()` ensures the work won't re-schedule and any in-flight execution completes before `curr_conn` is cleared. This looks correct.

**However**, `curr_conn` is also accessed in `detect_ctx` which runs from the connector detect path (HPD work). There's no synchronization between `detect_ctx` reading `hdmi->scramb_enabled` / `hdmi->curr_conn` and `atomic_disable` clearing them. If a detect fires while a modeset is tearing down, `detect_ctx` could see `scramb_enabled == true` but `curr_conn` could be in-flight being set to NULL. In practice, the connection_mutex should serialize these paths, but it might be worth adding a comment or a `READ_ONCE` for `curr_conn` reads as well, similar to what was done for `scramb_enabled`.

**SCDC rollback logic in `dw_hdmi_qp_set_scramb`:**
```c
	done = drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, true);
	if (!done)
		return -EIO;

	done = drm_scdc_set_scrambling(hdmi->curr_conn, true);
	if (!done) {
		drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, false);
		return -EIO;
	}
```
Good — if scrambling fails, the high TMDS clock ratio is rolled back. The v5 changelog mentions this was added in response to review feedback.

**In `dw_hdmi_qp_enable_scramb`:**
```c
	WRITE_ONCE(hdmi->scramb_enabled, true);

	ret = dw_hdmi_qp_set_scramb(hdmi);
	if (ret) {
		hdmi->scramb_enabled = false;
		return;
	}
```

Minor: The set uses `WRITE_ONCE` but the reset on failure uses a plain store. Since the work item reads with `READ_ONCE`, the reset should also use `WRITE_ONCE` for consistency. Though in practice, at this point the work hasn't been scheduled yet (it's scheduled inside `set_scramb` which failed), so it's not a live bug.

**`dw_hdmi_qp_reset_crtc` in detect_ctx:**
```c
	if (!!(config & SCDC_SCRAMBLING_ENABLE) == hdmi->scramb_enabled)
		return 0;
```
This compares the sink's current SCDC scrambling state with the driver's expectation. If they mismatch (e.g., sink was power-cycled), trigger a CRTC reset. This is the key logic for recovering from sink-side resets.

**`dw_hdmi_qp_bridge_tmds_char_rate_valid` changes:**
```c
	if (hdmi->no_hpd && rate > HDMI14_MAX_TMDSCLK) {
		...
		return MODE_CLOCK_HIGH;
	}

	if (rate > HDMI20_MAX_TMDSRATE) {
		...
		return MODE_CLOCK_HIGH;
	}
```
This correctly resolves the TODO that was in the original code: no_hpd mode is limited to 340 MHz (no SCDC without a connected sink), and the overall cap is 600 MHz (HDMI 2.0 TMDS limit, above which FRL is needed).

**`INIT_DELAYED_WORK` placement:** It's initialized in `dw_hdmi_qp_bind()` before `hdmi->dev` is assigned:
```c
	INIT_DELAYED_WORK(&hdmi->scramb_work, dw_hdmi_qp_scramb_work);

	hdmi->dev = dev;
```
This is fine — `INIT_DELAYED_WORK` only sets up the work struct metadata and the callback pointer, it doesn't use `hdmi->dev`.

**Overall for patch 5:** The design is solid. The main nit is the inconsistent `WRITE_ONCE` on the `scramb_enabled = false` failure path, and it would be good to have a brief comment about the locking assumptions for `curr_conn` access in `detect_ctx` vs `atomic_disable`.

---

---
Generated by Claude Code Patch Reviewer