Mike Frysinger wrote:
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.
You're defining the interface -- there are no existing users.
easy enough to turn it into: #ifndef NAND_PLAT_WRITE_CMD # error "You must define NAND_PLAT_WRITE_CMD" #endif
Or just let the compiler give an undefined symbol error.
- /* 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.
I realize that there is a "sync" defined in every architecture (otherwise, my comment would have been "this breaks on XXX arch").
However, the need to do a sync in this specific situation is specific to how NAND_PLAT_WRITE_* are implemented (in many cases, they will have already included a sync or something similar -- they're often included in the basic I/O accessors). And the specific comment about a "writebuffer" seems even more out of place in generic code.
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.
That convention has been the subject of some (quite justified, IMHO) complaints recently.
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.
The root cause of that is the namespace-polluting #define, not the function. It would just as easily cause problems with code in .c files (including when your macros get expanded) as with inline functions in headers.
see all the random times this has caused a problem with linux/glibc/uClibc and just function prototypes let alone function definitions.
This is an internal header file, not a public library header that is standards-constrained to accept #define interference from the application.
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.
On the other hand, that means that errors get caught immediately rather than when usage changes.
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
And maybe it gets used differently in the future? Or someone copies the bad example to somewhere else where it matters?
-Scott