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/rockchip: analogix_dp: Enable hclk for RK3588 Date: Tue, 26 May 2026 07:34:41 +1000 Message-ID: In-Reply-To: <20260525082033.117569-6-damon.ding@rock-chips.com> References: <20260525082033.117569-1-damon.ding@rock-chips.com> <20260525082033.117569-6-damon.ding@rock-chips.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review Acquires the hclk using `devm_clk_get_optional_enabled()`: ```c + clk = devm_clk_get_optional_enabled(dev, "hclk"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "failed to get hclk property\n"); ``` Good use of `devm_clk_get_optional_enabled()` -- it handles both the "not present" case (for RK3288/RK3399) and the error case cleanly. The clock is managed by devres so it stays enabled for the device lifetime, which matches the commit message's statement that hclk "must remain enabled during probe." The local `clk` variable is unused after the call, which is fine since devm handles the lifecycle. One thing to note: this means hclk is always-on once probed. The existing `pclk` is toggled in `rockchip_dp_poweron`/`rockchip_dp_powerdown`, but hclk is not. Per the commit message this is intentional ("must remain enabled during probe") and the hardware requires it to stay on for datapath gating. This seems correct. --- Generated by Claude Code Patch Reviewer