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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
