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