Messages in this thread | | | Date | Wed, 25 Feb 2009 10:05:18 -0300 | Subject | Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver | From | Thiago Galesi <> |
| |
On Wed, Feb 25, 2009 at 1:12 AM, David Brownell <david-b@pacbell.net> wrote: > On Tuesday 24 February 2009, Thiago Galesi wrote: >> OK a couple of things >> + >> > + >> > +#ifdef CONFIG_MTD_PARTITIONS >> > +static inline int mtd_has_partitions(void) { return 1; } >> > +#else >> > +static inline int mtd_has_partitions(void) { return 0; } >> > +#endif > > My preference would be to have those functions live in the > MTD headers. > > Needing #ifdefs in the body of probe() is *extremely* error > prone. Having function versions makes it harder to commit > a lot of the common errors I've seen, like having some mix > of options gratuitously break compiles because of missing > code fragments or variable declarations. It also makes it > easier to see when code is obviously doing the wrong thing.
Agreed. Especially about having #ifdefs in probe, no question about that, this way is _much_better_
> > Note that #ifdefs-in-functions is contrary to standard > kernel coding practices.
Yes, I know (and agree with) that 100% :)
> > I too would like to see <linux/mtd/*.h> use u8/u32/etc. But > in this case, it's just doing what the MTD framework does. > If I used u8/u32/etc the usual feedback would be "do what > all the other drivers do", "match the interface decls", etc.
Yes, that happens a lot. But a movement towards 'the right way' is always welcome.
> >> > +static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) >> > +{ >> > + struct nand_chip *chip = mtd->priv; >> > + >> > + if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0) >> > + ioread32_rep(chip->IO_ADDR_R, buf, len >> 2); >> > + else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0) >> >> What are those 0x03 and 0x01 (and other places as well), you'll have >> to spell out those, preferably using defines. > > The "0x03" is "low two bits", "0x01" is "low one bit", etc. > Those functions can't be used except when memory address > is "naturally" aligned, and the data length likewise. > > You might have an argument if you suggested a comment were > appropriate ... but those are very common idioms,
Oh, ok, now I get it. But this is still confusing. (Yes, put a comment there) No need for the ifdefs though.
Also, in the case of non-aligned acesses what is commonly done goes like this: you write the small unaligned part with 8/16 bit ops, then the rest with 32 bit ops.
Maybe it's really not worth speedwise to do all of this, but this is ARM after all :)
-- - Thiago Galesi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |