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

[Andrew Morton <akpm@linux-foundation.org>]

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.


> +const u32 GPIO_DATA_COUNT[] = { 2, 4, 4, 6 };
> +const u32 GPIO_DATA_SHIFT[] = { 0, 2, 6, 10 };
> +const u32 GPIO_DATA_MASK[] = { 0x03, 0x0f, 0x0f, 0x3f };
> +u32 * const GPIO_DATA_REG[] = {
> +	(u32 *)GPIO_DATA1_REG, (u32 *)GPIO_DATA2_REG,
> +	(u32 *)GPIO_DATA3_REG, (u32 *)GPIO_DATA4_REG,
> +};
> +const u32 GPIO_CFG_MASK[] = { 0x0000ff, 0x00ffff, 0x00ffff, 0xffffff };
> +u32 * const GPIO_CFG_REG[] = {
> +	(u32 *)GPIO_CFG1_REG, (u32 *)GPIO_CFG2_REG,
> +	(u32 *)GPIO_CFG3_REG, (u32 *)GPIO_CFG4_REG,
> +};
> +
> +/* Maps MODE to allowed pin mask */
> +const u32 MSP_GPIO_MODE_ALLOWED[] = {
> +	0xfffff,	/* Mode 0 - INPUT */
> +	0x00000,	/* Mode 1 - INTERRUPT */
> +	0x00030,	/* Mode 2 - UART_INPUT (GPIO 4, 5)*/
> +	0, 0, 0, 0, 0,	/* Modes 3, 4, 5, 6, and 7 are reserved */
> +	0xfffff,	/* Mode 8 - OUTPUT */
> +	0x0000f,	/* Mode 9 - UART_OUTPUT/PERF_TIMERA (GPIO 0,1,2,3) */
> +	0x00003,	/* Mode a - PERF_TIMERB (GPIO 0, 1) */
> +	0x00000,	/* Mode b - Not really a mode! */
> +};

all the above should be lower-case and have static scope.

> +#define GPIO_CFG_SHIFT(i)	(i * 4)
> +#define GPIO_CFG_PINMASK	0xf
> +
> +/* The extended gpio register */
> +
> +#define EXTENDED_GPIO_COUNT	4
> +#define EXTENDED_GPIO_SHIFT	16
> +#define EXTENDED_GPIO_MASK	0x0f
> +
> +#define EXTENDED_GPIO_DATA_SHIFT(i)	(i * 2)
> +#define EXTENDED_GPIO_DATA_MASK(i)	(0x3 << (i*2))
> +#define EXTENDED_GPIO_DATA_SET		0x2
> +#define EXTENDED_GPIO_DATA_CLR		0x1
> +#define EXTENDED_GPIO_CFG_SHIFT(i)	((i * 2) + 16)
> +#define EXTENDED_GPIO_CFG_MASK(i)	(0x3 << ((i*2)+16))
> +#define EXTENDED_GPIO_CFG_DISABLE	0x2
> +#define EXTENDED_GPIO_CFG_ENABLE	0x1
> +
> +/* -- Data structures -- */
> +
> +static DEFINE_SPINLOCK(msp_gpio_spinlock);
> +
> +static struct task_struct *msp_blinkthread;
> +static DEFINE_SPINLOCK(msp_blink_lock);
> +static DECLARE_COMPLETION(msp_blink_wait);
> +
> +struct blink_table {
> +	u32 count;
> +	u32 period;
> +	u32 dcycle;
> +};
> +
> +static struct blink_table blink_table[MSP_NUM_GPIOS];
> +
> +/* -- Utility functions -- */
> +
> +/* Define the following for extra debug output */
> +#undef DEBUG

The under shouldn't be needed: -DDEBUG can be provided on the kbuild
command line.

> +#ifdef DEBUG
> +#define DBG(args...) printk(KERN_DEBUG args)
> +#else
> +#define DBG(args...) do {} while (0)
> +#endif
> +
> +/* Reads the data bits from a single register set */
> +static u32 msp_gpio_read_data_basic(int reg)
> +{
> +	return read_reg32(GPIO_DATA_REG[reg], GPIO_DATA_MASK[reg]);
> +}
> +
> +/* Reads the data bits from the extended register set */
> +static u32 msp_gpio_read_data_extended(void)
> +{
> +	int pin;
> +	u32 tmp = *EXTENDED_GPIO_REG;

`tmp' is always a bad name for a variable.

> +	u32 retval = 0;
> +	
> +	for (pin = 0; pin < EXTENDED_GPIO_COUNT; pin++) {
> +		u32 bit = 0;
> +		
> +		/*
> +		 * In output mode, read CLR bit
> +		 * In input mode, read SET bit
> +		 */
> +		if (tmp & (EXTENDED_GPIO_CFG_ENABLE <<
> +				EXTENDED_GPIO_CFG_SHIFT(pin)))
> +			bit = EXTENDED_GPIO_DATA_CLR <<
> +				EXTENDED_GPIO_DATA_SHIFT(pin);
> +		else
> +			bit = EXTENDED_GPIO_DATA_SET <<
> +				EXTENDED_GPIO_DATA_SHIFT(pin);
> +
> +		if (tmp & bit)
> +			retval |= 1 << pin;
> +	}
> +	
> +	return retval;
> +}
> +
> +/*
> + * Reads the current state of all 20 pins, putting the values in
> + * the lowest 20 bits (1=HI, 0=LO)
> + */
> +static u32 msp_gpio_read_data(void)
> +{
> +	int reg;
> +	u32 retval = 0;
> +	
> +	spin_lock(&msp_gpio_spinlock);
> +	for (reg = 0; reg < GPIO_REG_COUNT; reg++)
> +		retval |= msp_gpio_read_data_basic(reg) <<
> +				GPIO_DATA_SHIFT[reg];
> +	retval |= msp_gpio_read_data_extended() << EXTENDED_GPIO_SHIFT;
> +	spin_unlock(&msp_gpio_spinlock);
> +	
> +	DBG("%s: 0x%08x\n", __FUNCTION__, retval);
> +	return retval;
> +}
> +
>
> ...
>
> +/* 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(!)

> +/* 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.