Messages in this thread | | | Date | Fri, 25 Feb 2011 21:40:00 -0800 | From | Saravana Kannan <> | Subject | Re: [PATCH] msm: gpiomux: Remove GPIOMUX_VALID and merge config enums |
| |
On 02/25/2011 05:20 PM, Dima Zavin wrote: > On Fri, Feb 25, 2011 at 12:19 PM, Rohit Vaswani<rvaswani@codeaurora.org> wrote: >> diff --git a/arch/arm/mach-msm/board-qsd8x50.c >> b/arch/arm/mach-msm/board-qsd8x50.c >> index 33ab1fe..d665b0e 100644 >> --- a/arch/arm/mach-msm/board-qsd8x50.c >> +++ b/arch/arm/mach-msm/board-qsd8x50.c >> @@ -38,19 +38,26 @@ >> #include "devices.h" >> #include "gpiomux.h" >> >> -#define UART3_SUSPENDED (GPIOMUX_DRV_2MA | GPIOMUX_PULL_DOWN |\ >> - GPIOMUX_FUNC_1 | GPIOMUX_VALID) >> +static struct gpiomux_setting uart3_suspended = { >> + .drv = GPIOMUX_DRV_2MA, >> + .pull = GPIOMUX_PULL_DOWN, >> + .func = GPIOMUX_FUNC_1, >> +}; >> >> extern struct sys_timer msm_timer; >> >> -struct msm_gpiomux_config qsd8x50_uart3_configs[] __initdata = { >> +struct msm_gpiomux_config qsd8x50_uart3_configs[] = { >> { >> .gpio = 86, /* UART3 RX */ >> - .suspended = UART3_SUSPENDED, >> + .settings = { >> + [GPIOMUX_SUSPENDED] =&uart3_suspended, >> + }, >> }, >> { >> .gpio = 87, /* UART3 TX */ >> - .suspended = UART3_SUSPENDED, >> + .settings = { >> + [GPIOMUX_SUSPENDED] =&uart3_suspended, >> + }, >> }, >> }; > > I think this new interface is way too verbose and will quickly get > unwieldy for configurations that have more than a few pins. For > instance, imagine what the above would look like when muxing a 24bit > LCD pin list... > > How about adding a "bool valid" to gpiomux_setting, and convert the > "sets" array to an array of settings and not pointers to settings. > This will allow us to do (in gpiomux.h): > > struct msm_gpiomux_rec { > struct gpiomux_setting sets[GPIOMUX_NSETTINGS]; > int ref; > }; > > struct gpiomux_setting { > enum gpiomux_func func; > enum gpiomux_drv drv; > enum gpiomux_pull pull; > bool valid; > }; > > This way, I can do something like (very rough): > > #define GPIOMUX_SET(func,drv,pull) { \ > .func = GPIOMUX_##func, \ > .drv = GPIOMUX_##drv, \ > .pull = GPIOMUX_##pull, \ > .valid = true, \ > } > > #define GPIOMUX_SET_NONE { .valid = false, } > > #define GPIOMUX_CFG(g, active, suspended) { \ > .gpio = g, \ > .sets = { \ > [GPIOMUX_ACTIVE] = active, \ > [GPIOMUX_SUSPENDED] = suspended, \ > }, \ > } > > This will then allow me to define the uart3 pinmuxing in my board file > as follows: > > struct msm_gpiomux_rec uart3_mux_cfg[] = { > GPIOMUX_CFG(86, GPIOMUX_SET_NONE, > GPIOMUX_SET(FUNC_1, DRV_2MA, PULL_DOWN)), > GPIOMUX_CFG(87, GPIOMUX_SET_NONE, > GPIOMUX_SET(FUNC_1, DRV_2MA, PULL_DOWN)), > }; > > Thoughts? >
I haven't read this GPIO code thoroughly, but by looking just at the diff, I think you can still have these type of macros with the structure definition Rohit chose. I have no opinion one which struct definition is better (not enough context). Just trying to help with writing helper macros.
The trick is to use pointers to anonymous struct. A very rough macro:
#define GPIOMUX_SET(f, d, p) \ &(struct gpiomux_setting) { .func = f, .drv = d, .pull = p, }
#define GPIOMUX_CFG(g, active, suspended) { \ .gpio = g, .settings = { [ACTIVE] = active, [SUSPENDED] = suspended, } }
struct msm_gpiomux_config foo_bar[] = { GPIOMUX_CFG(10, GPIOMUX_SET(FUNC, 2MA, PULL_UP), NULL), GPIOMUX_CFG(11, GPIOMUX_SET(FUNC, 2MA, PULL_UP), NULL), };
I'm certain the pointer to anonymous struct stuff works. You might have to tweak the macros a bit though. Hope this help.
-Saravana
-- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
| |