From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Wed, 03 Jun 2009 14:50:40 -0500
Subject: [U-Boot] [PATCH 2/2] KB9202: Add NAND support
In-Reply-To: <20090603174200.GC3813@darwin>
References: <20090515221544.GF7923@traven> <4A0DED18.9000806@freescale.com>
	<20090603174200.GC3813@darwin>
Message-ID: <4A26D410.8050806@freescale.com>
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

Matthias Kaehlcke wrote:
> hi,
> 
> El Fri, May 15, 2009 at 05:30:48PM -0500 Scott Wood ha dit:
> 
>> Matthias Kaehlcke wrote:
>>> +/*
>>> + * Board-specific function to access the device ready signal.
>>> + */
>>> +static int kb9202_nand_ready(struct mtd_info *mtd)
>>> +{
>>> +	return (((AT91C_BASE_PIOC->PIO_PDSR) & KB9202_NAND_BUSY) != 0);
>>> +}
>> Use I/O accessors.
[snip]
> +		if (ctrl & NAND_NCE)
> +			AT91C_BASE_PIOC->PIO_CODR = KB9202_NAND_NCE;
> +		else
> +			AT91C_BASE_PIOC->PIO_SODR = KB9202_NAND_NCE;

You're still not using I/O accessors in many places.

> +#ifdef CONFIG_CMD_NAND

Put this in the makefile instead.

> +static int kb9202_nand_ready(struct mtd_info *mtd)
> +{
> +	const unsigned int value = readl(AT91C_PIOC_PDSR);
> +
> +	return ((value & KB9202_NAND_BUSY) != 0);
> +}

static int kb9202_nand_ready(struct mtd_info *mtd)
{
	return readl(AT91C_PIOC_PDSR) & KB9202_NAND_BUSY;
}

> +int board_nand_init(struct nand_chip *nand)
> +{
> +	unsigned int value;
> +	struct _AT91S_SMC2 *at91s_smc2 = AT91C_BASE_SMC2;
> +
> +	nand->ecc.mode = NAND_ECC_SOFT;
> +	nand->options &= ~(NAND_BUSWIDTH_16);

Unnecessary parens.

> +	nand->cmd_ctrl = kb9202_nand_hwcontrol;
> +	nand->dev_ready = kb9202_nand_ready;
> +
> +	/* in case running outside of bootloader */
> +	AT91C_BASE_PMC->PMC_PCER = ((unsigned) 1 << AT91C_ID_PIOC);

Unnecessary cast and parens.

> +	at91s_smc2->SMC2_CSR[3] =
> +		AT91C_SMC2_WSEN |
> +		(4 & AT91C_SMC2_NWS) |
> +		((1 << 8) & AT91C_SMC2_TDF) |
> +		AT91C_SMC2_DBW_8 |
> +		((1 << 24) & AT91C_SMC2_RWSETUP) |
> +		((1 << 29) & AT91C_SMC2_RWHOLD);

Are those instances of (constant1 & constant2) ever going to evaluate to 
anything but constant1?  Can we get rid of the magic numbers?

Otherwise, ACK.

-Scott