Claude review: drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

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

Patch Review

This patch has the most issues:

**Broken abstraction:**

The `get_aux_send_ctl` callback is changed from returning a `u32` value to `void` with side effects (writing to hardware). This is a poor abstraction change — the function name says "get" but it now "sets". It also tightly couples what was a clean value-computation function to specific register operations.

**PSR code is broken:**

```c
/* Start with bits set for DDI_AUX_CTL register */
intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
                           aux_clock_divider);
```

The old code called `get_aux_send_ctl` to get a value, then masked it to select only PSR-valid bits, and wrote it to the **PSR AUX CTL register** (a *different* register than `ch_ctl`). The new code calls `get_aux_send_ctl` which now writes directly to the **DDI AUX CTL register** as a side effect. Then it does:

```c
intel_de_rmw(display, ch_ctl,
             (EDP_PSR_AUX_CTL_TIME_OUT_MASK | ...),
             aux_ctl);
```

Where `ch_ctl` is `psr_aux_ctl_reg(...)`, but `aux_ctl` is now set to just the mask values themselves — not actual configuration values read from the DDI register. The mask is being used as both the clear-mask and the value, which will just set all those bits to 1. This is clearly wrong.

Additionally, the function now triggers SEND_BUSY on the PSR AUX CTL register:

```c
intel_de_rmw(display, ch_ctl,
             DP_AUX_CH_CTL_SEND_BUSY,
             DP_AUX_CH_CTL_SEND_BUSY);
```

The original code never triggered SEND_BUSY — `hsw_psr_setup_aux` just configures the register for the PSR hardware to use later autonomously. Triggering it here would start an unwanted AUX transaction.

**RMW for status clearing is wrong:**

```c
/* Clear done status and any errors */
intel_de_rmw(display, ch_ctl,
             (DP_AUX_CH_CTL_DONE |
              DP_AUX_CH_CTL_TIME_OUT_ERROR |
              DP_AUX_CH_CTL_RECEIVE_ERROR),
             (DP_AUX_CH_CTL_DONE |
              DP_AUX_CH_CTL_TIME_OUT_ERROR |
              DP_AUX_CH_CTL_RECEIVE_ERROR));
```

`intel_de_rmw` does read-modify-write: `(reg & ~clear) | set`. Since the clear and set masks are the same, this will always set these status bits regardless of their current state. These are W1C (write-1-to-clear) bits in hardware, so you want to write 1 to clear them, but you should only clear the bits that are actually set (which the original `status |` approach did correctly by using the read-back `status` value). By always writing all three, you might clear status bits prematurely or cause unintended behavior.

**`return;` at end of void functions:**

```c
	     value);
	return;
}
```

Unnecessary `return;` at end of void functions (both `g4x_get_aux_send_ctl` and `skl_get_aux_send_ctl`).

**Missing `DP_AUX_CH_CTL_RECEIVE_ERROR` in g4x clear mask:**

In `g4x_get_aux_send_ctl`, the RMW clear mask includes `DP_AUX_CH_CTL_RECEIVE_ERROR` but the value being set also includes it (from the original code). However, the original value computation included `DP_AUX_CH_CTL_RECEIVE_ERROR` which is a W1C bit — it was set in the original `send_ctl` to ensure it gets cleared before the transaction. The RMW approach changes this semantic.

**TBT_IO handling:**

```c
if (intel_tc_port_in_tbt_alt_mode(dig_port))
    intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
                 DP_AUX_CH_CTL_TBT_IO);
```

This only ever sets `DP_AUX_CH_CTL_TBT_IO`, never clears it. If the port transitions out of TBT alt mode between transactions, the bit will remain set. The clear mask and set value are the same, so `rmw` with the same mask/value is just an OR. It should clear `DP_AUX_CH_CTL_TBT_IO` in the main rmw call above and only conditionally set it.

**Dropped `XELPDP_DP_AUX_CH_CTL_POWER_REQUEST` handling:**

The original code explicitly preserved this bit. The new RMW approach should naturally preserve it since it's not in the clear mask, but the cover letter frames this as intentional. This should be called out more clearly in the commit message as it's the key behavioral change, and needs careful validation that no code path depends on the explicit set.

---
Generated by Claude Code Patch Reviewer