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/amdgpu: deduplicate register access and helper routines Date: Wed, 01 Apr 2026 08:02:08 +1000 Message-ID: In-Reply-To: <20260330224503.548284-1-gabrielsousa230@gmail.com> References: <20260330224503.548284-1-gabrielsousa230@gmail.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: drm/amdgpu: deduplicate register access and helper routines Author: Gabriel Almeida Patches: 4 Reviewed: 2026-04-01T08:02:08.456948 --- **NAK.** This patch has good intentions (deduplicating identical helper fun= ctions) but introduces two **functional regressions** and has naming/design= issues that need to be addressed. ### Critical Issues 1. **`common_sw_init()` breaks `soc15.c`**: The `soc15_common_sw_init()` fu= nction calls `xgpu_ai_mailbox_add_irq_id()` (for Vega/AI GPUs), **not** `xg= pu_nv_mailbox_add_irq_id()` (for Navi+ GPUs). The patch does not touch `soc= 15_common_sw_init()` in the diff context, but the shared `common_sw_init()`= unconditionally calls `xgpu_nv_mailbox_add_irq_id()`. This means `soc15.c`= cannot use it =E2=80=94 yet the function is presented as a universal repla= cement. More importantly, `soc15_common_sw_init()` also contains additional= logic (`adev->df.funcs->sw_init()`) that is not in the shared version, so = it is **not** actually a duplicate. 2. **`common_sw_init()` breaks `soc_v1_0.c`**: The existing `soc_v1_0_commo= n_sw_init()` in the tree is simply `return 0;` =E2=80=94 it does **not** ca= ll `xgpu_nv_mailbox_add_irq_id()`. The patch replaces it with `common_sw_in= it()` which adds a spurious `xgpu_nv_mailbox_add_irq_id()` call for soc_v1_= 0 hardware that never had it. This is a **functional change** on hardware t= hat may not support the NV mailbox at all, contradicting the commit message= 's claim of "no functional changes intended." ### Design/Naming Issues 3. **Overly generic names**: `read_indexed_register`, `program_aspm`, and `= common_sw_init` are extremely generic names in the global symbol namespace.= In a codebase as large as amdgpu, these names risk collisions and are hard= to grep for. They should have an `amdgpu_` prefix at minimum (e.g., `amdgp= u_common_read_indexed_register`). 4. **File naming**: `amdgpu_common.c/h` is vague. Consider whether these tr= uly belong together or whether the register helper belongs in the existing = `amdgpu_reg_access.c` and the ASPM helper belongs near the NBIO code. 5. **Unnecessary `#include `** in `amdgpu_common.c` =E2=80= =94 nothing in this file uses module macros. 6. **Missing forward declarations in the header**: `amdgpu_common.h` uses `= struct amdgpu_device` and `struct amdgpu_ip_block` without including or for= ward-declaring them. It works only because every consumer happens to includ= e `amdgpu.h` first, but the header is not self-contained. --- --- Generated by Claude Code Patch Reviewer