[PATCHv7 0/7] External controller support for TWLxxxx

[PATCHv7 0/7] External controller support for TWLxxxx

[Tero Kristo]

Hi,

Changes compared to previous version:
- External controller support is now built in to twl-regulator.c as
  an option that gets configured during probe
- Selected controller is based on option passed in platform data,
  which also needs to be passed through twl-core.c (thus the change
  to drivers/mfd/twl-core.c)
- Dropped a couple of silly debug prints

-Tero

[PATCHv7 1/7] regulator: twl: fix twl4030 support for smps regulators

[Tero Kristo]

SMPS regulator voltage control differs from the one of the LDO ones.
Current TWL code was using LDO regulator ops for controlling the SMPS
regulators, which fails. This was fixed fixed by adding separate
regulator type which uses correct logic and calculations for the
voltage levels.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/twl-regulator.c |   46 +++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index ee8747f..11cc308 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -71,6 +71,7 @@ struct twlreg_info {
 #define VREG_TYPE		1
 #define VREG_REMAP		2
 #define VREG_DEDICATED		3	/* LDO control */
+#define VREG_VOLTAGE_SMPS_4030	9
 /* TWL6030 register offsets */
 #define VREG_TRANS		1
 #define VREG_STATE		2
@@ -514,6 +515,32 @@ static struct regulator_ops twl4030ldo_ops = {
 	.get_status	= twl4030reg_get_status,
 };
 
+static int
+twl4030smps_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
+			unsigned *selector)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+	int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
+
+	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
+		vsel);
+	return 0;
+}
+
+static int twl4030smps_get_voltage(struct regulator_dev *rdev)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+	int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+		VREG_VOLTAGE_SMPS_4030);
+
+	return vsel * 12500 + 600000;
+}
+
+static struct regulator_ops twl4030smps_ops = {
+	.set_voltage	= twl4030smps_set_voltage,
+	.get_voltage	= twl4030smps_get_voltage,
+};
+
 static int twl6030ldo_list_voltage(struct regulator_dev *rdev, unsigned index)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -856,6 +883,21 @@ static struct regulator_ops twlsmps_ops = {
 		}, \
 	}
 
+#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, turnon_delay, remap_conf) \
+	{ \
+	.base = offset, \
+	.id = num, \
+	.delay = turnon_delay, \
+	.remap = remap_conf, \
+	.desc = { \
+		.name = #label, \
+		.id = TWL4030_REG_##label, \
+		.ops = &twl4030smps_ops, \
+		.type = REGULATOR_VOLTAGE, \
+		.owner = THIS_MODULE, \
+		}, \
+	}
+
 #define TWL6030_ADJUSTABLE_LDO(label, offset, min_mVolts, max_mVolts) { \
 	.base = offset, \
 	.min_mV = min_mVolts, \
@@ -947,8 +989,8 @@ static struct twlreg_info twl_regs[] = {
 	TWL4030_ADJUSTABLE_LDO(VINTANA2, 0x43, 12, 100, 0x08),
 	TWL4030_FIXED_LDO(VINTDIG, 0x47, 1500, 13, 100, 0x08),
 	TWL4030_ADJUSTABLE_LDO(VIO, 0x4b, 14, 1000, 0x08),
-	TWL4030_ADJUSTABLE_LDO(VDD1, 0x55, 15, 1000, 0x08),
-	TWL4030_ADJUSTABLE_LDO(VDD2, 0x63, 16, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD1, 0x55, 15, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD2, 0x63, 16, 1000, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V5, 0x71, 1500, 17, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V8, 0x74, 1800, 18, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB3V1, 0x77, 3100, 19, 150, 0x08),
-- 
1.7.4.1

[PATCHv7 2/7] TEMP: OMAP3: beagle rev-c4: enable OPP6

[Tero Kristo]

Beagleboard rev-c4 has a speed sorted OMAP3530 chip which can run at 720MHz.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/board-omap3beagle.c |   29 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c      |    4 ++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 3ae16b4..a7c3d60 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -489,6 +489,35 @@ static void __init beagle_opp_init(void)
 		return;
 	}
 
+	if (omap3_beagle_version == OMAP3BEAGLE_BOARD_C4) {
+		struct device *mpu_dev, *iva_dev;
+
+		mpu_dev = omap2_get_mpuss_device();
+		iva_dev = omap2_get_iva_device();
+
+		if (!mpu_dev || !iva_dev) {
+			pr_err("%s: Aiee.. no mpu/dsp devices? %p %p\n",
+				__func__, mpu_dev, iva_dev);
+			return;
+		}
+		/* Enable MPU 720MHz opp */
+		r = opp_enable(mpu_dev, 720000000);
+
+		/* Enable IVA 520MHz opp */
+		r |= opp_enable(iva_dev, 520000000);
+
+		if (r) {
+			pr_err("%s: failed to enable higher opp %d\n",
+				__func__, r);
+			/*
+			 * Cleanup - disable the higher freqs - we dont care
+			 * about the results
+			 */
+			opp_disable(mpu_dev, 720000000);
+			opp_disable(iva_dev, 520000000);
+		}
+	}
+
 	/* Custom OPP enabled for all xM versions */
 	if (cpu_is_omap3630()) {
 		struct device *mpu_dev, *iva_dev;
diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
index d95f3f9..a0f5fe1 100644
--- a/arch/arm/mach-omap2/opp3xxx_data.c
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -98,6 +98,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("mpu", true, 550000000, OMAP3430_VDD_MPU_OPP4_UV),
 	/* MPU OPP5 */
 	OPP_INITIALIZER("mpu", true, 600000000, OMAP3430_VDD_MPU_OPP5_UV),
+	/* MPU OPP6 : omap3530 high speed grade only */
+	OPP_INITIALIZER("mpu", false, 720000000, OMAP3430_VDD_MPU_OPP5_UV),
 
 	/*
 	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
@@ -123,6 +125,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("iva", true, 400000000, OMAP3430_VDD_MPU_OPP4_UV),
 	/* DSP OPP5 */
 	OPP_INITIALIZER("iva", true, 430000000, OMAP3430_VDD_MPU_OPP5_UV),
+	/* DSP OPP6 : omap3530 high speed grade only */
+	OPP_INITIALIZER("iva", false, 520000000, OMAP3430_VDD_MPU_OPP5_UV),
 };
 
 static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
-- 
1.7.4.1

[PATCHv7 3/7] omap3: voltage: fix channel configuration

[Tero Kristo]

OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
overlap with VDD2 and attempting to modify VDD1 voltage will actually change
VDD2 voltage.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/vc.c          |    5 ++++-
 arch/arm/mach-omap2/vc3xxx_data.c |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
index 031d116..d42dd5f 100644
--- a/arch/arm/mach-omap2/vc.c
+++ b/arch/arm/mach-omap2/vc.c
@@ -334,9 +334,12 @@ void __init omap_vc_init_channel(struct voltagedomain *voltdm)
 		voltdm->rmw(vc->smps_cmdra_mask,
 			    vc->cmd_reg_addr << __ffs(vc->smps_cmdra_mask),
 			    vc->smps_cmdra_reg);
-		vc->cfg_channel |= vc_cfg_bits->rac | vc_cfg_bits->racen;
+		vc->cfg_channel |= vc_cfg_bits->rac;
 	}
 
+	if (vc->cmd_reg_addr == vc->volt_reg_addr)
+		vc->cfg_channel |= vc_cfg_bits->racen;
+
 	/* Set up the on, inactive, retention and off voltage */
 	on_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->on_volt);
 	onlp_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->onlp_volt);
diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
index cfe348e..0136ad5 100644
--- a/arch/arm/mach-omap2/vc3xxx_data.c
+++ b/arch/arm/mach-omap2/vc3xxx_data.c
@@ -46,6 +46,7 @@ static struct omap_vc_common omap3_vc_common = {
 };
 
 struct omap_vc_channel omap3_vc_mpu = {
+	.flags = OMAP_VC_CHANNEL_DEFAULT,
 	.common = &omap3_vc_common,
 	.smps_sa_reg	 = OMAP3_PRM_VC_SMPS_SA_OFFSET,
 	.smps_volra_reg	 = OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET,
-- 
1.7.4.1

[PATCHv7 4/7] omap3: add common twl configurations for vdd1 and vdd2

[Tero Kristo]

VDD1 and VDD2 are the core voltage regulators on OMAP3. VDD1 is used
to control MPU/IVA voltage, and VDD2 is used for CORE. These regulators
are needed by DVFS.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/twl-common.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
index daa056e..7cf5347 100644
--- a/arch/arm/mach-omap2/twl-common.c
+++ b/arch/arm/mach-omap2/twl-common.c
@@ -115,6 +115,38 @@ static struct regulator_init_data omap3_vpll2_idata = {
 	.consumer_supplies		= omap3_vpll2_supplies,
 };
 
+static struct regulator_consumer_supply omap3_vdd1_supply[] = {
+	REGULATOR_SUPPLY("vcc", "mpu.0"),
+};
+
+static struct regulator_consumer_supply omap3_vdd2_supply[] = {
+	REGULATOR_SUPPLY("vcc", "l3_main.0"),
+};
+
+static struct regulator_init_data omap3_vdd1 = {
+	.constraints = {
+		.name			= "VDD1",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd1_supply),
+	.consumer_supplies		= omap3_vdd1_supply,
+};
+
+static struct regulator_init_data omap3_vdd2 = {
+	.constraints = {
+		.name			= "VDD2",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd2_supply),
+	.consumer_supplies		= omap3_vdd2_supply,
+};
+
 void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 				  u32 pdata_flags, u32 regulators_flags)
 {
@@ -122,6 +154,10 @@ void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 		pmic_data->irq_base = TWL4030_IRQ_BASE;
 	if (!pmic_data->irq_end)
 		pmic_data->irq_end = TWL4030_IRQ_END;
+	if (!pmic_data->vdd1)
+		pmic_data->vdd1 = &omap3_vdd1;
+	if (!pmic_data->vdd2)
+		pmic_data->vdd2 = &omap3_vdd2;
 
 	/* Common platform data configurations */
 	if (pdata_flags & TWL_COMMON_PDATA_USB && !pmic_data->usb)
-- 
1.7.4.1

[PATCHv7 5/7] mfd: twl-core: pass driver data from pdata to add_regulator for VDD1 and VDD2

[Tero Kristo]

This way, we can add custom flags for VDD1 and VDD2 regulators that
get passed all the way to regulator init. This is needed for SMPS
regulator support to select used controller mode for these regulators
(either voltage processor or default.)

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
---
 drivers/mfd/twl-core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 01ecfee..af93fce 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -846,12 +846,14 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
 			return PTR_ERR(child);
 
 		child = add_regulator(TWL4030_REG_VDD1, pdata->vdd1,
-					features);
+					features |
+					(u32)pdata->vdd1->driver_data);
 		if (IS_ERR(child))
 			return PTR_ERR(child);
 
 		child = add_regulator(TWL4030_REG_VDD2, pdata->vdd2,
-					features);
+					features |
+					(u32)pdata->vdd2->driver_data);
 		if (IS_ERR(child))
 			return PTR_ERR(child);
 
-- 
1.7.4.1

[PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

TWL regulator now has two alternative control paths: the default
I2C path or an optional voltage processor path for OMAP chips.
If the voltage processor path should be used, this can be
indicated within the platform data by adding flag TWL_VP_SMPS_MODE
to regulator_init_data->driver_data.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/twl-regulator.c |   39 ++++++++++++++++++++++++++++++------
 include/linux/i2c/twl.h           |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 11cc308..b674ae4 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/i2c/twl.h>
 
+#include <plat/voltage.h>
 
 /*
  * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
@@ -37,6 +38,12 @@ struct twlreg_info {
 	/* twl resource ID, for resource control state machine */
 	u8			id;
 
+	/* voltagedomain, only used for VP controlled smps regulators */
+	union {
+		const char		*name;
+		struct voltagedomain	*ptr;
+	} voltdm;
+
 	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
 	u8			table_len;
 	const u16		*table;
@@ -520,17 +527,28 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 			unsigned *selector)
 {
 	struct twlreg_info *info = rdev_get_drvdata(rdev);
-	int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
 
-	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
-		vsel);
+	if (info->voltdm.ptr)
+		voltdm_scale(info->voltdm.ptr, min_uV);
+	else {
+		int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
+
+		twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+			VREG_VOLTAGE_SMPS_4030, vsel);
+	}
+
 	return 0;
 }
 
 static int twl4030smps_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info *info = rdev_get_drvdata(rdev);
-	int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+	int vsel;
+
+	if (info->voltdm.ptr)
+		return voltdm_get_voltage(info->voltdm.ptr);
+
+	vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
 		VREG_VOLTAGE_SMPS_4030);
 
 	return vsel * 12500 + 600000;
@@ -883,9 +901,13 @@ static struct regulator_ops twlsmps_ops = {
 		}, \
 	}
 
-#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, turnon_delay, remap_conf) \
+#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, voltdm_name, turnon_delay, \
+	remap_conf) \
 	{ \
 	.base = offset, \
+	.voltdm = { \
+		.name = #voltdm_name, \
+	}, \
 	.id = num, \
 	.delay = turnon_delay, \
 	.remap = remap_conf, \
@@ -989,8 +1011,8 @@ static struct twlreg_info twl_regs[] = {
 	TWL4030_ADJUSTABLE_LDO(VINTANA2, 0x43, 12, 100, 0x08),
 	TWL4030_FIXED_LDO(VINTDIG, 0x47, 1500, 13, 100, 0x08),
 	TWL4030_ADJUSTABLE_LDO(VIO, 0x4b, 14, 1000, 0x08),
-	TWL4030_ADJUSTABLE_SMPS(VDD1, 0x55, 15, 1000, 0x08),
-	TWL4030_ADJUSTABLE_SMPS(VDD2, 0x63, 16, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD1, 0x55, 15, mpu_iva, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD2, 0x63, 16, core, 1000, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V5, 0x71, 1500, 17, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V8, 0x74, 1800, 18, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB3V1, 0x77, 3100, 19, 150, 0x08),
@@ -1091,6 +1113,9 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 		break;
 	}
 
+	if (info->voltdm.name && info->features & TWL_VP_SMPS_MODE)
+		info->voltdm.ptr = voltdm_lookup(info->voltdm.name);
+
 	switch (pdev->id) {
 	case TWL6025_REG_SMPS3:
 		if (twl_get_smps_mult() & SMPS_MULTOFFSET_SMPS3)
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 114c0f6..17000e2 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -172,6 +172,7 @@ TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
 TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
 
 #define TWL6025_SUBCLASS	BIT(4)  /* TWL6025 has changed registers */
+#define TWL_VP_SMPS_MODE	BIT(5)	/* Use VP control path for SMPS regs */
 
 /*
  * Read and write single 8-bit registers
-- 
1.7.4.1

[PATCHv7 7/7] omap3: twl-common: enable VP SMPS mode for VDD1 and VDD2

[Tero Kristo]

This changes the control path for VDD1 and VDD2 regulators from
the I2C to be voltage processor.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/twl-common.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
index 7cf5347..01632aa 100644
--- a/arch/arm/mach-omap2/twl-common.c
+++ b/arch/arm/mach-omap2/twl-common.c
@@ -133,6 +133,7 @@ static struct regulator_init_data omap3_vdd1 = {
 	},
 	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd1_supply),
 	.consumer_supplies		= omap3_vdd1_supply,
+	.driver_data			= (void *)TWL_VP_SMPS_MODE,
 };
 
 static struct regulator_init_data omap3_vdd2 = {
@@ -145,6 +146,7 @@ static struct regulator_init_data omap3_vdd2 = {
 	},
 	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd2_supply),
 	.consumer_supplies		= omap3_vdd2_supply,
+	.driver_data			= (void *)TWL_VP_SMPS_MODE,
 };
 
 void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
-- 
1.7.4.1

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Mark Brown]

On Mon, Nov 28, 2011 at 04:53:24PM +0200, Tero Kristo wrote:

> +++ b/drivers/regulator/twl-regulator.c
> @@ -18,6 +18,7 @@
>  #include <linux/regulator/machine.h>
>  #include <linux/i2c/twl.h>
>  
> +#include <plat/voltage.h>

You shouldn't be including platform specific headers in generic code.

> +	/* voltagedomain, only used for VP controlled smps regulators */
> +	union {
> +		const char		*name;
> +		struct voltagedomain	*ptr;
> +	} voltdm;
> +

This looks pretty icky...  Why are you using a union of a pointer to a
struct and a name?  How do things know which to use?

> -	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
> -		vsel);
> +	if (info->voltdm.ptr)
> +		voltdm_scale(info->voltdm.ptr, min_uV);
> +	else {


Use braces on both branches.

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

On Mon, 2011-11-28 at 14:58 +0000, Mark Brown wrote:
> On Mon, Nov 28, 2011 at 04:53:24PM +0200, Tero Kristo wrote:
> 
> > +++ b/drivers/regulator/twl-regulator.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/regulator/machine.h>
> >  #include <linux/i2c/twl.h>
> >  
> > +#include <plat/voltage.h>
> 
> You shouldn't be including platform specific headers in generic code.

Hmm, should I pass the function pointers through platform data also?
Currently this does not seem to be too easy seeing we have a limited
number of parameters that can be passed from board and these are already
in use. regulator_init_data->driver_data is already used as a bitmask
passing feature flags around.

I could change driver_data to be a struct pointer and add the func
pointers + feature flags inside it. However it will end up changing more
code around.

> 
> > +	/* voltagedomain, only used for VP controlled smps regulators */
> > +	union {
> > +		const char		*name;
> > +		struct voltagedomain	*ptr;
> > +	} voltdm;
> > +
> 
> This looks pretty icky...  Why are you using a union of a pointer to a
> struct and a name?  How do things know which to use?

Name is only used during probe, after that we always use the ptr. If
name is not defined, ptr ends up as NULL and we use default mode.

I can change this and add func pointers for both get/set voltages and
also separate pointer for the voltagedomain itself, if I am going to
pass all of these via platform data.

> 
> > -	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
> > -		vsel);
> > +	if (info->voltdm.ptr)
> > +		voltdm_scale(info->voltdm.ptr, min_uV);
> > +	else {
> 
> 
> Use braces on both branches.

Okay.

-Tero

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Mark Brown]

On Mon, Nov 28, 2011 at 05:43:41PM +0200, Tero Kristo wrote:
> On Mon, 2011-11-28 at 14:58 +0000, Mark Brown wrote:

> > You shouldn't be including platform specific headers in generic code.

> Hmm, should I pass the function pointers through platform data also?
> Currently this does not seem to be too easy seeing we have a limited
> number of parameters that can be passed from board and these are already
> in use. regulator_init_data->driver_data is already used as a bitmask
> passing feature flags around.

> I could change driver_data to be a struct pointer and add the func
> pointers + feature flags inside it. However it will end up changing more
> code around.

Whatever you do you should preserve the ability of the driver to build
on non-OMAP platforms.

> > > +	/* voltagedomain, only used for VP controlled smps regulators */
> > > +	union {
> > > +		const char		*name;
> > > +		struct voltagedomain	*ptr;
> > > +	} voltdm;

> > This looks pretty icky...  Why are you using a union of a pointer to a
> > struct and a name?  How do things know which to use?

> Name is only used during probe, after that we always use the ptr. If
> name is not defined, ptr ends up as NULL and we use default mode.

That's fairly nasty, just use two variables or something.  The above is
just asking for breakage.

Re: [PATCHv7 1/7] regulator: twl: fix twl4030 support for smps regulators

[Mark Brown]

On Mon, Nov 28, 2011 at 04:53:19PM +0200, Tero Kristo wrote:
> SMPS regulator voltage control differs from the one of the LDO ones.
> Current TWL code was using LDO regulator ops for controlling the SMPS
> regulators, which fails. This was fixed fixed by adding separate
> regulator type which uses correct logic and calculations for the
> voltage levels.

Applied, thanks.

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Bedia, Vaibhav]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kristo, Tero
> Sent: Monday, November 28, 2011 8:23 PM
> To: linux-omap@vger.kernel.org
> Cc: sameo@linux.intel.com; broonie@opensource.wolfsonmicro.com;
> Girdwood, Liam; Hilman, Kevin; Cousson, Benoit; Nayak, Rajendra;
> gg@slimlogic.co.uk
> Subject: [PATCHv7 6/7] regulator: twl: add support for external
> controller
> 
> TWL regulator now has two alternative control paths: the default
> I2C path or an optional voltage processor path for OMAP chips.
> If the voltage processor path should be used, this can be
> indicated within the platform data by adding flag TWL_VP_SMPS_MODE
> to regulator_init_data->driver_data.
>

Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
TWL_SR_SMPS_MODE?

AFAIK the second I2C interface in all these PMICs is meant only
for the SMPS outputs. Moreover, which of the two interfaces to use
would be known during the init phase.

Is there a scenario where someone would want to change this at runtime? 

At least for TPS65910, it is possible to use the SR-I2C interface 
just like the CTL interface. In other words, even SoCs which do not 
have VP/VC can choose to change the SMPS voltage using the SR-I2C 
interface. The only catch is that only the SMPS registers can be 
accessed from this interface.

If this is the case with TWL series also, we could have non VP/VC 
specific smps_get/set_voltage APIs for the SR-I2C interface which 
SoCs with VP/VC override at init time. Note that the driver will have
two smps_get/set_voltage implementations, one corresponding to CTL-I2C
and the other corresponding to SR-I2C. During the init phase, which set
of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.

With such a change, the voltage change during DVFS would also boil down
to normal regulator calls for changing the voltage. SoCs which do not
have the VP/VC functionality can use the default implementations 
via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
use their specific implementations.

Regards,
Vaibhav

P.S. - Somehow this patch series didn't appear in my mailbox. Reply from Gmane
seems to have gotten lost. So I replying to a forwared version. Hope this appears ok.

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

On Tue, 2011-11-29 at 08:10 +0100, Bedia, Vaibhav wrote:
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Kristo, Tero
> > Sent: Monday, November 28, 2011 8:23 PM
> > To: linux-omap@vger.kernel.org
> > Cc: sameo@linux.intel.com; broonie@opensource.wolfsonmicro.com;
> > Girdwood, Liam; Hilman, Kevin; Cousson, Benoit; Nayak, Rajendra;
> > gg@slimlogic.co.uk
> > Subject: [PATCHv7 6/7] regulator: twl: add support for external
> > controller
> > 
> > TWL regulator now has two alternative control paths: the default
> > I2C path or an optional voltage processor path for OMAP chips.
> > If the voltage processor path should be used, this can be
> > indicated within the platform data by adding flag TWL_VP_SMPS_MODE
> > to regulator_init_data->driver_data.
> >
> 
> Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
> the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
> instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
> TWL_SR_SMPS_MODE?

I was actually thinking about naming it something like SR mode, however
I think I will drop the flag away as unnecessary once I add the function
pointers for the voltage_get / voltage_scale in to the platform data.

> 
> AFAIK the second I2C interface in all these PMICs is meant only
> for the SMPS outputs. Moreover, which of the two interfaces to use
> would be known during the init phase.

Thats my understanding also.

> 
> Is there a scenario where someone would want to change this at runtime? 

Only testing purposes come to my mind, it is easier to try out some
interesting stuff if you can change it runtime, other than that, not
really. This set is also setting the used mode during init time also,
and there is no runtime configuration possibility.

> 
> At least for TPS65910, it is possible to use the SR-I2C interface 
> just like the CTL interface. In other words, even SoCs which do not 
> have VP/VC can choose to change the SMPS voltage using the SR-I2C 
> interface. The only catch is that only the SMPS registers can be 
> accessed from this interface.

Yea, the SR-I2C should be possible to use with non VP/VC setup, if
someone really wanted to do this. If anybody would actually want to do
it is another question, as you can control everything from the normal
I2C control interface. If you don't have dedicated VP hardware, the use
for the secondary I2C channel is not that obvious.

>
> If this is the case with TWL series also, we could have non VP/VC 
> specific smps_get/set_voltage APIs for the SR-I2C interface which 
> SoCs with VP/VC override at init time. Note that the driver will have
> two smps_get/set_voltage implementations, one corresponding to CTL-I2C
> and the other corresponding to SR-I2C. During the init phase, which set
> of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.
> 
> With such a change, the voltage change during DVFS would also boil down
> to normal regulator calls for changing the voltage. SoCs which do not
> have the VP/VC functionality can use the default implementations 
> via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
> use their specific implementations.

Yea, for next version I am planning to add voltage_get / voltage_set and
the voltagedomain pointer as platform data for the regulator, these can
be defined if the user wants to use them, otherwise twl-regulator will
use the default implementation.

-Tero

> 
> Regards,
> Vaibhav
> 
> P.S. - Somehow this patch series didn't appear in my mailbox. Reply from Gmane
> seems to have gotten lost. So I replying to a forwared version. Hope this appears ok.

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Bedia, Vaibhav]

On Tue, Nov 29, 2011 at 16:49:49, Kristo, Tero wrote:
[...]
> > 
> > Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
> > the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
> > instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
> > TWL_SR_SMPS_MODE?
> 
> I was actually thinking about naming it something like SR mode, however
> I think I will drop the flag away as unnecessary once I add the function
> pointers for the voltage_get / voltage_scale in to the platform data.
> 

Sounds ok.

> > 
> > AFAIK the second I2C interface in all these PMICs is meant only
> > for the SMPS outputs. Moreover, which of the two interfaces to use
> > would be known during the init phase.
> 
> Thats my understanding also.
> 
> > 
> > Is there a scenario where someone would want to change this at runtime? 
> 
> Only testing purposes come to my mind, it is easier to try out some
> interesting stuff if you can change it runtime, other than that, not
> really. This set is also setting the used mode during init time also,
> and there is no runtime configuration possibility.
> 

So there's not much value is trying to implement runtime switch between 
the interfaces in the driver.

> > 
> > At least for TPS65910, it is possible to use the SR-I2C interface 
> > just like the CTL interface. In other words, even SoCs which do not 
> > have VP/VC can choose to change the SMPS voltage using the SR-I2C 
> > interface. The only catch is that only the SMPS registers can be 
> > accessed from this interface.
> 
> Yea, the SR-I2C should be possible to use with non VP/VC setup, if
> someone really wanted to do this. If anybody would actually want to do
> it is another question, as you can control everything from the normal
> I2C control interface. If you don't have dedicated VP hardware, the use
> for the secondary I2C channel is not that obvious.
> 

Well multi-processor scenario is something that come to mind. If there's 
a different core which can also talk to the PMIC, it could use one and the
host processor can use another. There are obvious issues like race conditions
etc which have to be taken into account but it's very much possible. 

> >
> > If this is the case with TWL series also, we could have non VP/VC 
> > specific smps_get/set_voltage APIs for the SR-I2C interface which 
> > SoCs with VP/VC override at init time. Note that the driver will have
> > two smps_get/set_voltage implementations, one corresponding to CTL-I2C
> > and the other corresponding to SR-I2C. During the init phase, which set
> > of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.
> > 
> > With such a change, the voltage change during DVFS would also boil down
> > to normal regulator calls for changing the voltage. SoCs which do not
> > have the VP/VC functionality can use the default implementations 
> > via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
> > use their specific implementations.
> 
> Yea, for next version I am planning to add voltage_get / voltage_set and
> the voltagedomain pointer as platform data for the regulator, these can
> be defined if the user wants to use them, otherwise twl-regulator will
> use the default implementation.
> 

Ok.

Regards,
Vaibhav

Re: [PATCHv7 3/7] omap3: voltage: fix channel configuration

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
> overlap with VDD2 and attempting to modify VDD1 voltage will actually change
> VDD2 voltage.
>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>

I've forgotten a bit how this was supposed to work (again), Can you
elaborate more on how this fails?

Setting the OMAP_VC_CHANNEL_DEFAULT flag makes sense to me, but the
other change doesn't.

Kevin

Re: [PATCHv7 4/7] omap3: add common twl configurations for vdd1 and vdd2

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> VDD1 and VDD2 are the core voltage regulators on OMAP3. VDD1 is used
> to control MPU/IVA voltage, and VDD2 is used for CORE. These regulators
> are needed by DVFS.
>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>

[...]

> +static struct regulator_init_data omap3_vdd1 = {
> +	.constraints = {
> +		.name			= "VDD1",
> +		.min_uV			= 600000,
> +		.max_uV			= 1450000,

nit: can you add a bit to the changelog referencing the document where
theses min/max voltage values come from?

Kevin

Re: [PATCHv7 3/7] omap3: voltage: fix channel configuration

[Tero Kristo]

On Fri, 2011-12-02 at 15:55 -0800, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
> > VDD2 voltage.
> >
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> 
> I've forgotten a bit how this was supposed to work (again), Can you
> elaborate more on how this fails?

There is a diagram in the OMAP TRM for setting the bits in this
register, however the racen fix part appears to be needed only for
omap4. I can drop this part of the fix from the series if you want for
the next version, alternatively I can split this patch into two.

The idea for this part of the fix is anyway that the channel
configuration is more complex in omap4, we define volt_reg and cmd_reg
addresses for each omap4_X_pmic, however we only want to enable racen
bit only if cmd and volt register addresses are the same. Otherwise the
voltage commands are not sent (or are sent to invalid address) which
causes the voltage commands not to change voltages at all.

-Tero

> Setting the OMAP_VC_CHANNEL_DEFAULT flag makes sense to me, but the
> other change doesn't.
> 
> Kevin

Re: [PATCHv7 3/7] omap3: voltage: fix channel configuration

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> On Fri, 2011-12-02 at 15:55 -0800, Kevin Hilman wrote:
>> Tero Kristo <t-kristo@ti.com> writes:
>> 
>> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
>> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
>> > VDD2 voltage.
>> >
>> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> 
>> I've forgotten a bit how this was supposed to work (again), Can you
>> elaborate more on how this fails?
>
> There is a diagram in the OMAP TRM for setting the bits in this
> register, however the racen fix part appears to be needed only for
> omap4. I can drop this part of the fix from the series if you want for
> the next version, alternatively I can split this patch into two.

A separate patch, with a descriptive changelog would be preferred.

> The idea for this part of the fix is anyway that the channel
> configuration is more complex in omap4, we define volt_reg and cmd_reg
> addresses for each omap4_X_pmic, however we only want to enable racen
> bit only if cmd and volt register addresses are the same. 

This part still doesn't make sense to me.

If the volt register address (RA_VOL in OMAP4 terms) and command
register address (RA_CMD) are the same, then RACEN shouldn't matter,
since either way the commands go the same address.   

My understanding of RACEN is that it is only for the case where RA_VOL
and RA_CMD are different, so you can select between them.

The current code assumes that if you pass in command register address
you plan to use it.  If it isn't being used (RACEN == 0) then the PMIC
setup should probably not pass in a separate command register address.

What am I missing (or mis-reading in the TRM) here?

Kevin

Re: [PATCHv7 3/7] omap3: voltage: fix channel configuration

[Tero Kristo]

On Mon, 2011-12-05 at 12:23 -0800, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > On Fri, 2011-12-02 at 15:55 -0800, Kevin Hilman wrote:
> >> Tero Kristo <t-kristo@ti.com> writes:
> >> 
> >> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
> >> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
> >> > VDD2 voltage.
> >> >
> >> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >> 
> >> I've forgotten a bit how this was supposed to work (again), Can you
> >> elaborate more on how this fails?
> >
> > There is a diagram in the OMAP TRM for setting the bits in this
> > register, however the racen fix part appears to be needed only for
> > omap4. I can drop this part of the fix from the series if you want for
> > the next version, alternatively I can split this patch into two.
> 
> A separate patch, with a descriptive changelog would be preferred.
> 
> > The idea for this part of the fix is anyway that the channel
> > configuration is more complex in omap4, we define volt_reg and cmd_reg
> > addresses for each omap4_X_pmic, however we only want to enable racen
> > bit only if cmd and volt register addresses are the same. 
> 
> This part still doesn't make sense to me.
> 
> If the volt register address (RA_VOL in OMAP4 terms) and command
> register address (RA_CMD) are the same, then RACEN shouldn't matter,
> since either way the commands go the same address.   
> 
> My understanding of RACEN is that it is only for the case where RA_VOL
> and RA_CMD are different, so you can select between them.

I am not sure if you got the polarity of the bit right. RACEN should be
enabled only if the register addresses are the same. My understanding is
that: if command register is defined => RAC=1, if voltage register is
defined => RAV=1. If VFSM should use voltage register address for
sending voltage commands => RACEN=1, otherwise it will use command
register address.

> 
> The current code assumes that if you pass in command register address
> you plan to use it.  If it isn't being used (RACEN == 0) then the PMIC
> setup should probably not pass in a separate command register address.
> 
> What am I missing (or mis-reading in the TRM) here?

The register addresses defined in omap_twl for omap4 pmics are
different, and this is causing trouble at the moment if RACEN is
enabled, as VFSM commands will end up going to voltage register address.
One could probably also argue that the register addresses for these
should be the same...

The documentation for this part of the chip is rather bad anyway. Added
Nishanth to CC as he might have some comments on this also.

-Tero

Re: [PATCHv7 3/7] omap3: voltage: fix channel configuration

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> On Mon, 2011-12-05 at 12:23 -0800, Kevin Hilman wrote:
>> Tero Kristo <t-kristo@ti.com> writes:
>> 
>> > On Fri, 2011-12-02 at 15:55 -0800, Kevin Hilman wrote:
>> >> Tero Kristo <t-kristo@ti.com> writes:
>> >> 
>> >> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
>> >> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
>> >> > VDD2 voltage.
>> >> >
>> >> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> >> 
>> >> I've forgotten a bit how this was supposed to work (again), Can you
>> >> elaborate more on how this fails?
>> >
>> > There is a diagram in the OMAP TRM for setting the bits in this
>> > register, however the racen fix part appears to be needed only for
>> > omap4. I can drop this part of the fix from the series if you want for
>> > the next version, alternatively I can split this patch into two.
>> 
>> A separate patch, with a descriptive changelog would be preferred.
>> 
>> > The idea for this part of the fix is anyway that the channel
>> > configuration is more complex in omap4, we define volt_reg and cmd_reg
>> > addresses for each omap4_X_pmic, however we only want to enable racen
>> > bit only if cmd and volt register addresses are the same. 
>> 
>> This part still doesn't make sense to me.
>> 
>> If the volt register address (RA_VOL in OMAP4 terms) and command
>> register address (RA_CMD) are the same, then RACEN shouldn't matter,
>> since either way the commands go the same address.   
>> 
>> My understanding of RACEN is that it is only for the case where RA_VOL
>> and RA_CMD are different, so you can select between them.
>
> I am not sure if you got the polarity of the bit right. 

Neither am I.  :(

> RACEN should be enabled only if the register addresses are the same.

This is the statement that I don't understand, or find evidence for in
the docs.

> My understanding is that: if command register is defined => RAC=1, if
> voltage register is defined => RAV=1. 

So far, so good, and this follows the flow diagram in the OMAP3 TRM.

> If VFSM should use voltage register address for sending voltage
> commands => RACEN=1, otherwise it will use command register address.

This is where things start to deviate from my reading of the TRM (I'm
looking at Figure 4-98 in OMAP34XX ES3.x NDA TRM vZK.)

Specifially, see the block: "Voltage FSM uses voltage register address
to send voltage commands sending voltage commands."   Only in the "NO"
case is RACEN set to 1.  To me this means:

- RACEN = 0: use voltage register address (the 'YES' branch in figure)
- RACEN = 1: use command register address (the 'NO' branch in figure)

This matches for OMAP4, where the RACEN bit descriptions in
PRM_VC_CFG_CHANNEL say:

- 0x0: use VOLRA values for VFSM comands
- 0x1: use CMDRA values for VFSM commands

So, in the code, I made the assumption that if a comand register address
is passed in, it should be used for VFSM commands.  Therefore, RACEN is
set whenever RAC is set.  If the user wants the voltage register address
used for VFSM commands, it should simply not pass in a command register
address.

>> 
>> The current code assumes that if you pass in command register address
>> you plan to use it.  If it isn't being used (RACEN == 0) then the PMIC
>> setup should probably not pass in a separate command register ddress.
>> 
>> What am I missing (or mis-reading in the TRM) here?
>
> The register addresses defined in omap_twl for omap4 pmics are
> different, and this is causing trouble at the moment if RACEN is
> enabled, as VFSM commands will end up going to voltage register address.

Strange, if VFSM commands are going to the voltage register address when
RACEN=1, this doesn't match my reading of the docs (see my
interpretation of the docs above.)

> One could probably also argue that the register addresses for these
> should be the same...
>
> The documentation for this part of the chip is rather bad anyway. Added
> Nishanth to CC as he might have some comments on this also.

Hopefully Nishanth can shed some light.

It certainly won't be the first time that the TRM has been less than
helpful.

Kevin

Re: [PATCHv7 5/7] mfd: twl-core: pass driver data from pdata to add_regulator for VDD1 and VDD2

[Samuel Ortiz]

Hi Tero,


On Mon, Nov 28, 2011 at 04:53:23PM +0200, Tero Kristo wrote:
> This way, we can add custom flags for VDD1 and VDD2 regulators that
> get passed all the way to regulator init. This is needed for SMPS
> regulator support to select used controller mode for these regulators
> (either voltage processor or default.)
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  drivers/mfd/twl-core.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 01ecfee..af93fce 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -846,12 +846,14 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
>  			return PTR_ERR(child);
>  
>  		child = add_regulator(TWL4030_REG_VDD1, pdata->vdd1,
> -					features);
> +					features |
> +					(u32)pdata->vdd1->driver_data);
That looks hackish to me. Do you have any guarantee that your driver_data and
your features bitmaps have zero intersections ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCHv7 5/7] mfd: twl-core: pass driver data from pdata to add_regulator for VDD1 and VDD2

[Tero Kristo]

On Mon, 2011-12-12 at 19:04 +0100, Samuel Ortiz wrote:
> Hi Tero,
> 
> 
> On Mon, Nov 28, 2011 at 04:53:23PM +0200, Tero Kristo wrote:
> > This way, we can add custom flags for VDD1 and VDD2 regulators that
> > get passed all the way to regulator init. This is needed for SMPS
> > regulator support to select used controller mode for these regulators
> > (either voltage processor or default.)
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > ---
> >  drivers/mfd/twl-core.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> > index 01ecfee..af93fce 100644
> > --- a/drivers/mfd/twl-core.c
> > +++ b/drivers/mfd/twl-core.c
> > @@ -846,12 +846,14 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> >  			return PTR_ERR(child);
> >  
> >  		child = add_regulator(TWL4030_REG_VDD1, pdata->vdd1,
> > -					features);
> > +					features |
> > +					(u32)pdata->vdd1->driver_data);
> That looks hackish to me. Do you have any guarantee that your driver_data and
> your features bitmaps have zero intersections ?

That is somewhat hackish yes. I changed the implementation a bit for v8
(which you should also have in your inbox now, I sent it out on Friday),
I split the driver_data to a struct that contains a couple of function
pointers and the features flag. It is still using a bitwise OR for
setting the flags inside twl-core.c, but the entity that initializes the
driver_data sets the features always at zero, so twl-core is the only
one who sets these. That version of the patch could be changed from
twl-core point of view to just set the features field instead of OR.

v8 of the patch set actually merges the regulator driver and the
twl-core data passing parts together to avoid merge conflicts, so you
should take a look at the twl-core.c / include/linux/i2c/twl.h part of
patch:

[PATCHv8 4/5] twl4030: add support for external voltage get/set

-Tero