[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 [off-list ref] wrote:

Hi Boris,

On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote:

quoted

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 <redacted>
Cc: Julia Lawall <redacted>
---
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.

quoted

---
 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.

quoted

 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(-)

...

quoted

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

...

quoted

@@ -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?

quoted

 			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?

quoted

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