Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c
On Mon, Nov 9, 2009 at 8:34 AM, Aggarwal, Anuj 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 = 180, \
> .max_uV = 180, \
> .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
>> >> > +
>> >> > +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 = 180,
>> >> > + .max_uV = 180,
>> >> > + .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 = 180,
>> >> > + .max_uV = 180,
>> >> > + .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 = 185,
>> >> > + .max_uV = 315,
>> >> > + .valid_modes_mask = REGULATOR_MODE_NORMAL
>> >> > + | REGULATOR_MODE_STANDBY,
>> >> > + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE
>> >> > + | REGULATOR_CHANGE_MODE |
>> >> RE
RE: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c
> -Original Message-
> From: Mike Rapoport [mailto:mike.rapop...@gmail.com]
> Sent: Friday, November 06, 2009 8:06 PM
> To: Aggarwal, Anuj
> Cc: linux-omap@vger.kernel.org; broo...@opensource.wolfsonmicro.com;
> l...@slimlogic.co.uk
> Subject: Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code
> in board-omap35x-pmic.c
>
> On Fri, Nov 6, 2009 at 8:47 AM, Aggarwal, Anuj
> wrote:
> >> -Original Message-
> >> From: Mike Rapoport [mailto:mike.rapop...@gmail.com]
> >> Sent: Friday, November 06, 2009 2:46 AM
> >> To: Aggarwal, Anuj
> >> Cc: linux-omap@vger.kernel.org; broo...@opensource.wolfsonmicro.com;
> >> l...@slimlogic.co.uk
> >> Subject: Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific
> code
> >> in board-omap35x-pmic.c
> >>
> >> On Thu, Nov 5, 2009 at 6:39 PM, Anuj Aggarwal
> >> wrote:
> >> > Adding various regulator-consumers for OMAP3EVM-TWL4030 combination
> >> > in board-omap35x-pmic.c. Also, populating the respective fields
> >> > for omap3evm_twldata structure.
> >> >
> >> > Signed-off-by: Anuj Aggarwal
> >> > ---
> >> > arch/arm/mach-omap2/board-omap35x-pmic.c | 81
> >> +-
> >> > arch/arm/mach-omap2/board-omap3evm.c | 2 +-
> >> > 2 files changed, 80 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-omap2/board-omap35x-pmic.c
> b/arch/arm/mach-
> >> omap2/board-omap35x-pmic.c
> >> > index aae07ab..2ef4932 100644
> >> > --- a/arch/arm/mach-omap2/board-omap35x-pmic.c
> >> > +++ b/arch/arm/mach-omap2/board-omap35x-pmic.c
> >> > @@ -24,10 +24,87 @@
> >> > * Definitions specific to TWL4030/TPS65950
> >> > */
> >> > #if defined(CONFIG_PMIC_TWL4030)
> >> > -static inline void pmic_twl4030_init(void)
> >> > +#if defined(CONFIG_MACH_OMAP3EVM)
> >>
> >> What about beagle, overo and others that use the same regulator for
> >> the same purposes?
> > [Aggarwal, Anuj] This file can be used to accommodate all the common
> > Regulator f/w related code for OMAP3 based platforms. Depending upon
> > the regulators' usage across EVMs, the code can be split in generic and
> > platform specific code.
>
> I agree that common regulator functionality should reside in one place
> rather than be copy-pasted between board files. But adding file with
> #defined(CONFIG_MACH_OMAP3EVM) and #defined(CONFIG_MACH_OMAP3BEAGLE)
> does not make it generic. Every single line of code that is inside
> such #ifdef can be perfectly well placed in the appropriate board-X.c
> As far as I can tell, all platforms use the very same definitions of
> regulator and regulator supplies both for MMC and VDVI and VENC. So,
> why not just create some macros wrapping these definitions and use
> them every time 'struct regulator_consumer_supply' and 'struct
> regulator_init_data' need to be instantiated?
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) { \
static struct regulator_consumer_supply vdac_supply[] = { \
{ \
.supply = "vdac", \
.dev = device, \
}, \
}; \
}
#define TWL_VPLL2_SUPPLY(device){ \
static struct regulator_consumer_supply vpll2_supply[] = { \
{ \
.supply = "vpll2", \
.dev = device, \
}, \
}; \
}
And similarly:
#define TWL_VAUX1_VDAC_DATA() { \
static struct regulator_init_data vdac_data = { \
.constraints = { \
.min_uV = 180, \
.max_uV = 180, \
.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 regulator
RE: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c
> -Original Message-
> From: Mike Rapoport [mailto:mike.rapop...@gmail.com]
> Sent: Friday, November 06, 2009 2:46 AM
> To: Aggarwal, Anuj
> Cc: linux-omap@vger.kernel.org; broo...@opensource.wolfsonmicro.com;
> l...@slimlogic.co.uk
> Subject: Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code
> in board-omap35x-pmic.c
>
> On Thu, Nov 5, 2009 at 6:39 PM, Anuj Aggarwal
> wrote:
> > Adding various regulator-consumers for OMAP3EVM-TWL4030 combination
> > in board-omap35x-pmic.c. Also, populating the respective fields
> > for omap3evm_twldata structure.
> >
> > Signed-off-by: Anuj Aggarwal
> > ---
> > arch/arm/mach-omap2/board-omap35x-pmic.c | 81
> +-
> > arch/arm/mach-omap2/board-omap3evm.c | 2 +-
> > 2 files changed, 80 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/board-omap35x-pmic.c b/arch/arm/mach-
> omap2/board-omap35x-pmic.c
> > index aae07ab..2ef4932 100644
> > --- a/arch/arm/mach-omap2/board-omap35x-pmic.c
> > +++ b/arch/arm/mach-omap2/board-omap35x-pmic.c
> > @@ -24,10 +24,87 @@
> > * Definitions specific to TWL4030/TPS65950
> > */
> > #if defined(CONFIG_PMIC_TWL4030)
> > -static inline void pmic_twl4030_init(void)
> > +#if defined(CONFIG_MACH_OMAP3EVM)
>
> What about beagle, overo and others that use the same regulator for
> the same purposes?
[Aggarwal, Anuj] This file can be used to accommodate all the common
Regulator f/w related code for OMAP3 based platforms. Depending upon
the regulators' usage across EVMs, the code can be split in generic and
platform specific code.
>
> > +#include
> > +
> > +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 = 180,
> > + .max_uV = 180,
> > + .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 = 180,
> > + .max_uV = 180,
> > + .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 = 185,
> > + .max_uV = 315,
> > + .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 regu
Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c
On Thu, Nov 5, 2009 at 6:39 PM, Anuj Aggarwal wrote:
> Adding various regulator-consumers for OMAP3EVM-TWL4030 combination
> in board-omap35x-pmic.c. Also, populating the respective fields
> for omap3evm_twldata structure.
>
> Signed-off-by: Anuj Aggarwal
> ---
> arch/arm/mach-omap2/board-omap35x-pmic.c | 81
> +-
> arch/arm/mach-omap2/board-omap3evm.c | 2 +-
> 2 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-omap35x-pmic.c
> b/arch/arm/mach-omap2/board-omap35x-pmic.c
> index aae07ab..2ef4932 100644
> --- a/arch/arm/mach-omap2/board-omap35x-pmic.c
> +++ b/arch/arm/mach-omap2/board-omap35x-pmic.c
> @@ -24,10 +24,87 @@
> * Definitions specific to TWL4030/TPS65950
> */
> #if defined(CONFIG_PMIC_TWL4030)
> -static inline void pmic_twl4030_init(void)
> +#if defined(CONFIG_MACH_OMAP3EVM)
What about beagle, overo and others that use the same regulator for
the same purposes?
> +#include
> +
> +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 = 180,
> + .max_uV = 180,
> + .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 = 180,
> + .max_uV = 180,
> + .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 = 185,
> + .max_uV = 315,
> + .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.
> #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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Sincerely Yours,
Mike.
--
To unsubscribe from this list:
