> -----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 <anuj.aggar...@ti.com>
> 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 <anuj.aggar...@ti.com>
> >> 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 <anuj.aggar...@ti.com>
> >> > ---
> >> > 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 = 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?
>
> >>
> >> > +#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 majord...@vger.kernel.org
> >> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> >
> >>
> >>
> >>
> >> --
> >> Sincerely Yours,
> >> Mike.
> >
> >
>
>
>
> --
> Sincerely Yours,
> Mike.
--
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
