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

[rvaswani@codeaurora.org (Rohit Vaswani)]

On 2/25/2011 5:20 PM, Dima Zavin wrote:
> On Fri, Feb 25, 2011 at 12:19 PM, Rohit Vaswani<rvaswani@codeaurora.org>  wrote:
>> Remove GPIOMUX_VALID, simplify the API, and absorb the complexity
>> of determining which gpiomux configs are used and which are
>> not back into the gpiomux implementation where it belongs.
>> Allow NULL settings to be represented appropriately.
>> The configuration enumerations represent a single abstraction
>> and were needlessly split. As such, collapse the conflicting
>> abstractions into a consistent one and move the implementation
>> complexity down into the implementation (where it belongs) and
>> away from the interface (where it doesn't belong).
>>
>> Signed-off-by: Rohit Vaswani<rvaswani@codeaurora.org>
>> ---
>>   arch/arm/mach-msm/board-msm7x30.c |   23 +++++++--
>>   arch/arm/mach-msm/board-qsd8x50.c |   17 +++++--
>>   arch/arm/mach-msm/gpiomux-v1.c    |    9 ++-
>>   arch/arm/mach-msm/gpiomux-v1.h    |   59 ----------------------
>>   arch/arm/mach-msm/gpiomux-v2.c    |    8 ++-
>>   arch/arm/mach-msm/gpiomux-v2.h    |   59 ----------------------
>>   arch/arm/mach-msm/gpiomux.c       |   77 +++++++++++++++++-------------
>>   arch/arm/mach-msm/gpiomux.h       |   96
>> +++++++++++++++++++++++-------------
>>   8 files changed, 145 insertions(+), 203 deletions(-)
>>   delete mode 100644 arch/arm/mach-msm/gpiomux-v1.h
>>   delete mode 100644 arch/arm/mach-msm/gpiomux-v2.h
>>
>> diff --git a/arch/arm/mach-msm/board-msm7x30.c
>> b/arch/arm/mach-msm/board-msm7x30.c
>> index 59b91de..4223329 100644
>> --- a/arch/arm/mach-msm/board-msm7x30.c
>> +++ b/arch/arm/mach-msm/board-msm7x30.c
>> @@ -39,8 +39,11 @@
>>   #include "gpiomux.h"
>>   #include "proc_comm.h"
>>
>> -#define UART2_SUSPENDED (GPIOMUX_DRV_2MA | GPIOMUX_PULL_DOWN |\
>> -            GPIOMUX_FUNC_2 | GPIOMUX_VALID)
>> +static struct gpiomux_setting uart2_suspended = {
>> +    .func = GPIOMUX_FUNC_2,
>> +    .drv = GPIOMUX_DRV_2MA,
>> +    .pull = GPIOMUX_PULL_DOWN,
>> +};
>>
>>   extern struct sys_timer msm_timer;
>>
>> @@ -59,19 +62,27 @@ static struct msm_otg_platform_data msm_otg_pdata = {
>>   struct msm_gpiomux_config msm7x30_uart2_configs[] __initdata = {
>>      {
>>          .gpio = 49, /* UART2 RFR */
>> -        .suspended = UART2_SUSPENDED,
>> +        .settings = {
>> +            [GPIOMUX_SUSPENDED] =&uart2_suspended,
>> +        },
>>      },
>>      {
>>          .gpio = 50, /* UART2 CTS */
>> -        .suspended = UART2_SUSPENDED,
>> +        .settings = {
>> +            [GPIOMUX_SUSPENDED] =&uart2_suspended,
>> +        },
>>      },
>>      {
>>          .gpio = 51, /* UART2 RX */
>> -        .suspended = UART2_SUSPENDED,
>> +        .settings = {
>> +            [GPIOMUX_SUSPENDED] =&uart2_suspended,
>> +        },
>>      },
>>      {
>>          .gpio = 52, /* UART2 TX */
>> -        .suspended = UART2_SUSPENDED,
>> +        .settings = {
>> +            [GPIOMUX_SUSPENDED] =&uart2_suspended,
>> +        },
>>      },
>>   };
>>
>> 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?
>
> --Dima
>

Thanks for you feedback Dima. I agree that the above change makes the 
pin configurations verbose - but its neither too verbose to so as to 
become cumbersome to use nor typically unmanageable. We did think about 
adding a valid/invalid bool to the configurations - but the fact was 
that they it simply did not belong there. The gpiomux configurations are 
never "invalid" as such. Another important part to this decision was 
that - with adding another variable as a part of the gpiomux 
configurations meant that we were transferring responsibility of 
maintaining something that belongs to the "gpiomux internals" to the 
user. We felt that having the user to append a valid/invalid flag with 
every configuration to was not a step in the right direction.
On the contrary, the explicit structures improved readability and kept 
things pretty simple. Thinking about it more - it doesn't even make it 
verbose - but it just may seem that way because we expand it into a 
structure instead of hiding behind a macro.
Also, we currently use this same design for a number of GPIOs far 
greater than 24 and my/our experience has never been bothersome.
Hope that answers your query.

  Thanks,
Rohit Vaswani

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.