Hi Brian, On Mon, 16 Nov 2015 19:00:19 -0800 Brian Norris <computersforpeace@xxxxxxxxx> 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 <boris.brezillon@xxxxxxxxxxxxxxxxxx> > > Cc: Julia Lawall <Julia.Lawall@xxxxxxx> > > --- > > 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/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, 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? > > > diff --git a/drivers/mtd/nand/au1550nd.c b/drivers/mtd/nand/au1550nd.c > > index 73fceb8..7e2a376 100644 > > --- a/drivers/mtd/nand/au1550nd.c > > +++ b/drivers/mtd/nand/au1550nd.c > > @@ -23,7 +23,6 @@ > > > > > > struct au1550nd_ctx { > > - struct mtd_info info; > > struct nand_chip chip; > > > > int cs; > > @@ -197,7 +196,8 @@ static void au_read_buf16(struct mtd_info *mtd, u_char *buf, int len) > > > > static void au1550_hwcontrol(struct mtd_info *mtd, int cmd) > > { > > - struct au1550nd_ctx *ctx = container_of(mtd, struct au1550nd_ctx, info); > > + struct au1550nd_ctx *ctx = container_of(mtd_to_nand(mtd), > > + struct au1550nd_ctx, chip); > > struct nand_chip *this = mtd_to_nand(mtd); > > This is another good example. It's a little awkward to do this at all > (function call within a macro): > > container_of(mtd_to_nand(mtd), ...); > > but that's not unforgiveable. It's a bit worse, though, when followed by > assigning the next field to the same thing: > > struct nand_chip *this = mtd_to_nand(mtd); > > i.e., this would be nicer to see as: > > struct nand_chip *this = mtd_to_nand(mtd); > struct au1550nd_ctx *ctx = container_of(this, > struct au1550nd_ctx, chip); > > Again, I'm not sure how best to automate this kind of transformation. Yes, I noticed all those problems too, but as I said, I don't know how to handle them using a coccinelle script. Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
