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/v3d: Allocate all resources before enabling the clock Date: Wed, 01 Apr 2026 07:45:10 +1000 Message-ID: In-Reply-To: <20260331-v3d-power-management-v9-2-f52ff87bfd36@igalia.com> References: <20260331-v3d-power-management-v9-0-f52ff87bfd36@igalia.com> <20260331-v3d-power-management-v9-2-f52ff87bfd36@igalia.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 **Status: Looks good with minor notes.** This patch reorders probe so that all resource allocations (register mappin= gs, reset control, clock lookup, IRQ setup, perfmon init, DMA allocation, g= em init) happen before `clk_prepare_enable()`. This is a prerequisite for p= atch 3 where the clock enable moves into the runtime PM resume callback. Key observations: 1. **IRQ init vs. IRQ enable separation**: The patch splits `v3d_irq_init()= ` =E2=80=94 it moves the interrupt clearing and `v3d_irq_enable()` call out= of `v3d_irq_init()` into a new explicit `v3d_irq_enable()` call after the = clock is enabled. The clearing of pending interrupts is moved into `v3d_irq= _enable()`. This is correct since you can't touch HW registers before the c= lock is on, while `request_irq()` itself doesn't need the HW powered. 2. **`v3d_init_hw_state()` made non-static**: The function is moved in `v3d= _gem.c` and its declaration is added to `v3d_drv.h`. There's a spurious ext= ra blank line after the function definition: ```c +void +v3d_init_hw_state(struct v3d_dev *v3d) +{ + v3d_init_core(v3d, 0); +} + + static void ``` Very minor formatting nit =E2=80=94 two blank lines instead of one. 3. **Remove order in `v3d_platform_drm_remove()`**: The patch correctly reo= rders removal to power off SMS and disable clock *before* destroying gem an= d freeing DMA =E2=80=94 the reverse of the new probe order. Good. 4. **Error path labels**: The error cleanup path now has `clk_disable` plac= ed between `irq_disable` and `gem_destroy`, matching the reverse allocation= order. Correct. --- --- Generated by Claude Code Patch Reviewer