[U-Boot] [PATCH] mtd: nand: new base driver for memory mapped nand devices

[Scott Wood]

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