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: Tue, 31 Mar 2026 17:53:43 +1000 Message-ID: In-Reply-To: <20260328-v3d-power-management-v8-2-94336830df5f@igalia.com> References: <20260328-v3d-power-management-v8-0-94336830df5f@igalia.com> <20260328-v3d-power-management-v8-2-94336830df5f@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, one minor nit.** This patch reorders probe so that all resource allocation (reg mapping, res= et control, clk get, IRQ init, perfmon init, DMA alloc, gem init) happens b= efore the clock is enabled and HW is accessed. This is the right preparatio= n for runtime PM. Key changes: - `v3d_init_hw_state()` and `v3d_mmu_set_page_table()` are moved out of `v3= d_gem_init()` and called explicitly after `clk_prepare_enable()`. `v3d_init= _hw_state()` is made non-static (exported via header). - `v3d_irq_init()` is split: registration happens early, but `v3d_irq_enabl= e()` (which touches HW) is deferred until after the clock is on. The interr= upt clearing is moved from `v3d_irq_init()` into `v3d_irq_enable()`, which = makes sense since it needs HW access. - The remove order is corrected to be the reverse of init (clock disable be= fore gem_destroy, as the gem layer shouldn't need HW at teardown). **Minor nit:** In `v3d_gem.c` after moving `v3d_init_hw_state()`, there's a= double blank line: ```c void v3d_init_hw_state(struct v3d_dev *v3d) { v3d_init_core(v3d, 0); } static void v3d_huge_mnt_init(struct v3d_dev *v3d) ``` One blank line would be sufficient per kernel style. **One concern about the error paths:** After the reorder, there's a potenti= al issue in the probe error cleanup. The `irq_disable` label calls `v3d_irq= _disable(v3d)`, but now falls through to `clk_disable` which calls `v3d_pow= er_off_sms(v3d)` and `clk_disable_unprepare(v3d->clk)`, then to `gem_destro= y` and `dma_free`. This ordering looks correct =E2=80=94 if `drm_dev_regist= er()` fails, we need to disable IRQs, turn off the clock, then clean up gem= /dma. The label ordering in the diff (`irq_disable` =E2=86=92 `clk_disable`= =E2=86=92 `gem_destroy` =E2=86=92 `dma_free`) correctly reverses the init = sequence. --- Generated by Claude Code Patch Reviewer