Re: [PATCH] SDIO driver for Marvell SoCs

[Nicolas Pitre]

On Thu, 19 Feb 2009, Andrew Morton wrote:

> On Tue, 17 Feb 2009 23:46:21 -0500 (EST)
> Nicolas Pitre <nico@xxxxxxx> wrote:
> 
> > From: Maen Suleiman <maen@xxxxxxxxxxx>
> > 
> > This supports MMC/SD/SDIO currently found on the Kirkwood 88F6281 and
> > 88F6192 SoC controllers.
> > 
> > Signed-off-by: Nicolas Pitre <nico@xxxxxxxxxxx>
> 
> It would be nice to have Maen's signoff.

Sure, I can add it if this is Maen's wish.  Still, this single signoff 
was made in conformity with option B of the Developer's Certificate of 
Origin 1.1 as found in Documentation/SubmittingPatches.  It is actually 
my duty at Marvell to act as such.

> >
> > ...
> >
> > +#define mvsd_write(offs, val)	writel(val, iobase + (offs))
> > +#define mvsd_read(offs)		readl(iobase + (offs))
> 
> It's rather grotty to have a macro which secretly relies upon the
> presence of a particularly-named local variable.

My call.  Otherwise the code becomes bloated with repeated iobase noise 
everywhere, making many lines bust the 80 column limit imposed by the 
checkpatch.pl.  The alternative is to split those statement on multiple 
lines making things not prettier.  So, unless you want to help 
maintaining this driver, I prefer this grottiness to remains as is.

> > +static int __init mvsd_probe(struct platform_device *pdev)
> > +{
> > +	struct mmc_host *mmc = NULL;
> > +	struct mvsd_host *host = NULL;
> > +	const struct mvsdio_platform_data *mvsd_data;
> > +	struct resource *r;
> > +	int ret, irq;
> > +
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	irq = platform_get_irq(pdev, 0);
> > +	mvsd_data = pdev->dev.platform_data;
> > +	if (!r || irq < 0 || !mvsd_data)
> > +		return -ENXIO;
> > +
> > +	r = request_mem_region(r->start, SZ_1K, DRIVER_NAME);
> > +	if (!r)
> > +		return -EBUSY;
> > +
> > +	mmc = mmc_alloc_host(sizeof(struct mvsd_host), &pdev->dev);
> > +	if (!mmc) {
> > +		ret = -ENOMEM;
> > +		goto out;
> 
> Resource `r' is leaked here.

Good catch, thanks.

> > +static int __exit mvsd_remove(struct platform_device *pdev)
> > +{
> > +	struct mmc_host *mmc = platform_get_drvdata(pdev);
> > +
> > +	if (mmc) {
> > +		struct mvsd_host *host = mmc_priv(mmc);
> > +
> > +		if (host->gpio_card_detect) {
> > +			free_irq(gpio_to_irq(host->gpio_card_detect), host);
> > +			gpio_free(host->gpio_card_detect);
> > +		}
> > +		mmc_remove_host(mmc);
> > +		free_irq(host->irq, host);
> > +		if (host->gpio_write_protect)
> > +			gpio_free(host->gpio_write_protect);
> > +		del_timer_sync(&host->timer);
> > +		mvsd_power_down(host);
> > +		iounmap(host->base);
> > +		release_resource(host->res);
> > +		mmc_free_host(mmc);
> 
> Perhaps this function and the error path ("out:") in the preceding
> function could share code.

Well, one is marked __init and the other __exit.  Having common code 
would mean this needs to be in a non discardable section which is a 
waste.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/