Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
On Tue, 17 Nov 2015, Boris Brezillon wrote: > Hi Julia, > > On Tue, 17 Nov 2015 10:05:03 +0100 (CET) > Julia Lawall wrote: > > > > > (This isn't the worst one, but it just happens to be one of the first.) > > > > There are many cases where the typical style would be to declare a new > > > > variable at the top of the function, where you perform the > > > > macro/function-call to convert from one abstraction to another. Like > > > > > > > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int > > > > bank) > > > > { > > > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip); > > > > ... > > > > > > > > and then use it later. Can that be done very easily? > > > > > > > > > return -EINVAL; > > > > > nfc_writel(host->nfc->hsmc_regs, BANK, > > > > > ATMEL_HSMC_NFC_BANK1); > > > > > } else { > > > > > > > > ... > > > > > > Honestly, I don't know how to do that with a coccinelle script, and it > > > will probably take me more time to find how to do it than addressing > > > those problems manually. > > > > > > Julia, could you give us some hint? > > > > Probably something like the following would be easiest. You can just run > > it after your other transformations: > > > > @r exists@ > > identifier f; > > expression e; > > @@ > > > > f(...) { <+... nand_to_mtd(e) ...+> } > > > > @@ > > identifier r.f; > > expression r.e; > > @@ > > > > f(...) { > > + struct mtd_info *mtd = nand_to_mtd(e); > > ... > > } > > Thanks for the the suggestion... > > > > > This won't work if there is more than one possible value of e. If that is > > likely, then I could come up with something more complex. It also assumes > > that you want to convert all such calls. If you only want to convert calls > > that occur in a particular context, eg a field reference, then you could > > enhance the pattern inside the <+... ...+> in the first rule. > > ... unfortunately, as you've guessed, it's a bit more complicated. > This mtd local variable is usually extracted from another local > variable (struct nand_chip *chip), so we have to declare > > struct mtd_info *mtd = nand_to_mtd(e); > > after > > struct nand_chip *chip = expression; > > But this is not the only particular case. Sometime the chip variable is > not assigned where it's declared (especially when it is dynamically > allocated), and sometime it does not exist at all (we just extract it > from a private struct: &priv->chip). > The mtd variable can also be declared in sub-code blocks (loop or > conditional statements). > And, as you stated, we might want to keep direct calls to nand_to_mtd() > if it's only called once in a function context. > > I'm pretty sure we could describe all those cases with specific context > description, but I must admit that it takes less time for me to fix > those specific cases manually than figuring out how to describe them > correctly in a coccinelle script :). > > This being said, I'd be happy to see how you would handle all these > different cases. Maybe the following would be useful. It won't handle all the cases, but maybe it will take care of a good number of the nontrivial ones. julia @r@ local idexpression x; identifier fld,y; @@ nand_to_mtd(&x->fld)->y @exists@ type T; local idexpression r.x; identifier xx,r.y,r.fld; @@ T xx; + struct mtd_info *mtd = nand_to_mtd(&x->fld); ... nand_to_mtd(&x@xx->fld)->y @@ local idexpression r.x; identifier r.fld; @@ struct mtd_info *mtd = nand_to_mtd(&x->fld); <... - nand_to_mtd(&x->fld) + mtd ...> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
Hi Julia, On Tue, 17 Nov 2015 10:05:03 +0100 (CET) Julia Lawall wrote: > > > (This isn't the worst one, but it just happens to be one of the first.) > > > There are many cases where the typical style would be to declare a new > > > variable at the top of the function, where you perform the > > > macro/function-call to convert from one abstraction to another. Like > > > > > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int > > > bank) > > > { > > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip); > > > ... > > > > > > and then use it later. Can that be done very easily? > > > > > > > return -EINVAL; > > > > nfc_writel(host->nfc->hsmc_regs, BANK, > > > > ATMEL_HSMC_NFC_BANK1); > > > > } else { > > > > > > ... > > > > Honestly, I don't know how to do that with a coccinelle script, and it > > will probably take me more time to find how to do it than addressing > > those problems manually. > > > > Julia, could you give us some hint? > > Probably something like the following would be easiest. You can just run > it after your other transformations: > > @r exists@ > identifier f; > expression e; > @@ > > f(...) { <+... nand_to_mtd(e) ...+> } > > @@ > identifier r.f; > expression r.e; > @@ > > f(...) { > + struct mtd_info *mtd = nand_to_mtd(e); > ... > } Thanks for the the suggestion... > > This won't work if there is more than one possible value of e. If that is > likely, then I could come up with something more complex. It also assumes > that you want to convert all such calls. If you only want to convert calls > that occur in a particular context, eg a field reference, then you could > enhance the pattern inside the <+... ...+> in the first rule. ... unfortunately, as you've guessed, it's a bit more complicated. This mtd local variable is usually extracted from another local variable (struct nand_chip *chip), so we have to declare struct mtd_info *mtd = nand_to_mtd(e); after struct nand_chip *chip = expression; But this is not the only particular case. Sometime the chip variable is not assigned where it's declared (especially when it is dynamically allocated), and sometime it does not exist at all (we just extract it from a private struct: &priv->chip). The mtd variable can also be declared in sub-code blocks (loop or conditional statements). And, as you stated, we might want to keep direct calls to nand_to_mtd() if it's only called once in a function context. I'm pretty sure we could describe all those cases with specific context description, but I must admit that it takes less time for me to fix those specific cases manually than figuring out how to describe them correctly in a coccinelle script :). This being said, I'd be happy to see how you would handle all these different cases. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
> > (This isn't the worst one, but it just happens to be one of the first.) > > There are many cases where the typical style would be to declare a new > > variable at the top of the function, where you perform the > > macro/function-call to convert from one abstraction to another. Like > > > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int > > bank) > > { > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip); > > ... > > > > and then use it later. Can that be done very easily? > > > > > return -EINVAL; > > > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1); > > > } else { > > > > ... > > Honestly, I don't know how to do that with a coccinelle script, and it > will probably take me more time to find how to do it than addressing > those problems manually. > > Julia, could you give us some hint? Probably something like the following would be easiest. You can just run it after your other transformations: @r exists@ identifier f; expression e; @@ f(...) { <+... nand_to_mtd(e) ...+> } @@ identifier r.f; expression r.e; @@ f(...) { + struct mtd_info *mtd = nand_to_mtd(e); ... } This won't work if there is more than one possible value of e. If that is likely, then I could come up with something more complex. It also assumes that you want to convert all such calls. If you only want to convert calls that occur in a particular context, eg a field reference, then you could enhance the pattern inside the <+... ...+> in the first rule. julia -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
Hi Brian, On Mon, 16 Nov 2015 19:00:19 -0800 Brian Norris wrote: > Hi Boris, > > On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote: > > struct nand_chip now embeds an mtd device. Patch all drivers to make use > > of this mtd instance instead of using the instance embedded in their > > private struct or dynamically allocated. > > > > Signed-off-by: Boris Brezillon > > Cc: Julia Lawall > > --- > > Most of those changes were generate with this coccinelle script: > > http://code.bulix.org/5vxuih-89429 > > I appreciate that this patch is mostly autogenerated (a good thing for > preventing errors!), but there are some issues that I don't think play > out very well stylistically. Hopefully the cocci script can be improved > to handle some of this? > > I'll try to point out a few snippets below. > > Also, in case others are interested in reviewing your cocci script > directly, it might be better to paste it inline than to link to it. > Given the size of the patch, I don't think people would mind a few dozen > extra lines to show how it wsa generated. Or maybe stick some in the > cover letter too, if you end up reusing them in several patches. Sure, I'll paste the script directly in the commit message next time. > > > --- > > drivers/mtd/nand/ams-delta.c | 13 ++-- > > drivers/mtd/nand/atmel_nand.c | 11 ++- > > drivers/mtd/nand/au1550nd.c| 18 ++--- > > drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 - > > drivers/mtd/nand/bcm47xxnflash/main.c | 7 +- > > drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +- > > drivers/mtd/nand/bf5xx_nand.c | 14 ++-- > > drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++- > > drivers/mtd/nand/cafe_nand.c | 10 +-- > > drivers/mtd/nand/cmx270_nand.c | 11 ++- > > drivers/mtd/nand/cs553x_nand.c | 13 ++-- > > drivers/mtd/nand/davinci_nand.c| 25 +++ > > drivers/mtd/nand/denali.c | 61 + > > drivers/mtd/nand/denali.h | 1 - > > drivers/mtd/nand/diskonchip.c | 11 ++- > > drivers/mtd/nand/docg4.c | 18 +++-- > > drivers/mtd/nand/fsl_elbc_nand.c | 22 +++--- > > drivers/mtd/nand/fsl_ifc_nand.c| 23 +++ > > drivers/mtd/nand/fsl_upm.c | 26 +++ > > drivers/mtd/nand/fsmc_nand.c | 59 +--- > > drivers/mtd/nand/gpio.c| 16 ++--- > > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +- > > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++--- > > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 - > > drivers/mtd/nand/hisi504_nand.c| 11 ++- > > drivers/mtd/nand/jz4740_nand.c | 9 ++- > > drivers/mtd/nand/lpc32xx_mlc.c | 7 +- > > drivers/mtd/nand/lpc32xx_slc.c | 7 +- > > drivers/mtd/nand/mpc5121_nfc.c | 3 +- > > drivers/mtd/nand/mxc_nand.c| 5 +- > > drivers/mtd/nand/nandsim.c | 12 ++-- > > drivers/mtd/nand/ndfc.c| 22 +++--- > > drivers/mtd/nand/nuc900_nand.c | 21 +++--- > > drivers/mtd/nand/omap2.c | 94 > > +++--- > > drivers/mtd/nand/orion_nand.c | 4 +- > > drivers/mtd/nand/pasemi_nand.c | 14 ++-- > > drivers/mtd/nand/plat_nand.c | 14 ++-- > > drivers/mtd/nand/pxa3xx_nand.c | 33 - > > ^^ BTW, this file already has a few conflicts. Sorry :( > > I'll try to keep any eye out for things like this once we're close to > being able to apply something like this, so I don't merge unnecessary > churn. But for now, I hope we can review this series, and it won't be > too much work to rebase/resend once the bigger things have been worked > out. No problem, resolving this conflict was pretty easy. > > > drivers/mtd/nand/r852.c| 34 -- > > drivers/mtd/nand/r852.h| 1 - > > drivers/mtd/nand/s3c2410.c | 19 +++--- > > drivers/mtd/nand/sh_flctl.c| 8 +-- > > drivers/mtd/nand/sharpsl.c | 18 ++--- > > drivers/mtd/nand/socrates_nand.c | 5 +- > > drivers/mtd/nand/sunxi_nand.c | 13 ++-- > > drivers/mtd/nand/tmio_nand.c | 7 +- > > drivers/mtd/nand/txx9ndfmc.c | 3 +- > > drivers/mtd/nand/vf610_nfc.c | 5 +- > > include/linux/mtd/sh_flctl.h | 3 +- > > 49 files changed, 383 insertions(+), 385 deletions(-) > > > > ... > > > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > > index f8aac0a..51748b4 100644 > > --- a/drivers/mtd/nand/atmel_nand.c > > +++ b/driv
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
Hi Boris, On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote: > struct nand_chip now embeds an mtd device. Patch all drivers to make use > of this mtd instance instead of using the instance embedded in their > private struct or dynamically allocated. > > Signed-off-by: Boris Brezillon > Cc: Julia Lawall > --- > Most of those changes were generate with this coccinelle script: > http://code.bulix.org/5vxuih-89429 I appreciate that this patch is mostly autogenerated (a good thing for preventing errors!), but there are some issues that I don't think play out very well stylistically. Hopefully the cocci script can be improved to handle some of this? I'll try to point out a few snippets below. Also, in case others are interested in reviewing your cocci script directly, it might be better to paste it inline than to link to it. Given the size of the patch, I don't think people would mind a few dozen extra lines to show how it wsa generated. Or maybe stick some in the cover letter too, if you end up reusing them in several patches. > --- > drivers/mtd/nand/ams-delta.c | 13 ++-- > drivers/mtd/nand/atmel_nand.c | 11 ++- > drivers/mtd/nand/au1550nd.c| 18 ++--- > drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 - > drivers/mtd/nand/bcm47xxnflash/main.c | 7 +- > drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +- > drivers/mtd/nand/bf5xx_nand.c | 14 ++-- > drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++- > drivers/mtd/nand/cafe_nand.c | 10 +-- > drivers/mtd/nand/cmx270_nand.c | 11 ++- > drivers/mtd/nand/cs553x_nand.c | 13 ++-- > drivers/mtd/nand/davinci_nand.c| 25 +++ > drivers/mtd/nand/denali.c | 61 + > drivers/mtd/nand/denali.h | 1 - > drivers/mtd/nand/diskonchip.c | 11 ++- > drivers/mtd/nand/docg4.c | 18 +++-- > drivers/mtd/nand/fsl_elbc_nand.c | 22 +++--- > drivers/mtd/nand/fsl_ifc_nand.c| 23 +++ > drivers/mtd/nand/fsl_upm.c | 26 +++ > drivers/mtd/nand/fsmc_nand.c | 59 +--- > drivers/mtd/nand/gpio.c| 16 ++--- > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +- > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++--- > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 - > drivers/mtd/nand/hisi504_nand.c| 11 ++- > drivers/mtd/nand/jz4740_nand.c | 9 ++- > drivers/mtd/nand/lpc32xx_mlc.c | 7 +- > drivers/mtd/nand/lpc32xx_slc.c | 7 +- > drivers/mtd/nand/mpc5121_nfc.c | 3 +- > drivers/mtd/nand/mxc_nand.c| 5 +- > drivers/mtd/nand/nandsim.c | 12 ++-- > drivers/mtd/nand/ndfc.c| 22 +++--- > drivers/mtd/nand/nuc900_nand.c | 21 +++--- > drivers/mtd/nand/omap2.c | 94 > +++--- > drivers/mtd/nand/orion_nand.c | 4 +- > drivers/mtd/nand/pasemi_nand.c | 14 ++-- > drivers/mtd/nand/plat_nand.c | 14 ++-- > drivers/mtd/nand/pxa3xx_nand.c | 33 - ^^ BTW, this file already has a few conflicts. Sorry :( I'll try to keep any eye out for things like this once we're close to being able to apply something like this, so I don't merge unnecessary churn. But for now, I hope we can review this series, and it won't be too much work to rebase/resend once the bigger things have been worked out. > drivers/mtd/nand/r852.c| 34 -- > drivers/mtd/nand/r852.h| 1 - > drivers/mtd/nand/s3c2410.c | 19 +++--- > drivers/mtd/nand/sh_flctl.c| 8 +-- > drivers/mtd/nand/sharpsl.c | 18 ++--- > drivers/mtd/nand/socrates_nand.c | 5 +- > drivers/mtd/nand/sunxi_nand.c | 13 ++-- > drivers/mtd/nand/tmio_nand.c | 7 +- > drivers/mtd/nand/txx9ndfmc.c | 3 +- > drivers/mtd/nand/vf610_nfc.c | 5 +- > include/linux/mtd/sh_flctl.h | 3 +- > 49 files changed, 383 insertions(+), 385 deletions(-) > ... > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > index f8aac0a..51748b4 100644 > --- a/drivers/mtd/nand/atmel_nand.c > +++ b/drivers/mtd/nand/atmel_nand.c ... > @@ -318,7 +317,7 @@ static int nfc_set_sram_bank(struct atmel_nand_host > *host, unsigned int bank) > > if (bank) { > /* Only for a 2k-page or lower flash, NFC can handle 2 banks */ > - if (host->mtd.writesize > 2048) > + if (nand_to_mtd(&host->nand_chip)->writesize > 2048) (This isn't the worst one, b
[PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
struct nand_chip now embeds an mtd device. Patch all drivers to make use of this mtd instance instead of using the instance embedded in their private struct or dynamically allocated. Signed-off-by: Boris Brezillon Cc: Julia Lawall --- Most of those changes were generate with this coccinelle script: http://code.bulix.org/5vxuih-89429 --- drivers/mtd/nand/ams-delta.c | 13 ++-- drivers/mtd/nand/atmel_nand.c | 11 ++- drivers/mtd/nand/au1550nd.c| 18 ++--- drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 - drivers/mtd/nand/bcm47xxnflash/main.c | 7 +- drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +- drivers/mtd/nand/bf5xx_nand.c | 14 ++-- drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++- drivers/mtd/nand/cafe_nand.c | 10 +-- drivers/mtd/nand/cmx270_nand.c | 11 ++- drivers/mtd/nand/cs553x_nand.c | 13 ++-- drivers/mtd/nand/davinci_nand.c| 25 +++ drivers/mtd/nand/denali.c | 61 + drivers/mtd/nand/denali.h | 1 - drivers/mtd/nand/diskonchip.c | 11 ++- drivers/mtd/nand/docg4.c | 18 +++-- drivers/mtd/nand/fsl_elbc_nand.c | 22 +++--- drivers/mtd/nand/fsl_ifc_nand.c| 23 +++ drivers/mtd/nand/fsl_upm.c | 26 +++ drivers/mtd/nand/fsmc_nand.c | 59 +--- drivers/mtd/nand/gpio.c| 16 ++--- drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +- drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++--- drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 - drivers/mtd/nand/hisi504_nand.c| 11 ++- drivers/mtd/nand/jz4740_nand.c | 9 ++- drivers/mtd/nand/lpc32xx_mlc.c | 7 +- drivers/mtd/nand/lpc32xx_slc.c | 7 +- drivers/mtd/nand/mpc5121_nfc.c | 3 +- drivers/mtd/nand/mxc_nand.c| 5 +- drivers/mtd/nand/nandsim.c | 12 ++-- drivers/mtd/nand/ndfc.c| 22 +++--- drivers/mtd/nand/nuc900_nand.c | 21 +++--- drivers/mtd/nand/omap2.c | 94 +++--- drivers/mtd/nand/orion_nand.c | 4 +- drivers/mtd/nand/pasemi_nand.c | 14 ++-- drivers/mtd/nand/plat_nand.c | 14 ++-- drivers/mtd/nand/pxa3xx_nand.c | 33 - drivers/mtd/nand/r852.c| 34 -- drivers/mtd/nand/r852.h| 1 - drivers/mtd/nand/s3c2410.c | 19 +++--- drivers/mtd/nand/sh_flctl.c| 8 +-- drivers/mtd/nand/sharpsl.c | 18 ++--- drivers/mtd/nand/socrates_nand.c | 5 +- drivers/mtd/nand/sunxi_nand.c | 13 ++-- drivers/mtd/nand/tmio_nand.c | 7 +- drivers/mtd/nand/txx9ndfmc.c | 3 +- drivers/mtd/nand/vf610_nfc.c | 5 +- include/linux/mtd/sh_flctl.h | 3 +- 49 files changed, 383 insertions(+), 385 deletions(-) diff --git a/drivers/mtd/nand/ams-delta.c b/drivers/mtd/nand/ams-delta.c index b2b49c4..0f638c6 100644 --- a/drivers/mtd/nand/ams-delta.c +++ b/drivers/mtd/nand/ams-delta.c @@ -183,19 +183,16 @@ static int ams_delta_init(struct platform_device *pdev) return -ENXIO; /* Allocate memory for MTD device structure and private data */ - ams_delta_mtd = kzalloc(sizeof(struct mtd_info) + - sizeof(struct nand_chip), GFP_KERNEL); - if (!ams_delta_mtd) { + this = kzalloc(sizeof(struct nand_chip), GFP_KERNEL); + if (!this) { printk (KERN_WARNING "Unable to allocate E3 NAND MTD device structure.\n"); err = -ENOMEM; goto out; } + ams_delta_mtd = nand_to_mtd(this); ams_delta_mtd->owner = THIS_MODULE; - /* Get pointer to private data */ - this = (struct nand_chip *) (&ams_delta_mtd[1]); - /* Link the private data with the MTD structure */ ams_delta_mtd->priv = this; @@ -256,7 +253,7 @@ out_gpio: gpio_free(AMS_DELTA_GPIO_PIN_NAND_RB); iounmap(io_base); out_free: - kfree(ams_delta_mtd); + kfree(this); out: return err; } @@ -276,7 +273,7 @@ static int ams_delta_cleanup(struct platform_device *pdev) iounmap(io_base); /* Free the MTD device structure */ - kfree(ams_delta_mtd); + kfree(mtd_to_nand(ams_delta_mtd)); return 0; } diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index f8aac0a..51748b4 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -116,7 +116,6 @@ static struct atmel_nfc nand_nfc;