[PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Sundar Iyer]

From: Sundar R Iyer <sundar.iyer@stericsson.com>

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 drivers/regulator/Kconfig  |    8 +
 drivers/regulator/Makefile |    1 +
 drivers/regulator/ab8500.c |  425 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 434 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/ab8500.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7cd8a29..6c14afd 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -221,5 +221,13 @@ config REGULATOR_AD5398
 	help
 	  This driver supports AD5398 and AD5821 current regulator chips.
 	  If building into module, its name is ad5398.ko.
+
+config REGULATOR_AB8500
+	bool "ST-Ericsson AB8500 Power Regulators"
+	depends on AB8500_CORE
+	help
+	  This driver supports the regulators found on the ST-Ericsson mixed
+	  signal AB8500 PMIC
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 74a4638..fc696c5 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,5 +34,6 @@ obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6507X) += tps6507x-regulator.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
+obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
new file mode 100644
index 0000000..29188e6
--- /dev/null
+++ b/drivers/regulator/ab8500.c
@@ -0,0 +1,425 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ *
+ * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
+ *
+ * AB8500 peripheral regulators
+ *
+ * AB8500 supports the following regulators,
+ * LDOs - VAUDIO, VANAMIC2/2, VDIGMIC, VINTCORE12, VTVOUT,
+ *        VAUX1/2/3, VANA
+ *
+ * for DB8500 cut 1.0 and previous versions of the silicon, all accesses
+ * to registers are through the DB8500 SPI. In cut 1.1 onwards, these
+ * accesses are through the DB8500 PRCMU I2C
+ *
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/ab8500.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/ab8500.h>
+
+/**
+ * struct ab8500_regulator_info - ab8500 regulator information
+ * @desc: regulator description
+ * @ab8500: ab8500 parent
+ * @regulator_dev: regulator device
+ * @max_uV: maximum voltage (for variable voltage supplies)
+ * @min_uV: minimum voltage (for variable voltage supplies)
+ * @fixed_uV: typical voltage (for fixed voltage supplies)
+ * @update_reg: register to control on/off
+ * @mask: mask to enable/disable regulator
+ * @enable: bits to enable the regulator in normal(high power) mode
+ * @voltage_reg: register to control regulator voltage
+ * @voltage_mask: mask to control regulator voltage
+ * @supported_voltages: supported voltage table
+ * @voltages_len: number of supported voltages for the regulator
+ */
+struct ab8500_regulator_info {
+	struct regulator_desc	desc;
+	struct ab8500		*ab8500;
+	struct regulator_dev	*regulator;
+	int max_uV;
+	int min_uV;
+	int fixed_uV;
+	int update_reg;
+	int mask;
+	int enable;
+	int voltage_reg;
+	int voltage_mask;
+	int const *supported_voltages;
+	int voltages_len;
+};
+
+/* voltage tables for the vauxn/vintcore supplies */
+static const int ldo_vauxn_voltages[] = {
+	1100000,
+	1200000,
+	1300000,
+	1400000,
+	1500000,
+	1800000,
+	1850000,
+	1900000,
+	2500000,
+	2650000,
+	2700000,
+	2750000,
+	2800000,
+	2900000,
+	3000000,
+	3300000,
+};
+
+static const int ldo_vintcore_voltages[] = {
+	1200000,
+	1225000,
+	1250000,
+	1275000,
+	1300000,
+	1325000,
+	1350000,
+};
+
+static int ab8500_regulator_enable(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_set_bits(info->ab8500, info->update_reg,
+			info->mask, info->enable);
+	if (ret < 0)
+		dev_dbg(rdev_get_dev(rdev),
+			"couldnt set enable bits for regulator\n");
+	return ret;
+}
+
+static int ab8500_regulator_disable(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_set_bits(info->ab8500, info->update_reg,
+			info->mask, 0x0);
+	if (ret < 0)
+		dev_dbg(rdev_get_dev(rdev),
+			"couldnt set disable bits for regulator\n");
+	return ret;
+}
+
+static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_read(info->ab8500, info->update_reg);
+	if (ret < 0) {
+		dev_dbg(rdev_get_dev(rdev),
+			"couldnt read 0x%x register\n", info->update_reg);
+		return ret;
+	}
+
+	if (ret & info->mask)
+		return true;
+	else
+		return false;
+}
+
+static int ab8500_list_voltage(struct regulator_dev *rdev, unsigned selector)
+{
+	int regulator_id;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	/* return the uV for the fixed regulators */
+	if (info->fixed_uV)
+		return info->fixed_uV;
+
+	if (selector > info->voltages_len)
+		return -EINVAL;
+
+	return info->supported_voltages[selector];
+}
+
+static int ab8500_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	int regulator_id, ret, val;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_read(info->ab8500, info->voltage_reg);
+	if (ret < 0) {
+		dev_dbg(rdev_get_dev(rdev),
+			"couldnt read voltage reg for regulator\n");
+		return ret;
+	}
+
+	/* vintcore has a different layout */
+	val = ret & info->voltage_mask;
+	if (regulator_id == AB8500_LDO_INTCORE)
+		ret = info->supported_voltages[val >> 0x3];
+	else
+		ret = info->supported_voltages[val];
+
+	return ret;
+}
+
+static int ab8500_get_best_voltage_index(struct regulator_dev *rdev,
+		int min_uV, int max_uV)
+{
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+	int i;
+
+	/* check the supported voltage */
+	for (i = 0; i < info->voltages_len; i++) {
+		if ((info->supported_voltages[i] >= min_uV) &&
+		    (info->supported_voltages[i] <= max_uV))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int ab8500_regulator_set_voltage(struct regulator_dev *rdev,
+		int min_uV, int max_uV)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	/* get the appropriate voltages within the range */
+	ret = ab8500_get_best_voltage_index(rdev, min_uV, max_uV);
+	if (ret < 0) {
+		dev_dbg(rdev_get_dev(rdev),
+				"coudlnt get best voltage for regulator\n");
+		return ret;
+	}
+
+	/* set the registers for the request */
+	ret = ab8500_set_bits(info->ab8500, info->voltage_reg,
+				info->voltage_mask, ret);
+	if (ret < 0)
+		dev_dbg(rdev_get_dev(rdev),
+		"couldnt set voltage reg for regulator\n");
+
+	return ret;
+}
+
+static struct regulator_ops ab8500_regulator_ops = {
+	.enable		= ab8500_regulator_enable,
+	.disable	= ab8500_regulator_disable,
+	.is_enabled	= ab8500_regulator_is_enabled,
+	.get_voltage	= ab8500_regulator_get_voltage,
+	.set_voltage	= ab8500_regulator_set_voltage,
+	.list_voltage	= ab8500_list_voltage,
+};
+
+static int ab8500_fixed_get_voltage(struct regulator_dev *rdev)
+{
+	int regulator_id;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	return info->fixed_uV;
+}
+
+static struct regulator_ops ab8500_ldo_fixed_ops = {
+	.enable		= ab8500_regulator_enable,
+	.disable	= ab8500_regulator_disable,
+	.is_enabled	= ab8500_regulator_is_enabled,
+	.get_voltage	= ab8500_fixed_get_voltage,
+	.list_voltage	= ab8500_list_voltage,
+};
+
+#define AB8500_LDO(_id, min, max, reg, reg_mask, reg_enable,	\
+		volt_reg, volt_mask, voltages,			\
+			len_volts)				\
+{								\
+	.desc	= {						\
+		.name	= "LDO-" #_id,				\
+		.ops	= &ab8500_regulator_ops,		\
+		.type	= REGULATOR_VOLTAGE,			\
+		.id	= AB8500_LDO_##_id,			\
+		.owner	= THIS_MODULE,				\
+	},							\
+	.min_uV		= (min) * 1000,				\
+	.max_uV		= (max) * 1000,				\
+	.update_reg	= reg,					\
+	.mask		= reg_mask,				\
+	.enable		= reg_enable,				\
+	.voltage_reg	= volt_reg,				\
+	.voltage_mask	= volt_mask,				\
+	.supported_voltages = voltages,				\
+	.voltages_len	= len_volts,				\
+	.fixed_uV	= 0,					\
+}
+
+#define AB8500_FIXED_LDO(_id, fixed, reg, reg_mask,	\
+				reg_enable)		\
+{							\
+	.desc	= {					\
+		.name	= "LDO-" #_id,			\
+		.ops	= &ab8500_ldo_fixed_ops,	\
+		.type	= REGULATOR_VOLTAGE,		\
+		.id	= AB8500_LDO_##_id,		\
+		.owner	= THIS_MODULE,			\
+	},						\
+	.fixed_uV	= fixed * 1000,			\
+	.update_reg	= reg,				\
+	.mask		= reg_mask,			\
+	.enable		= reg_enable,			\
+}
+
+static struct ab8500_regulator_info ab8500_regulator_info[] = {
+	/*
+	 * Variable Voltage LDOs
+	 * name, min uV, max uV, ctrl reg, reg mask, enable mask,
+	 *	volt ctrl reg, volt ctrl mask, volt table, num supported volts
+	 */
+	AB8500_LDO(AUX1, 1100, 3300, 0x0409, 0x3, 0x1, 0x041f, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(AUX2, 1100, 3300, 0x0409, 0xc, 0x4, 0x0420, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(AUX3, 1100, 3300, 0x040a, 0x3, 0x1, 0x0421, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(INTCORE, 1100, 3300, 0x0380, 0x4, 0x4, 0x0380, 0x38,
+		ldo_vintcore_voltages, ARRAY_SIZE(ldo_vintcore_voltages)),
+
+	/*
+	 * Fixed Voltage LDOs
+	 *		 name,	o/p uV, ctrl reg, enable, disable
+	 */
+	AB8500_FIXED_LDO(TVOUT,	  2000,   0x0380,   0x2,    0x2),
+	AB8500_FIXED_LDO(AUDIO,   2000,   0x0383,   0x2,    0x2),
+	AB8500_FIXED_LDO(ANAMIC1, 2050,   0x0383,   0x4,    0x4),
+	AB8500_FIXED_LDO(ANAMIC2, 2050,   0x0383,   0x8,    0x8),
+	AB8500_FIXED_LDO(DMIC,    1800,   0x0383,   0x10,   0x10),
+	AB8500_FIXED_LDO(ANA,     1200,   0x0383,   0xc,    0x4),
+};
+
+static inline struct ab8500_regulator_info *find_regulator_info(int id)
+{
+	struct ab8500_regulator_info *info;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		info = &ab8500_regulator_info[i];
+		if (info->desc.id == id)
+			return info;
+	}
+	return NULL;
+}
+
+static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
+{
+	struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent);
+	struct ab8500_platform_data *pdata = dev_get_platdata(ab8500->dev);
+	int i, err;
+
+	if (!ab8500) {
+		dev_err(&pdev->dev, "null mfd parent\n");
+		return -EINVAL;
+	}
+
+	/* register all regulators */
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		struct ab8500_regulator_info *info = NULL;
+
+		/* assign per-regulator data */
+		info = &ab8500_regulator_info[i];
+		info->ab8500 = ab8500;
+
+		info->regulator = regulator_register(&info->desc, &pdev->dev,
+				pdata->regulator[i], info);
+		if (IS_ERR(info->regulator)) {
+			err = PTR_ERR(info->regulator);
+			dev_err(&pdev->dev, "failed to register regulator %s\n",
+					info->desc.name);
+			/* when we fail, un-register all earlier regulators */
+			i--;
+			while (i > 0) {
+				info = &ab8500_regulator_info[i];
+				regulator_unregister(info->regulator);
+				i--;
+			}
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static __devexit int ab8500_regulator_remove(struct platform_device *pdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		struct ab8500_regulator_info *info = NULL;
+		info = &ab8500_regulator_info[i];
+		regulator_unregister(info->regulator);
+	}
+
+	return 0;
+}
+
+static struct platform_driver ab8500_regulator_driver = {
+	.probe = ab8500_regulator_probe,
+	.remove = __devexit_p(ab8500_regulator_remove),
+	.driver         = {
+		.name   = "ab8500-regulator",
+		.owner  = THIS_MODULE,
+	},
+};
+
+static int __init ab8500_regulator_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&ab8500_regulator_driver);
+	if (ret != 0)
+		pr_err("Failed to register ab8500 regulator: %d\n", ret);
+
+	return ret;
+}
+subsys_initcall(ab8500_regulator_init);
+
+static void __exit ab8500_regulator_exit(void)
+{
+	platform_driver_unregister(&ab8500_regulator_driver);
+}
+module_exit(ab8500_regulator_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sundar Iyer <sundar.iyer@stericsson.com>");
+MODULE_DESCRIPTION("Regulator Driver for ST-Ericsson AB8500 Mixed-Sig PMIC");
+MODULE_ALIAS("platform:ab8500-regulator");
-- 
1.7.0

[PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar Iyer]

From: Sundar R Iyer <sundar.iyer@stericsson.com>

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
 arch/arm/mach-ux500/Makefile                  |    1 +
 arch/arm/mach-ux500/board-mop500-regulators.c |  189 +++++++++++++++++++++++++
 2 files changed, 190 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ux500/board-mop500-regulators.c

diff --git a/arch/arm/mach-ux500/Makefile b/arch/arm/mach-ux500/Makefile
index 0753a69..311f996 100644
--- a/arch/arm/mach-ux500/Makefile
+++ b/arch/arm/mach-ux500/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_LOCAL_TIMERS)	+= localtimer.o
 obj-$(CONFIG_HOTPLUG_CPU)       += hotplug.o
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq.o
 obj-$(CONFIG_AB8500_PRCMU_I2C)	+= ab8500-prcmu-i2c.o
+obj-$(CONFIG_REGULATOR_AB8500)	+= board-mop500-regulators.o
diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
new file mode 100644
index 0000000..34d16c6
--- /dev/null
+++ b/arch/arm/mach-ux500/board-mop500-regulators.c
@@ -0,0 +1,189 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ *
+ * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
+ *
+ * MOP500 board specific initialization for regulators
+ */
+#include <linux/kernel.h>
+#include <linux/regulator/machine.h>
+
+static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
+	{ .supply = "vaux1", },
+};
+
+struct regulator_init_data ab8500_vaux1_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux1",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux1_consumers),
+	.consumer_supplies = ab8500_vaux1_consumers,
+};
+
+static struct regulator_consumer_supply ab8500_vaux2_consumers[] = {
+	{ .supply = "vaux2", },
+};
+
+struct regulator_init_data ab8500_vaux2_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux2",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux2_consumers),
+	.consumer_supplies = ab8500_vaux2_consumers,
+};
+
+static struct regulator_consumer_supply ab8500_vaux3_consumers[] = {
+	{ .supply = "vaux3", },
+};
+
+struct regulator_init_data ab8500_vaux3_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux3",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux3_consumers),
+	.consumer_supplies = ab8500_vaux3_consumers,
+};
+
+/* supply for tvout, gpadc, TVOUT LDO */
+static struct regulator_consumer_supply ab8500_vtvout_consumers[] = {
+	{ .supply = "vtvout", },
+};
+
+struct regulator_init_data ab8500_vtvout_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vtvout",
+		.min_uV = 1900000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vtvout_consumers),
+	.consumer_supplies = ab8500_vtvout_consumers,
+};
+
+/* supply for ab8500-vaudio, VAUDIO LDO */
+static struct regulator_consumer_supply ab8500_vaudio_consumers[] = {
+	{ .supply = "vaudio", },
+};
+
+struct regulator_init_data ab8500_vaudio_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaudio",
+		.min_uV = 1925000,
+		.max_uV = 2075000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaudio_consumers),
+	.consumer_supplies = ab8500_vaudio_consumers,
+};
+
+/* supply for v-anamic1 VAMic1-LDO */
+static struct regulator_consumer_supply ab8500_vamic1_consumers[] = {
+	{ .supply = "vana-mic1", },
+};
+
+struct regulator_init_data ab8500_vamic1_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vamic1",
+		.min_uV = 2000000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic1_consumers),
+	.consumer_supplies = ab8500_vamic1_consumers,
+};
+
+/* supply for v-amic2, VAMIC2 LDO*/
+static struct regulator_consumer_supply ab8500_vamic2_consumers[] = {
+	{ .supply = "vana-mic2", },
+};
+
+struct regulator_init_data ab8500_vamic2_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vamic2",
+		.min_uV = 2000000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic2_consumers),
+	.consumer_supplies = ab8500_vamic2_consumers,
+};
+
+/* supply for v-dmic, VDMIC LDO */
+static struct regulator_consumer_supply ab8500_vdmic_consumers[] = {
+	{ .supply = "vdig-mic", },
+};
+
+struct regulator_init_data ab8500_vdmic_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vdmic",
+		.min_uV = 1700000,
+		.max_uV = 1950000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vdmic_consumers),
+	.consumer_supplies = ab8500_vdmic_consumers,
+};
+
+/* supply for v-intcore12, VINTCORE12 LDO */
+static struct regulator_consumer_supply ab8500_vintcore_consumers[] = {
+	{ .supply = "vintcore", },
+};
+
+struct regulator_init_data ab8500_vintcore_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vintcore",
+		.min_uV = 1200000,
+		.max_uV = 1350000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vintcore_consumers),
+	.consumer_supplies = ab8500_vintcore_consumers,
+};
+
+/* supply for U8500 CSI/DSI, VANA LDO */
+static struct regulator_consumer_supply ab8500_vana_consumers[] = {
+	{ .supply = "vana", },
+};
+
+struct regulator_init_data ab8500_vana_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vana",
+		.min_uV = 0,
+		.max_uV = 1200000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vana_consumers),
+	.consumer_supplies = ab8500_vana_consumers,
+};
+
-- 
1.7.0

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Tue, Jul 13, 2010 at 07:39:33PM +0530, Sundar Iyer wrote:

> +static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
> +	{ .supply = "vaux1", },
> +};

It is extremely disappointing to see you reposting this without engaging
with my previous review at all.

RE: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

Mark,

>It is extremely disappointing to see you reposting this without engaging
>with my previous review at all.
Sincere apologies. As I said, I tried to repost after fixing your comments; but my
mistake that I didn't engage prior to the posting.

>> +static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
>> +	{ .dev = NULL, .supply = "vaux1", },
>> +};
>
>All these supplies with NULL devices are bogus, supplies are in terms of
>the device being supplied not the labels on the board.  If you've got a
>supply with no device and the name of the supply on either the regulator
>or the board you're most likely doing it wrong.  The only exception is
>for supplies used in cpufreq since we don't have a struct device we can
>use there.

I had these supplies as NULL, so that later on, when we add devices for our platform, 
each developer can edit this file to hook up his own device. The reason I wanted this
file in the patch set was to include the machine constraints for the regulators on AB8500.

As admitted earlier, I will wait for your valuable comments before posting the next patch set.
(Also, I will try to make sure(still learning) to post the next patches into the same thread.)

Regards,
Sundar

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Tue, Jul 13, 2010 at 04:41:34PM +0200, Sundar R IYER wrote:
> >> +static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
> >> +	{ .dev = NULL, .supply = "vaux1", },
> >> +};

> >All these supplies with NULL devices are bogus, supplies are in terms of
> >the device being supplied not the labels on the board.  If you've got a
> >supply with no device and the name of the supply on either the regulator
> >or the board you're most likely doing it wrong.  The only exception is
> >for supplies used in cpufreq since we don't have a struct device we can
> >use there.

> I had these supplies as NULL, so that later on, when we add devices for our platform, 
> each developer can edit this file to hook up his own device. The reason I wanted this
> file in the patch set was to include the machine constraints for the regulators on AB8500.

I'm pretty sure that people will be able to figure out how to add a
consumer list by themselves when they need it, and the lists you're
currently including are actively harmful in that they are encouraging
people to hard code board specific supply names or start passing supply
names around as platform data.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

> I'm pretty sure that people will be able to figure out how to add a
> consumer list by themselves when they need it, and the lists you're
> currently including are actively harmful in that they are encouraging
> people to hard code board specific supply names or start passing supply
> names around as platform data.
OK. so I include only regulator_init_data and keep the
regulator_consumer_supply empty. Is this the right thing forward?

Regards,
Sundar

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Tue, Jul 13, 2010 at 08:38:15PM +0530, Sundar R IYER wrote:

> OK. so I include only regulator_init_data and keep the
> regulator_consumer_supply empty. Is this the right thing forward?

Yes, if you don't have any actual devices to supply.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

Mark,

> Yes, if you don't have any actual devices to supply.
Please find the updated patch below.

>From 824b75ee4a38014fad6e17dd6d69bd3913053b45 Mon Sep 17 00:00:00 2001
From: Sundar R Iyer <sundar.iyer@stericsson.com>
Date: Sun, 6 Jun 2010 19:16:03 +0530
Subject: [PATCH v3 2/2] ux500: add ab8500-regulators machine specific data

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
CHANGELOG

v2 -> v3 
 - Empty the regulator_consumer_supply since there is no actual
   device to supply

v1 -> v2
 - Removed NULL device reference

 arch/arm/mach-ux500/Makefile                  |    1 +
 arch/arm/mach-ux500/board-mop500-regulators.c |  179 +++++++++++++++++++++++++
 2 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ux500/board-mop500-regulators.c

diff --git a/arch/arm/mach-ux500/Makefile b/arch/arm/mach-ux500/Makefile
index 0753a69..311f996 100644
--- a/arch/arm/mach-ux500/Makefile
+++ b/arch/arm/mach-ux500/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_LOCAL_TIMERS)	+= localtimer.o
 obj-$(CONFIG_HOTPLUG_CPU)       += hotplug.o
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq.o
 obj-$(CONFIG_AB8500_PRCMU_I2C)	+= ab8500-prcmu-i2c.o
+obj-$(CONFIG_REGULATOR_AB8500)	+= board-mop500-regulators.o
diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
new file mode 100644
index 0000000..08bc966
--- /dev/null
+++ b/arch/arm/mach-ux500/board-mop500-regulators.c
@@ -0,0 +1,179 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ *
+ * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
+ *
+ * MOP500 board specific initialization for regulators
+ */
+#include <linux/kernel.h>
+#include <linux/regulator/machine.h>
+
+static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vaux1_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux1",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux1_consumers),
+	.consumer_supplies = ab8500_vaux1_consumers,
+};
+
+static struct regulator_consumer_supply ab8500_vaux2_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vaux2_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux2",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux2_consumers),
+	.consumer_supplies = ab8500_vaux2_consumers,
+};
+
+static struct regulator_consumer_supply ab8500_vaux3_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vaux3_regulator = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaux3",
+		.min_uV = 1100000,
+		.max_uV = 3300000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaux3_consumers),
+	.consumer_supplies = ab8500_vaux3_consumers,
+};
+
+/* supply for tvout, gpadc, TVOUT LDO */
+static struct regulator_consumer_supply ab8500_vtvout_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vtvout_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vtvout",
+		.min_uV = 1900000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vtvout_consumers),
+	.consumer_supplies = ab8500_vtvout_consumers,
+};
+
+/* supply for ab8500-vaudio, VAUDIO LDO */
+static struct regulator_consumer_supply ab8500_vaudio_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vaudio_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vaudio",
+		.min_uV = 1925000,
+		.max_uV = 2075000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vaudio_consumers),
+	.consumer_supplies = ab8500_vaudio_consumers,
+};
+
+/* supply for v-anamic1 VAMic1-LDO */
+static struct regulator_consumer_supply ab8500_vamic1_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vamic1_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vamic1",
+		.min_uV = 2000000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic1_consumers),
+	.consumer_supplies = ab8500_vamic1_consumers,
+};
+
+/* supply for v-amic2, VAMIC2 LDO*/
+static struct regulator_consumer_supply ab8500_vamic2_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vamic2_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vamic2",
+		.min_uV = 2000000,
+		.max_uV = 2100000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic2_consumers),
+	.consumer_supplies = ab8500_vamic2_consumers,
+};
+
+/* supply for v-dmic, VDMIC LDO */
+static struct regulator_consumer_supply ab8500_vdmic_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vdmic_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vdmic",
+		.min_uV = 1700000,
+		.max_uV = 1950000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vdmic_consumers),
+	.consumer_supplies = ab8500_vdmic_consumers,
+};
+
+/* supply for v-intcore12, VINTCORE12 LDO */
+static struct regulator_consumer_supply ab8500_vintcore_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vintcore_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vintcore",
+		.min_uV = 1200000,
+		.max_uV = 1350000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vintcore_consumers),
+	.consumer_supplies = ab8500_vintcore_consumers,
+};
+
+/* supply for U8500 CSI/DSI, VANA LDO */
+static struct regulator_consumer_supply ab8500_vana_consumers[] = {
+};
+
+struct regulator_init_data ab8500_vana_init = {
+	.supply_regulator_dev = NULL,
+	.constraints = {
+		.name = "ab8500-vana",
+		.min_uV = 0,
+		.max_uV = 1200000,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|
+					REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(ab8500_vana_consumers),
+	.consumer_supplies = ab8500_vana_consumers,
+};
+
-- 
1.7.0

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Tue, Jul 13, 2010 at 09:43:44PM +0530, Sundar R IYER wrote:

> Subject: [PATCH v3 2/2] ux500: add ab8500-regulators machine specific data

> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
> Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>

> +static struct regulator_consumer_supply ab8500_vaux1_consumers[] = {
> +};

You may as well just remove these if they're not used but still...

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

One other thing...

> +struct regulator_init_data ab8500_vaudio_init = {
> +	.supply_regulator_dev = NULL,
> +	.constraints = {
> +		.name = "ab8500-vaudio",
> +		.min_uV = 1925000,
> +		.max_uV = 2075000,

Are you *sure* that all these constraints are accurate for the board?
It seems every single voltage is variable even though there are no
consumers set up, and they can all be disabled too.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

Hello Mark,

> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Thanks

> > +struct regulator_init_data ab8500_vaudio_init = {
> > +	.supply_regulator_dev = NULL,
> > +	.constraints = {
> > +		.name = "ab8500-vaudio",
> > +		.min_uV = 1925000,
> > +		.max_uV = 2075000,
> 
> Are you *sure* that all these constraints are accurate for the board?
As far as the min/max values of the voltage go, yes they are as per the
data sheet.

> It seems every single voltage is variable even though there are no
> consumers set up, and they can all be disabled too.
Yes. all these regulators can be disabled/enabled. does this answer your
qeury? ( or i didnt understand it properly??)

Thanks,
Sundar

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Wed, Jul 14, 2010 at 08:20:54PM +0530, Sundar R IYER wrote:

> > > +		.name = "ab8500-vaudio",
> > > +		.min_uV = 1925000,
> > > +		.max_uV = 2075000,

> > Are you *sure* that all these constraints are accurate for the board?

> As far as the min/max values of the voltage go, yes they are as per the
> data sheet.

Which datasheet, and will the system design actually be varying them at
runtime - if it will how will it do so?  This is the settings for the
particular system and generally a lot of these rails will get fixed at
design time for various reasons (for example, the analogue supplies will
usually depend on the analogue system design).

> > It seems every single voltage is variable even though there are no
> > consumers set up, and they can all be disabled too.

> Yes. all these regulators can be disabled/enabled. does this answer your
> qeury? ( or i didnt understand it properly??)

Again, is it really the case that this will happen in this system?
Nothing is currently able to actually do that, and unless every consumer
using a given supply is hooked into the regulator API things will go
wrong when some of them start doing so.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

> Which datasheet, and will the system design actually be varying them at
> runtime - if it will how will it do so?  This is the settings for the
> particular system and generally a lot of these rails will get fixed at
> design time for various reasons (for example, the analogue supplies will
> usually depend on the analogue system design).
I am referring to the AB8500 device data sheet; not sure if its
available open.  I have taken the minimal/maximum figures as what is
mentioned for each supplies.

> Again, is it really the case that this will happen in this system?
Yes, if you are referring to regulator enable/disable.

> Nothing is currently able to actually do that, and unless every consumer
> using a given supply is hooked into the regulator API things will go
> wrong when some of them start doing so.
As i said earlier, my intention is to hard code the machine constraints.
The actual control in terms of enable/disable, controlling supply
voltages will happen, as you say when consumers are hooked up.

Thanks,
Sundar

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Wed, Jul 14, 2010 at 09:06:44PM +0530, Sundar R IYER wrote:

> > Which datasheet, and will the system design actually be varying them at
> > runtime - if it will how will it do so?  This is the settings for the

> I am referring to the AB8500 device data sheet; not sure if its
> available open.  I have taken the minimal/maximum figures as what is
> mentioned for each supplies.

OK, you're missing the point here.  The system constraints say what's
going to be used on this actual system not what an individual component
is capable of supporting.  Regulators are almost always vastly more
flexible than any system can use and so the constraints are used to tell
the regulator core what configurations can be used on a given system.
You need to check what makes sense on the system for the things that are
connected.

> > Again, is it really the case that this will happen in this system?

> Yes, if you are referring to regulator enable/disable.

For *all* supplies?

> > Nothing is currently able to actually do that, and unless every consumer
> > using a given supply is hooked into the regulator API things will go
> > wrong when some of them start doing so.

> As i said earlier, my intention is to hard code the machine constraints.
> The actual control in terms of enable/disable, controlling supply
> voltages will happen, as you say when consumers are hooked up.

Again, you need to think about what's actually hooked up.  Permission to
do any of this stuff depends heavily on the set of consumers that are
actually hooked up - think about the example I mentioned above where
some of the consumers on a shared supply are hooked up and doing enables
and disables, for example.  What happens when they cause the supply to
be disabled but another consumer is running?

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

> OK, you're missing the point here.  The system constraints say what's
> going to be used on this actual system not what an individual component
> is capable of supporting.  Regulators are almost always vastly more
> flexible than any system can use and so the constraints are used to tell
> the regulator core what configurations can be used on a given system.
> You need to check what makes sense on the system for the things that are
> connected.
Ok.

> For *all* supplies?
Yes. whatever supplies I have listed here all can be enabled/disabled by
their consumers. Sorry to ask, but please help me to understand the
emphasis of the question. There are other supplies, which are controlled
outside the kernel and so I haven't exposed them here.
 
> Again, you need to think about what's actually hooked up.  Permission to
> do any of this stuff depends heavily on the set of consumers that are
> actually hooked up - think about the example I mentioned above where
Agreed.

> some of the consumers on a shared supply are hooked up and doing enables
> and disables, for example.  What happens when they cause the supply to
> be disabled but another consumer is running?
Again, sorry to ask(this is confusing :() - but isn't this managed by
the core? It is the core's responsibility to effectively disable a
supply when none of the consumers are using it; and to block a disable
even when a single consumer is still using it.

For eg, all the audio digital and analog supplies listed here can be
disabled/enabled by the consumer. Same goes for the VAUXn peripheral
supplies, which have shared consumers running at the same voltages.

Regards,
Sundar

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Wed, Jul 14, 2010 at 09:39:42PM +0530, Sundar R IYER wrote:

> > For *all* supplies?

> Yes. whatever supplies I have listed here all can be enabled/disabled by
> their consumers. Sorry to ask, but please help me to understand the
> emphasis of the question. There are other supplies, which are controlled
> outside the kernel and so I haven't exposed them here.

Are you positive that in your system it is sensible for consumers to
enable and disable all the supplies?  Usually there are restrictions on
what can sensibly be done on a given system.  For example, disabling the
CPU core or RAM supplies from software would normally not work terribly
well.

> > some of the consumers on a shared supply are hooked up and doing enables
> > and disables, for example.  What happens when they cause the supply to
> > be disabled but another consumer is running?

> Again, sorry to ask(this is confusing :() - but isn't this managed by
> the core? It is the core's responsibility to effectively disable a
> supply when none of the consumers are using it; and to block a disable
> even when a single consumer is still using it.

Right, but think about the case I'm talking about: if you've only hooked
up some but not all of the consumers then the core has no idea about the
consumers you didn't hook up.  You can only do power control when *all*
the consumers needed are configured.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

> Are you positive that in your system it is sensible for consumers to
> enable and disable all the supplies?  Usually there are restrictions on
> what can sensibly be done on a given system.  For example, disabling the
> CPU core or RAM supplies from software would normally not work terribly
> well.
As I said earlier, there are other supplies which I havent exposed here,
simply because,
 1. they are controlled out of the kernel, which makes it meaningless
    to include them for kernel modules
 2. Even if those were included, the risk of mis-controlling them due
    to bad SW is very high as you say and hence safely out of SW control.

I assure you that such supplies are *not* included in this list.

> CPU core or RAM supplies from software would normally not work terribly
Also, usually the deepest(lowest) power state for the CPU core is
~0V(atleast on our platform); which can be possible only by switching
off the supplies to the core; thus effectively resulting in being
controlled by SW. Further, I dont see the point of running full supplies
to the RAM in a system idle state, when it is okay for the RAM to be
powered @ a half rating OIW accountable to the idle state latencies.

> Right, but think about the case I'm talking about: if you've only hooked
> up some but not all of the consumers then the core has no idea about the
> consumers you didn't hook up.  You can only do power control when *all*
> the consumers needed are configured.
I see your point. But from an other perspective - it is *not* neccessary
to have power control only when *all* consumers are in. For eg: we have
2 peripherals sharing one of the VAUX supplies. At this moment, both the
peripherals drivers are integrated with the regulator APIs; which means
the core handles most of the work regarding control. If, one of the
peripherals isnt included in the final configuration, still, IMO, it
*does* make sense that the other active peripheral optimally manage the
supply control; which is gauranteed by the core. IOW & IMO, a consumer
that hasnt hooked up to the regulator and thus is *aware* that it isnt
sourced can be safely assumed to be non-existent. 

Regards,
Sundar 

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Wed, Jul 14, 2010 at 10:17:09PM +0530, Sundar R IYER wrote:
> > Are you positive that in your system it is sensible for consumers to
> > enable and disable all the supplies?  Usually there are restrictions on
> > what can sensibly be done on a given system.  For example, disabling the
> > CPU core or RAM supplies from software would normally not work terribly
> > well.
> As I said earlier, there are other supplies which I havent exposed here,
> simply because,

>  1. they are controlled out of the kernel, which makes it meaningless
>     to include them for kernel modules

There's userspace-consumer.c for exposing the control for userspace.

>  2. Even if those were included, the risk of mis-controlling them due
>     to bad SW is very high as you say and hence safely out of SW control.

> I assure you that such supplies are *not* included in this list.

OK, but your current set of supplies is *very* suspect since you're
offering all this control to lists of consumers that don't exist, and
you've got every single supply on the board varying at runtime.  This is
unusual and normally means that someone's done what you were describing
earlier and just put in the capabilities of the regulator rather than a
set of constraints for the particular board.

> > CPU core or RAM supplies from software would normally not work terribly

> Also, usually the deepest(lowest) power state for the CPU core is
> ~0V(atleast on our platform); which can be possible only by switching
> off the supplies to the core; thus effectively resulting in being
> controlled by SW. Further, I dont see the point of running full supplies
> to the RAM in a system idle state, when it is okay for the RAM to be
> powered @ a half rating OIW accountable to the idle state latencies.

This is normal, but for fairly obvious reasons the very lowest power
states are generally handled outside of the regulator API at a hardware
level via hardware signals to the regulator.  It's not normally part of
the runtime constraints for use while the CPU is live.

> > Right, but think about the case I'm talking about: if you've only hooked
> > up some but not all of the consumers then the core has no idea about the
> > consumers you didn't hook up.  You can only do power control when *all*
> > the consumers needed are configured.

> I see your point. But from an other perspective - it is *not* neccessary
> to have power control only when *all* consumers are in. For eg: we have
> 2 peripherals sharing one of the VAUX supplies. At this moment, both the
> peripherals drivers are integrated with the regulator APIs; which means
> the core handles most of the work regarding control. If, one of the
> peripherals isnt included in the final configuration, still, IMO, it
> *does* make sense that the other active peripheral optimally manage the
> supply control; which is gauranteed by the core. IOW & IMO, a consumer
> that hasnt hooked up to the regulator and thus is *aware* that it isnt
> sourced can be safely assumed to be non-existent. 

I'm not sure how you expect this to actually work in practice?  It's
going to be pretty hard for a driver to do anything constructive if the
power to the hardware gets cut, for example.  Unless you can guarantee
that there will never be any use of the hardware without a driver with
regulator support one driver's idea of optimal may not join up with what
the other consumers need at all.

If you really can safely turn off all these supplies then presumably for
the time being it'd be best to use regulator_has_full_constraints() so
they can all be powered off at runtime now.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Sundar R IYER]

> There's userspace-consumer.c for exposing the control for userspace.
No. I wasnt referring to a user space control of critical supplies.

> OK, but your current set of supplies is *very* suspect since you're
> offering all this control to lists of consumers that don't exist, and
As said, dont exist for *now*.

> you've got every single supply on the board varying at runtime.  This is
> unusual and normally means that someone's done what you were describing
> earlier and just put in the capabilities of the regulator rather than a
> set of constraints for the particular board.
The AB8500 is the companion chip for the DB8500. For our reference HW,
these set of regulator constraints map to the constraints for the
particular board.  But, someone deciding to use the AB8500 as standalone
can have his set of regulators integrated (leaving out much more than
what I did), set of constraints as deemed to be fit!

Probably, I should remove the REGULATOR_CHANGE_VOLTAGE flag for the fixed
supplies (as in the driver) to clear up any confusing link. Should I?

> This is normal, but for fairly obvious reasons the very lowest power
> states are generally handled outside of the regulator API at a hardware
> level via hardware signals to the regulator.  It's not normally part of
> the runtime constraints for use while the CPU is live.
Yes. But my point was that even at a lower level than kernel (BIOS/firmware?)
the switching would happen via SW. Please correct me if I am wrong!

> I'm not sure how you expect this to actually work in practice?  It's
> going to be pretty hard for a driver to do anything constructive if the
> power to the hardware gets cut, for example.  Unless you can guarantee
> that there will never be any use of the hardware without a driver with
> regulator support one driver's idea of optimal may not join up with what
> the other consumers need at all.
Very true. But, I think this will *enforce* drivers using/sharing
regulators to adhere to the framework to avoid surprises and sole-owner
misuses, which is good, now that the AB8500 regulators *are* supported.

> If you really can safely turn off all these supplies then presumably for
> the time being it'd be best to use regulator_has_full_constraints() so
> they can all be powered off at runtime now.
OOoops! I did have the patch for the platform data where I used this;
but dropped the patch for a later push. But, yes I am aware of this.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Wed, Jul 14, 2010 at 11:06:51PM +0530, Sundar R IYER wrote:

> > OK, but your current set of supplies is *very* suspect since you're
> > offering all this control to lists of consumers that don't exist, and

> As said, dont exist for *now*.

This comes back to my point about the control only making sense when the
consumers are present - if you've got missing or partial consumer setup
done then control is questionable.

> Probably, I should remove the REGULATOR_CHANGE_VOLTAGE flag for the fixed
> supplies (as in the driver) to clear up any confusing link. Should I?

Yes.

> > This is normal, but for fairly obvious reasons the very lowest power
> > states are generally handled outside of the regulator API at a hardware
> > level via hardware signals to the regulator.  It's not normally part of
> > the runtime constraints for use while the CPU is live.

> Yes. But my point was that even at a lower level than kernel (BIOS/firmware?)
> the switching would happen via SW. Please correct me if I am wrong!

Well, ultimately it's always triggered by software but the actual signal
to the regulator is often a logic level output by the SoC as the
processor enters a low power state rather than an I2C/SPI write.

> > I'm not sure how you expect this to actually work in practice?  It's
> > going to be pretty hard for a driver to do anything constructive if the
> > power to the hardware gets cut, for example.  Unless you can guarantee
> > that there will never be any use of the hardware without a driver with
> > regulator support one driver's idea of optimal may not join up with what
> > the other consumers need at all.

> Very true. But, I think this will *enforce* drivers using/sharing
> regulators to adhere to the framework to avoid surprises and sole-owner
> misuses, which is good, now that the AB8500 regulators *are* supported.

If you think your users will be sympathetic to this approach then I
guess...  obviously, it does have the potential to go rather badly wrong
especially if there are some drivers without regulator support out
there.

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Linus Walleij]

2010/7/14 Mark Brown <broonie@opensource.wolfsonmicro.com>:

>> > This is normal, but for fairly obvious reasons the very lowest power
>> > states are generally handled outside of the regulator API at a hardware
>> > level via hardware signals to the regulator.  It's not normally part of
>> > the runtime constraints for use while the CPU is live.
>
>> Yes. But my point was that even at a lower level than kernel (BIOS/firmware?)
>> the switching would happen via SW. Please correct me if I am wrong!
>
> Well, ultimately it's always triggered by software but the actual signal
> to the regulator is often a logic level output by the SoC as the
> processor enters a low power state rather than an I2C/SPI write.

I can answer this: on the U300 we had such autonomous signals that
would augment the power state of the regulators by special sleep
signals.

For U8500 there is a dedicated autonomous system in the silicon, called
PRCMU (Power Reset Clock Management Unit) that will actually do
this using I2C because it has its own CPU and can transmit I2C
messages even when the ARM CPU cores are turned off. So this is
indeed a first-timer and not strange that it looks unfamiliar ...

Yours,
Linus Walleij

Re: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data

[Mark Brown]

On Thu, Jul 15, 2010 at 12:51:50AM +0200, Linus Walleij wrote:

> For U8500 there is a dedicated autonomous system in the silicon, called
> PRCMU (Power Reset Clock Management Unit) that will actually do
> this using I2C because it has its own CPU and can transmit I2C
> messages even when the ARM CPU cores are turned off. So this is
> indeed a first-timer and not strange that it looks unfamiliar ...

This sort of design is becoming more and more common but it's still the
same thing from the point of view of the kernel - something else goes
off and does magic without the kernel explicitly doing anything.

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Tue, Jul 13, 2010 at 07:39:32PM +0530, Sundar Iyer wrote:

> + * @mask: mask to enable/disable regulator
> + * @enable: bits to enable the regulator in normal(high power) mode

Have you addressed my comments here?

> +	ret = ab8500_get_best_voltage_index(rdev, min_uV, max_uV);
> +	if (ret < 0) {
> +		dev_dbg(rdev_get_dev(rdev),
> +				"coudlnt get best voltage for regulator\n");

Typo here.  Also, shouldn't your error messages be errors rather than
debug output?

RE: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Sundar R IYER]

Hello Mark,

>> + * @mask: mask to enable/disable regulator
>> + * @enable: bits to enable the regulator in normal(high power) mode
>Have you addressed my comments here?
Sorry that I couldn't inline my replies into the patch itself :(. I changed the comments for the variable
from the previous patch set. But I agree I messed up!

Yes. The mask here is different for the fact that we have two enable bits; enable LP(low power)
mode and enable HP(high power). We enable only the HP modes and hence the different mask
for enable/disable as well as for the enable.

>> +	ret = ab8500_get_best_voltage_index(rdev, min_uV, max_uV);
>> +	if (ret < 0) {
>> +		dev_dbg(rdev_get_dev(rdev),
>> +				"coudlnt get best voltage for regulator\n");
>
>Typo here.  Also, shouldn't your error messages be errors rather than
>debug output?
Yes. I will correct this.

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Tue, Jul 13, 2010 at 04:34:56PM +0200, Sundar R IYER wrote:

> >> + * @mask: mask to enable/disable regulator
> >> + * @enable: bits to enable the regulator in normal(high power) mode
> >Have you addressed my comments here?
> Sorry that I couldn't inline my replies into the patch itself :(. I changed the comments for the variable
> from the previous patch set. But I agree I messed up!

> Yes. The mask here is different for the fact that we have two enable bits; enable LP(low power)
> mode and enable HP(high power). We enable only the HP modes and hence the different mask
> for enable/disable as well as for the enable.

OK.

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Tue, Jul 13, 2010 at 04:34:56PM +0200, Sundar R IYER wrote:

> Sorry that I couldn't inline my replies into the patch itself :(. I changed the comments for the variable

Oh, and on this point you should either address things in reply to the
review comments or in the non-changelog part of the patch (after the
---).

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Sundar R IYER]

> Oh, and on this point you should either address things in reply to the
> review comments or in the non-changelog part of the patch (after the
> ---).
Thanx again for the tips!. I have jumped to mutt and I will take care
to ensure this.

To confirm, I replace all the error output mistaken as debug output and
repost. Any further comments which I have left out??(excuse me if I did
so)

Reg,
Sundar

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Tue, Jul 13, 2010 at 08:41:16PM +0530, Sundar R IYER wrote:

> To confirm, I replace all the error output mistaken as debug output and
> repost. Any further comments which I have left out??(excuse me if I did
> so)

I think that's everything.

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Sundar R IYER]

Hi Mark,
> I think that's everything.
Please find the updated patch set as below.

>From f4bf7eec4d210db5075c0bce4521d9be6bc76c8c Mon Sep 17 00:00:00 2001
From: Sundar R Iyer <sundar.iyer@stericsson.com>
Date: Sun, 6 Jun 2010 19:12:12 +0530
Subject: [PATCH v3 1/2] regulator: add support for regulators on the ab8500 MFD

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
---
CHANGELOG

v2 -> v3
 - replaced dev_dbg calls with dev_err.

v1 -> v2
 - Fixed comments from Mark

 drivers/regulator/Kconfig  |    8 +
 drivers/regulator/Makefile |    1 +
 drivers/regulator/ab8500.c |  427 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 436 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/ab8500.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7cd8a29..6c14afd 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -221,5 +221,13 @@ config REGULATOR_AD5398
 	help
 	  This driver supports AD5398 and AD5821 current regulator chips.
 	  If building into module, its name is ad5398.ko.
+
+config REGULATOR_AB8500
+	bool "ST-Ericsson AB8500 Power Regulators"
+	depends on AB8500_CORE
+	help
+	  This driver supports the regulators found on the ST-Ericsson mixed
+	  signal AB8500 PMIC
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 74a4638..fc696c5 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,5 +34,6 @@ obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6507X) += tps6507x-regulator.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
+obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
new file mode 100644
index 0000000..dc3f1a4
--- /dev/null
+++ b/drivers/regulator/ab8500.c
@@ -0,0 +1,427 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ *
+ * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
+ *
+ * AB8500 peripheral regulators
+ *
+ * AB8500 supports the following regulators,
+ * LDOs - VAUDIO, VANAMIC2/2, VDIGMIC, VINTCORE12, VTVOUT,
+ *        VAUX1/2/3, VANA
+ *
+ * for DB8500 cut 1.0 and previous versions of the silicon, all accesses
+ * to registers are through the DB8500 SPI. In cut 1.1 onwards, these
+ * accesses are through the DB8500 PRCMU I2C
+ *
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/ab8500.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/ab8500.h>
+
+/**
+ * struct ab8500_regulator_info - ab8500 regulator information
+ * @desc: regulator description
+ * @ab8500: ab8500 parent
+ * @regulator_dev: regulator device
+ * @max_uV: maximum voltage (for variable voltage supplies)
+ * @min_uV: minimum voltage (for variable voltage supplies)
+ * @fixed_uV: typical voltage (for fixed voltage supplies)
+ * @update_reg: register to control on/off
+ * @mask: mask to enable/disable regulator
+ * @enable: bits to enable the regulator in normal(high power) mode
+ * @voltage_reg: register to control regulator voltage
+ * @voltage_mask: mask to control regulator voltage
+ * @supported_voltages: supported voltage table
+ * @voltages_len: number of supported voltages for the regulator
+ */
+struct ab8500_regulator_info {
+	struct device		*dev;
+	struct regulator_desc	desc;
+	struct ab8500		*ab8500;
+	struct regulator_dev	*regulator;
+	int max_uV;
+	int min_uV;
+	int fixed_uV;
+	int update_reg;
+	int mask;
+	int enable;
+	int voltage_reg;
+	int voltage_mask;
+	int const *supported_voltages;
+	int voltages_len;
+};
+
+/* voltage tables for the vauxn/vintcore supplies */
+static const int ldo_vauxn_voltages[] = {
+	1100000,
+	1200000,
+	1300000,
+	1400000,
+	1500000,
+	1800000,
+	1850000,
+	1900000,
+	2500000,
+	2650000,
+	2700000,
+	2750000,
+	2800000,
+	2900000,
+	3000000,
+	3300000,
+};
+
+static const int ldo_vintcore_voltages[] = {
+	1200000,
+	1225000,
+	1250000,
+	1275000,
+	1300000,
+	1325000,
+	1350000,
+};
+
+static int ab8500_regulator_enable(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_set_bits(info->ab8500, info->update_reg,
+			info->mask, info->enable);
+	if (ret < 0)
+		dev_err(rdev_get_dev(rdev),
+			"couldn't set enable bits for regulator\n");
+	return ret;
+}
+
+static int ab8500_regulator_disable(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_set_bits(info->ab8500, info->update_reg,
+			info->mask, 0x0);
+	if (ret < 0)
+		dev_err(rdev_get_dev(rdev),
+			"couldn't set disable bits for regulator\n");
+	return ret;
+}
+
+static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_read(info->ab8500, info->update_reg);
+	if (ret < 0) {
+		dev_err(rdev_get_dev(rdev),
+			"couldn't read 0x%x register\n", info->update_reg);
+		return ret;
+	}
+
+	if (ret & info->mask)
+		return true;
+	else
+		return false;
+}
+
+static int ab8500_list_voltage(struct regulator_dev *rdev, unsigned selector)
+{
+	int regulator_id;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	/* return the uV for the fixed regulators */
+	if (info->fixed_uV)
+		return info->fixed_uV;
+
+	if (selector > info->voltages_len)
+		return -EINVAL;
+
+	return info->supported_voltages[selector];
+}
+
+static int ab8500_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	int regulator_id, ret, val;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	ret = ab8500_read(info->ab8500, info->voltage_reg);
+	if (ret < 0) {
+		dev_err(rdev_get_dev(rdev),
+			"couldn't read voltage reg for regulator\n");
+		return ret;
+	}
+
+	/* vintcore has a different layout */
+	val = ret & info->voltage_mask;
+	if (regulator_id == AB8500_LDO_INTCORE)
+		ret = info->supported_voltages[val >> 0x3];
+	else
+		ret = info->supported_voltages[val];
+
+	return ret;
+}
+
+static int ab8500_get_best_voltage_index(struct regulator_dev *rdev,
+		int min_uV, int max_uV)
+{
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+	int i;
+
+	/* check the supported voltage */
+	for (i = 0; i < info->voltages_len; i++) {
+		if ((info->supported_voltages[i] >= min_uV) &&
+		    (info->supported_voltages[i] <= max_uV))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int ab8500_regulator_set_voltage(struct regulator_dev *rdev,
+		int min_uV, int max_uV)
+{
+	int regulator_id, ret;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	/* get the appropriate voltages within the range */
+	ret = ab8500_get_best_voltage_index(rdev, min_uV, max_uV);
+	if (ret < 0) {
+		dev_err(rdev_get_dev(rdev),
+				"couldn't get best voltage for regulator\n");
+		return ret;
+	}
+
+	/* set the registers for the request */
+	ret = ab8500_set_bits(info->ab8500, info->voltage_reg,
+				info->voltage_mask, ret);
+	if (ret < 0)
+		dev_err(rdev_get_dev(rdev),
+		"couldn't set voltage reg for regulator\n");
+
+	return ret;
+}
+
+static struct regulator_ops ab8500_regulator_ops = {
+	.enable		= ab8500_regulator_enable,
+	.disable	= ab8500_regulator_disable,
+	.is_enabled	= ab8500_regulator_is_enabled,
+	.get_voltage	= ab8500_regulator_get_voltage,
+	.set_voltage	= ab8500_regulator_set_voltage,
+	.list_voltage	= ab8500_list_voltage,
+};
+
+static int ab8500_fixed_get_voltage(struct regulator_dev *rdev)
+{
+	int regulator_id;
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= AB8500_NUM_REGULATORS)
+		return -EINVAL;
+
+	return info->fixed_uV;
+}
+
+static struct regulator_ops ab8500_ldo_fixed_ops = {
+	.enable		= ab8500_regulator_enable,
+	.disable	= ab8500_regulator_disable,
+	.is_enabled	= ab8500_regulator_is_enabled,
+	.get_voltage	= ab8500_fixed_get_voltage,
+	.list_voltage	= ab8500_list_voltage,
+};
+
+#define AB8500_LDO(_id, min, max, reg, reg_mask, reg_enable,	\
+		volt_reg, volt_mask, voltages,			\
+			len_volts)				\
+{								\
+	.desc	= {						\
+		.name	= "LDO-" #_id,				\
+		.ops	= &ab8500_regulator_ops,		\
+		.type	= REGULATOR_VOLTAGE,			\
+		.id	= AB8500_LDO_##_id,			\
+		.owner	= THIS_MODULE,				\
+	},							\
+	.min_uV		= (min) * 1000,				\
+	.max_uV		= (max) * 1000,				\
+	.update_reg	= reg,					\
+	.mask		= reg_mask,				\
+	.enable		= reg_enable,				\
+	.voltage_reg	= volt_reg,				\
+	.voltage_mask	= volt_mask,				\
+	.supported_voltages = voltages,				\
+	.voltages_len	= len_volts,				\
+	.fixed_uV	= 0,					\
+}
+
+#define AB8500_FIXED_LDO(_id, fixed, reg, reg_mask,	\
+				reg_enable)		\
+{							\
+	.desc	= {					\
+		.name	= "LDO-" #_id,			\
+		.ops	= &ab8500_ldo_fixed_ops,	\
+		.type	= REGULATOR_VOLTAGE,		\
+		.id	= AB8500_LDO_##_id,		\
+		.owner	= THIS_MODULE,			\
+	},						\
+	.fixed_uV	= fixed * 1000,			\
+	.update_reg	= reg,				\
+	.mask		= reg_mask,			\
+	.enable		= reg_enable,			\
+}
+
+static struct ab8500_regulator_info ab8500_regulator_info[] = {
+	/*
+	 * Variable Voltage LDOs
+	 * name, min uV, max uV, ctrl reg, reg mask, enable mask,
+	 *	volt ctrl reg, volt ctrl mask, volt table, num supported volts
+	 */
+	AB8500_LDO(AUX1, 1100, 3300, 0x0409, 0x3, 0x1, 0x041f, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(AUX2, 1100, 3300, 0x0409, 0xc, 0x4, 0x0420, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(AUX3, 1100, 3300, 0x040a, 0x3, 0x1, 0x0421, 0xf,
+			ldo_vauxn_voltages, ARRAY_SIZE(ldo_vauxn_voltages)),
+	AB8500_LDO(INTCORE, 1100, 3300, 0x0380, 0x4, 0x4, 0x0380, 0x38,
+		ldo_vintcore_voltages, ARRAY_SIZE(ldo_vintcore_voltages)),
+
+	/*
+	 * Fixed Voltage LDOs
+	 *		 name,	o/p uV, ctrl reg, enable, disable
+	 */
+	AB8500_FIXED_LDO(TVOUT,	  2000,   0x0380,   0x2,    0x2),
+	AB8500_FIXED_LDO(AUDIO,   2000,   0x0383,   0x2,    0x2),
+	AB8500_FIXED_LDO(ANAMIC1, 2050,   0x0383,   0x4,    0x4),
+	AB8500_FIXED_LDO(ANAMIC2, 2050,   0x0383,   0x8,    0x8),
+	AB8500_FIXED_LDO(DMIC,    1800,   0x0383,   0x10,   0x10),
+	AB8500_FIXED_LDO(ANA,     1200,   0x0383,   0xc,    0x4),
+};
+
+static inline struct ab8500_regulator_info *find_regulator_info(int id)
+{
+	struct ab8500_regulator_info *info;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		info = &ab8500_regulator_info[i];
+		if (info->desc.id == id)
+			return info;
+	}
+	return NULL;
+}
+
+static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
+{
+	struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent);
+	struct ab8500_platform_data *pdata = dev_get_platdata(ab8500->dev);
+	int i, err;
+
+	if (!ab8500) {
+		dev_err(&pdev->dev, "null mfd parent\n");
+		return -EINVAL;
+	}
+
+	/* register all regulators */
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		struct ab8500_regulator_info *info = NULL;
+
+		/* assign per-regulator data */
+		info = &ab8500_regulator_info[i];
+		info->dev = &pdev->dev;
+		info->ab8500 = ab8500;
+
+		info->regulator = regulator_register(&info->desc, &pdev->dev,
+				pdata->regulator[i], info);
+		if (IS_ERR(info->regulator)) {
+			err = PTR_ERR(info->regulator);
+			dev_err(&pdev->dev, "failed to register regulator %s\n",
+					info->desc.name);
+			/* when we fail, un-register all earlier regulators */
+			i--;
+			while (i > 0) {
+				info = &ab8500_regulator_info[i];
+				regulator_unregister(info->regulator);
+				i--;
+			}
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static __devexit int ab8500_regulator_remove(struct platform_device *pdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
+		struct ab8500_regulator_info *info = NULL;
+		info = &ab8500_regulator_info[i];
+		regulator_unregister(info->regulator);
+	}
+
+	return 0;
+}
+
+static struct platform_driver ab8500_regulator_driver = {
+	.probe = ab8500_regulator_probe,
+	.remove = __devexit_p(ab8500_regulator_remove),
+	.driver         = {
+		.name   = "ab8500-regulator",
+		.owner  = THIS_MODULE,
+	},
+};
+
+static int __init ab8500_regulator_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&ab8500_regulator_driver);
+	if (ret != 0)
+		pr_err("Failed to register ab8500 regulator: %d\n", ret);
+
+	return ret;
+}
+subsys_initcall(ab8500_regulator_init);
+
+static void __exit ab8500_regulator_exit(void)
+{
+	platform_driver_unregister(&ab8500_regulator_driver);
+}
+module_exit(ab8500_regulator_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sundar Iyer <sundar.iyer@stericsson.com>");
+MODULE_DESCRIPTION("Regulator Driver for ST-Ericsson AB8500 Mixed-Sig PMIC");
+MODULE_ALIAS("platform:ab8500-regulator");
-- 
1.7.0

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Tue, Jul 13, 2010 at 09:48:56PM +0530, Sundar R IYER wrote:

> From f4bf7eec4d210db5075c0bce4521d9be6bc76c8c Mon Sep 17 00:00:00 2001
> From: Sundar R Iyer <sundar.iyer@stericsson.com>
> Date: Sun, 6 Jun 2010 19:12:12 +0530
> Subject: [PATCH v3 1/2] regulator: add support for regulators on the ab8500 MFD
> 
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
> Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Liam Girdwood]

On Tue, 2010-07-13 at 21:40 +0100, Mark Brown wrote:
> On Tue, Jul 13, 2010 at 09:48:56PM +0530, Sundar R IYER wrote:
> 
> > From f4bf7eec4d210db5075c0bce4521d9be6bc76c8c Mon Sep 17 00:00:00 2001
> > From: Sundar R Iyer <sundar.iyer@stericsson.com>
> > Date: Sun, 6 Jun 2010 19:12:12 +0530
> > Subject: [PATCH v3 1/2] regulator: add support for regulators on the ab8500 MFD
> > 
> > Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> > Acked-by: Bengt JONSSON <bengt.g.jonsson@stericsson.com>
> > Signed-off-by: Sundar R Iyer <sundar.iyer@stericsson.com>
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Thiago Farina]

On Tue, Jul 13, 2010 at 12:09 PM, Sundar Iyer
<sundar.iyer@stericsson.com> wrote:
> +static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +       int regulator_id, ret;
> +       struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +       regulator_id = rdev_get_id(rdev);
> +       if (regulator_id >= AB8500_NUM_REGULATORS)
> +               return -EINVAL;
> +
> +       ret = ab8500_read(info->ab8500, info->update_reg);
> +       if (ret < 0) {
> +               dev_dbg(rdev_get_dev(rdev),
> +                       "couldnt read 0x%x register\n", info->update_reg);
> +               return ret;
> +       }
> +
> +       if (ret & info->mask)
> +               return true;
> +       else
> +               return false;
> +}

Shouldn't be this returning 1 instead of true and 0 instead false
(since the return type is int not bool)?

Maybe like this?
return (ret & info->mask) ? 1: 0;

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Wed, Oct 27, 2010 at 02:25:44PM -0200, Thiago Farina wrote:

> Shouldn't be this returning 1 instead of true and 0 instead false
> (since the return type is int not bool)?

There is no reason to do this, logical values are treated as 0 and 1 in
C.  Using false and true is clear and won't cause any difference in
code.

> Maybe like this?
> return (ret & info->mask) ? 1: 0;

No, that's needlessly obfuscated.

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Thiago Farina]

On Wed, Oct 27, 2010 at 3:33 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Oct 27, 2010 at 02:25:44PM -0200, Thiago Farina wrote:
>
>> Shouldn't be this returning 1 instead of true and 0 instead false
>> (since the return type is int not bool)?
>
> There is no reason to do this, logical values are treated as 0 and 1 in
> C.  Using false and true is clear and won't cause any difference in
> code.
>
In C99 I suppose that is true and legal?

>> Maybe like this?
>> return (ret & info->mask) ? 1: 0;
>
> No, that's needlessly obfuscated.
>
Obfuscated? What you mean? It is a driver, and people reading and
writing a driver would know what it means, no?

Would be much simpler if it was just (like done in ab3100.c):

return (ret & info->mask);

Re: [PATCH v2 1/2] regulator: add support for regulators on the ab8500 MFD

[Mark Brown]

On Wed, Oct 27, 2010 at 03:42:53PM -0200, Thiago Farina wrote:
> On Wed, Oct 27, 2010 at 3:33 PM, Mark Brown

> > There is no reason to do this, logical values are treated as 0 and 1 in
> > C.  Using false and true is clear and won't cause any difference in
> > code.

> In C99 I suppose that is true and legal?

Yes.  C has always used 1 and 0 as the numerical mappings for logical
values, the addition of the keywords did not change them.

> >> Maybe like this?
> >> return (ret & info->mask) ? 1: 0;

> > No, that's needlessly obfuscated.

> Obfuscated? What you mean? It is a driver, and people reading and
> writing a driver would know what it means, no?

Adding the ternery operator just makes the code more noisy for no
benefit.

> Would be much simpler if it was just (like done in ab3100.c):

> return (ret & info->mask);

Yes, though there's no problem with the current code either.