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

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

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