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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1283375606.21049.20.camel@c-dwalke-linux.qualcomm.com>
Date:	Wed, 01 Sep 2010 14:13:26 -0700
From:	Daniel Walker <dwalker@...eaurora.org>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	Gregory Bean <gbean@...eaurora.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 1/2] msm: Install the Google-Android gpio driver.

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@...roid.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@...roid.com>
> > Signed-off-by: Gregory Bean <gbean@...eaurora.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
> };
> 

Yeah, this macro method is much nicer. It drops 204 lines down to less
than half that.. Less duplication and less lines to maintain.

Thanks for the review Hartley.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/