Claude review: Re: [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)

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

Patch Review

**Correctness: Mostly good, with a logical redundancy.**

**Panel power guard in atombios_set_edp_panel_power():**

```c
-	if (!ASIC_IS_DCE4(rdev))
+	if (!ASIC_IS_DCE31(rdev))
 		goto done;
```

This is the key fix — without it, the eDP panel power-on sequence is skipped entirely for DCE3.1. The connector type check (`!= DRM_MODE_CONNECTOR_eDP`) above this line already gates entry, so extending to DCE3.1 won't affect non-eDP systems. The author reports HPD polling works correctly on iMac11,1.

**DP video on/off changes:**

```c
-			if (ASIC_IS_DCE4(rdev))
+			if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev))
 				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_ON, 0);
```

```c
-		if (ASIC_IS_DCE4(rdev)) {
+		if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)) {
```

**Issue: Logical redundancy.** Since `ASIC_IS_DCE31()` is `>= CHIP_RV770` and `ASIC_IS_DCE4()` is `>= CHIP_CEDAR`, and `CHIP_CEDAR > CHIP_RV770` in the enum, every chip matching DCE4 also matches DCE31. Therefore `ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)` simplifies to just `ASIC_IS_DCE31(rdev)`. The `ASIC_IS_DCE4` term is redundant.

However, this has a subtle semantic consequence: the original code only sent `DP_VIDEO_ON`/`DP_VIDEO_OFF` for DCE4+. By changing to `ASIC_IS_DCE31(rdev)` (even with the redundant `||`), this now also sends these commands for **DCE3.2** hardware (RV730, RV710, RV740) — not just RV770. The author should confirm this is intentional and safe for DCE3.2 systems, or narrow the condition if only RV770 needs it.

**Other issues:**

1. **Missing `Fixes:` tag** — same as patch 2.

2. **Missing `Signed-off-by` chain context.** This patch depends on patch v2 (it uses `ASIC_IS_DCE31` introduced there and its `diff --git` index line `4e984973c043` matches the post-v2 state). The dependency should be explicitly stated via an `In-Reply-To:` to the v2 patch or a cover letter, and the two should be submitted as a proper series (`[PATCH v2 1/2]` and `[PATCH v2 2/2]`).

3. **The `ATOM_ENCODER_CMD_DP_VIDEO_ON/OFF` on DCE3.1** — the author should confirm the ATOM BIOS table on RV770 actually implements these encoder commands. If the VBIOS table doesn't handle them, `atom_execute_table()` might silently succeed or could misbehave. The author says it's tested on iMac11,1, which is good, but broader DCE3.1/3.2 implications should be considered.

---
Generated by Claude Code Patch Reviewer