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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090413/7d02c21b/attachment.pgp