Claude review: drm/tyr: Use register! macro

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

Overall Series Review

Subject: drm/tyr: Use register! macro
Author: Deborah Brouwer <deborah.brouwer@collabora.com>
Patches: 30
Reviewed: 2026-03-25T07:08:41.969168

---

This is a well-structured 12-patch series converting the Tyr (Mali GPU) driver's register definitions from ad-hoc `const Register<OFFSET>` values with manual bit manipulation to the kernel's `register!` macro. The conversion is cleanly split: patch 1 handles GPU_CONTROL (the largest change), patches 2-3 add small behavioral refinements, patch 4 does JOB_CONTROL, patch 5 does MMU_CONTROL, patch 6 removes the now-dead custom Register struct, and patches 7-12 incrementally define MMU address-space sub-registers and doorbell block registers.

The series is generally high quality. Register addresses match the panthor C driver reference, the enum/field definitions are well documented, and the split into per-register-page patches makes review tractable. A few issues are noted below.

**Key positive observations:**
- Fixes a pre-existing bug: `L2_PWRTRANS_HI` was at 0x204 (should be 0x224); the new `L2_PWRTRANS(u64) @ 0x220` is correct.
- The 64-bit register pairs (SHADER_PRESENT_LO/HI etc.) are properly unified into single u64 register definitions.
- Good use of `=> bool` for single-bit flags and `?=>` for fallible enum conversions.
- Thorough documentation on each register and field.

**Issues found:**

1. **Patch 1 (GPU_CONTROL)**: The `COHERENCY_ENABLE` register field `l2_cache_protocol_select` is defined as `31:0` (a full 32-bit field) mapped to `CoherencyMode`. The panthor C driver uses only the low bits for this. Using all 32 bits as a `Bounded<u32, 32>` means the field width is 32 bits, but the actual valid values (0, 1, 31) would fit in fewer bits. This works but is unusual – a narrower field (e.g. `4:0`) would be more conventional and match the hardware spec more closely.

2. **Patch 1 (GPU_CONTROL)**: `McuControlMode` `TryFrom<Bounded<u32, 2>>` uses `?=>` (fallible) conversion, but a 2-bit field can only have values 0-3, and all three defined enum variants (0, 1, 2) leave only value 3 without a mapping. This is correct to use `?=>` since 3 is undefined, but worth noting that `McuStatus` covers all 4 values and uses infallible `=>` – the asymmetry is intentional and correct.

3. **Patch 2 (Print GPU_ID)**: The format string changes from `"id"` to `"GPU_ID"` which is a visible log format change. The commit message explains this well, but downstream log parsers (if any) will see a different format.

4. **Patch 3 (Coherency during probe)**: The `io` binding acquires `iomem.access()` but this `io` goes out of scope before `GpuInfo::new()` is called. `GpuInfo::new()` acquires its own `io`. This is fine from a correctness standpoint, but the extra `access()` call is a minor inefficiency.

5. **Patch 7 (MMU AS registers)**: The `MMU_MEMATTR_STAGE1` sub-register is declared with `@ 0x0` – this is a "virtual" register (not actually at address 0x0) used as a bitfield type for the MEMATTR register fields. The comment says "This is not an actual register" which is good, but the `@ 0x0` address is potentially confusing. This appears to be a limitation of the `register!` macro requiring an address.

6. **Patch 8 (MEMATTR)**: The register block for `MEMATTR` is split across two `register!` blocks (one for `TRANSTAB`, then enum/impl definitions, then another `register!` for `MEMATTR` onwards). This breaks the arrayed registers into separate `register!` invocations. This should work but may affect whether the macro can enforce contiguity of the register array.

7. **Patch 11 (TRANSCFG)**: The `InaBits` enum has a very large `TryFrom` match block (25 arms). While correct, this could potentially be simplified with a range check plus offset calculation. However, the explicit match is arguably clearer and more maintainable for a hardware register definition, so this is a style preference not a bug.

8. **Patch 11 (TRANSCFG)**: `AddressSpaceMode::Aarch64_64K = 8` - the value 8 requires a 4-bit field (`3:0`, max value 15). This is fine.

---
Generated by Claude Code Patch Reviewer