Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754462AbbLAPqg (ORCPT ); Tue, 1 Dec 2015 10:46:36 -0500 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:44980 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751324AbbLAPqe (ORCPT ); Tue, 1 Dec 2015 10:46:34 -0500 Date: Tue, 1 Dec 2015 15:46:24 +0000 From: Russell King - ARM Linux To: Marek Szyprowski Cc: linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ulf Hansson , Tomeu Vizoso , Greg Kroah-Hartman , Kukjin Kim , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz Subject: Re: [PATCH v3 2/3] ARM: amba: Move reading of periphid to amba_match() Message-ID: <20151201154624.GW8644@n2100.arm.linux.org.uk> References: <1448976866-7892-1-git-send-email-m.szyprowski@samsung.com> <1448976866-7892-3-git-send-email-m.szyprowski@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448976866-7892-3-git-send-email-m.szyprowski@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4659 Lines: 151 On Tue, Dec 01, 2015 at 02:34:25PM +0100, Marek Szyprowski wrote: > From: Tomeu Vizoso > > Reading the periphid when the Primecell device is registered means that > the apb pclk must be available by then or the device won't be registered > at all. > > By reading the periphid in amba_match() we can return -EPROBE_DEFER if > the apb pclk isn't there yet and the device will be retried later. > > Signed-off-by: Tomeu Vizoso > [minor code adjustments, added missing comment] > Signed-off-by: Marek Szyprowski > --- > drivers/amba/bus.c | 77 +++++++++++++++++++++++++++++------------------------- > 1 file changed, 41 insertions(+), 36 deletions(-) > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > index f009936..879a421 100644 > --- a/drivers/amba/bus.c > +++ b/drivers/amba/bus.c > @@ -24,6 +24,8 @@ > > #define to_amba_driver(d) container_of(d, struct amba_driver, drv) > > +static int amba_read_periphid(struct amba_device *dev); > + Please avoid forward declarations where possible. > static const struct amba_id * > amba_lookup(const struct amba_id *table, struct amba_device *dev) > { > @@ -43,11 +45,23 @@ static int amba_match(struct device *dev, struct device_driver *drv) > { > struct amba_device *pcdev = to_amba_device(dev); > struct amba_driver *pcdrv = to_amba_driver(drv); > + int ret; > > /* When driver_override is set, only bind to the matching driver */ > if (pcdev->driver_override) > return !strcmp(pcdev->driver_override, drv->name); > > + /* Do plug-n-play if no hard-coded primecell ID has been provided */ > + if (!pcdev->periphid) { > + ret = amba_read_periphid(pcdev); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Failed to read periphid: %d", > + ret); > + return ret; > + } > + } > + > return amba_lookup(pcdrv->id_table, pcdev) != NULL; > } > > @@ -336,31 +350,11 @@ static void amba_device_release(struct device *dev) > kfree(d); > } > > -/** > - * amba_device_add - add a previously allocated AMBA device structure > - * @dev: AMBA device allocated by amba_device_alloc > - * @parent: resource parent for this devices resources > - * > - * Claim the resource, and read the device cell ID if not already > - * initialized. Register the AMBA device with the Linux device > - * manager. > - */ > -int amba_device_add(struct amba_device *dev, struct resource *parent) > +static int amba_read_periphid(struct amba_device *dev) > { > u32 size; > void __iomem *tmp; > - int i, ret; > - > - WARN_ON(dev->irq[0] == (unsigned int)-1); > - WARN_ON(dev->irq[1] == (unsigned int)-1); > - > - ret = request_resource(parent, &dev->res); > - if (ret) > - goto err_out; > - > - /* Hard-coded primecell ID instead of plug-n-play */ > - if (dev->periphid != 0) > - goto skip_probe; > + int i, ret = 0; > > /* > * Dynamically calculate the size of the resource > @@ -368,10 +362,8 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) > */ > size = resource_size(&dev->res); > tmp = ioremap(dev->res.start, size); > - if (!tmp) { > - ret = -ENOMEM; > - goto err_release; > - } > + if (!tmp) > + return -ENOMEM; > > ret = amba_get_enable_pclk(dev); > if (ret == 0) { > @@ -399,26 +391,39 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) > > iounmap(tmp); > > + return ret; > +} > + > +/** > + * amba_device_add - add a previously allocated AMBA device structure > + * @dev: AMBA device allocated by amba_device_alloc > + * @parent: resource parent for this devices resources > + * > + * Claim the resource, and register the AMBA device with the Linux device > + * manager. > + */ > +int amba_device_add(struct amba_device *dev, struct resource *parent) > +{ > + int ret; > + > + WARN_ON(dev->irq[0] == (unsigned int)-1); > + WARN_ON(dev->irq[1] == (unsigned int)-1); > + > + ret = request_resource(parent, &dev->res); > if (ret) > - goto err_release; > + return ret; > > - skip_probe: > ret = device_add(&dev->dev); > if (ret) > - goto err_release; > + return ret; NAK. The original code handled error conditions correctly. This new code completely breaks the error handling here by removing the release of the above requested resource. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/