Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Thiago Galesi <thiagogalesi@gmail.com>]

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