Re: [PATCH 4/4] drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

Re: [PATCH 4/4] drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 5912 bytes --]

Aiaiai! :-) [1] [2]

I've build-tested this using aiaiai and it reports that this change breaks the build:

dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox 
Tested the patch(es) on top of the following commits:
ba64756 Quick fixes - applied by aiaiai
651c6fa JFFS2: don't fail on bitflips in OOB
e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data
ea9d312 mtd: cmdlinepart: minor cleanups

--------------------------------------------------------------------------------
Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)":

0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

...
drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe':
drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable]
drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable]
drivers/built-in.o: In function `mpc5121_nfc_probe':
mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get'
make[1]: *** [vmlinux] Error 1

--------------------------------------------------------------------------------

I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people.

So, not taking this patch.

References:

1. http://git.infradead.org/users/dedekind/aiaiai.git
2. http://git.infradead.org/users/dedekind/maintaining.git

On Sat, 2012-09-01 at 18:33 +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> devm free functions should not have to be explicitly used.
> 
> The only thing left that is useful in the function mpc5121_nfc_free is the
> call to clk_disable, which is moved to the call sites.
> 
> This function also incorrectly called iounmap on devm_ioremap allocated
> data.
> 
> Use devm_request_and_ioremap in place of devm_request_mem_region and
> devm_ioremap.
> 
> Use devm_clk_get.
> 
> A semantic match that finds the first problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> @@
> 
> (
> * devm_kfree(...);
> |
> * devm_free_irq(...);
> |
> * devm_iounmap(...);
> |
> * devm_release_region(...);
> |
> * devm_release_mem_region(...);
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/mtd/nand/mpc5121_nfc.c |   35 +++++------------------------------
>  1 file changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
> index c259c24..45183ba 100644
> --- a/drivers/mtd/nand/mpc5121_nfc.c
> +++ b/drivers/mtd/nand/mpc5121_nfc.c
> @@ -632,21 +632,6 @@ out:
>  	return ret;
>  }
>  
> -/* Free driver resources */
> -static void mpc5121_nfc_free(struct device *dev, struct mtd_info *mtd)
> -{
> -	struct nand_chip *chip = mtd->priv;
> -	struct mpc5121_nfc_prv *prv = chip->priv;
> -
> -	if (prv->clk) {
> -		clk_disable(prv->clk);
> -		clk_put(prv->clk);
> -	}
> -
> -	if (prv->csreg)
> -		iounmap(prv->csreg);
> -}
> -
>  static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  {
>  	struct device_node *rootnode, *dn = op->dev.of_node;
> @@ -713,12 +698,7 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  	regs_paddr = res.start;
>  	regs_size = resource_size(&res);
>  
> -	if (!devm_request_mem_region(dev, regs_paddr, regs_size, DRV_NAME)) {
> -		dev_err(dev, "Error requesting memory region!\n");
> -		return -EBUSY;
> -	}
> -
> -	prv->regs = devm_ioremap(dev, regs_paddr, regs_size);
> +	prv->regs = devm_request_and_ioremap(dev, &res);
>  	if (!prv->regs) {
>  		dev_err(dev, "Error mapping memory region!\n");
>  		return -ENOMEM;
> @@ -752,11 +732,10 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  	of_node_put(rootnode);
>  
>  	/* Enable NFC clock */
> -	prv->clk = clk_get(dev, "nfc_clk");
> +	prv->clk = devm_clk_get(dev, "nfc_clk");
>  	if (IS_ERR(prv->clk)) {
>  		dev_err(dev, "Unable to acquire NFC clock!\n");
> -		retval = PTR_ERR(prv->clk);
> -		goto error;
> +		return PTR_ERR(prv->clk);
>  	}
>  
>  	clk_enable(prv->clk);
> @@ -803,7 +782,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  	/* Detect NAND chips */
>  	if (nand_scan(mtd, be32_to_cpup(chips_no))) {
>  		dev_err(dev, "NAND Flash not found !\n");
> -		devm_free_irq(dev, prv->irq, mtd);
>  		retval = -ENXIO;
>  		goto error;
>  	}
> @@ -828,7 +806,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  
>  	default:
>  		dev_err(dev, "Unsupported NAND flash!\n");
> -		devm_free_irq(dev, prv->irq, mtd);
>  		retval = -ENXIO;
>  		goto error;
>  	}
> @@ -839,13 +816,12 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op)
>  	retval = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
>  	if (retval) {
>  		dev_err(dev, "Error adding MTD device!\n");
> -		devm_free_irq(dev, prv->irq, mtd);
>  		goto error;
>  	}
>  
>  	return 0;
>  error:
> -	mpc5121_nfc_free(dev, mtd);
> +	clk_disable(prv->clk);
>  	return retval;
>  }
>  
> @@ -857,8 +833,7 @@ static int __devexit mpc5121_nfc_remove(struct platform_device *op)
>  	struct mpc5121_nfc_prv *prv = chip->priv;
>  
>  	nand_release(mtd);
> -	devm_free_irq(dev, prv->irq, mtd);
> -	mpc5121_nfc_free(dev, mtd);
> +	clk_disable(prv->clk);
>  
>  	return 0;
>  }
> 
> --
> 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/

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

[Lars-Peter Clausen]

On 09/04/2012 10:42 AM, Artem Bityutskiy wrote:
> Aiaiai! :-) [1] [2]
> 
> I've build-tested this using aiaiai and it reports that this change breaks the build:
> 
> dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox 
> Tested the patch(es) on top of the following commits:
> ba64756 Quick fixes - applied by aiaiai
> 651c6fa JFFS2: don't fail on bitflips in OOB
> e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data
> ea9d312 mtd: cmdlinepart: minor cleanups
> 
> --------------------------------------------------------------------------------
> Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)":
> 
> 0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups
> 
> ...
> drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe':
> drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable]
> drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable]
> drivers/built-in.o: In function `mpc5121_nfc_probe':
> mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get'
> make[1]: *** [vmlinux] Error 1
> 
> --------------------------------------------------------------------------------
> 
> I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people.
> 

I've been bitten by the same issue recently, also cause by one of these
cocci devm patches. devm_clk_get is only available if the generic
clk_get/clk_put implementation is used. Not all architectures do this and
some implement their own clk_get/clk_put, etc functions. Since devm_clk_get
is merely a wrapper around clk_get/clk_put there is no reason why it should
depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically
available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set),
but it is on a different machine right now, will try to submit it later today.

- Lars

Re: [PATCH 4/4] drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

[Julia Lawall]

On Tue, 4 Sep 2012, Lars-Peter Clausen wrote:

> On 09/04/2012 10:42 AM, Artem Bityutskiy wrote:
> > Aiaiai! :-) [1] [2]
> >
> > I've build-tested this using aiaiai and it reports that this change breaks the build:
> >
> > dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox
> > Tested the patch(es) on top of the following commits:
> > ba64756 Quick fixes - applied by aiaiai
> > 651c6fa JFFS2: don't fail on bitflips in OOB
> > e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data
> > ea9d312 mtd: cmdlinepart: minor cleanups
> >
> > --------------------------------------------------------------------------------
> > Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)":
> >
> > 0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups
> >
> > ...
> > drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe':
> > drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable]
> > drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable]
> > drivers/built-in.o: In function `mpc5121_nfc_probe':
> > mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get'
> > make[1]: *** [vmlinux] Error 1
> >
> > --------------------------------------------------------------------------------
> >
> > I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people.
> >
>
> I've been bitten by the same issue recently, also cause by one of these
> cocci devm patches. devm_clk_get is only available if the generic
> clk_get/clk_put implementation is used. Not all architectures do this and
> some implement their own clk_get/clk_put, etc functions. Since devm_clk_get
> is merely a wrapper around clk_get/clk_put there is no reason why it should
> depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically
> available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set),
> but it is on a different machine right now, will try to submit it later today.

Sorry about this.  I wasn't aware that devm_clk_get wasn't supported by
all architectures, and I have no way of compiling code for these
architectures...  But I wonder why it is not, since devm-ness doesn't seem
to have anything to do with architecture-specific details?  It would be
really nice to have it for all architectures, because the clock functions
are just as (or at least almost as) common as kzalloc, ioremap, etc.

thanks,
julia

Re: [PATCH 4/4] drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

On Tue, 2012-09-04 at 11:44 +0200, Julia Lawall wrote:
> > I've been bitten by the same issue recently, also cause by one of these
> > cocci devm patches. devm_clk_get is only available if the generic
> > clk_get/clk_put implementation is used. Not all architectures do this and
> > some implement their own clk_get/clk_put, etc functions. Since devm_clk_get
> > is merely a wrapper around clk_get/clk_put there is no reason why it should
> > depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically
> > available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set),
> > but it is on a different machine right now, will try to submit it later today.
> 
> Sorry about this.  I wasn't aware that devm_clk_get wasn't supported by
> all architectures, and I have no way of compiling code for these
> architectures...  But I wonder why it is not, since devm-ness doesn't seem
> to have anything to do with architecture-specific details?  It would be
> really nice to have it for all architectures, because the clock functions
> are just as (or at least almost as) common as kzalloc, ioremap, etc.

It looks like Lars is going to fix this.

I am personally fine if you send patches without build-testing them.
Your patches are generally of good quality and you send many of them, so
build-testing each would be too much for you. And at least for MTD, I
can build-test myself.


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]