From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Mon, 13 Apr 2009 16:42:00 -0500 Subject: [U-Boot] [PATCH] mtd: nand: new base driver for memory mapped nand devices In-Reply-To: <200904131718.19031.vapier@gentoo.org> References: <1239499602-19356-1-git-send-email-vapier@gentoo.org> <20090413155930.GD12632@ld0162-tx32.am.freescale.net> <200904131718.19031.vapier@gentoo.org> Message-ID: <49E3B1A8.3060104@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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