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

[Scott Wood]

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