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

Bill Gatliff
2005-06-22T21:03:46Z

Willy:

Willy Tarreau wrote:

>Hi,
>
>On Wed, Jun 22, 2005 at 10:43:40PM +0300, Pekka Enberg wrote:
>
>
>>On 6/22/05, Bouchard, Sebastien <Sebastien.Bouchard@ca.kontron.com> wrote:
>>
>>
>>>+#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.
>
>

What Sebastien is after is something like this:

enum tclk_regid {TCLK_BASE=0xa80, TCLK_REG0=TCLK_BASE, TCLK_REG1=TCLK_BASE+1...};
enum tclk_regid tclk;

And then later on, if you ask gdb with the value of tclk is, it can tell you "TCLK_REG1", instead of just 0xa801. You can also assign values to tclk from within gdb using the enumerations, rather than magic numbers.

If you insist on using #defines, then you need to do them like this at the very least:

#define TCLK_REG7 (TCLK_BASE+7)

... so as to prevent operator precedence problems later on. I.e. what happens here:

tclk = TCLK_REG7 / 2;

Not implying that the above is a realistic example, I'm just pointing out a potential gotcha that is easily avoided...

b.g.

--
Bill Gatliff
"A DTI spokesman said Wicks would use his debut announcement to
'reaffirm the government's commitment to developing wind' to tackle
the threat of climate change." -- The Observer, May 22, 2005
bgat@billgatliff.com