On Monday 13 April 2009 19:02:05 Scott Wood wrote:
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.
as i said, i'm not familiar with other use cases, so i cant pick out exactly what is common/required and what isnt. you can just state which is which and i'll do it.
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.
there's a difference between "used" and "required". the #error makes sense in the face of no documentation.
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".
that works for me
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).
OK
"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".
it used to be in there, but it isnt anymore. guess it changed since i last read it (but that was back in like 1.1.6 days).
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.
which is why so many of the exported headers use macros rather than static inlines. they only get expanded and make a mess when they actually get used.
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.
except using defines avoiding the need to "fix" files constantly
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.
static inline usage works in the kernel because the headers are separated logically -- there is no config.h which gets included by every file and contains static inline functions or complex macros.
my point is that "static inline" vs "define" isnt as cut as try as you may like. if we had a dedicated header file that only was included when needed (or included with the nand headers), then i'd have no problem with static inline. but considering the board config header is included *everywhere*, including assembly files and when building the utilities on the host, C code should be avoided in it whenever possible.
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.
thanks -mike