Mike Frysinger wrote:
On Monday 13 April 2009 17:42:00 Scott Wood wrote:
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.
Every device that is a reasonable candidate for using such a shared driver does need it. If a device is different enough that a "write a command byte/word" operation doesn't map well (such as Freescale's eLBC), then we shouldn't try to force it into a so-called generic framework.
In other words, this is just to templatize a common NAND driver pattern, not to replace the main NAND driver interface.
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.
If we did that every place some arch- or platform-defined symbol were used, the source code would be (even more of) a mess.
Instead, how about putting a comment at the top of the file stating what it requires from the platform, and describing the semantics of each item? That would save more head-scratching than simply turning "not defined" into "you must define".
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.
Um, no. See powerpc for example.
Explicit barriers are for when you use raw I/O accessors in optimized paths, or when you need to synchronize accesses to main memory (such as in a DMA buffer).
"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.
I did grep, and the only non-arch-specific use I found was in cfi_flash. I'm not sure what you mean by the "smsc driver".
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
I care very much about avoiding accidents. Preprocessor macros cause accidents. :-)
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" ?
Linux contains headers used by glibc; those parts are indeed standards constrained. And while the user-visible headers that aren't directly included in a glibc header aren't specifically constrained by the C standard, they're intended to be used in more or less arbitrary C environments in ways similar to the C library, so quality-of-implementation dictates that similar care be taken.
As for the internal headers, any such problem could be remedied by fixing the offending #define. Further, inline functions seem to be preferred over macros in Linux, so I'm not sure that they're the best example to pick to justify using macros.
Indeed, I'm not sure what relevance this has at all -- the solution to the "variable has the same name as a user define" problem in public headers is to use a reserved namespace, not to use a macro. Macros make the problem worse by making it possible to alias local variables in the expanding context (which may have been used as macro parameters), as well as #defines.
Which reminds 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))
Put parentheses around chip, in case the caller provided a complex expression.
-Scott