Re: [PATCH v2 5/7] drm/stm: ltdc: add devm_reset_control & platform_get_ressource

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Philippe CORNU <philippe.cornu@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Yannick Fertre <yannick.fertre@st.com>,
	Fabien Dessenne <fabien.dessenne@st.com>,
	Mickael Reulier <mickael.reulier@st.com>,
	Vincent Abriou <vincent.abriou@st.com>,
	Gabriel Fernandez <gabriel.fernandez@st.com>,
	Ludovic Barre <ludovic.barre@st.com>
Subject: Re: [PATCH v2 5/7] drm/stm: ltdc: add devm_reset_control & platform_get_ressource
Date: Thu, 20 Jul 2017 14:38:38 +0200	[thread overview]
Message-ID: <1500554318.2354.61.camel@pengutronix.de> (raw)
In-Reply-To: <1500552357-29487-6-git-send-email-philippe.cornu@st.com>

Hi Philippe,

On Thu, 2017-07-20 at 14:05 +0200, Philippe CORNU wrote:
> Use devm_reset_control_get_exclusive to avoid resource leakage (based
> on patch "Convert drivers to explicit reset API" from Philipp Zabel).
> 
> Also use platform_get_resource, which is more usual and
> consistent with platform_get_irq called later.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> Signed-off-by: Philippe CORNU <philippe.cornu@st.com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>

Looking at the usage below, this driver only seems to care about the
reset deassertion before register use, so this could use the shared API.
Further, it seems that this reset is optional.

> ---
>  drivers/gpu/drm/stm/ltdc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 92e58ba..d826045 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -874,7 +874,7 @@ int ltdc_load(struct drm_device *ddev)
>  	struct drm_panel *panel;
>  	struct drm_crtc *crtc;
>  	struct reset_control *rstc;
> -	struct resource res;
> +	struct resource *res;
>  	int irq, ret, i;
>  
>  	DRM_DEBUG_DRIVER("\n");
> @@ -883,7 +883,7 @@ int ltdc_load(struct drm_device *ddev)
>  	if (ret)
>  		return ret;
>  
> -	rstc = of_reset_control_get(np, NULL);
> +	rstc = devm_reset_control_get_exclusive(dev, NULL);

I would suggest to change this to

-	rstc = of_reset_control_get(np, NULL);
+	rstc = devm_reset_control_get_optional_shared(dev, NULL);
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);

>  	mutex_init(&ldev->err_lock);
>  
> @@ -898,13 +898,14 @@ int ltdc_load(struct drm_device *ddev)
>  		return -ENODEV;
>  	}
>  
> -	if (of_address_to_resource(np, 0, &res)) {
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
>  		DRM_ERROR("Unable to get resource\n");
>  		ret = -ENODEV;
>  		goto err;
>  	}
>  
> -	ldev->regs = devm_ioremap_resource(dev, &res);
> +	ldev->regs = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(ldev->regs)) {
>  		DRM_ERROR("Unable to get ltdc registers\n");
>  		ret = PTR_ERR(ldev->regs);

then below you can change:

-	if (!IS_ERR(rstc))
-		reset_control_deassert(rstc);
+	reset_control_deassert(rstc);

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel