From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
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: <review-patch1-20260330224503.548284-1-gabrielsousa230@gmail.com>
In-Reply-To: <20260330224503.548284-1-gabrielsousa230@gmail.com>
References: <20260330224503.548284-1-gabrielsousa230@gmail.com> <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

Patch Review

**`amdgpu_common.c` =E2=80=94 `common_sw_init()`:**

```c
int common_sw_init(struct amdgpu_ip_block *ip_block)
{
	struct amdgpu_device *adev =3D ip_block->adev;

	if (amdgpu_sriov_vf(adev))
		xgpu_nv_mailbox_add_irq_id(adev);

	return 0;
}
```

This is **not** a universal replacement:
- `soc15_common_sw_init()` calls `xgpu_ai_mailbox_add_irq_id()` (different =
function) and also has `adev->df.funcs->sw_init()` logic. The patch doesn't=
 replace `soc15_common_sw_init()` in the diff, but the function is misleadi=
ngly named as if it were universal.
- `soc_v1_0_common_sw_init()` is just `return 0;` =E2=80=94 it should **not=
** gain an `xgpu_nv_mailbox_add_irq_id()` call.

**`soc_v1_0.c` change:**
```c
 static int soc_v1_0_common_sw_init(struct amdgpu_ip_block *ip_block)
 {
-	return 0;
+	return common_sw_init(ip_block);
 }
```
This introduces a **behavioral change** on soc_v1_0 hardware =E2=80=94 the =
function now calls `xgpu_nv_mailbox_add_irq_id()` in SR-IOV VF mode, which =
it never did before.

**`read_indexed_register()` and `program_aspm()` deduplication**: These two=
 are genuinely identical across all call sites and the mechanical replaceme=
nt is correct. However, the function names need an `amdgpu_` prefix to avoi=
d namespace pollution.

**`amdgpu_common.h`:**
```c
#ifndef __AMDGPU_COMMON_H__
#define __AMDGPU_COMMON_H__

uint32_t read_indexed_register(struct amdgpu_device *adev,
			       u32 se_num, u32 sh_num, u32 reg_offset);
```
Missing forward declarations for `struct amdgpu_device` and `struct amdgpu_=
ip_block`. Add them or `#include "amdgpu.h"`.

**Recommendation**: Drop the `common_sw_init()` consolidation entirely (it'=
s not actually a common function). Keep the `read_indexed_register` and `pr=
ogram_aspm` deduplication but add proper `amdgpu_` prefixes. Consider placi=
ng them in existing files rather than creating a new catch-all `amdgpu_comm=
on.c`.

---
Generated by Claude Code Patch Reviewer