linux-mips
[Top] [All Lists]

Re: [PATCH] eXcite nand flash driver

To: Ulrich Eckhardt <eckhardt@satorlaser.com>
Subject: Re: [PATCH] eXcite nand flash driver
From: Thomas Koeller <thomas.koeller@baslerweb.com>
Date: Thu, 8 Feb 2007 11:20:25 +0100
Cc: linux-mips@linux-mips.org
In-reply-to: <200702080920.12723.eckhardt@satorlaser.com>
Organization: Basler AG
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200702080157.25432.thomas.koeller@baslerweb.com> <200702080920.12723.eckhardt@satorlaser.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: KMail/1.9.5
On Thursday 08 February 2007 09:20, Ulrich Eckhardt wrote:
> Just some cosmetic notes ...
>
> On Thursday 08 February 2007 01:57, Thomas Koeller wrote:
> > @@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
> >       Even if you leave this disabled, you can enable BBT writes at module
> >       load time (assuming you build diskonchip as a module) with the module
> >       parameter "inftl_bbt_write=1".
> > -
> > +
>
> This just inserts whitespace.

No, it actually _removes_ whitspace.

>
> >         tristate "NAND support for OLPC CAFÉ chip"
>
> The accent on the E might cause problems, but I'm not sure.

This has been in there before.

>
> > +#define io_readb(__a__)            __raw_readb((__a__))
> > +#define io_writeb(__v__, __a__)    __raw_writeb((__v__), (__a__))
>
> I would have used an inline function. Also, aren't there functions to use
> IOs already? What's the reason here?
>
> > +/* prefix for debug output */
> > +static const char module_id[] = "excite_nandflash";
>
> I'd have used
>
>   static const char* module_id = "excite_nandflash";
>
> except if you need the sizeof it to capture the length.

Then I'd have used
        static const char * const module_id = "excite_nandflash";

but what is the advantage of doing so?

>
> > +static inline const struct resource *excite_nand_get_resource
> > +    (struct platform_device *d, unsigned long flags, const char
> > *basename){ +       const char fmt[] = "%s_%u";
>
> Here, too, is no need for a local buffer, just use a pointer.
>
> > +/* excite_nand_probe
> > + *
> > + * called by device layer when it finds a device matching
> > + * one our driver can handled. [...]
>
> "... when it finds a device our driver can handle."

Right.

>
> Otherwise I'd say it looks very clean, better than quite some other stuff I
> have seen.
>
> Uli
>

Thanks for your comments!


_______________________________

Thomas Köller, Software Developer

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel. +49 (0) 4102 - 463 390
Fax +49 (0) 4102 - 463 390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Vorstand: Dr.-Ing. Dietmar Ley (Vorsitzender) · John P. Jennings · Peter 
Krumhoff · Aufsichtsratsvorsitzender: Norbert Basler
Basler AG · Amtsgericht Ahrensburg HRB 4090 · Ust-IdNr.: DE 135 098 121 · 
Steuer-Nr.: 30 292 04497

_______________________________


<Prev in Thread] Current Thread [Next in Thread>