From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Fri, 2 May 2014 15:56:25 -0500
Subject: [U-Boot] [PATCH 02/12] SPL: NAND: add support for mxs nand
In-Reply-To: <1398716258-8420-3-git-send-email-tharvey@gateworks.com>
References: <1398716258-8420-1-git-send-email-tharvey@gateworks.com>
	<1398716258-8420-3-git-send-email-tharvey@gateworks.com>
Message-ID: <1399064185.15726.63.camel@snotra.buserror.net>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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