[U-Boot] [PATCH] mtd: nand: new base driver for memory mapped nand devices

[Mike Frysinger]

On Monday 13 April 2009 11:59:30 Scott Wood wrote:
> On Sat, Apr 11, 2009 at 09:26:42PM -0400, Mike Frysinger wrote:
> > +#ifdef NAND_PLAT_WRITE_CMD
>
> Why would a user select this driver without providing the necessary
> definitions -- and if they do, why do you want anything other than
> a compilation error to result?

*shrug* ... i'm not completely familiar with the nand layers and what people 
have done to know exactly what is optional.  easy enough to turn it into:
#ifndef NAND_PLAT_WRITE_CMD
# error "You must define NAND_PLAT_WRITE_CMD"
#endif

> +	/* Drain the writebuffer */
> +	sync();
>
> This doesn't look generic to me.

yes it does.  every arch should define "sync()" in asm/io.h.  if it doesnt, 
your arch is broken.

> > +#define BFIN_NAND_CLE(chip) ((unsigned long)chip->IO_ADDR_W | (1 << 2))
> > +#define BFIN_NAND_ALE(chip) ((unsigned long)chip->IO_ADDR_W | (1 << 1))
> > +#define BFIN_NAND_READY     PF3
>
> You have a global variable called "PF3"?

a global define actually, but yes

> > +#define NAND_PLAT_WRITE_CMD(cmd, chip) bfin_write8(BFIN_NAND_CLE(chip),
> > cmd) +#define NAND_PLAT_WRITE_ADR(cmd, chip)
> > bfin_write8(BFIN_NAND_ALE(chip), cmd) +#define NAND_PLAT_DEV_READY(chip) 
> >     ((*pPORTFIO & BFIN_NAND_READY) ? 1 : 0) +#define NAND_PLAT_INIT() \
> > +	*pPORTF_FER &= ~BFIN_NAND_READY; \
> > +	*pPORTFIO_DIR &= ~BFIN_NAND_READY; \
> > +	*pPORTFIO_INEN |= BFIN_NAND_READY;
>
> I'm not too fond of such things being done through header files -- it
> means that only one type of so-called "memory mapped" NAND device can be
> supported in a given u-boot image.  If it doesn't add too much image size
> overhead, I'd prefer having platform code register a struct of callbacks
> (or just live with the duplication of 10-15 almost-but-not-quite-generic
> lines, and focus on factoring out instances where they're truly
> identical).

doing it in the header follows u-boot convention, and it's much easier than 
creating a dedicated file.  doesnt matter to me.

> If we do do it in the header file, though, at least use static inline
> functions rather than macros -- besides being less visually obnoxious,
> they provide type checking of arguments and avoid problems with name
> collisions.

actually, it kind of does the opposite.  it increases name space pollution.  
if someone does a #define with the same variable name or similar as is used in 
the function, then you can easily get a build failure.  see all the random 
times this has caused a problem with linux/glibc/uClibc and just function 
prototypes let alone function definitions.  plus, not so critically, using 
static inlines would slow down the compiler as it would need to compile & 
optimize & consider it in every single file rather than letting the CPP cull 
it early on.

> The latter will break if you put it in the body of a single-line if
> statement.

i'm fully aware of this, but didnt care since i knew how it was used
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090413/227bf00c/attachment.pgp