Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c

[Mike Rapoport <mike.rapoport@gmail.com>]

On Mon, Nov 9, 2009 at 8:34 AM, Aggarwal, Anuj <anuj.aggarwal@ti.com> wrote:

> Based on your suggestions, I tried creating macros specific to TWL
> regulators in a common header file. This file then can be included by
> the different board files and the structures can then be appropriately
> created.
> Here is the sample of what could be done on these lines:
>
> #define TWL_VDAC_SUPPLY(device) { \

The brace is not needed here  --------------^

>        static struct regulator_consumer_supply vdac_supply[] = { \
>                { \
>                        .supply = "vdac", \
>                        .dev = device, \
>                }, \
>        }; \
> }

ditto

> #define TWL_VPLL2_SUPPLY(device)        { \

the same here --------------------------------------------^

>        static struct regulator_consumer_supply vpll2_supply[] = { \
>                { \
>                        .supply = "vpll2", \
>                        .dev = device, \
>                }, \
>        }; \
> }

and here

> And similarly:
> #define TWL_VAUX1_VDAC_DATA()   { \
>        static struct regulator_init_data vdac_data = { \
>                .constraints = { \
>                        .min_uV                 = 1800000, \
>                        .max_uV                 = 1800000, \
>                        .apply_uV               = true, \
>                        .valid_modes_mask       = REGULATOR_MODE_NORMAL \
>                                                | REGULATOR_MODE_STANDBY, \
>                        .valid_ops_mask         = REGULATOR_CHANGE_MODE \
>                                                | REGULATOR_CHANGE_STATUS, \
>                }, \
>                .num_consumer_supplies  = ARRAY_SIZE(vdac_supply), \
>                .consumer_supplies      = vdac_supply, \
>        }; \
> }
>
> This way, user can define his board-specific regulators in the board-*
> file.
> Only problem I can forsee in this approach is how to create regulators
> supplying multiple devices? VLPP2 might be supplying 2-3 devices in the
> system which the above #define doesn't take care. How that can be
> solved?

The macros should be usable for the common case to avoid code
duplication in the board-* files. Boards with different supplies
configuration will explicitly define their regulator_consumer_supply
and regulator_init_data.

>
>> >>
>> >> > +#include <linux/i2c/twl4030.h>
>> >> > +
>> >> > +extern struct twl4030_platform_data omap3evm_twldata;
>> >>
>> >> The *twldata does not have to be global, it can be passed to pmic_init
>> >> as a parameter.
>> >>
>> >> > +/* VDAC */
>> >> > +static struct regulator_consumer_supply vdac_consumers[] = {
>> >> > +       {
>> >> > +               .supply = "dac",
>> >> > +       },
>> >> > +};
>> >> > +
>> >> > +static struct regulator_init_data vdac_data = {
>> >> > +       .constraints = {
>> >> > +               .name = "VDAC",
>> >> > +               .min_uV = 1800000,
>> >> > +               .max_uV = 1800000,
>> >> > +               .apply_uV = true,
>> >> > +               .valid_modes_mask = REGULATOR_MODE_NORMAL
>> >> > +                       | REGULATOR_MODE_STANDBY,
>> >> > +               .valid_ops_mask = REGULATOR_CHANGE_MODE
>> >> > +                       | REGULATOR_CHANGE_STATUS,
>> >> > +       },
>> >> > +       .num_consumer_supplies = ARRAY_SIZE(vdac_consumers),
>> >> > +       .consumer_supplies = vdac_consumers,
>> >> > +};
>> >> > +
>> >> > +/* VPLL2 */
>> >> > +static struct regulator_consumer_supply vpll2_consumers[] = {
>> >> > +       {
>> >> > +               .supply = "lcd",
>> >> > +       },
>> >> > +       {
>> >> > +               .supply = "sdi",
>> >> > +       },
>> >> > +};
>> >> > +
>> >> > +static struct regulator_init_data vpll2_data = {
>> >> > +       .constraints = {
>> >> > +               .name = "VPLL2",
>> >> > +               .min_uV = 1800000,
>> >> > +               .max_uV = 1800000,
>> >> > +               .apply_uV = true,
>> >> > +               .valid_modes_mask = REGULATOR_MODE_NORMAL
>> >> > +                       | REGULATOR_MODE_STANDBY,
>> >> > +               .valid_ops_mask = REGULATOR_CHANGE_MODE
>> >> > +                       | REGULATOR_CHANGE_STATUS,
>> >> > +       },
>> >> > +       .num_consumer_supplies = ARRAY_SIZE(vpll2_consumers),
>> >> > +       .consumer_supplies = vpll2_consumers,
>> >> > +};
>> >> > +
>> >> > +/* VMMC1 */
>> >> > +struct regulator_consumer_supply vmmc1_consumers[] = {
>> >> > +       {
>> >> > +               .supply = "mmc",
>> >> > +       },
>> >> > +};
>> >> > +
>> >> > +static struct regulator_init_data vmmc1_data = {
>> >> > +       .constraints = {
>> >> > +               .name = "VMMC1",
>> >> > +               .min_uV = 1850000,
>> >> > +               .max_uV = 3150000,
>> >> > +               .valid_modes_mask = REGULATOR_MODE_NORMAL
>> >> > +                       | REGULATOR_MODE_STANDBY,
>> >> > +               .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE
>> >> > +                       | REGULATOR_CHANGE_MODE |
>> >> REGULATOR_CHANGE_STATUS,
>> >> > +       },
>> >> > +       .num_consumer_supplies = ARRAY_SIZE(vmmc1_consumers),
>> >> > +       .consumer_supplies = vmmc1_consumers,
>> >> > +};
>> >> > +
>> >> > +static void __init pmic_twl4030_init(void)
>> >> >  {
>> >> > -       /* TWL4030 specific init code */
>> >> > +       /* Initialize the regulator specific fields here */
>> >> > +       omap3evm_twldata.vdac = &vdac_data;
>> >> > +       omap3evm_twldata.vpll2 = &vpll2_data;
>> >> > +       omap3evm_twldata.vmmc1 = &vmmc1_data;
>> >> >  }
>> >> > +#endif /* CONFIG_MACH_OMAP3EVM */
>> >>
>> >> I don't see why would you move board specific code from board specific
>> >> file to some "generic" file and add #ifdefs to enable this code only
>> >> for that board. Indeed, many OMAP3 boards use TWL/TPS in very similar
>> >> way and it does make sence to factor the common code out. But with
>> >> your approach each board will have to add its own #ifdef with almost
>> >> identical code inside it.
>> > [Aggarwal, Anuj] As explained above, same code can be used for OMAP3
>> based
>> > platforms having the same regulator requirements.
>> >>
>> >> >  #else
>> >> >  static inline void pmic_twl4030_init(void)
>> >> >  {
>> >> > diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-
>> >> omap2/board-omap3evm.c
>> >> > index dbdf062..10ac0d2 100644
>> >> > --- a/arch/arm/mach-omap2/board-omap3evm.c
>> >> > +++ b/arch/arm/mach-omap2/board-omap3evm.c
>> >> > @@ -197,7 +197,7 @@ static struct twl4030_madc_platform_data
>> >> omap3evm_madc_data = {
>> >> >        .irq_line       = 1,
>> >> >  };
>> >> >
>> >> > -static struct twl4030_platform_data omap3evm_twldata = {
>> >> > +struct twl4030_platform_data omap3evm_twldata = {
>> >> >        .irq_base       = TWL4030_IRQ_BASE,
>> >> >        .irq_end        = TWL4030_IRQ_END,
>> >> >
>> >> > --
>> >> > 1.6.2.4
>> >> >
>> >> > --
>> >> > To unsubscribe from this list: send the line "unsubscribe linux-omap"
>> in
>> >> > the body of a message to majordomo@vger.kernel.org
>> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >>       Sincerely Yours,
>> >>               Mike.
>> >
>> >
>>
>>
>>
>> --
>>       Sincerely Yours,
>>               Mike.
>
>



-- 
	Sincerely Yours,
		Mike.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html