Claude review: Re: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems

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

Patch Review

Superseded by v5. Combined everything into a single patch. Still introduces `ASIC_IS_DCE31()` in `radeon.h` but only uses it in the encoder picker — other uses are `dmi_match()`.

### PATCH 6 (LATEST): [PATCH v5] drm/radeon: fix internal display on iMac11,1 (RV770/DCE3.1)

This is the version that matters. Three changes across two files:

**1. atombios_crtc.c — PLL fractional feedback divider quirk**

```c
+#include <linux/dmi.h>
```
Correct — needed for `dmi_match()`, not previously included in this file.

```c
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 165000)
+		if ((ASIC_IS_DCE32(rdev) || dmi_match(DMI_PRODUCT_NAME, "iMac11,1"))
+		    && mode->clock > 165000)
 			radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
```
This enables the fractional feedback divider for high-clock modes on the iMac11,1's 2560x1440 panel. The 165000 kHz threshold corresponds to single-link DVI limits. This is correct — the iMac11,1's RV770 (DCE3.1) was excluded from the DCE3.2 check because `CHIP_RV770 < CHIP_RV730` in the enum.

Note: the other DCE32 check at line ~583 (`ASIC_IS_DCE32(rdev) && mode->clock > 200000` for `RADEON_PLL_PREFER_HIGH_FB_DIV`) is intentionally left untouched. This seems reasonable if the iMac11,1 works without the high-FB-div preference.

**2. atombios_encoders.c — DP_VIDEO_ON quirk**

```c
-			if (ASIC_IS_DCE4(rdev))
+			if (ASIC_IS_DCE4(rdev) || dmi_match(DMI_PRODUCT_NAME, "iMac11,1"))
 				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_ON, 0);
```
Enables the DP video-on command for the iMac11,1. Reasonable for initial display bring-up.

**Concern: DP_VIDEO_OFF not patched.** The corresponding `DPMS_OFF` path at line 1727:
```c
		if (ASIC_IS_DCE4(rdev)) {
			if (ENCODER_MODE_IS_DP(atombios_get_encoder_mode(encoder)) && connector)
				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_OFF, 0);
		}
```
is not updated with the same `dmi_match()`. In earlier versions (v3 2/2), both ON and OFF were patched. If the encoder needs an explicit VIDEO_ON to light up, symmetry suggests it should also get VIDEO_OFF when powering down, otherwise the encoder may be left in an inconsistent state on DPMS off or suspend. The author should clarify whether omitting the OFF path is intentional or an oversight.

**3. atombios_encoders.c — Encoder picker restructure**

```c
-	if (ASIC_IS_DCE32(rdev)) {
-		if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") ||
-		    dmi_match(DMI_PRODUCT_NAME, "iMac11,2"))
-			enc_idx = (dig->linkb) ? 1 : 0;
-		else
-			enc_idx = radeon_crtc->crtc_id;
-
+	if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") ||
+	    dmi_match(DMI_PRODUCT_NAME, "iMac11,1") ||
+	    dmi_match(DMI_PRODUCT_NAME, "iMac11,2")) {
+		enc_idx = (dig->linkb) ? 1 : 0;
 		goto assigned;
 	}
+
+	/* on DCE32 and encoder can driver any block so just crtc id */
+	if (ASIC_IS_DCE32(rdev)) {
+		enc_idx = radeon_crtc->crtc_id;
+		goto assigned;
+	}
```

This restructuring is the cleanest part of the patch. It:
- Separates the Apple DMI quirk from the DCE version check
- Adds iMac11,1 to the quirk list
- Preserves identical behavior for all existing paths:
  - Apple iMac10,1/iMac11,2 (DCE3.2): still gets linkb (now matched by DMI before the DCE32 check)
  - Non-Apple DCE3.2: still gets `crtc_id`
  - iMac11,1 (DCE3.1): **now correctly gets linkb** (previously fell through to DCE3 LVTMA logic)
  - Non-Apple DCE3.1: unchanged (DMI fails, DCE32 fails, falls to DCE3 logic)

**Minor issue: typo in comment.** The new comment reads:
```c
	/* on DCE32 and encoder can driver any block so just crtc id */
```
Should be: `/* on DCE32 any encoder can drive any block so just use crtc id */`

**Other observations:**

- **Missing Fixes/Cc-stable tags**: This fixes a real dark-display regression. A `Cc: stable@vger.kernel.org` tag would be appropriate. A `Fixes:` tag pointing to the commit that introduced the DCE32 encoder picker logic would help stable backports.

- **No `atombios_set_edp_panel_power()` change**: The v5 does not extend the eDP panel power function to iMac11,1. The earlier v3 series included this as patch 2/2 for suspend/resume. The commit message no longer mentions suspend/resume, so this is apparently deferred. This is a reasonable scoping decision — fix initial display first, tackle suspend/resume separately.

- **The ASIC_IS_DCE31 macro is no longer added**: v5 dropped this entirely in favor of targeted `dmi_match()` calls, which is the safer approach for a legacy driver where broad DCE3.1 changes could regress other hardware.

**Summary**: v5 is a well-targeted, low-risk quirk fix. The encoder picker restructure is clean and correct. Two items need attention before merge: the comment typo, and clarification on whether the missing `DP_VIDEO_OFF` dmi_match is intentional.

---
Generated by Claude Code Patch Reviewer