Re: [PATCH] msm: gpiomux: Remove GPIOMUX_VALID and merge config enums

[Saravana Kannan <skannan@codeaurora.org>]

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.