On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
> Dear José Miguel Gonçalves,
>
> > NAND Flash driver with HW ECC for the S3C24XX SoCs.
> > Currently it only supports SLC NAND chips.
> >
> > Signed-off-by: José Miguel Gonçalves <jose.goncal...@inov.pt>
>
> [...]
>
> > +#include <common.h>
> > +#include <nand.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/s3c24xx_cpu.h>
> > +#include <asm/errno.h>
> > +
> > +#define MAX_CHIPS 2
> > +static int nand_cs[MAX_CHIPS] = { 0, 1 };
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +#define printf(arg...) do {} while (0)
>
> This doesn't seem quite right ...
>
> 1) this should be in CPU directory
> 2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
> 3) should be inline function, not a macro
What specifically should be in the CPU directory?
> > +#endif
> > +
> > +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
> > +{
> > + struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
> > + u_long nfcont;
> > +
> > + nfcont = readl(&nand->nfcont);
> > +
> > + switch (chip) {
> > + case -1:
> > + nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
> > + break;
> > + case 0:
> > + nfcont &= ~NFCONT_NCE0;
> > + break;
> > + case 1:
> > + nfcont &= ~NFCONT_NCE1;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + writel(nfcont, &nand->nfcont);
> > +}
> > +
> > +/*
> > + * Hardware specific access to control-lines function
> > + */
> > +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
> > ctrl) +{
> > + struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
> > + struct nand_chip *this = mtd->priv;
> > +
> > + if (ctrl & NAND_CTRL_CHANGE) {
> > + if (ctrl & NAND_CLE)
> > + this->IO_ADDR_W = &nand->nfcmmd;
> > + else if (ctrl & NAND_ALE)
> > + this->IO_ADDR_W = &nand->nfaddr;
> > + else
> > + this->IO_ADDR_W = &nand->nfdata;
>
> Why don't you use local variable here?
Because then you'll access the wrong data when the function is called
again without NAND_CTRL_CHANGE. This is a common way of writing the
hwcontrol function, though the way ndfc.c does it is simpler (you can use
a local variable if you ignore NAND_CTRL_CHANGE altogether).
> > + if (ctrl & NAND_NCE)
> > + s3c_nand_select_chip(mtd, *(int *)this->priv);
>
> Uh, how's this *(int *) supposed to work?
Usually driver-private data is a struct; apparently in this driver it's
an int.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
