[PATCHv6 0/6] OMAP: SMPS support for TWL regulators

[PATCHv6 0/6] OMAP: SMPS support for TWL regulators

[Tero Kristo]

Hello,

Unfortunately the previous version of this work (SMPS regulator
was a separate driver) was rejected by the omap maintainers, thus
I am sending a new version. This implementation is going somewhat
back towards the original version, we are having an external
controller that replaces the default controller from a regulator
driver, in this case from twl-regulator. External controller
support is built in to regulator core now, and it is used when
available.

I think the main force behind driving the SMPS support back
towards this has been Kevin Hilman and Benoit Cousson from TI.

Rajendra Nayak also had a quick look at these patches and said
that adding device tree support on top of this is rather trivial,
however it can't be done at this point due to missing dependencies.

This set applies on top of Kevin's for_3.2/voltage-cleanup branch.

Tested on omap3 beagle, by changing voltages of both vdd1 and vdd2
regulators and measuring it from the hw.

-Tero

[PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Tero Kristo]

Regulator users can now set override functions for get_voltage and
set_voltage. This is required by some regulators, which have two
alternate control paths. E.g., OMAP SMPS regulators can be controlled
either through the I2C interface or voltage processor control path,
which uses specialized hardware.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Graeme Gregory <gg@slimlogic.co.uk>
---
 drivers/regulator/core.c           |   61 ++++++++++++++++++++++++++++++++++-
 include/linux/regulator/consumer.h |   20 ++++++++++++
 include/linux/regulator/driver.h   |   17 ++++++++++
 3 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d8e6a42..d70dd7a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1676,7 +1676,11 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	min_uV += rdev->constraints->uV_offset;
 	max_uV += rdev->constraints->uV_offset;
 
-	if (rdev->desc->ops->set_voltage) {
+	if (rdev->desc->ext_ctrl && rdev->desc->ext_ctrl->ops->set_voltage) {
+		ret = rdev->desc->ext_ctrl->ops->
+			set_voltage(rdev->desc->ext_ctrl->data, min_uV, max_uV);
+		selector = -1;
+	} else if (rdev->desc->ops->set_voltage) {
 		ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV,
 						   &selector);
 
@@ -1902,7 +1906,10 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 {
 	int sel, ret;
 
-	if (rdev->desc->ops->get_voltage_sel) {
+	if (rdev->desc->ext_ctrl && rdev->desc->ext_ctrl->ops->get_voltage) {
+		ret = rdev->desc->ext_ctrl->ops->
+			get_voltage(rdev->desc->ext_ctrl->data);
+	} else if (rdev->desc->ops->get_voltage_sel) {
 		sel = rdev->desc->ops->get_voltage_sel(rdev);
 		if (sel < 0)
 			return sel;
@@ -2886,6 +2893,56 @@ void regulator_set_drvdata(struct regulator *regulator, void *data)
 EXPORT_SYMBOL_GPL(regulator_set_drvdata);
 
 /**
+ * regulator_set_external_ctrl - set external controller for a regulator
+ * @devname: device for regulator "consumer"
+ * @id: Supply name or regulator ID 
+ * @ops: external controller operations table
+ * @data: user data
+ */
+void regulator_set_external_ctrl(const char *devname, const char *id,
+				 struct regulator_ext_ops *ops, void *data)
+{
+	struct regulator_map *map;
+	struct regulator_dev *rdev;
+
+	/* Look for the regulator device */
+	list_for_each_entry(map, &regulator_map_list, list) {
+		/* If the mapping has a device set up it must match */
+		if (map->dev_name &&
+		    (!devname || strcmp(map->dev_name, devname)))
+			continue;
+
+		if (strcmp(map->supply, id) == 0) {
+			rdev = map->regulator;
+			goto found;
+		}
+	}
+
+	return;
+
+found:
+	/* If no ops defined, disable external controller and return */
+	if (ops == NULL) {
+		kfree(rdev->desc->ext_ctrl);
+		rdev->desc->ext_ctrl = NULL;
+		return;
+	}
+
+	/* Alloc memory if needed */
+	if (!rdev->desc->ext_ctrl)
+		rdev->desc->ext_ctrl =
+			kzalloc(sizeof(struct regulator_ext_ctrl), GFP_KERNEL);
+
+	if (!rdev->desc->ext_ctrl) {
+		pr_warning("%s: kzalloc failed\n", __func__);
+		return;
+	}
+
+	rdev->desc->ext_ctrl->ops = ops;
+	rdev->desc->ext_ctrl->data = data;
+}
+
+/**
  * regulator_get_id - get regulator ID
  * @rdev: regulator
  */
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 26f6ea4..abd271e 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -127,6 +127,17 @@ struct regulator_bulk_data {
 	int ret;
 };
 
+/**
+ * struct regulator_ext_ops - Override operations for a regulator.
+ *
+ * @set_voltage: External override for the set_voltage operation for regulator.
+ * @get_voltage: External override for the get_voltage operation for regulator.
+ */
+struct regulator_ext_ops {
+	int (*set_voltage) (void *data, int min_uV, int max_uV);
+	int (*get_voltage) (void *data);
+};
+
 #if defined(CONFIG_REGULATOR)
 
 /* regulator get and put */
@@ -177,6 +188,8 @@ int regulator_unregister_notifier(struct regulator *regulator,
 /* driver data - core doesn't touch */
 void *regulator_get_drvdata(struct regulator *regulator);
 void regulator_set_drvdata(struct regulator *regulator, void *data);
+void regulator_set_external_ctrl(const char *devname, const char *id,
+				 struct regulator_ext_ops *ops, void *data);
 
 #else
 
@@ -301,6 +314,13 @@ static inline void regulator_set_drvdata(struct regulator *regulator,
 {
 }
 
+static inline void regulator_set_external_ctrl(const char *devname,
+					       const char *id,
+					       struct regulator_ext_ops *ops,
+					       void *data)
+{
+}
+
 #endif
 
 #endif
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 1a80bc7..b77bf69 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -147,6 +147,21 @@ enum regulator_type {
 };
 
 /**
+ * struct regulator_ext_ctrl - External controller for regulator
+ *
+ * If regulator is using external controller, i.e. control mechanism
+ * provided by some other driver, this defines the linking between the
+ * regulator and the external driver.
+ *
+ * @ops: External operations table
+ * @data: User data
+ */
+struct regulator_ext_ctrl {
+	struct regulator_ext_ops *ops;
+	void *data;
+};
+
+/**
  * struct regulator_desc - Regulator descriptor
  *
  * Each regulator registered with the core is described with a structure of
@@ -156,6 +171,7 @@ enum regulator_type {
  * @id: Numerical identifier for the regulator.
  * @n_voltages: Number of selectors available for ops.list_voltage().
  * @ops: Regulator operations table.
+ * @ext_ctrl: Optional external controller.
  * @irq: Interrupt number for the regulator.
  * @type: Indicates if the regulator is a voltage or current regulator.
  * @owner: Module providing the regulator, used for refcounting.
@@ -165,6 +181,7 @@ struct regulator_desc {
 	int id;
 	unsigned n_voltages;
 	struct regulator_ops *ops;
+	struct regulator_ext_ctrl *ext_ctrl;
 	int irq;
 	enum regulator_type type;
 	struct module *owner;
-- 
1.7.4.1

[PATCHv6 2/6] 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

[PATCHv6 3/6] 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

[PATCHv6 4/6] 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>
---
 drivers/regulator/twl-regulator.c |   49 +++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index ee8747f..97fe2f2 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,35 @@ 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);
+
+	pr_info("attempting to write: %02x\n", vsel);
+	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);
+
+	pr_info("vsel = %02x\n", vsel);
+
+	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 +886,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 +992,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

[PATCHv6 5/6] 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

[PATCHv6 6/6] omap3: voltage: add external controller for VDD1 and VDD2

[Tero Kristo]

VDD1 and VDD2 will now use voltage controller when regulator voltage
is changed.

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

diff --git a/arch/arm/mach-omap2/omap_twl.c b/arch/arm/mach-omap2/omap_twl.c
index f515a1a..4f69d8d 100644
--- a/arch/arm/mach-omap2/omap_twl.c
+++ b/arch/arm/mach-omap2/omap_twl.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/i2c/twl.h>
+#include <linux/regulator/consumer.h>
 
 #include "voltage.h"
 
@@ -72,6 +73,23 @@ static bool __initdata twl_sr_enable_autoinit;
 #define REG_SMPS_OFFSET         0xE0
 #define SMARTREFLEX_ENABLE     BIT(3)
 
+static int twl_set_voltage(void *data, int min_uV, int max_uV)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *)data;
+	return voltdm_scale(voltdm, min_uV);
+}
+
+static int twl_get_voltage(void *data)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *)data;
+	return voltdm_get_voltage(voltdm);
+}
+
+struct regulator_ext_ops twl_ext_ops = {
+	.get_voltage = twl_get_voltage,
+	.set_voltage = twl_set_voltage,
+};
+
 static unsigned long twl4030_vsel_to_uv(const u8 vsel)
 {
 	return (((vsel * 125) + 6000)) * 100;
@@ -284,6 +302,7 @@ int __init omap4_twl_init(void)
 int __init omap3_twl_init(void)
 {
 	struct voltagedomain *voltdm;
+	struct regulator *reg;
 
 	if (!cpu_is_omap34xx())
 		return -ENODEV;
@@ -309,9 +328,11 @@ int __init omap3_twl_init(void)
 
 	voltdm = voltdm_lookup("mpu_iva");
 	omap_voltage_register_pmic(voltdm, &omap3_mpu_pmic);
+	regulator_set_external_ctrl("mpu.0", "vcc", &twl_ext_ops, voltdm);
 
 	voltdm = voltdm_lookup("core");
 	omap_voltage_register_pmic(voltdm, &omap3_core_pmic);
+	regulator_set_external_ctrl("l3_main.0", "vcc", &twl_ext_ops, voltdm);
 
 	return 0;
 }
-- 
1.7.4.1

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Mark Brown]

On Fri, Nov 25, 2011 at 06:29:17PM +0200, Tero Kristo wrote:

Why is this only on the OMAP list?  Always CC the relevant discussion
list for the subsystem, especially when proposing changes to the
subsystem!

> Regulator users can now set override functions for get_voltage and
> set_voltage. This is required by some regulators, which have two
> alternate control paths. E.g., OMAP SMPS regulators can be controlled
> either through the I2C interface or voltage processor control path,
> which uses specialized hardware.

My basic reaction to this is "eew, ick".  Doing this with a runtime call
just feels badly joined up, and there's nothing here which hands off the
configuration between the various drivers involved in the transitions.
We need to make sure that the voltage doesn't suddenly lurch around when
doing transitions.

There's also a lack of locking in the code which would seem to be
required and I'd expect regulator_set_external_ctl() to return an error
if it fails.

Re: [PATCHv6 4/6] regulator: twl: fix twl4030 support for smps regulators

[Mark Brown]

On Fri, Nov 25, 2011 at 06:29:20PM +0200, Tero Kristo wrote:

> +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> +	int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
> +
> +	pr_info("attempting to write: %02x\n", vsel);

There's lots of prints in the driver which are at most dev_dbg().

Re: [PATCHv6 4/6] regulator: twl: fix twl4030 support for smps regulators

[Tero Kristo]

On Fri, 2011-11-25 at 16:55 +0000, Mark Brown wrote:
> On Fri, Nov 25, 2011 at 06:29:20PM +0200, Tero Kristo wrote:
> 
> > +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> > +	int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
> > +
> > +	pr_info("attempting to write: %02x\n", vsel);
> 
> There's lots of prints in the driver which are at most dev_dbg().

Oh yea, sorry. I accidentally left my debug prints in there, will be
removed in next version.

-Tero

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Tero Kristo]

On Fri, 2011-11-25 at 16:52 +0000, Mark Brown wrote:
> On Fri, Nov 25, 2011 at 06:29:17PM +0200, Tero Kristo wrote:
> 
> Why is this only on the OMAP list?  Always CC the relevant discussion
> list for the subsystem, especially when proposing changes to the
> subsystem!
> 
> > Regulator users can now set override functions for get_voltage and
> > set_voltage. This is required by some regulators, which have two
> > alternate control paths. E.g., OMAP SMPS regulators can be controlled
> > either through the I2C interface or voltage processor control path,
> > which uses specialized hardware.
> 
> My basic reaction to this is "eew, ick".  Doing this with a runtime call
> just feels badly joined up, and there's nothing here which hands off the
> configuration between the various drivers involved in the transitions.
> We need to make sure that the voltage doesn't suddenly lurch around when
> doing transitions.

Would you feel better if we just hacked around with the twl-regulator
driver and added a compile time switch for the voltage get/set for smps
regulators?

> 
> There's also a lack of locking in the code which would seem to be
> required and I'd expect regulator_set_external_ctl() to return an error
> if it fails.

Well, I can add locking in to the code if you think this dynamic
approach would be something that is acceptable, and even useful for
someone. Either way, we need to do this somehow, it is just agreeing
what is the cleanest way to do it.

-Tero

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Mark Brown]

On Fri, Nov 25, 2011 at 07:20:32PM +0200, Tero Kristo wrote:
> On Fri, 2011-11-25 at 16:52 +0000, Mark Brown wrote:

> > My basic reaction to this is "eew, ick".  Doing this with a runtime call
> > just feels badly joined up, and there's nothing here which hands off the
> > configuration between the various drivers involved in the transitions.
> > We need to make sure that the voltage doesn't suddenly lurch around when
> > doing transitions.

> Would you feel better if we just hacked around with the twl-regulator
> driver and added a compile time switch for the voltage get/set for smps
> regulators?

No, that's clearly going to break multi-board kernel images.  If this
is something board specific platform/device tree data sounds like the
way forwards.

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Tero Kristo]

On Fri, 2011-11-25 at 17:29 +0000, Mark Brown wrote:
> On Fri, Nov 25, 2011 at 07:20:32PM +0200, Tero Kristo wrote:
> > On Fri, 2011-11-25 at 16:52 +0000, Mark Brown wrote:
> 
> > > My basic reaction to this is "eew, ick".  Doing this with a runtime call
> > > just feels badly joined up, and there's nothing here which hands off the
> > > configuration between the various drivers involved in the transitions.
> > > We need to make sure that the voltage doesn't suddenly lurch around when
> > > doing transitions.
> 
> > Would you feel better if we just hacked around with the twl-regulator
> > driver and added a compile time switch for the voltage get/set for smps
> > regulators?
> 
> No, that's clearly going to break multi-board kernel images.  If this
> is something board specific platform/device tree data sounds like the
> way forwards.

Okay, so I will update this patch with proper locking and you should be
okay with that approach? Usually in the omap case we just want to switch
to using the external controller during init time and leave it be like
that, there is no really use-case as such for runtime switching of
controller, except maybe for some experimental / debug usage.

-Tero

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Mark Brown]

On Fri, Nov 25, 2011 at 07:59:03PM +0200, Tero Kristo wrote:
> On Fri, 2011-11-25 at 17:29 +0000, Mark Brown wrote:

> > No, that's clearly going to break multi-board kernel images.  If this
> > is something board specific platform/device tree data sounds like the
> > way forwards.

> Okay, so I will update this patch with proper locking and you should be
> okay with that approach? Usually in the omap case we just want to switch
> to using the external controller during init time and leave it be like
> that, there is no really use-case as such for runtime switching of
> controller, except maybe for some experimental / debug usage.

No - as I said "platform/device tree data sounds like the way forwards."

Re: [PATCHv6 1/6] regulator: core: add support for external get/set_voltage

[Tero Kristo]

On Fri, 2011-11-25 at 18:48 +0000, Mark Brown wrote:
> On Fri, Nov 25, 2011 at 07:59:03PM +0200, Tero Kristo wrote:
> > On Fri, 2011-11-25 at 17:29 +0000, Mark Brown wrote:
> 
> > > No, that's clearly going to break multi-board kernel images.  If this
> > > is something board specific platform/device tree data sounds like the
> > > way forwards.
> 
> > Okay, so I will update this patch with proper locking and you should be
> > okay with that approach? Usually in the omap case we just want to switch
> > to using the external controller during init time and leave it be like
> > that, there is no really use-case as such for runtime switching of
> > controller, except maybe for some experimental / debug usage.
> 
> No - as I said "platform/device tree data sounds like the way forwards."

Ok thanks, I'll take a look at this and try to forge the code into a
platform data type solution.

-Tero