From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: clk: qcom: gdsc: Add custom disable callback for GX GDSC Date: Sun, 12 Apr 2026 13:46:54 +1000 Message-ID: In-Reply-To: <20260407-gfx-clk-fixes-v1-1-4bb5583a5054@oss.qualcomm.com> References: <20260407-gfx-clk-fixes-v1-0-4bb5583a5054@oss.qualcomm.com> <20260407-gfx-clk-fixes-v1-1-4bb5583a5054@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The logic is clean and follows the existing `gdsc_gx_do_nothing_enable()` pattern nicely. The use of `domain->synced_poweroff` as the gate for actual hardware disable is a good design choice. **Issue: Missing `#else` stub in gdsc.h.** The header adds the declaration under `#ifdef CONFIG_QCOM_GDSC`: ```c int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain); +int gdsc_gx_disable(struct generic_pm_domain *domain); #else ``` But no corresponding `static inline` stub is added in the `#else` block (lines 92-100 of `gdsc.h`). The existing `gdsc_gx_do_nothing_enable()` also lacks an `#else` stub, so this is a pre-existing pattern gap, but since `gxclkctl-kaanapali.c` (Patch 2) references `gdsc_gx_disable` directly, this will cause a link error if `CONFIG_QCOM_GDSC=n`. Worth adding stubs for both functions. **Minor:** The comment could be slightly more precise -- "allow explicitly disable" should read "allow explicit disable of". --- Generated by Claude Code Patch Reviewer