From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/tyr: Use register! macro Date: Sun, 12 Apr 2026 10:45:08 +1000 Message-ID: In-Reply-To: <20260409-b4-tyr-use-register-macro-v5-v5-0-8abfff8a0204@collabora.com> References: <20260409-b4-tyr-use-register-macro-v5-v5-0-8abfff8a0204@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: drm/tyr: Use register! macro Author: Deborah Brouwer Patches: 7 Reviewed: 2026-04-12T10:45:08.463527 --- This is a well-structured 6-patch series that converts the Tyr GPU driver's register definitions from hand-written constants with manual bit manipulation to the kernel's typed `register!` macro. The series is organized logically: GPU_CONTROL first (patch 1, the main user), a small behavioral fixup (patch 2), JOB_CONTROL (patch 3), MMU_CONTROL (patch 4), removal of the now-unused custom `Register` struct (patch 5), and adding new DOORBELL_BLOCK registers (patch 6). **Strengths:** - The conversion is thorough and well-documented with doc comments on every register and field. - Typed enums for multi-valued fields prevent invalid values at compile time. - Split 64-bit registers are handled correctly with both the logical 64-bit typed view and explicit LO/HI 32-bit registers. - The series **silently fixes a real bug**: the old `L2_PWRTRANS_HI` was at offset `0x204` which is actually `SHADER_PWRTRANS_HI`; the new code correctly places it at `0x224`. - Good use of `=> bool` for 1-bit flags and `?=>` for fallible enum conversions. **Concerns (minor):** - The bug fix for `L2_PWRTRANS_HI` offset (0x204 -> 0x224) is buried silently inside the large patch 1 rewrite. The v5 changelog mentions "Fix typo in the field name for SHADER_PWRTRANS_HI" which alludes to this but doesn't call out that it was a register address bug. This deserves explicit mention in the commit message. - `read_u64_no_tearing()` is added but never used in this series. The `#![allow(dead_code)]` covers it, but it's worth noting. **Verdict:** The series looks good overall. The register definitions are carefully aligned with the panthor C reference. The approach of defining typed registers with proper field accessors is a clear improvement for readability and maintainability. --- --- Generated by Claude Code Patch Reviewer