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 JPEG v5.0 interrupt routine Date: Thu, 23 Apr 2026 07:54:54 +1000 Message-ID: In-Reply-To: <20260421200236.30927-1-andrebueno.mac@gmail.com> References: <20260421200236.30927-1-andrebueno.mac@gmail.com> <20260421200236.30927-1-andrebueno.mac@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 **Co-developed-by / Signed-off-by ordering is wrong.** The patch has: ``` Signed-off-by: Andre Luiz Batista Bueno Co-developed-by: Enzo Furegatti Spinella Signed-off-by: Enzo Furegatti Spinella ``` Per `Documentation/process/submitting-patches.rst`, "the last Signed-off-by= : must always be that of the developer submitting the patch." Since Andre i= s the `From:` author and submitter, it should be: ``` Co-developed-by: Enzo Furegatti Spinella Signed-off-by: Enzo Furegatti Spinella Signed-off-by: Andre Luiz Batista Bueno ``` **Code review =E2=80=94 the extraction is correct.** The new `jpeg_v5_0_pro= cess_interrupt_common()` in `jpeg_v5_0_interrupt.c` is an exact copy of the= duplicated logic. Both callers in `jpeg_v5_0_1.c` and `jpeg_v5_0_2.c` are = converted to thin wrappers that correctly drop the unused `source` paramete= r: ```c static int jpeg_v5_0_1_process_interrupt(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) { return jpeg_v5_0_process_interrupt_common(adev, entry); } ``` This preserves the `.process` callback signature while the common function = only takes what it actually uses. Fine. **Header is well-structured.** The include guard, forward declarations, and= prototype are all correct: ```c struct amdgpu_device; struct amdgpu_iv_entry; int jpeg_v5_0_process_interrupt_common(struct amdgpu_device *adev, struct amdgpu_iv_entry *entry); ``` **Naming observation.** The file is named `jpeg_v5_0_interrupt` which could= imply it covers all v5.0.x variants, but `jpeg_v5_0_0.c` and `jpeg_v5_3_0.= c` have substantially different interrupt handlers =E2=80=94 they are singl= e-instance, don't use `node_id_to_phys_map`, and only handle `VCN_5_0__SRCI= D__JPEG_DECODE` (one ring, not ten). So this common function is only applic= able to the multi-instance variants (v5_0_1 and v5_0_2). The name isn't wro= ng, but something like `jpeg_v5_0_multi_interrupt` or placing the function = in an existing common file might be clearer. This is a minor nit. **Missed opportunity.** The 10-case switch mapping sequential `JPEG[N]_DECO= DE` source IDs to `ring_dec[N]` could potentially be replaced with arithmet= ic if the SRCID values are sequential, but that's a separate improvement an= d would change behavior during a refactoring patch, so keeping the switch a= s-is is the correct choice here. **Makefile change is correct.** The new object file is added in alphabetica= l order in the right section. **Verdict:** The code change is correct. The `Co-developed-by` / `Signed-of= f-by` tag ordering needs to be fixed before merging. --- Generated by Claude Code Patch Reviewer