* [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c @ 2009-11-05 16:39 Anuj Aggarwal 2009-11-05 21:16 ` Mike Rapoport 0 siblings, 1 reply; 6+ messages in thread From: Anuj Aggarwal @ 2009-11-05 16:39 UTC (permalink / raw) To: linux-omap; +Cc: broonie, lrg, Anuj Aggarwal 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.aggarwal@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) +#include <linux/i2c/twl4030.h> + +extern struct twl4030_platform_data omap3evm_twldata; + +/* 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 */ #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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c 2009-11-05 16:39 [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c Anuj Aggarwal @ 2009-11-05 21:16 ` Mike Rapoport 2009-11-06 6:47 ` Aggarwal, Anuj 0 siblings, 1 reply; 6+ messages in thread From: Mike Rapoport @ 2009-11-05 21:16 UTC (permalink / raw) To: Anuj Aggarwal; +Cc: linux-omap, broonie, lrg On Thu, Nov 5, 2009 at 6:39 PM, Anuj Aggarwal <anuj.aggarwal@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.aggarwal@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? > +#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. > #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. -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c 2009-11-05 21:16 ` Mike Rapoport @ 2009-11-06 6:47 ` Aggarwal, Anuj 2009-11-06 14:36 ` Mike Rapoport 0 siblings, 1 reply; 6+ messages in thread From: Aggarwal, Anuj @ 2009-11-06 6:47 UTC (permalink / raw) To: Mike Rapoport; +Cc: linux-omap, broonie, lrg > -----Original Message----- > From: Mike Rapoport [mailto:mike.rapoport@gmail.com] > Sent: Friday, November 06, 2009 2:46 AM > To: Aggarwal, Anuj > Cc: linux-omap@vger.kernel.org; broonie@opensource.wolfsonmicro.com; > lrg@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.aggarwal@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.aggarwal@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. > > > +#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. -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c 2009-11-06 6:47 ` Aggarwal, Anuj @ 2009-11-06 14:36 ` Mike Rapoport 2009-11-09 6:34 ` Aggarwal, Anuj 0 siblings, 1 reply; 6+ messages in thread From: Mike Rapoport @ 2009-11-06 14:36 UTC (permalink / raw) To: Aggarwal, Anuj; +Cc: linux-omap, broonie, lrg On Fri, Nov 6, 2009 at 8:47 AM, Aggarwal, Anuj <anuj.aggarwal@ti.com> wrote: >> -----Original Message----- >> From: Mike Rapoport [mailto:mike.rapoport@gmail.com] >> Sent: Friday, November 06, 2009 2:46 AM >> To: Aggarwal, Anuj >> Cc: linux-omap@vger.kernel.org; broonie@opensource.wolfsonmicro.com; >> lrg@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.aggarwal@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.aggarwal@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? >> >> > +#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. -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c 2009-11-06 14:36 ` Mike Rapoport @ 2009-11-09 6:34 ` Aggarwal, Anuj 2009-11-09 7:20 ` Mike Rapoport 0 siblings, 1 reply; 6+ messages in thread From: Aggarwal, Anuj @ 2009-11-09 6:34 UTC (permalink / raw) To: 'Mike Rapoport'; +Cc: linux-omap, broonie, lrg > -----Original Message----- > From: Mike Rapoport [mailto:mike.rapoport@gmail.com] > Sent: Friday, November 06, 2009 8:06 PM > To: Aggarwal, Anuj > Cc: linux-omap@vger.kernel.org; broonie@opensource.wolfsonmicro.com; > lrg@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.aggarwal@ti.com> > wrote: > >> -----Original Message----- > >> From: Mike Rapoport [mailto:mike.rapoport@gmail.com] > >> Sent: Friday, November 06, 2009 2:46 AM > >> To: Aggarwal, Anuj > >> Cc: linux-omap@vger.kernel.org; broonie@opensource.wolfsonmicro.com; > >> lrg@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.aggarwal@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.aggarwal@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 majordomo@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c 2009-11-09 6:34 ` Aggarwal, Anuj @ 2009-11-09 7:20 ` Mike Rapoport 0 siblings, 0 replies; 6+ messages in thread From: Mike Rapoport @ 2009-11-09 7:20 UTC (permalink / raw) To: Aggarwal, Anuj; +Cc: linux-omap, broonie, lrg 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-11-09 7:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-11-05 16:39 [PATCH 4/5] Regulator: Adding OMAP3EVM/TWL4030 specific code in board-omap35x-pmic.c Anuj Aggarwal 2009-11-05 21:16 ` Mike Rapoport 2009-11-06 6:47 ` Aggarwal, Anuj 2009-11-06 14:36 ` Mike Rapoport 2009-11-09 6:34 ` Aggarwal, Anuj 2009-11-09 7:20 ` Mike Rapoport
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.