From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Read real-time clock frequencies Date: Sun, 12 Apr 2026 14:21:09 +1000 Message-ID: In-Reply-To: <20260406220526.4027917-1-lizhi.hou@amd.com> References: <20260406220526.4027917-1-lizhi.hou@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: accel/amdxdna: Read real-time clock frequencies Author: Lizhi Hou Patches: 2 Reviewed: 2026-04-12T14:21:09.757535 --- This is a single patch that bundles two logically distinct changes: 1. **Refactoring `hw_ops` from an embedded struct to a const pointer** =E2= =80=94 enabling sharing of the ops struct across NPU generations and making= `npu4_set_dpm` static. 2. **Adding a new `update_counters` callback** that reads real-time clock f= requencies from the AMD PMF (Platform Management Framework) interface. The refactoring is clean and well-motivated: npu5 and npu6 already reused `= npu4_set_dpm`, so sharing a single `npu4_hw_ops` const struct is the right = consolidation. However, kernel convention strongly favors splitting these i= nto at least two patches (refactor first, new feature second) for easier bi= section and review. The core concern is the **field name mapping** in `npu4_update_counters` be= tween PMF metrics fields and driver fields, which is confusing and needs ex= plicit justification. There are also issues around silent error suppression= and a missing `max_tops` update in the new callback. **Overall: Needs revision.** The approach is sound but the naming confusion= warrants clarification, the error handling in the macro should be addresse= d, and the patch should ideally be split. --- --- Generated by Claude Code Patch Reviewer