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

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

Patch Review

**This is the core patch. Several observations:**

1. **Pre-existing EDID leak in `no_hpd` detect path (line 927-931):**
   ```c
   drm_edid = drm_edid_read_ddc(connector, bridge->ddc);
   if (drm_edid)
       return connector_status_connected;
   ```
   The `drm_edid` is never freed. This is a pre-existing bug, but since this patch changes the function signature and touches surrounding code, it would be good to fix it here with a `drm_edid_free(drm_edid)` before the return.

2. **`curr_conn` lifetime concerns:** `hdmi->curr_conn` is set in `atomic_enable` and cleared in `atomic_disable`. The `scramb_work` delayed work accesses `hdmi->curr_conn` (line 910 via `dw_hdmi_qp_scramb_work` → `dw_hdmi_qp_set_scramb`). While `cancel_delayed_work_sync` is called in `disable_scramb`, there's a window: if the work fires and calls `dw_hdmi_qp_set_scramb()` which calls `schedule_delayed_work()` *after* the cancel but before `scramb_enabled = false` — actually no, `cancel_delayed_work_sync` waits for completion, and `scramb_enabled` is set to false before cancel. Wait, looking at the order in `dw_hdmi_qp_disable_scramb()`:

   ```c
   hdmi->scramb_enabled = false;
   cancel_delayed_work_sync(&hdmi->scramb_work);
   ```

   The work function `dw_hdmi_qp_scramb_work` doesn't check `scramb_enabled`, so if the work is currently executing when `disable_scramb` sets `scramb_enabled = false`, the work could re-schedule itself via `dw_hdmi_qp_set_scramb()` → `schedule_delayed_work()` *before* `cancel_delayed_work_sync` completes. `cancel_delayed_work_sync` will wait for the currently-running work to finish, but if the work re-schedules itself, the cancellation might not catch the newly scheduled instance. This is a potential race. Consider checking `scramb_enabled` in `dw_hdmi_qp_scramb_work()` before re-scheduling, or using a different cancellation pattern.

3. **`SCDC_MIN_SOURCE_VERSION` naming:** The constant is named `SCDC_MIN_SOURCE_VERSION` with value `0x1`, but it's used as the maximum version the source supports (line 922-923: `min_t(u8, ver, SCDC_MIN_SOURCE_VERSION)`). The `min_t` clamps the source version to at most `0x1`. The naming is confusing — something like `SCDC_MAX_SOURCE_VERSION` would be clearer.

4. **Error handling in `dw_hdmi_qp_set_scramb` (lines 795-799):** Neither `drm_scdc_set_high_tmds_clock_ratio()` nor `drm_scdc_set_scrambling()` have their return values checked. If these I2C writes fail (e.g., due to a flaky connection), scrambling is assumed to be configured but it isn't. At minimum, log a warning on failure.

5. **`dw_hdmi_qp_reset_crtc` error propagation:** When called from `dw_hdmi_qp_bridge_detect()` (line 940), the return value of `dw_hdmi_qp_reset_crtc()` is ignored. If the CRTC reset fails, the function still returns `connector_status_connected`. This seems intentional (detection succeeded, reset is best-effort), but worth a comment.

6. **`dw_hdmi_qp_enable_scramb` called before PHY init:** In `atomic_enable`, `dw_hdmi_qp_enable_scramb()` is called (line 851) *before* `hdmi->phy.ops->init()` (line 859). The `enable_scramb` function does I2C writes to the sink via SCDC and writes to hardware register `SCRAMB_CONFIG0`, then sleeps 1ms. The PHY init happens after. Is this ordering correct? The HDMI 2.0 spec requires the source to stop TMDS transmission, configure scrambling, then resume — so doing scramb setup before PHY init (which presumably starts TMDS) seems correct.

7. **`HDMI20_MAX_TMDSRATE` value of 600 MHz:** HDMI 2.0 specifies a maximum of 600 MHz TMDS character rate (corresponding to 18 Gbps). This is correct.

---
Generated by Claude Code Patch Reviewer