[U-Boot] [PATCH 02/12] SPL: NAND: add support for mxs nand

[Scott Wood]

On Mon, 2014-04-28 at 13:17 -0700, Tim Harvey wrote:
> +static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
> +			     int column, int page_addr)
> +{
> +	register struct nand_chip *chip = mtd->priv;
> +	int ctrl = NAND_NCE | NAND_CTRL_CLE | NAND_CTRL_CHANGE;
> +	u32 timeo, time_start;
> +
> +	/* write out the command to the device */
> +	chip->cmd_ctrl(mtd, command, ctrl);
> +
> +	/* Address cycle, when necessary */
> +	ctrl = NAND_NCE | NAND_CTRL_ALE | NAND_CTRL_CHANGE;
> +	/* Serially input address */
> +	if (column != -1) {
> +		ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;

What happened to NAND_NCE?

It looks like the cmd_ctrl in nand_mxs.c doesn't care about NAND_MCE at
all, so there's no functional impact, but either use it consistently or
don't use it at all.

> +		chip->cmd_ctrl(mtd, column, ctrl);
> +		ctrl &= ~NAND_CTRL_CHANGE;
> +		chip->cmd_ctrl(mtd, column >> 8, ctrl);
> +	}

Why not pass the ctrl flags directly to cmd_ctrl as you do in most of
the rest of the function?  nand_mxs.c doesn't even look at NAND_CTRL_CHANGE.

> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +{
> +	register struct nand_chip *chip;

Why "register"?

> +		if (!(page % nand_page_per_block)) {
> +			/*
> +			 * Yes, new block. See if this block is good. If not,
> +			 * loop until we find a good block.
> +			 */
> +			while (is_badblock(&mtd, offs, 1)) {
> +				page = page + nand_page_per_block;
> +				/* Check i we've reached the end of flash. */
> +				if (page >= mtd.size >> chip->page_shift)
> +					return -1;

Why -1 rather than a real error value?

-Scott