From: Heiko Stuebner <heiko.stuebner@cherry.de>

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".

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Andy Yan <andy.yan@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 385cf6881504..ecfae8d5da89 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -448,15 +448,13 @@ 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");
+		DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");
 		return PTR_ERR(lvds->pclk);
 	}
 
@@ -480,12 +478,6 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
 		}
 	}
 
-	ret = clk_prepare(lvds->pclk);
-	if (ret < 0) {
-		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -728,20 +720,15 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, lvds);
 
 	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
-	if (ret < 0) {
+	if (ret < 0)
 		DRM_DEV_ERROR(dev, "failed to add component\n");
-		clk_unprepare(lvds->pclk);
-	}
 
 	return ret;
 }
 
 static void rockchip_lvds_remove(struct platform_device *pdev)
 {
-	struct rockchip_lvds *lvds = platform_get_drvdata(pdev);
-
 	component_del(&pdev->dev, &rockchip_lvds_component_ops);
-	clk_unprepare(lvds->pclk);
 }
 
 struct platform_driver rockchip_lvds_driver = {
-- 
2.47.2

Hello Heiko,

On 2025-03-04 13:44, Heiko Stuebner wrote:
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 385cf6881504..ecfae8d5da89 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -448,15 +448,13 @@ 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");
> +		DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");

I'm wondering why this patch isn't replacing deprecated DRM
logging macros with their preferred successors in a couple of
places, just like what the patch 2/3 from this series does?

Am Dienstag, 4. März 2025, 21:30:22 MEZ schrieb Dragan Simic:
> Hello Heiko,
> 
> On 2025-03-04 13:44, Heiko Stuebner wrote:
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> > b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> > index 385cf6881504..ecfae8d5da89 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> > @@ -448,15 +448,13 @@ 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");
> > +		DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");
> 
> I'm wondering why this patch isn't replacing deprecated DRM
> logging macros with their preferred successors in a couple of
> places, just like what the patch 2/3 from this series does?

because this patch is about reorganizing the clock handling :-)

Changing the logging functions is a different topic and so has no place
in _this_ patch.

And of course drm-logging is a separate beast to tame altogether.
drm_err vs. dev_err; lvds->dev vs. drm_dev (there was an argument over
what belongs where recently) .

And I had neither the capacity nor time to delve into all that, so limited
myself to stuff I understood :-)

Heiko