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 clock "hclk" for RK3588 Date: Sun, 22 Mar 2026 04:26:15 +1000 Message-ID: In-Reply-To: <20260319104031.1986946-5-damon.ding@rock-chips.com> References: <20260319104031.1986946-1-damon.ding@rock-chips.com> <20260319104031.1986946-5-damon.ding@rock-chips.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 ```c + clk =3D 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"); ``` **Concern**: Using `devm_clk_get_optional_enabled()` means the clock is ena= bled at probe time and stays enabled for the lifetime of the device. This i= s different from how `pclk` is handled =E2=80=94 `pclk` is stored in `dp->p= clk` and explicitly enabled/disabled in `rockchip_dp_poweron()`/`rockchip_d= p_powerdown()`. If `hclk` is a video datapath clock needed for operation, i= t would be more power-efficient to enable/disable it alongside `pclk` in th= e power on/off callbacks. That said, if the clock domain is shared and alwa= ys-on in practice, this approach is simpler and acceptable. The error message says "failed to get hclk property" =E2=80=94 "property" i= s misleading for a clock; "failed to get hclk clock" would be clearer, matc= hing the terminology used elsewhere. The Fixes tag is appropriate. --- Generated by Claude Code Patch Reviewer