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