[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Scott Wood]

On Mon, 2013-12-02 at 11:24 +0800, Bo Shen wrote:
> Prepare for nand spl boot support. It supports nand software ECC and
> hardware PMECC.
> This patch is take <drivers/mtd/nand/nand_spl_simple.c> as reference.
> 
> Signed-off-by: Bo Shen <voice.shen at atmel.com>
> ---
>  drivers/mtd/nand/atmel_nand.c |  206 +++++++++++++++++++++++++++++++++++++++++
>  include/nand.h                |    6 ++
>  2 files changed, 212 insertions(+)

Looks OK but some style nits:
 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index da83f06..64e11e1 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -32,6 +32,12 @@
>  
>  #ifdef CONFIG_ATMEL_NAND_HW_PMECC
>  
> +#ifdef CONFIG_SPL_BUILD
> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
> +#undef CONFIG_SYS_NAND_ONFI_DETECTION
> +#endif
> +#endif

There's no need to ifdef before undeffing.

>  struct atmel_nand_host {
>  	struct pmecc_regs __iomem *pmecc;
>  	struct pmecc_errloc_regs __iomem *pmerrloc;
> @@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd)
>  }
>  #endif
>  
> +#ifndef CONFIG_SPL_BUILD

Use ifdef rather than ifndef for if/else.

> +#ifdef CONFIG_SPL_NAND_SOFTECC

This symbol needs to be documented (I realize it isn't new).

> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \
> +		  CONFIG_SYS_NAND_ECCSIZE)
> +#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES)
> +#endif

Is this stuff used anywhere but nand_read_page()?  If not, move it
there (as local variables, not #defines).

> +static int nand_read_page(int block, int page, void *dst)
> +{
> +	struct nand_chip *this = mtd.priv;
> +#ifdef CONFIG_SPL_NAND_SOFTECC
> +	u_char ecc_calc[ECCTOTAL];
> +	u_char ecc_code[ECCTOTAL];
> +	u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
> +	int eccsize = CONFIG_SYS_NAND_ECCSIZE;
> +	int eccbytes = CONFIG_SYS_NAND_ECCBYTES;
> +	int eccsteps = ECCSTEPS;
> +	int i;
> +	uint8_t *p = dst;
> +#endif
> +	nand_command(block, page, 0, NAND_CMD_READ0);
> +
> +#ifdef CONFIG_SPL_NAND_SOFTECC
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		if (this->ecc.mode != NAND_ECC_SOFT)
> +			this->ecc.hwctl(&mtd, NAND_ECC_READ);
> +		this->read_buf(&mtd, p, eccsize);
> +		this->ecc.calculate(&mtd, p, &ecc_calc[i]);
> +	}
> +	this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);
> +
> +	for (i = 0; i < ECCTOTAL; i++)
> +		ecc_code[i] = oob_data[nand_ecc_pos[i]];
> +
> +	eccsteps = ECCSTEPS;
> +	p = dst;
> +
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
> +		this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]);
> +#else
> +	atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page);
> +#endif
> +
> +	return 0;
> +}

These don't share enough code to warrant interleaving like this -- just
have one big ifdef/else.  It will be more readable.

-Scott