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/tyr: replace fixed sleeps with read_poll_timeout
Date: Mon, 09 Mar 2026 08:02:34 +1000
Message-ID: <review-patch1-20260307204708.60398-1-iprintercanon@gmail.com>
In-Reply-To: <20260307204708.60398-1-iprintercanon@gmail.com>
References: <20260307204708.60398-1-iprintercanon@gmail.com> <20260307204708.60398-1-iprintercanon@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

**Positive aspects:**
- Commit message is well-written with clear rationale and per-function time=
out justification.
- The conversion correctly uses the `read_poll_timeout` API, matching its s=
ignature (`op`, `cond`, `sleep_delta`, `timeout_delta`).
- Error code change from `EIO` to `ETIMEDOUT` is appropriate (and the commi=
t message correctly notes no callers inspect the specific error code).

**Issues:**

1. **Polling intervals differ from what landed upstream.** For `l2_power_on=
`, the patch uses:
   ```rust
   Delta::from_micros(100),
   Delta::from_millis(20),
   ```
   The commit message says this "matches the C panthor driver," which may b=
e true for the C driver's values, but the version that landed in drm-next u=
ses `Delta::from_millis(1)` / `Delta::from_millis(100)`. The 100=C2=B5s pol=
l interval is quite aggressive and may cause unnecessary CPU overhead on sl=
ow hardware. On the other hand, 20ms total timeout could be too tight for s=
ome platforms. The maintainer clearly chose more conservative values.

2. **`.map(|_| ())` pattern vs `?` + `Ok(())`.** The patch uses:
   ```rust
   .map(|_| ())
   .inspect_err(|_| dev_err!(dev, "soft reset timed out\n"))
   ```
   The version that landed uses the more idiomatic:
   ```rust
   .inspect_err(|_| dev_err!(dev, "GPU reset failed."))?;
   Ok(())
   ```
   The `.map(|_| ())` approach works but is slightly less clear than the `?=
` + `Ok(())` pattern, which makes the error propagation path more explicit =
and is a more common Rust idiom.

3. **Trailing `\n` in error messages.** The patch includes `\n` in `dev_err=
!` strings (e.g., `"soft reset timed out\n"`). The kernel `dev_err!` macro =
already appends a newline, so the explicit `\n` would result in a double ne=
wline. The landed version correctly omits it (e.g., `"GPU reset failed."`).

4. **Lost diagnostic information in `issue_soft_reset`.** The original code=
 printed the raw register value on failure:
   ```rust
   dev_err!(dev, "GPU_INT_RAWSTAT is {}\n",
       regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
   );
   ```
   The patch replaces this with just `"soft reset timed out"`, losing usefu=
l debug information. While `read_poll_timeout` makes it harder to report th=
e last polled value (since it's consumed internally), this is a minor regre=
ssion in debuggability.

**Verdict:** The change itself is correct and addresses a real TODO. Howeve=
r, since this work has already landed in drm-next, this patch is moot. If i=
t were being reviewed for merge, the main feedback would be: fix the double=
-newline issue, consider using `?` + `Ok(())` instead of `.map(|_| ())`, an=
d verify the timeout values are appropriate for all supported platforms.

---
Generated by Claude Code Patch Reviewer