Re: Patch of a new driver for kernel 2.4.x that need review

[Willy Tarreau]

Hi,

On Wed, Jun 22, 2005 at 10:43:40PM +0300, Pekka Enberg wrote:
> On 6/22/05, Bouchard, Sebastien <Sebastien.Bouchard@xxxxxxxxxxxxxx> wrote:
> > --- 2.4.31-ori/drivers/char/tlclk.c     Wed Dec 31 19:00:00 1969
> > +++ 2.4.31-mod/drivers/char/tlclk.c     Wed Jun 22 09:43:10 2005
> > +/* Telecom clock I/O register definition */
> > +#define TLCLK_BASE 0xa08
> > +#define TLCLK_REG0 TLCLK_BASE
> > +#define TLCLK_REG1 TLCLK_BASE+1
> > +#define TLCLK_REG2 TLCLK_BASE+2
> > +#define TLCLK_REG3 TLCLK_BASE+3
> > +#define TLCLK_REG4 TLCLK_BASE+4
> > +#define TLCLK_REG5 TLCLK_BASE+5
> > +#define TLCLK_REG6 TLCLK_BASE+6
> > +#define TLCLK_REG7 TLCLK_BASE+7
> 
> Please use enums instead.

I dont agree with you here : enums are good to simply specify an ordering.
But they must not be used to specify static mapping. Eg: if REG4 *must* be
equal to BASE+4, you should not use enums, otherwise it will render the
code unreadable. I personnaly don't want to count the position of REG7 in
the enum to discover that it's at BASE+7.

> > +#define RESET_ON 0x00
> > +#define RESET_OFF 0x01
> 
> Please use enums instead (applies to other parts of this file too).

Same comment here : if writing the *bit* 0 asserts the reset line, and
writing the *bit* 1 deasserts it, enum is not adequate at all. Enums are
adequate when you don't care about the values themselves, eg the sysctl
entries, etc... (eventhough most of them are statically assigned). But
if you need something more verbose to say exactly "write 7 to port 123",
it's better to use defines (or even variables sometimes) than enums.

Regards,
Willy

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/