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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Gregory Bean <[email protected]> > > 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-arm-msm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
