Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935243AbdGTMio (ORCPT ); Thu, 20 Jul 2017 08:38:44 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:49107 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934643AbdGTMin (ORCPT ); Thu, 20 Jul 2017 08:38:43 -0400 Message-ID: <1500554318.2354.61.camel@pengutronix.de> Subject: Re: [PATCH v2 5/7] drm/stm: ltdc: add devm_reset_control & platform_get_ressource From: Philipp Zabel To: Philippe CORNU Cc: Yannick Fertre , Benjamin Gaignard , Vincent Abriou , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Fabien Dessenne , Mickael Reulier , Gabriel Fernandez , Ludovic Barre , Alexandre Torgue , Maxime Coquelin Date: Thu, 20 Jul 2017 14:38:38 +0200 In-Reply-To: <1500552357-29487-6-git-send-email-philippe.cornu@st.com> References: <1500552357-29487-1-git-send-email-philippe.cornu@st.com> <1500552357-29487-6-git-send-email-philippe.cornu@st.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2335 Lines: 77 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 > Signed-off-by: Philippe CORNU > Reviewed-by: Benjamin Gaignard > Cc: Philipp Zabel 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