public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
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	[thread overview]
Message-ID: <review-patch2-20260331-v3d-power-management-v9-2-f52ff87bfd36@igalia.com> (raw)
In-Reply-To: <20260331-v3d-power-management-v9-2-f52ff87bfd36@igalia.com>

Patch Review

**Status: Looks good with minor notes.**

This patch reorders probe so that all resource allocations (register mappings, reset control, clock lookup, IRQ setup, perfmon init, DMA allocation, gem init) happen before `clk_prepare_enable()`. This is a prerequisite for patch 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()` — 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 clock 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 extra 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 — two blank lines instead of one.

3. **Remove order in `v3d_platform_drm_remove()`**: The patch correctly reorders removal to power off SMS and disable clock *before* destroying gem and freeing DMA — the reverse of the new probe order. Good.

4. **Error path labels**: The error cleanup path now has `clk_disable` placed between `irq_disable` and `gem_destroy`, matching the reverse allocation order. Correct.

---

---
Generated by Claude Code Patch Reviewer

  reply	other threads:[~2026-03-31 21:45 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-31 12:35 [PATCH v9 0/3] Power Management for Raspberry Pi V3D GPU Maíra Canal
2026-03-31 12:35 ` [PATCH v9 1/3] drm/v3d: Use devm_reset_control_get_optional_exclusive() Maíra Canal
2026-03-31 21:45   ` Claude review: " Claude Code Review Bot
2026-03-31 12:35 ` [PATCH v9 2/3] drm/v3d: Allocate all resources before enabling the clock Maíra Canal
2026-03-31 21:45   ` Claude Code Review Bot [this message]
2026-03-31 12:35 ` [PATCH v9 3/3] drm/v3d: Introduce Runtime Power Management Maíra Canal
2026-03-31 15:33   ` Florian Fainelli
2026-03-31 21:45   ` Claude review: " Claude Code Review Bot
2026-03-31 21:45 ` Claude review: Power Management for Raspberry Pi V3D GPU Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-03-28 18:52 [PATCH v8 0/3] " Maíra Canal
2026-03-28 18:52 ` [PATCH v8 2/3] drm/v3d: Allocate all resources before enabling the clock Maíra Canal
2026-03-31  7:53   ` Claude review: " Claude Code Review Bot
2026-03-12 21:34 [PATCH v7 0/5] Power Management for Raspberry Pi V3D GPU Maíra Canal
2026-03-12 21:34 ` [PATCH v7 4/5] drm/v3d: Allocate all resources before enabling the clock Maíra Canal
2026-03-13  3:47   ` Claude review: " Claude Code Review Bot
2026-02-13 18:52 [PATCH v5 0/7] Power Management for Raspberry Pi V3D GPU Maíra Canal
2026-02-13 18:52 ` [PATCH v5 6/7] drm/v3d: Allocate all resources before enabling the clock Maíra Canal
2026-02-13 21:21   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch2-20260331-v3d-power-management-v9-2-f52ff87bfd36@igalia.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox