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

[Scott Wood]

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?

+	/* Drain the writebuffer */
+	sync();

This doesn't look generic to me.

> +#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"?

> +#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).

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.  And if you must use macros, always use this:

#define NAND_PLAT_INIT() do { \
	multi; \
	line; \
	macro; \
} while (0) \

rather than this:

#define NAND_PLAT_INIT() \
	multi; \
	line; \
	macro;

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

-Scott