Re: [PATCH 7/12] drivers: PMC MSP71xx GPIO char driver

[Marc St-Jean <Marc_St-Jean@pmc-sierra.com>]

Andrew Morton wrote:
> On Fri, 15 Jun 2007 14:46:03 -0600
> Marc St-Jean <stjeanma@pmc-sierra.com> 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