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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Gregory Bean <gbean@xxxxxxxxxxxxxx>
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
