On Monday 13 April 2009 17:42:00 Scott Wood wrote:
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.
just because *my* device needs it doesnt mean every device needs it. i was trying to be nice from the get go.
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.
true, but i think that route leads to people grepping files and scratching their heads. an #error would save them some time.
- /* 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.
if they're included in the I/O accessors, then the arch most likely defines sync() to nothing, so it doesnt matter. "write buffer" may not be entirely arch independent, but it conveys the exact same thing as "make sure write makes it to external device". this is how sync() is used -- just grep the drivers tree and see the smsc driver for example.
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.
or accidental shadowing of global state, but i guess you dont care much about that usage either
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.
really ? you call internal kernel headers "standards constrained" ? my point is that it's seen in both scenarios.
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.
indeed
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?
people should check their work before they hand in their paper ;) -mike