lkml.org 
[lkml]   [2010]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCH 1/2] msm: Install the Google-Android gpio driver.
    From
    Date
    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
    > };
    >

    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-09-01 23:15    [W:4.958 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site