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: Re: [PATCH] drm/amdgpu: use ACK polling for page-write completion
Date: Thu, 04 Jun 2026 14:17:39 +1000
Message-ID: <review-patch2-3206d5c3e94f694b4f0a5b5db4dc1ebaaa86aac9@intel.com>
In-Reply-To: <3206d5c3e94f694b4f0a5b5db4dc1ebaaa86aac9@intel.com>
References: <20260601093226.1255621-1-kunal.devanandzodape@amd.com> <3206d5c3e94f694b4f0a5b5db4dc1ebaaa86aac9@intel.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: Good.** The `read_poll_timeout()` usage is correct:

```c
ret =3D read_poll_timeout(i2c_transfer, r,
                         r =3D=3D 1,
                         200, 10 * USEC_PER_MSEC,
                         false,
                         i2c_adap, &msgs[0], 1);
```

- `op =3D i2c_transfer`, called as `r =3D i2c_transfer(i2c_adap, &msgs[0], =
1)` =E2=80=94 polls with a single-message (offset-only) write, which is the=
 correct ACK polling probe when the adapter doesn't support zero-length tra=
nsfers.
- `cond =3D r =3D=3D 1` =E2=80=94 correct: `i2c_transfer` returns the numbe=
r of messages transferred on success.
- `sleep_us =3D 200` =E2=80=94 internally uses `usleep_range(51, 200)` betw=
een polls. Reasonable: at worst ~50 polls over 10 ms.
- `timeout_us =3D 10 * USEC_PER_MSEC` =E2=80=94 preserves the existing 10 m=
s upper bound.
- `sleep_before_read =3D false` =E2=80=94 first poll is immediate. An initi=
al sleep might save one wasted I2C transaction (the EEPROM is definitely bu=
sy right after the write), but the overhead is negligible.

**Error handling: Correct.** After timeout, `read_poll_timeout()` returns `=
-ETIMEDOUT`, the code breaks out of the for loop, and `r` holds the last `i=
2c_transfer` return value (typically `-ENXIO` from NACK), which propagates =
correctly through:

```c
return r < 0 ? r : eeprom_buf - p;
```

**Variable reuse =E2=80=94 minor concern.** The macro writes into `r`, whic=
h is also the outer for-loop's transfer result variable. This works correct=
ly because:
- On successful poll: `r =3D=3D 1`, the loop continues, and the next iterat=
ion overwrites `r` with the actual two-message transfer result before check=
ing it.
- On last page (loop exits with `buf_size =3D=3D 0`): `r =3D=3D 1 >=3D 0`, =
so the byte count `eeprom_buf - p` is returned, which is correct.

However, this coupling is subtle. A reader seeing `r` used for both `ARRAY_=
SIZE(msgs)=3D=3D2` success checks and `=3D=3D1` polling success needs to tr=
ace through carefully. It's not wrong, but worth being aware of. If the mai=
ntainers prefer, the `read_poll_timeout` could use the local `ret` variable=
 instead and set `r` explicitly on error:

```c
read_poll_timeout(i2c_transfer, ret,
                   ret =3D=3D 1, ...);
if (ret !=3D 1) {
    r =3D ret;
    break;
}
```

But this is a style nit, not a correctness issue.

**Comment quality: Good.** The block comment explaining why an offset-only =
dummy write is used instead of a zero-length probe, and why the address poi=
nter update is harmless, is helpful and well-written.

**Include addition: Correct.** `<linux/iopoll.h>` is needed for `read_poll_=
timeout()` and is properly placed before the local headers.

**Commit message: Good.** Clear description of what and why, appropriate `S=
uggested-by` tag, v2 changelog, and test information (MI200/ALDEBARAN with =
`ras_eeprom_reset`).

**Verdict:** v2 looks good to me. The `read_poll_timeout()` usage is correc=
t, error paths are sound, and the approach follows standard EEPROM practice=
. The variable reuse is the only subtle point but works correctly as-is.

---
Generated by Claude Code Patch Reviewer