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

[Artem Bityutskiy <dedekind1@gmail.com>]

[-- 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 --]