Claude review: drm/amdgpu: deduplicate register access and helper routines

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

Overall Series Review

Subject: drm/amdgpu: deduplicate register access and helper routines
Author: Gabriel Almeida <gabrielsousa230@gmail.com>
Patches: 4
Reviewed: 2026-04-01T08:02:08.456948

---

**NAK.** This patch has good intentions (deduplicating identical helper functions) 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()` function calls `xgpu_ai_mailbox_add_irq_id()` (for Vega/AI GPUs), **not** `xgpu_nv_mailbox_add_irq_id()` (for Navi+ GPUs). The patch does not touch `soc15_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 — yet the function is presented as a universal replacement. 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_common_sw_init()` in the tree is simply `return 0;` — it does **not** call `xgpu_nv_mailbox_add_irq_id()`. The patch replaces it with `common_sw_init()` 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 that 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., `amdgpu_common_read_indexed_register`).

4. **File naming**: `amdgpu_common.c/h` is vague. Consider whether these truly 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 <linux/module.h>`** in `amdgpu_common.c` — 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 forward-declaring them. It works only because every consumer happens to include `amdgpu.h` first, but the header is not self-contained.

---

---
Generated by Claude Code Patch Reviewer