From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
Date: Mon, 18 May 2026 16:57:04 +1000
Message-ID: <review-patch2-20260516092420.3579-2-gilles.risch@gmail.com>
In-Reply-To: <20260516092420.3579-2-gilles.risch@gmail.com>
References: <20260510185426.4264-1-gilles.risch@gmail.com> <20260516092420.3579-2-gilles.risch@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness: Looks correct.** The new macro is well-placed and the logic =
follows the existing DCE versioning scheme.

**Issue 1 =E2=80=94 Missing `Fixes:` tag:** The commit message references "=
Fixes freedesktop issue 164" in prose, but does not include a kernel-style =
`Fixes:` tag pointing to the commit that introduced the regression (or the =
original DCE3.2 logic that omitted RV770). If this is a fix for existing be=
havior, it should have:
```
Fixes: <sha1> ("drm/radeon: ...")
Cc: stable@vger.kernel.org
```
If there's no single commit to blame (the logic was always wrong), a `Cc: s=
table` is still appropriate since this is a hardware enablement fix for a l=
ongstanding bug.

**Issue 2 =E2=80=94 PLL flag changes affect all RV770 systems, not just iMa=
c:**

```c
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 200000)
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)
```

This change means *every* RV770-based system (not just the iMac11,1) will n=
ow get `RADEON_PLL_PREFER_HIGH_FB_DIV` for high clocks and `RADEON_PLL_USE_=
FRAC_FB_DIV` for clocks > 165000. While this is likely the correct behavior=
 for DCE3.1 hardware in general, the commit message frames this purely as a=
n iMac fix. The author should confirm this is intentional for all RV770 car=
ds, or if it should be gated behind a DMI check. Given that line 588 alread=
y has `if (rdev->family < CHIP_RV770)` as a boundary, this is likely fine =
=E2=80=94 but it should be stated explicitly in the commit message.

**Issue 3 =E2=80=94 Audit of remaining `ASIC_IS_DCE32` call sites:** There =
are additional `ASIC_IS_DCE32()` users in `r600.c` (interrupt handling, HDM=
I, HPD) and `radeon_audio.c`. The author should confirm these do not also n=
eed the DCE3.1 treatment. If they are truly DCE3.2-specific features, the c=
ommit message should note this.

**Minor =E2=80=94 Subject line wrapping:** The subject line appears wrapped=
 across multiple header lines in the mbox (`Subject: [PATCH v2] drm/radeon:=
 fix internal display on iMac11,\n 1 (RV770/DCE3.1)`). This may be an email=
 client artifact, but the author should verify `git log --oneline` shows it=
 correctly.

---

---
Generated by Claude Code Patch Reviewer