Claude review: Re: [PATCH] drm/amdgpu: use ACK polling for page-write completion

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

Patch Review

**Correctness: Good.** The `read_poll_timeout()` usage is correct:

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

- `op = i2c_transfer`, called as `r = i2c_transfer(i2c_adap, &msgs[0], 1)` — polls with a single-message (offset-only) write, which is the correct ACK polling probe when the adapter doesn't support zero-length transfers.
- `cond = r == 1` — correct: `i2c_transfer` returns the number of messages transferred on success.
- `sleep_us = 200` — internally uses `usleep_range(51, 200)` between polls. Reasonable: at worst ~50 polls over 10 ms.
- `timeout_us = 10 * USEC_PER_MSEC` — preserves the existing 10 ms upper bound.
- `sleep_before_read = false` — first poll is immediate. An initial sleep might save one wasted I2C transaction (the EEPROM is definitely busy 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 `i2c_transfer` return value (typically `-ENXIO` from NACK), which propagates correctly through:

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

**Variable reuse — minor concern.** The macro writes into `r`, which is also the outer for-loop's transfer result variable. This works correctly because:
- On successful poll: `r == 1`, the loop continues, and the next iteration overwrites `r` with the actual two-message transfer result before checking it.
- On last page (loop exits with `buf_size == 0`): `r == 1 >= 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)==2` success checks and `==1` polling success needs to trace through carefully. It's not wrong, but worth being aware of. If the maintainers 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 == 1, ...);
if (ret != 1) {
    r = 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 pointer 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 `Suggested-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 correct, 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