Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip

[Brian Norris]

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.

> ---
>  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, 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 {

...

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

>  
>  	switch (cmd) {
> @@ -267,7 +267,8 @@ static void au1550_select_chip(struct mtd_info *mtd, int chip)
>   */
>  static void au1550_command(struct mtd_info *mtd, unsigned command, int column, int page_addr)
>  {
> -	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);

Same here.

>  	int ce_override = 0, i;
>  	unsigned long flags = 0;

Snipped the rest, since it's pretty similar comments that apply.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/