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

[Mike Frysinger]

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
-------------- 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/cc58369b/attachment.pgp