[RFC 0/4] TWL external controller support

[RFC 0/4] TWL external controller support

[Tero Kristo]

Hello,

Following patches add an external controller support for TWL SMPS regulators.
This is needed because OMAP has voltage processor support which provides
interface to control a few regulators (VDD1 / VDD2 for OMAP3), and this
is shared with smartreflex.

These patches work in a way that twl regulators now provide an external
controller registration interface, and this is used by the OMAP voltage
layer during init. set_voltage / get_voltage APIs now check for presence
of external controller, and if it is there, calls are routed to external
controller.

Baseline work for these patches was done by Thomas Petezzoni.

Any comments welcome, does something like this look like it could be pushed
to regulator framework?

-Tero


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

[RFC 1/4] twl-regulator: extend for SMPS regulators and external controllers

[Tero Kristo]

This commit adds two things to the TWL regulator driver code :

 * It extends the twl4030_set_voltage() and twl4030_get_voltage()
   functions to understand that VDD1 and VDD2 are different regulators
   from all the other regulators: they don't support a fixed set of
   voltages, but a wide range of voltages between two minimum and
   maximum limits.

 * It creates a twlreg_ext_ctrl structure, which allows code outside
   of the TWL regulator driver to implement a regulator
   controller. Such a controller is attached using the new
   twlreg_attach_external_controller() function of the driver. When
   such a controller is attached to a regulator, the ->set_voltage()
   and ->get_voltage() calls made on the regulator will be forwarded
   to the external controller. This facility will later be used to
   integrate the Voltage Controller and SmartReflex features of the
   OMAP CPU with this regulator driver.

TODO:

 * Create a proper mechanism to handle SMPS regulators instead of
   special casing VDD1/VDD2. Probably by creating a new regulator
   type, next to FIXED_LDO and ADJUSTABLE_LDO.

 * See if other methods than ->set_voltage() and ->get_voltage() need
   to be captured by the external controller.

 * Extend to TWL6030.

 * See if instead of using a late-binding method using
   twlreg_attach_external_controller(), it could be possible to pass
   the twlreg_ext_ctrl structure through the
   regulator_init_data->driver_data field. For the moment, this isn't
   possible since the OMAP voltage layer isn't initialized when the
   regulator is instantiated.

 * Make the twl-regulator driver actually work with VDD1/VDD2 when no
   external controller is attached (i.e, when the OMAP voltage layer
   code is disabled).

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Thomas Petazzoni <t-petazzoni@ti.com>
---
 drivers/regulator/twl-regulator.c |   74 ++++++++++++++++++++++++++++++++++++-
 include/linux/regulator/twl.h     |   37 ++++++++++++++++++
 2 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/regulator/twl.h

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 87fe0f7..2d8546d 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -17,6 +17,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/i2c/twl.h>
+#include <linux/regulator/twl.h>
 
 
 /*
@@ -58,6 +59,9 @@ struct twlreg_info {
 
 	/* chip specific features */
 	unsigned long 		features;
+
+	/* external controller */
+	struct twlreg_ext_ctrl	*ext_ctrl;
 };
 
 
@@ -71,6 +75,7 @@ struct twlreg_info {
 #define VREG_TYPE		1
 #define VREG_REMAP		2
 #define VREG_DEDICATED		3	/* LDO control */
+#define VREG_VSEL		9	/* SMPS voltage */
 /* TWL6030 register offsets */
 #define VREG_TRANS		1
 #define VREG_STATE		2
@@ -465,6 +470,22 @@ twl4030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 	int			vsel;
 
+	/* An external controller is controlling us */
+	if (info->ext_ctrl && info->ext_ctrl->set_voltage) {
+		*selector = 0;
+		return info->ext_ctrl->set_voltage(info->ext_ctrl, min_uV,
+			max_uV);
+	}
+
+	/* Handle SMPS regulators separatly */
+	if (info->desc.id == TWL4030_REG_VDD1 ||
+	    info->desc.id == TWL4030_REG_VDD2) {
+		int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
+		twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VSEL, vsel);
+		*selector = 0;
+		return 0;
+	}
+
 	for (vsel = 0; vsel < info->table_len; vsel++) {
 		int mV = info->table[vsel];
 		int uV;
@@ -489,8 +506,22 @@ twl4030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 static int twl4030ldo_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);
+	int vsel;
+
+	/* An external controller is controlling us */
+	if (info->ext_ctrl && info->ext_ctrl->get_voltage)
+		return info->ext_ctrl->get_voltage(info->ext_ctrl);
+
+	/* Handle SMPS regulators separatly */
+	if (info->desc.id == TWL4030_REG_VDD1 ||
+	    info->desc.id == TWL4030_REG_VDD2) {
+		vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+				   VREG_VSEL);
+		return (((vsel * 125) + 6000)) * 100;
+	}
+
+	vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+			   VREG_VOLTAGE);
 
 	if (vsel < 0)
 		return vsel;
@@ -1007,6 +1038,45 @@ static u8 twl_get_smps_mult(void)
 	return value;
 }
 
+int twlreg_attach_external_controller(const char *name,
+				      struct twlreg_ext_ctrl *ext_ctrl)
+{
+	struct twlreg_info *info = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(twl_regs); i++) {
+		if (!strcmp(twl_regs[i].desc.name, name)) {
+			info = twl_regs + i;
+			break;
+		}
+	}
+
+	if (!info)
+		return -ENOENT;
+
+	info->ext_ctrl = ext_ctrl;
+	return 0;
+}
+
+int twlreg_remove_external_controller(const char *name)
+{
+	struct twlreg_info *info = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(twl_regs); i++) {
+		if (!strcmp(twl_regs[i].desc.name, name)) {
+			info = twl_regs + i;
+			break;
+		}
+	}
+
+	if (!info)
+		return -ENOENT;
+
+	info->ext_ctrl = NULL;
+	return 0;
+}
+
 static int __devinit twlreg_probe(struct platform_device *pdev)
 {
 	int				i;
diff --git a/include/linux/regulator/twl.h b/include/linux/regulator/twl.h
new file mode 100644
index 0000000..a27a8e9
--- /dev/null
+++ b/include/linux/regulator/twl.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2008 Thomas Petazzoni
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_REGULATOR_TWL_H
+#define __LINUX_REGULATOR_TWL_H
+
+/*
+ * twlreg_ext_ctrl allows to define an external controller for TWL
+ * regulators. Such an external controller can be attached to the
+ * .driver_data field of the regulator_init_data structure when
+ * instantiating a TWL regulator. It is useful in situations where the
+ * TWL regulator is not directly controlled by software, but is
+ * controlled by another separate piece of hardware. The TWL regulator
+ * driver will forward the set_voltage/get_voltage calls to the
+ * external controller driver, so that from a regulator consumer
+ * perspective, the fact that the regulator is controlled is a special
+ * way remains transparent.
+ */
+struct twlreg_ext_ctrl {
+	int (*set_voltage)(struct twlreg_ext_ctrl *twl_ext_ctrl,
+			   int min_uV, int max_uV);
+	int (*get_voltage)(struct twlreg_ext_ctrl *twl_ext_ctrl);
+	void *data;
+};
+
+int twlreg_attach_external_controller(const char *name,
+				      struct twlreg_ext_ctrl *ext_ctrl);
+int twlreg_remove_external_controller(const char *name);
+
+#endif /* __LINUX_REGULATOR_TWL_H */
-- 
1.7.4.1


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 1/4] twl-regulator: extend for SMPS regulators and external controllers

[Liam Girdwood]

On Fri, 2011-07-08 at 17:56 +0200, Kristo, Tero wrote:
> This commit adds two things to the TWL regulator driver code :
> 
>  * It extends the twl4030_set_voltage() and twl4030_get_voltage()
>    functions to understand that VDD1 and VDD2 are different regulators
>    from all the other regulators: they don't support a fixed set of
>    voltages, but a wide range of voltages between two minimum and
>    maximum limits.
> 
>  * It creates a twlreg_ext_ctrl structure, which allows code outside
>    of the TWL regulator driver to implement a regulator
>    controller. Such a controller is attached using the new
>    twlreg_attach_external_controller() function of the driver. When
>    such a controller is attached to a regulator, the ->set_voltage()
>    and ->get_voltage() calls made on the regulator will be forwarded
>    to the external controller. This facility will later be used to
>    integrate the Voltage Controller and SmartReflex features of the
>    OMAP CPU with this regulator driver.

This just looks like it should just be another regulator to me. The
board will be responsible for setting up the client mapping (i.e.
whether it should use the "extended" or normal regulator).

Fwiw, Graeme has already done a lot of work on supporting Smartreflex
with TWL6025, it may be best to contact him to align your work.

Liam

Re: [RFC 1/4] twl-regulator: extend for SMPS regulators and external controllers

[Mark Brown]

On Fri, Jul 08, 2011 at 06:56:25PM +0300, Tero Kristo wrote:
> This commit adds two things to the TWL regulator driver code :

Why is this one commit rather than two commits implementing the two
changes?

>  * It creates a twlreg_ext_ctrl structure, which allows code outside
>    of the TWL regulator driver to implement a regulator
>    controller. Such a controller is attached using the new
>    twlreg_attach_external_controller() function of the driver. When
>    such a controller is attached to a regulator, the ->set_voltage()
>    and ->get_voltage() calls made on the regulator will be forwarded
>    to the external controller. This facility will later be used to
>    integrate the Voltage Controller and SmartReflex features of the
>    OMAP CPU with this regulator driver.

The regulator API has perfectly good support for multiple regulators in
the system already, why would this driver know anything about other
regulators in the system?  If there is something missing why are you
implementing it in a driver and not the core?

I've not read the actual patch.

[RFC 2/4] omap3beagle: Instantiate VDD1 and VDD2 regulators

[Tero Kristo]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Instantiate the VDD1 and VDD2 regulators and connect them to their
respective consumers: mpu.0 for VDD1 and l3_main.0 for VDD2.

TODO:

 * As these instantiations will be identical for all OMAP3 boards,
   find a way of sharing them throughout the different OMAP3 boards.

Signed-off-by: Thomas Petazzoni <t-petazzoni@ti.com>
---
 arch/arm/mach-omap2/board-omap3beagle.c |   35 +++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 78cf5f2..6d3a266 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -24,6 +24,7 @@
 #include <linux/input.h>
 #include <linux/gpio_keys.h>
 #include <linux/opp.h>
+#include <linux/regulator/driver.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -400,6 +401,38 @@ static struct regulator_init_data beagle_vpll2 = {
 	.consumer_supplies	= beagle_vdvi_supplies,
 };
 
+static struct regulator_consumer_supply beagle_vdd1_supply =
+	REGULATOR_SUPPLY("vcc", "mpu.0");
+
+static struct regulator_consumer_supply beagle_vdd2_supply =
+	REGULATOR_SUPPLY("vcc", "l3_main.0");
+
+static struct regulator_init_data beagle_vdd1 = {
+	.constraints = {
+		.name			= "VDD1",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL
+					| REGULATOR_MODE_STANDBY,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies	= 1,
+	.consumer_supplies	= &beagle_vdd1_supply,
+};
+
+static struct regulator_init_data beagle_vdd2 = {
+	.constraints = {
+		.name			= "VDD2",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL
+					| REGULATOR_MODE_STANDBY,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies	= 1,
+	.consumer_supplies	= &beagle_vdd2_supply,
+};
+
 static struct twl4030_usb_data beagle_usb_data = {
 	.usb_mode	= T2_USB_MODE_ULPI,
 };
@@ -423,6 +456,8 @@ static struct twl4030_platform_data beagle_twldata = {
 	.vsim		= &beagle_vsim,
 	.vdac		= &beagle_vdac,
 	.vpll2		= &beagle_vpll2,
+	.vdd1           = &beagle_vdd1,
+	.vdd2           = &beagle_vdd2,
 };
 
 static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
-- 
1.7.4.1


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 2/4] omap3beagle: Instantiate VDD1 and VDD2 regulators

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

Hi,

On Fri, Jul 08, 2011 at 06:56:26PM +0300, Tero Kristo wrote:
> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Instantiate the VDD1 and VDD2 regulators and connect them to their
> respective consumers: mpu.0 for VDD1 and l3_main.0 for VDD2.
> 
> TODO:
> 
>  * As these instantiations will be identical for all OMAP3 boards,
>    find a way of sharing them throughout the different OMAP3 boards.
> 
> Signed-off-by: Thomas Petazzoni <t-petazzoni@ti.com>
> ---
>  arch/arm/mach-omap2/board-omap3beagle.c |   35 +++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index 78cf5f2..6d3a266 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -24,6 +24,7 @@
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
>  #include <linux/opp.h>
> +#include <linux/regulator/driver.h>
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> @@ -400,6 +401,38 @@ static struct regulator_init_data beagle_vpll2 = {
>  	.consumer_supplies	= beagle_vdvi_supplies,
>  };
>  
> +static struct regulator_consumer_supply beagle_vdd1_supply =

should be array.

> +	REGULATOR_SUPPLY("vcc", "mpu.0");
> +
> +static struct regulator_consumer_supply beagle_vdd2_supply =

should be array

> +	REGULATOR_SUPPLY("vcc", "l3_main.0");
> +
> +static struct regulator_init_data beagle_vdd1 = {
> +	.constraints = {
> +		.name			= "VDD1",
> +		.min_uV			= 600000,
> +		.max_uV			= 1450000,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL
> +					| REGULATOR_MODE_STANDBY,
> +		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
> +	},
> +	.num_consumer_supplies	= 1,

ARRAY_SIZE(beagle_vdd1_supply)

> +	.consumer_supplies	= &beagle_vdd1_supply,
> +};
> +
> +static struct regulator_init_data beagle_vdd2 = {
> +	.constraints = {
> +		.name			= "VDD2",
> +		.min_uV			= 600000,
> +		.max_uV			= 1450000,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL
> +					| REGULATOR_MODE_STANDBY,
> +		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
> +	},
> +	.num_consumer_supplies	= 1,

ARRAY_SIZE(beagle_vdd1_supply)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[RFC 3/4] omap: attach external controller to VDD1/VDD2

[Tero Kristo]

Instiante a twlreg_ext_ctrl structure in the OMAP voltage code for
VDD1 and VDD2 and attach it as an external controller for these
regulators. It will allow the OMAP voltage code to take over the
default regulator driver code for ->set_voltage() and ->get_voltage().

TODO:

 * Does this really belong to the OMAP voltage code ? The PowerIC
   attached to the OMAP may not be the traditional TWL, so maybe this
   code belongs to a more board-specific location (but then we have
   the question of avoiding too much duplication of this code
   throughout the different board files)

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/voltage.c                 |   37 +++++++++++++++++++++++++
 arch/arm/mach-omap2/voltage.h                 |    4 +++
 arch/arm/mach-omap2/voltagedomains3xxx_data.c |    2 +
 3 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 9ef3789..e9b9bbb 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -59,6 +59,9 @@ static struct dentry *voltage_dir;
 /* Init function pointers */
 static int vp_forceupdate_scale_voltage(struct omap_vdd_info *vdd,
 					unsigned long target_volt);
+static int omap_twlreg_ext_ctrl_get_voltage(struct twlreg_ext_ctrl *ext_ctrl);
+static int omap_twlreg_ext_ctrl_set_voltage(struct twlreg_ext_ctrl *ext_ctrl,
+					    int min_uV, int max_uV);
 
 static u32 omap3_voltage_read_reg(u16 mod, u8 offset)
 {
@@ -1029,6 +1032,19 @@ void omap_change_voltscale_method(struct voltagedomain *voltdm,
 	}
 }
 
+static int omap_twlreg_ext_ctrl_get_voltage(struct twlreg_ext_ctrl *ext_ctrl)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *) ext_ctrl->data;
+	return omap_vp_get_curr_volt(voltdm);
+}
+
+static int omap_twlreg_ext_ctrl_set_voltage(struct twlreg_ext_ctrl *ext_ctrl,
+					    int min_uV, int max_uV)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *) ext_ctrl->data;
+	return omap_voltage_scale_vdd(voltdm, min_uV);
+}
+
 /**
  * omap_voltage_domain_lookup() - API to get the voltage domain pointer
  * @name:	Name of the voltage domain
@@ -1071,6 +1087,8 @@ struct voltagedomain *omap_voltage_domain_lookup(char *name)
 int __init omap_voltage_late_init(void)
 {
 	int i;
+	struct twlreg_ext_ctrl *ctrl;
+	struct voltagedomain *voltdm;
 
 	if (!vdd_info) {
 		pr_err("%s: Voltage driver support not added\n",
@@ -1088,6 +1106,25 @@ int __init omap_voltage_late_init(void)
 		omap_vc_init(vdd_info[i]);
 		vp_init(vdd_info[i]);
 		vdd_debugfs_init(vdd_info[i]);
+
+		voltdm = &vdd_info[i]->voltdm;
+		/* Attach twlreg external controller */
+		if (voltdm->reg_name) {
+			ctrl = kmalloc(sizeof(struct twlreg_ext_ctrl),
+				GFP_KERNEL);
+			if (!ctrl) {
+				pr_err("%s: can't alloc twlreg_ext_ctrl for"
+				       " vdd_%s\n", __func__, voltdm->name);
+				return -ENOMEM;
+			}
+			ctrl->set_voltage = omap_twlreg_ext_ctrl_set_voltage;
+			ctrl->get_voltage = omap_twlreg_ext_ctrl_get_voltage;
+			ctrl->data = voltdm;
+
+			twlreg_attach_external_controller(voltdm->reg_name,
+				ctrl);
+			voltdm->twl_ext_ctrl = ctrl;
+		}
 	}
 
 	return 0;
diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
index e9f5408..1fb1d85 100644
--- a/arch/arm/mach-omap2/voltage.h
+++ b/arch/arm/mach-omap2/voltage.h
@@ -15,6 +15,7 @@
 #define __ARCH_ARM_MACH_OMAP2_VOLTAGE_H
 
 #include <linux/err.h>
+#include <linux/regulator/twl.h>
 
 #include "vc.h"
 #include "vp.h"
@@ -52,9 +53,12 @@ struct omap_vfsm_instance_data {
  * struct voltagedomain - omap voltage domain global structure.
  * @name:	Name of the voltage domain which can be used as a unique
  *		identifier.
+ * @twl_ext_ctrl:  External controller for the voltage domain
  */
 struct voltagedomain {
 	char *name;
+	char *reg_name;
+	struct twlreg_ext_ctrl *twl_ext_ctrl;
 };
 
 /**
diff --git a/arch/arm/mach-omap2/voltagedomains3xxx_data.c b/arch/arm/mach-omap2/voltagedomains3xxx_data.c
index def230f..02a93c5 100644
--- a/arch/arm/mach-omap2/voltagedomains3xxx_data.c
+++ b/arch/arm/mach-omap2/voltagedomains3xxx_data.c
@@ -43,6 +43,7 @@ static struct omap_vdd_info omap3_vdd1_info = {
 	.vfsm = &omap3_vdd1_vfsm_data,
 	.voltdm = {
 		.name = "mpu",
+		.reg_name = "VDD1",
 	},
 };
 
@@ -58,6 +59,7 @@ static struct omap_vdd_info omap3_vdd2_info = {
 	.vfsm = &omap3_vdd2_vfsm_data,
 	.voltdm = {
 		.name = "core",
+		.reg_name = "VDD2",
 	},
 };
 
-- 
1.7.4.1


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 3/4] omap: attach external controller to VDD1/VDD2

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4113 bytes --]

Hi,

On Fri, Jul 08, 2011 at 06:56:27PM +0300, Tero Kristo wrote:
> Instiante a twlreg_ext_ctrl structure in the OMAP voltage code for
> VDD1 and VDD2 and attach it as an external controller for these
> regulators. It will allow the OMAP voltage code to take over the
> default regulator driver code for ->set_voltage() and ->get_voltage().
> 
> TODO:
> 
>  * Does this really belong to the OMAP voltage code ? The PowerIC
>    attached to the OMAP may not be the traditional TWL, so maybe this
>    code belongs to a more board-specific location (but then we have
>    the question of avoiding too much duplication of this code
>    throughout the different board files)
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  arch/arm/mach-omap2/voltage.c                 |   37 +++++++++++++++++++++++++
>  arch/arm/mach-omap2/voltage.h                 |    4 +++
>  arch/arm/mach-omap2/voltagedomains3xxx_data.c |    2 +
>  3 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 9ef3789..e9b9bbb 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -59,6 +59,9 @@ static struct dentry *voltage_dir;
>  /* Init function pointers */
>  static int vp_forceupdate_scale_voltage(struct omap_vdd_info *vdd,
>  					unsigned long target_volt);
> +static int omap_twlreg_ext_ctrl_get_voltage(struct twlreg_ext_ctrl *ext_ctrl);
> +static int omap_twlreg_ext_ctrl_set_voltage(struct twlreg_ext_ctrl *ext_ctrl,
> +					    int min_uV, int max_uV);
>  
>  static u32 omap3_voltage_read_reg(u16 mod, u8 offset)
>  {
> @@ -1029,6 +1032,19 @@ void omap_change_voltscale_method(struct voltagedomain *voltdm,
>  	}
>  }
>  
> +static int omap_twlreg_ext_ctrl_get_voltage(struct twlreg_ext_ctrl *ext_ctrl)
> +{
> +	struct voltagedomain *voltdm = (struct voltagedomain *) ext_ctrl->data;
> +	return omap_vp_get_curr_volt(voltdm);
> +}
> +
> +static int omap_twlreg_ext_ctrl_set_voltage(struct twlreg_ext_ctrl *ext_ctrl,
> +					    int min_uV, int max_uV)
> +{
> +	struct voltagedomain *voltdm = (struct voltagedomain *) ext_ctrl->data;
> +	return omap_voltage_scale_vdd(voltdm, min_uV);
> +}
> +
>  /**
>   * omap_voltage_domain_lookup() - API to get the voltage domain pointer
>   * @name:	Name of the voltage domain
> @@ -1071,6 +1087,8 @@ struct voltagedomain *omap_voltage_domain_lookup(char *name)
>  int __init omap_voltage_late_init(void)
>  {
>  	int i;
> +	struct twlreg_ext_ctrl *ctrl;
> +	struct voltagedomain *voltdm;
>  
>  	if (!vdd_info) {
>  		pr_err("%s: Voltage driver support not added\n",
> @@ -1088,6 +1106,25 @@ int __init omap_voltage_late_init(void)
>  		omap_vc_init(vdd_info[i]);
>  		vp_init(vdd_info[i]);
>  		vdd_debugfs_init(vdd_info[i]);
> +
> +		voltdm = &vdd_info[i]->voltdm;
> +		/* Attach twlreg external controller */
> +		if (voltdm->reg_name) {
> +			ctrl = kmalloc(sizeof(struct twlreg_ext_ctrl),
> +				GFP_KERNEL);
> +			if (!ctrl) {
> +				pr_err("%s: can't alloc twlreg_ext_ctrl for"
> +				       " vdd_%s\n", __func__, voltdm->name);
> +				return -ENOMEM;
> +			}
> +			ctrl->set_voltage = omap_twlreg_ext_ctrl_set_voltage;
> +			ctrl->get_voltage = omap_twlreg_ext_ctrl_get_voltage;
> +			ctrl->data = voltdm;
> +
> +			twlreg_attach_external_controller(voltdm->reg_name,
> +				ctrl);
> +			voltdm->twl_ext_ctrl = ctrl;
> +		}
>  	}
>  
>  	return 0;
> diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
> index e9f5408..1fb1d85 100644
> --- a/arch/arm/mach-omap2/voltage.h
> +++ b/arch/arm/mach-omap2/voltage.h
> @@ -15,6 +15,7 @@
>  #define __ARCH_ARM_MACH_OMAP2_VOLTAGE_H
>  
>  #include <linux/err.h>
> +#include <linux/regulator/twl.h>
>  
>  #include "vc.h"
>  #include "vp.h"
> @@ -52,9 +53,12 @@ struct omap_vfsm_instance_data {
>   * struct voltagedomain - omap voltage domain global structure.
>   * @name:	Name of the voltage domain which can be used as a unique
>   *		identifier.

missed reg_name

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[RFC 4/4] 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 |   32 +++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c      |    4 +++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 6d3a266..bc3e5ed 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -574,6 +574,38 @@ static void __init beagle_opp_init(void)
 		return;
 	}
 
+	/* Custom OPP enabled for C4 */
+	if (omap3_beagle_version == OMAP3BEAGLE_BOARD_C4) {
+		struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
+		struct omap_hwmod *dh = omap_hwmod_lookup("iva");
+		struct device *dev;
+
+		if (!mh || !dh) {
+			pr_err("%s: Aiee.. no mpu/dsp devices? %p %p\n",
+				__func__, mh, dh);
+		}
+		/* Enable MPU 720MHz opp */
+		dev = &mh->od->pdev.dev;
+		r = opp_enable(dev, 720000000);
+
+		/* Enable IVA 520MHz opp */
+		dev = &dh->od->pdev.dev;
+		r |= opp_enable(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
+			 */
+			dev = &mh->od->pdev.dev;
+			opp_disable(dev, 720000000);
+			dev = &dh->od->pdev.dev;
+			opp_disable(dev, 520000000);
+		}
+	}
+
 	/* Custom OPP enabled for all xM versions */
 	if (cpu_is_omap3630()) {
 		struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
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


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 4/4] OMAP3: beagle rev-c4: enable OPP6

[Koen Kooi]

Op 8 jul. 2011 om 16:56 heeft Tero Kristo <t-kristo@ti.com> het volgende geschreven:

> 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 |   32 +++++++++++++++++++++++++++++++
> arch/arm/mach-omap2/opp3xxx_data.c      |    4 +++
> 2 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index 6d3a266..bc3e5ed 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -574,6 +574,38 @@ static void __init beagle_opp_init(void)
>        return;
>    }
> 
> +    /* Custom OPP enabled for C4 */
> +    if (omap3_beagle_version == OMAP3BEAGLE_BOARD_C4) {

The 720 capable chips have a sku id you can read, so this is better done in the core code. Sanjeev Premi has been trying to get that merged for a while now, so please take this issue up with him.







> +        struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
> +        struct omap_hwmod *dh = omap_hwmod_lookup("iva");
> +        struct device *dev;
> +
> +        if (!mh || !dh) {
> +            pr_err("%s: Aiee.. no mpu/dsp devices? %p %p\n",
> +                __func__, mh, dh);
> +        }
> +        /* Enable MPU 720MHz opp */
> +        dev = &mh->od->pdev.dev;
> +        r = opp_enable(dev, 720000000);
> +
> +        /* Enable IVA 520MHz opp */
> +        dev = &dh->od->pdev.dev;
> +        r |= opp_enable(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
> +             */
> +            dev = &mh->od->pdev.dev;
> +            opp_disable(dev, 720000000);
> +            dev = &dh->od->pdev.dev;
> +            opp_disable(dev, 520000000);
> +        }
> +    }
> +
>    /* Custom OPP enabled for all xM versions */
>    if (cpu_is_omap3630()) {
>        struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
> 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
> 
> 
> Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
> 
> 
> --
> 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
> 

Re: [RFC 0/4] TWL external controller support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

hi,

On Fri, Jul 08, 2011 at 06:56:24PM +0300, Tero Kristo wrote:
> Following patches add an external controller support for TWL SMPS regulators.
> This is needed because OMAP has voltage processor support which provides
> interface to control a few regulators (VDD1 / VDD2 for OMAP3), and this
> is shared with smartreflex.
> 
> These patches work in a way that twl regulators now provide an external
> controller registration interface, and this is used by the OMAP voltage
> layer during init. set_voltage / get_voltage APIs now check for presence
> of external controller, and if it is there, calls are routed to external
> controller.
> 
> Baseline work for these patches was done by Thomas Petezzoni.
> 
> Any comments welcome, does something like this look like it could be pushed
> to regulator framework?

isn't this all the same as claiming the regulator but never actually
using the regulator APIs ? I mean, you could add the regulator, then on
smartreflex code, regulator_get(), but when it gets to get/set voltage,
you use the omap_*() functions instead of regulator_*().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 0/4] TWL external controller support

[Mark Brown]

On Fri, Jul 08, 2011 at 07:25:32PM +0300, Felipe Balbi wrote:

> isn't this all the same as claiming the regulator but never actually
> using the regulator APIs ? I mean, you could add the regulator, then on
> smartreflex code, regulator_get(), but when it gets to get/set voltage,
> you use the omap_*() functions instead of regulator_*().

That wasn't what I got from the patches but it also sounds like a bad
idea.  Why go around the core code?

Re: [RFC 0/4] TWL external controller support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

Hi,

On Sat, Jul 09, 2011 at 10:24:09AM +0900, Mark Brown wrote:
> On Fri, Jul 08, 2011 at 07:25:32PM +0300, Felipe Balbi wrote:
> 
> > isn't this all the same as claiming the regulator but never actually
> > using the regulator APIs ? I mean, you could add the regulator, then on
> > smartreflex code, regulator_get(), but when it gets to get/set voltage,
> > you use the omap_*() functions instead of regulator_*().
> 
> That wasn't what I got from the patches but it also sounds like a bad
> idea.  Why go around the core code?

a hack for a hack... what's the difference ? If it's only to solve a
limitation temporarily anyways... although it would be better to discuss
how to add such support to the framework already.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 0/4] TWL external controller support

[Mark Brown]

On Sat, Jul 09, 2011 at 01:40:08PM +0300, Felipe Balbi wrote:

> a hack for a hack... what's the difference ? If it's only to solve a
> limitation temporarily anyways... although it would be better to discuss
> how to add such support to the framework already.

I'm completely unable to identify an issue in the framework that this
patch addresses - the API already supports multiple devices supplying
regulators, it's extremely difficult to understand what motivates the
change.

Re: [RFC 0/4] TWL external controller support

[Tero Kristo]

On Sat, 2011-07-09 at 12:56 +0200, Mark Brown wrote:
> On Sat, Jul 09, 2011 at 01:40:08PM +0300, Felipe Balbi wrote:
> 
> > a hack for a hack... what's the difference ? If it's only to solve a
> > limitation temporarily anyways... although it would be better to discuss
> > how to add such support to the framework already.
> 
> I'm completely unable to identify an issue in the framework that this
> patch addresses - the API already supports multiple devices supplying
> regulators, it's extremely difficult to understand what motivates the
> change.

Hi,

So if I understood the comments right, what you are saying is that I
should probably implement a new regulator subset for the twl-regulator.
I.e. we have currently TWL4030_ADJUSTABLE_LDO etc., so I could add a
TWL4030_ADJUSTABLE_SMPS for example. These regulators would then use the
appropriate interface according to board specific setup. How would you
propose to use / register the alternate ops to access the omap_voltage
layer from here though for set/get voltage?

The main thing is that VDD1 and VDD2 regulators in TWL4030 can be
controlled through the typical TWL control interface (I2C), like the
rest of the TWL regulators, or it can be controlled through the voltage
processor interface which uses a dedicated I2C bus and is not accessible
to anything but the voltage processor. The used interface can be
configured from the TWL side. The voltage processor support is currently
provided by the omap platform code, and regulator code knows nothing
about this. It might also be possible to do compile time switch for the
interface here if that is acceptable, however a runtime interface for
doing this would provide more flexibility.

I'll also contact Graeme and check what he is doing on this front.

-Tero


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 0/4] TWL external controller support

[Mark Brown]

On Mon, Jul 11, 2011 at 11:23:11AM +0300, Tero Kristo wrote:
> On Sat, 2011-07-09 at 12:56 +0200, Mark Brown wrote:
> > On Sat, Jul 09, 2011 at 01:40:08PM +0300, Felipe Balbi wrote:

> > I'm completely unable to identify an issue in the framework that this
> > patch addresses - the API already supports multiple devices supplying
> > regulators, it's extremely difficult to understand what motivates the
> > change.

> So if I understood the comments right, what you are saying is that I
> should probably implement a new regulator subset for the twl-regulator.
> I.e. we have currently TWL4030_ADJUSTABLE_LDO etc., so I could add a
> TWL4030_ADJUSTABLE_SMPS for example. These regulators would then use the

No.  Why do you want these regulators to have anything to do with the
TWL4030?

> appropriate interface according to board specific setup. How would you
> propose to use / register the alternate ops to access the omap_voltage
> layer from here though for set/get voltage?

I have no visibility of the omap_voltage layer.  If it's peering into
the internals of the regulator API or regulator drivers you've got a
fairly serious abstraction problem that someone needs to fix...

> The main thing is that VDD1 and VDD2 regulators in TWL4030 can be
> controlled through the typical TWL control interface (I2C), like the
> rest of the TWL regulators, or it can be controlled through the voltage
> processor interface which uses a dedicated I2C bus and is not accessible
> to anything but the voltage processor. The used interface can be

That's not too unusual in hardware terms.

> configured from the TWL side. The voltage processor support is currently
> provided by the omap platform code, and regulator code knows nothing
> about this. It might also be possible to do compile time switch for the
> interface here if that is acceptable, however a runtime interface for
> doing this would provide more flexibility.

This isn't making much sense to me, what is the relationship between
this and the other regulators you're adding these bodge interfaces for?
Why would you want to switch between the two modes at runtime and how
would anyone take the decision to do so?

If some of the TWL4030 regulators are controlled by something other than
the CPU in your system then the TWL4030 driver shouldn't be configured
to do anything with them except possibly provide read only access.

Re: [RFC 0/4] TWL external controller support

[Tero Kristo]

On Mon, 2011-07-11 at 12:05 +0200, Mark Brown wrote:
> On Mon, Jul 11, 2011 at 11:23:11AM +0300, Tero Kristo wrote:
> > On Sat, 2011-07-09 at 12:56 +0200, Mark Brown wrote:
> > > On Sat, Jul 09, 2011 at 01:40:08PM +0300, Felipe Balbi wrote:
> 
> > > I'm completely unable to identify an issue in the framework that this
> > > patch addresses - the API already supports multiple devices supplying
> > > regulators, it's extremely difficult to understand what motivates the
> > > change.
> 
> > So if I understood the comments right, what you are saying is that I
> > should probably implement a new regulator subset for the twl-regulator.
> > I.e. we have currently TWL4030_ADJUSTABLE_LDO etc., so I could add a
> > TWL4030_ADJUSTABLE_SMPS for example. These regulators would then use the
> 
> No.  Why do you want these regulators to have anything to do with the
> TWL4030?

So, a completely new driver should be made for these? The reason I
wanted to put them within TWL4030 code is that they reside inside
TWL4030, and there is already some code for accessing these regulators
(in the standard I2C access method) from the twl-regulator.c.

> 
> > appropriate interface according to board specific setup. How would you
> > propose to use / register the alternate ops to access the omap_voltage
> > layer from here though for set/get voltage?
> 
> I have no visibility of the omap_voltage layer.  If it's peering into
> the internals of the regulator API or regulator drivers you've got a
> fairly serious abstraction problem that someone needs to fix...

At least currently it does not have any visibility to regulator
framework, and well, I am not too sure if it should.  It is providing
interfaces to control the voltage processor, for example to switch the
used nominal voltage level when cpufreq switches used OPP, the voltage
processor itself might be altering this nominal voltage level slightly
based on silicon statistics. The eventual target might be to make the
omap voltage control / processor code be a regulator driver itself.

> 
> > The main thing is that VDD1 and VDD2 regulators in TWL4030 can be
> > controlled through the typical TWL control interface (I2C), like the
> > rest of the TWL regulators, or it can be controlled through the voltage
> > processor interface which uses a dedicated I2C bus and is not accessible
> > to anything but the voltage processor. The used interface can be
> 
> That's not too unusual in hardware terms.
> 
> > configured from the TWL side. The voltage processor support is currently
> > provided by the omap platform code, and regulator code knows nothing
> > about this. It might also be possible to do compile time switch for the
> > interface here if that is acceptable, however a runtime interface for
> > doing this would provide more flexibility.
> 
> This isn't making much sense to me, what is the relationship between
> this and the other regulators you're adding these bodge interfaces for?
> Why would you want to switch between the two modes at runtime and how
> would anyone take the decision to do so?

Runtime switching would mostly be useful as a testing feature. In
typical setup the configuration is just selected during boot time, and
thats it. And normally we would just want to use the voltage processor
interfaces to control these regulators.

> 
> If some of the TWL4030 regulators are controlled by something other than
> the CPU in your system then the TWL4030 driver shouldn't be configured
> to do anything with them except possibly provide read only access.

I think this part I have not been too clear about... CPU is controlling
the voltage level for these regulators, but the used hardware interface
is different... and the CPU is giving a guideline that we should be
using the nominal voltage level based on cpufreq setup, but the actual
voltage set on the TWL chip is usually different.

-Tero



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [RFC 0/4] TWL external controller support

[Mark Brown]

On Mon, Jul 11, 2011 at 01:48:59PM +0300, Tero Kristo wrote:
> On Mon, 2011-07-11 at 12:05 +0200, Mark Brown wrote:

> > No.  Why do you want these regulators to have anything to do with the
> > TWL4030?

> So, a completely new driver should be made for these? The reason I
> wanted to put them within TWL4030 code is that they reside inside
> TWL4030, and there is already some code for accessing these regulators
> (in the standard I2C access method) from the twl-regulator.c.

Well, if they're not perceptibly part of the same chip from a control
point of view and need you to provide board specific callbacks it would
seem logical...

> > > configured from the TWL side. The voltage processor support is currently
> > > provided by the omap platform code, and regulator code knows nothing
> > > about this. It might also be possible to do compile time switch for the
> > > interface here if that is acceptable, however a runtime interface for
> > > doing this would provide more flexibility.

> > This isn't making much sense to me, what is the relationship between
> > this and the other regulators you're adding these bodge interfaces for?
> > Why would you want to switch between the two modes at runtime and how
> > would anyone take the decision to do so?

> Runtime switching would mostly be useful as a testing feature. In
> typical setup the configuration is just selected during boot time, and
> thats it. And normally we would just want to use the voltage processor
> interfaces to control these regulators.

It doesn't seem like a good idea to support that, then.  If there's no
benefit to switching dynamically we're just adding complexity to the
system which people will then spend time tweaking and making work
dealing with any robustness issues from managing the transitions.

> > If some of the TWL4030 regulators are controlled by something other than
> > the CPU in your system then the TWL4030 driver shouldn't be configured
> > to do anything with them except possibly provide read only access.

> I think this part I have not been too clear about... CPU is controlling
> the voltage level for these regulators, but the used hardware interface
> is different... and the CPU is giving a guideline that we should be
> using the nominal voltage level based on cpufreq setup, but the actual
> voltage set on the TWL chip is usually different.

Right, but from a software point of view the fact that we end up with
the same physical regulator is immaterial as you have to try *really*
hard to notice.

Re: [RFC 0/4] TWL external controller support

[Tero Kristo]

On Mon, 2011-07-11 at 14:11 +0200, Mark Brown wrote:
> On Mon, Jul 11, 2011 at 01:48:59PM +0300, Tero Kristo wrote:
> > On Mon, 2011-07-11 at 12:05 +0200, Mark Brown wrote:
> 
> > > No.  Why do you want these regulators to have anything to do with the
> > > TWL4030?
> 
> > So, a completely new driver should be made for these? The reason I
> > wanted to put them within TWL4030 code is that they reside inside
> > TWL4030, and there is already some code for accessing these regulators
> > (in the standard I2C access method) from the twl-regulator.c.
> 
> Well, if they're not perceptibly part of the same chip from a control
> point of view and need you to provide board specific callbacks it would
> seem logical...

Ok sounds fair enough, I'll rework this series to something like this
and re-post for commenting once I hear from Graeme also. I'll add a
compile time switch (or most likely use something existing) for
selecting which implementation to use for VDD1 and VDD2.

Thanks for comments.

-Tero


Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki