Re: [PATCH 1/2] msm: Install the Google-Android gpio driver.

[Ryan Mallon <ryan@bluewatersys.com>]

On 09/02/2010 09:13 AM, Daniel Walker wrote:
> On Wed, 2010-09-01 at 13:59 -0500, H Hartley Sweeten wrote:
>> On Monday, August 30, 2010 5:18 PM, Gregory Bean wrote:
>>> Subject: [PATCH 1/2] msm: Install the Google-Android gpio driver.
>>>
>>> From: Arve Hjønnevåg <arve@android.com>
>>>
>>> As part of the ongoing effort to converge on a common code base,
>>> adopt the Google-Android msmgpio driver, as it has a stronger pedigree
>>> than the previous submission from codeaurora.
>>>
>>> Cc: Arve Hjønnevåg <arve@android.com>
>>> Signed-off-by: Gregory Bean <gbean@codeaurora.org>
>>
>> Hello Greg,
>>
>> A couple comments on this.
>>
>>> +struct msm_gpio_chip msm_gpio_chips[] = {
>>> +	{
>>> +		.regs = {
>>> +			.out =         GPIO_OUT_0,
>>> +			.in =          GPIO_IN_0,
>>> +			.int_status =  GPIO_INT_STATUS_0,
>>> +			.int_clear =   GPIO_INT_CLEAR_0,
>>> +			.int_en =      GPIO_INT_EN_0,
>>> +			.int_edge =    GPIO_INT_EDGE_0,
>>> +			.int_pos =     GPIO_INT_POS_0,
>>> +			.oe =          GPIO_OE_0,
>>> +		},
>>> +		.chip = {
>>> +			.base = 0,
>>> +			.ngpio = 16,
>>> +			.get = msm_gpio_get,
>>> +			.set = msm_gpio_set,
>>> +			.direction_input = msm_gpio_direction_input,
>>> +			.direction_output = msm_gpio_direction_output,
>>> +			.to_irq = msm_gpio_to_irq,
>>> +		}
>>> +	},
>>
>> This is a bit ugly... A 204 line struct definition is a bit hard to follow,
>> especially the way it's broken up with all the #if defined stuff.
>>
>> Each gpio "bank" is code duplication other than the .base and .ngpio.  The
>> whole thing can be reduced using a helper macro.  Something like this:
>>
>> #define MSM_GPIO_BANK(bank, base_gpio, num_gpio)			\
>> 	{										\
>> 		.regs = {								\
>> 			.out			= GPIO_OUT_##bank,		\
>> 			.in			= GPIO_IN_##bank,			\
>> 			.int_status		= GPIO_INT_STATUS_##bank,	\
>> 			.int_clear		= GPIO_INT_CLEAR_##bank,	\
>> 			.int_en		= GPIO_INT_EN_##bank,		\
>> 			.int_edge		= GPIO_INT_EDGE_##bank,		\
>> 			.int_pos		= GPIO_INT_POS_##bank,		\
>> 			.oe			= GPIO_OE_##bank,			\
>> 		},									\
>> 		.chip = {								\
>> 			.direction_input	= msm_gpio_direction_input,	\
>> 			.direction_output	= msm_gpio_direction_output,	\
>> 			.get			= msm_gpio_get,			\
>> 			.set			= msm_gpio_set,			\
>> 			.to_irq		= msm_gpio_to_irq,		\
>> 			.base			= base_gpio,			\
>> 			.ngpio		= num_gpio,				\
>> 		},									\
>> 	}
>>
>> Then the struct definition can be reduced to this:
>>
>> struct msm_gpio_chip msm_gpio_chips[] = {
>> #if defined(CONFIG_ARCH_MSM7X30)
>> 	MSM_GPIO_BANK(0, 0, 16),	/* gpio 0-15 */
>> 	MSM_GPIO_BANK(1, 16, 28),	/* gpio 16-43 */
>> 	MSM_GPIO_BANK(2, 44, 24),	/* gpio 44-67 */
>> 	MSM_GPIO_BANK(3, 68, 27),	/* gpio 68-94 */
>> 	MSM_GPIO_BANK(4, 95, 12),	/* gpio 95-106 */
>> 	MSM_GPIO_BANK(5, 107, 27),	/* gpio 107-133 */
>> 	MSM_GPIO_BANK(6, 134, 17),	/* gpio 134-150 */
>> 	MSM_GPIO_BANK(7, 151, 31),	/* gpio 151-181 */
>> #elif defined(CONFIG_ARCH_QSD8X50)
>> 	MSM_GPIO_BANK(0, 0, 16),	/* gpio 0-15 */
>> 	MSM_GPIO_BANK(1, 16, 27),	/* gpio 16-42 */
>> 	MSM_GPIO_BANK(2, 43, 25),	/* gpio 43-67 */
>> 	MSM_GPIO_BANK(3, 68, 27),	/* gpio 68-94 */
>> 	MSM_GPIO_BANK(4, 95, 9),	/* gpio 95-103 */
>> 	MSM_GPIO_BANK(5, 104, 18),	/* gpio 104-121 */
>> 	MSM_GPIO_BANK(6, 122, 31),	/* gpio 122-152 */
>> 	MSM_GPIO_BANK(7, 153, 12),	/* gpio 153-164 */
>> #else
>> 	MSM_GPIO_BANK(0, 0, 16),	/* gpio 0-15 */
>> 	MSM_GPIO_BANK(1, 16, 27),	/* gpio 16-42 */
>> 	MSM_GPIO_BANK(2, 43, 25),	/* gpio 43-67 */
>> 	MSM_GPIO_BANK(3, 68, 27),	/* gpio 68-94 */
>> 	MSM_GPIO_BANK(4, 95, 12),	/* gpio 95-106 */
>> 	MSM_GPIO_BANK(5, 107, 15),	/* gpio 107-121 */
>> #endif
>> };

Which could also be further reduced to:

struct msm_gpio_chip msm_gpio_chips[] = {
	MSM_GPIO_BANK(0,   0, 16),	/* gpio 0-15 */
#if defined(CONFIG_ARCH_MSM7X30)
	MSM_GPIO_BANK(1,  16, 28),	/* gpio 16-43 */
 	MSM_GPIO_BANK(2,  44, 24),	/* gpio 44-67 */
 	MSM_GPIO_BANK(3,  68, 27),	/* gpio 68-94 */
 	MSM_GPIO_BANK(4,  95, 12),	/* gpio 95-106 */
 	MSM_GPIO_BANK(5, 107, 27),	/* gpio 107-133 */
 	MSM_GPIO_BANK(6, 134, 17),	/* gpio 134-150 */
 	MSM_GPIO_BANK(7, 151, 31),	/* gpio 151-181 */
#else
	MSM_GPIO_BANK(1,  16, 27),	/* gpio 16-42 */
	MSM_GPIO_BANK(2,  43, 25),	/* gpio 43-67 */
 	MSM_GPIO_BANK(3,  68, 27),	/* gpio 68-94 */
#if defined(CONFIG_ARCH_QSD8X50)
	MSM_GPIO_BANK(4,  95,  9),	/* gpio 95-103 */
 	MSM_GPIO_BANK(5, 104, 18),	/* gpio 104-121 */
 	MSM_GPIO_BANK(6, 122, 31),	/* gpio 122-152 */
 	MSM_GPIO_BANK(7, 153, 12),	/* gpio 153-164 */	
#else
	MSM_GPIO_BANK(4,  95, 12),	/* gpio 95-106 */
 	MSM_GPIO_BANK(5, 107, 15),	/* gpio 107-121 */
#endif
#endif

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934