Andrew Morton wrote:

On Fri, 15 Jun 2007 14:46:03 -0600
Marc St-Jean [off-list ref] wrote:

 > [PATCH 7/12] drivers: PMC MSP71xx GPIO char driver
 >
 > Patch to add a GPIO char driver for the PMC-Sierra MSP71xx devices.

 > ...
 >
 > +/* Maps 'basic' pins to relative offset from 0 per register */
 > +static int const MSP_GPIO_OFFSET[] = {
 > +     /* GPIO 0 and 1 on the first register */
 > +     0, 0,
 > +     /* GPIO 2, 3, 4, and 5 on the second register */
 > +     2, 2, 2, 2,
 > +     /* GPIO 6, 7, 8, and 9 on the third register */
 > +     6, 6, 6, 6,
 > +     /* GPIO 10, 11, 12, 13, 14, and 15 on the fourth register */
 > +     10, 10, 10, 10, 10, 10,
 > +};

This shouldn't be in a header file.  Because each compilation unit which
includes this header will (potentially) get its own copy of the data.

That includes any userspace apps which include this header(!)

There are other drivers which use these macros irrespective of the char
driver being compiled in or not. I can't move this to the driver .c file
as all the macros will become useless.

 > +/* This gives you the 'register relative ofet gpio' number */
 > +#define OFFSET_GPIO_NUMBER(gpio)     (gpio - MSP_GPIO_OFFSET[gpio])
 > +
 > +/* These take the 'register relative offset gpio' number */
 > +#define BASIC_MODE_REG_SHIFT(ogpio)  (ogpio * 4)
 > +#define BASIC_MODE_REG_VALUE(mode, ogpio) \
 > +                     (mode << BASIC_MODE_REG_SHIFT(ogpio))
 > +#define BASIC_MODE_REG_MASK(ogpio) \
 > +                     BASIC_MODE_REG_VALUE(0xf, ogpio)
 > +#define BASIC_DATA_REG_MASK(ogpio)   (1 << ogpio)
 > +
 > +/* These take the actual GPIO number (0 through 15) */
 > +#define BASIC_DATA_MASK(gpio) \
 > +             BASIC_DATA_REG_MASK(OFFSET_GPIO_NUMBER(gpio))
 > +#define BASIC_MODE_MASK(gpio) \
 > +             BASIC_MODE_REG_MASK(OFFSET_GPIO_NUMBER(gpio))
 > +#define BASIC_MODE(mode, gpio) \
 > +             BASIC_MODE_REG_VALUE(mode, OFFSET_GPIO_NUMBER(gpio))
 > +#define BASIC_MODE_SHIFT(gpio) \
 > +             BASIC_MODE_REG_SHIFT(OFFSET_GPIO_NUMBER(gpio))
 > +#define BASIC_MODE_FROM_REG(data, gpio)      \
 > +             BASIC_MODE_REG_FROM_REG(data, OFFSET_GPIO_NUMBER(gpio))
 > +
 > +/* This gives you the 'register relative offset gpio' number */
 > +#define EXTENDED_OFFSET_GPIO(gpio)   (gpio - 16)
 > +
 > +/* These take the 'register relative offset gpio' number */
 > +#define EXTENDED_REG_DISABLE(ogpio)  (0x2 << ((ogpio * 2) + 16))
 > +#define EXTENDED_REG_ENABLE(ogpio)   (0x1 << ((ogpio * 2) + 16))
 > +#define EXTENDED_REG_SET(ogpio)              (0x2 << (ogpio * 2))
 > +#define EXTENDED_REG_CLR(ogpio)              (0x1 << (ogpio * 2))
 > +
 > +/* These take the actual GPIO number (16 through 19) */
 > +#define EXTENDED_DISABLE(gpio) \
 > +             EXTENDED_REG_DISABLE(EXTENDED_OFFSET_GPIO(gpio))
 > +#define EXTENDED_ENABLE(gpio) \
 > +             EXTENDED_REG_ENABLE(EXTENDED_OFFSET_GPIO(gpio))
 > +#define EXTENDED_SET(gpio) \
 > +             EXTENDED_REG_SET(EXTENDED_OFFSET_GPIO(gpio))
 > +#define EXTENDED_CLR(gpio) \
 > +             EXTENDED_REG_CLR(EXTENDED_OFFSET_GPIO(gpio))

inlined functions are preferred over macros.  Only use macros when for some
reason you *must* use macros.

Even for simple macros that have a single +, - or << ?

I thought there was an advantage to using macros, allowing the compiler to
combine a series of simple macro calls into a single constant.

Marc