Re: [PATCH 1/2] drm/rockchip: lvds: move pclk preparation in with clk_get

[Quentin Schulz]

Hi Heiko,

On 2/28/25 5:57 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>
>
> The LVDS block needs a separate pclk only on some socs, so currently
> requests and prepares it in the soc-specific probe function, but common
> code is required to unprepare it in the error path or on driver remove.
>
> While this works because clk_unprepare just does nothing if clk is NULL,
> this mismatch of who is responsible still is not very nice.
> The clock-framework already has a helper for clk-get-and-prepare even
> with devres support in devm_clk_get_prepared().
>
> This will get and prepare the clock and also unprepare it on driver
> removal, saving the driver from having to handle it "manually".
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>

Reviewed-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>

Small nitpick below.

> ---
>   drivers/gpu/drm/rockchip/rockchip_lvds.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 385cf6881504..c19b7b1f6cb5 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -448,13 +448,11 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
>   static int rk3288_lvds_probe(struct platform_device *pdev,
>   			     struct rockchip_lvds *lvds)
>   {
> -	int ret;
> -
>   	lvds->regs = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(lvds->regs))
>   		return PTR_ERR(lvds->regs);
>   
> -	lvds->pclk = devm_clk_get(lvds->dev, "pclk_lvds");
> +	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
>   	if (IS_ERR(lvds->pclk)) {
>   		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");

Should we say

Could not get or prepare pclk_lvds\n

instead?

Thanks!
Quentin