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: The Apple iMac11, 1 (late 2009) has an integrated ATI Mobility Radeon HD 4850. This machine suffers from a similar problem as the iMac10, 1 (late 2009) and the iMac11, 2 (mid 2010). This small patch fixes the issue on this machine.
Date: Sat, 16 May 2026 15:26:37 +1000
Message-ID: <review-patch1-20260510185426.4264-1-gilles.risch@gmail.com>
In-Reply-To: <20260510185426.4264-1-gilles.risch@gmail.com>
References: <20260510185426.4264-1-gilles.risch@gmail.com> <20260510185426.4264-1-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

**Commit message formatting issues:**

- The subject line contains the entire commit description, which is against=
 kernel patch conventions. It should be a short imperative line like `drm/r=
adeon: Fix display on iMac11,1 with HD 4850` with details in the body.
- **Missing `Signed-off-by:` line** =E2=80=94 required by the kernel's Deve=
loper Certificate of Origin policy. The patch cannot be accepted without it.
- The `Fixes:` tag format is wrong. It references a freedesktop.org xf86-vi=
deo-ati issue, not a kernel commit. If a kernel commit introduced the regre=
ssion, that commit hash should be cited with the standard `Fixes: <hash> ("=
subject")` format.

**Incorrect macro definition =E2=80=94 the central problem:**

```c
#define ASIC_IS_DCE31(rdev) ((rdev->family >=3D CHIP_RV770))
#define ASIC_IS_DCE32(rdev) ((rdev->family >=3D CHIP_RV730))
```

>From `radeon_family.h`, the enum ordering is:
```
CHIP_RS780  =3D 72  (DCE 3.1)
CHIP_RS880  =3D 73  (DCE 3.1)
CHIP_RV770  =3D 74  (DCE 3.2)
CHIP_RV730  =3D 75  (DCE 3.2)
```

The new `ASIC_IS_DCE31` defined as `>=3D CHIP_RV770` does **not** match any=
 actual DCE 3.1 hardware (RS780, RS880). It only matches DCE 3.2 hardware =
=E2=80=94 it is effectively a corrected version of `ASIC_IS_DCE32`. The nam=
ing is misleading and will confuse future maintainers.

The real bug is that `ASIC_IS_DCE32` itself is wrong: it should be `>=3D CH=
IP_RV770` instead of `>=3D CHIP_RV730`, since RV770 is DCE 3.2 hardware. Th=
e correct fix is to fix `ASIC_IS_DCE32` directly, not introduce a new macro=
 with a confusing name. This would also fix the other ~7 callsites in `r600=
.c` and `radeon_audio.c` that currently exclude RV770 from DCE 3.2 logic.

**PLL changes are broader than necessary:**

```c
if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)	/* range limits??? */
    radeon_crtc->pll_flags |=3D RADEON_PLL_PREFER_HIGH_FB_DIV;
```

and:

```c
if (ASIC_IS_DCE31(rdev) && mode->clock > 165000)
    radeon_crtc->pll_flags |=3D RADEON_PLL_USE_FRAC_FB_DIV;
```

These changes affect **all** RV770 systems, not just Apple iMac11,1. Since =
RV770 genuinely is DCE 3.2 hardware, these PLL flags arguably should apply =
to it =E2=80=94 but this represents a behavior change for all RV770 users a=
nd should be explicitly acknowledged and tested. It's worth noting that lin=
e 588 already uses `rdev->family < CHIP_RV770` as a boundary, suggesting th=
e code already knows RV770 belongs with the newer chips.

**Encoder selection change is correctly scoped:**

```c
if (ASIC_IS_DCE31(rdev)) {
    if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") ||
        dmi_match(DMI_PRODUCT_NAME, "iMac11,1") ||
        dmi_match(DMI_PRODUCT_NAME, "iMac11,2"))
        enc_idx =3D (dig->linkb) ? 1 : 0;
```

This part is fine =E2=80=94 the DMI matching gates the Apple-specific quirk=
 so only the listed machines are affected. Adding `iMac11,1` here is the ri=
ght thing to do. The comment update is also appropriate.

**Recommended approach for a v2:**

1. Fix `ASIC_IS_DCE32` to be `>=3D CHIP_RV770` instead of creating a new `A=
SIC_IS_DCE31` macro. This fixes the root cause (RV770 being wrongly exclude=
d from DCE 3.2) and cleans up all callsites at once.
2. Add the `iMac11,1` DMI match in `atombios_encoders.c` =E2=80=94 this is =
unambiguously correct.
3. Use a proper short subject line: `drm/radeon: Fix display on Apple iMac1=
1,1 (HD 4850/RV770)`.
4. Add `Signed-off-by:`.
5. Write a proper commit message explaining that `ASIC_IS_DCE32` incorrectl=
y excluded RV770 due to the enum ordering.

---
Generated by Claude Code Patch Reviewer