[patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Support most of the LDO regulators in the twl4030 family chips.
In the case of LDOs supporting MMC/SD, the voltage controls are
used; but in most other cases, the regulator framework is only
used to enable/disable a supplies, conserving power when a given
voltage rail is not needed.

The drivers/mfd/twl4030-core.c code already sets up the various
regulators according to board-specific configuration, and knows
that some chips don't provide the full set of voltage rails.

The omitted regulators are intended to be under hardware control,
such as during the hardware-mediated system powerup, powerdown,
and suspend states.  Unless/until software hooks are known to
be safe, they won't be exported here.

These regulators implement the new get_status() operation, but
can't realistically implement get_mode(); the status output is
effectively the result of a vote, with the relevant hardware
inputs not exposed.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Note a minor regulator API glitch:  there is no clean way to
convert voltages supported by a regulator like VMMC1 or VMMC2
to/from the MMC/SD framework's voltage masks.

 drivers/regulator/Kconfig             |    7 
 drivers/regulator/Makefile            |    1 
 drivers/regulator/twl4030-regulator.c |  511 ++++++++++++++++++++++++++++++++
 include/linux/i2c/twl4030.h           |   47 ++
 4 files changed, 566 insertions(+)

--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -52,6 +52,13 @@ config REGULATOR_BQ24022
 	  charging select between 100 mA and 500 mA charging current
 	  limit.
 
+config REGULATOR_TWL4030
+	bool "TI TWL4030/TWL5030/TPS695x0 PMIC"
+	depends on TWL4030_CORE
+	help
+	  This driver supports the voltage regulators provided by
+	  this family of companion chips.
+
 config REGULATOR_WM8350
 	tristate "Wolfson Microelectroncis WM8350 AudioPlus PMIC"
 	depends on MFD_WM8350
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) +=
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 
 obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
+obj-$(CONFIG_REGULATOR_TWL4030) += twl4030-regulator.o
 obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
 obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
--- /dev/null
+++ b/drivers/regulator/twl4030-regulator.c
@@ -0,0 +1,511 @@
+/*
+ * twl4030-regulator.c -- support regulators in twl4030 family chips
+ *
+ * Copyright (C) 2008 David Brownell
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/i2c/twl4030.h>
+
+
+/*
+ * The TWL4030/TW5030/TPS659x0 family chips include power management, a
+ * USB OTG transceiver, an RTC, ADC, PWM, and lots more.  Some versions
+ * include an audio codec, battery charger, and more voltage regulators.
+ * These chips are often used in OMAP-based systems.
+ *
+ * This driver implements software-based resource control for various
+ * voltage regulators.  This is usually augmented with state machine
+ * based control.
+ */
+
+struct twlreg_info {
+	/* start of regulator's PM_RECEIVER control register bank */
+	u8			base;
+
+	/* twl4030 resource ID, for resource control state machine */
+	u8			id;
+
+	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
+	u8			table_len;
+	const u16		*table;
+
+	/* chip constraints on regulator behavior */
+	u16			min_mV;
+	u16			max_mV;
+
+	/* used by regulator core */
+	struct regulator_desc	desc;
+};
+
+
+/* LDO control registers ... offset is from the base of its register bank.
+ * The first three registers of all power resource banks help hardware to
+ * manage the various resource groups.
+ */
+#define VREG_GRP		0
+#define VREG_TYPE		1
+#define VREG_REMAP		2
+#define VREG_DEDICATED		3	/* LDO control */
+
+
+static inline int
+twl4030reg_read(struct twlreg_info *info, unsigned offset)
+{
+	u8 value;
+	int status;
+
+	status = twl4030_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER,
+			&value, info->base + offset);
+	return (status < 0) ? status : value;
+}
+
+static inline int
+twl4030reg_write(struct twlreg_info *info, unsigned offset, u8 value)
+{
+	return twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER,
+			value, info->base + offset);
+}
+
+/*----------------------------------------------------------------------*/
+
+/* generic power resource operations, which work on all regulators */
+
+static int twl4030reg_grp(struct regulator_dev *rdev)
+{
+	return twl4030reg_read(rdev_get_drvdata(rdev), VREG_GRP);
+}
+
+/*
+ * Enable/disable regulators by joining/leaving the P1 (processor) group.
+ * We assume nobody else is updating the DEV_GRP registers.
+ */
+
+#define P3_GRP		BIT(7)		/* "peripherals" */
+#define P2_GRP		BIT(6)		/* secondary processor, modem, etc */
+#define P1_GRP		BIT(5)		/* CPU/Linux */
+
+static int twl4030reg_is_enabled(struct regulator_dev *rdev)
+{
+	int	state = twl4030reg_grp(rdev);
+
+	if (state < 0)
+		return state;
+
+	return (state & P1_GRP) != 0;
+}
+
+static int twl4030reg_enable(struct regulator_dev *rdev)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			grp;
+
+	grp = twl4030reg_read(info, VREG_GRP);
+	if (grp < 0)
+		return grp;
+
+	grp |= P1_GRP;
+	return twl4030reg_write(info, VREG_GRP, grp);
+}
+
+static int twl4030reg_disable(struct regulator_dev *rdev)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			grp;
+
+	grp = twl4030reg_read(info, VREG_GRP);
+	if (grp < 0)
+		return grp;
+
+	grp &= ~P1_GRP;
+	return twl4030reg_write(info, VREG_GRP, grp);
+}
+
+static int twl4030reg_get_status(struct regulator_dev *rdev)
+{
+	int	state = twl4030reg_grp(rdev);
+
+	if (state < 0)
+		return state;
+	state &= 0x0f;
+
+	/* assume state != WARM_RESET; we'd not be running...  */
+	if (!state)
+		return REGULATOR_STATUS_OFF;
+	return (state & BIT(3))
+		? REGULATOR_STATUS_NORMAL
+		: REGULATOR_STATUS_STANDBY;
+}
+
+static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	unsigned		message;
+	int			status;
+
+	/* We can only set the mode through state machine commands... */
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		message = MSG_SINGULAR(DEV_GRP_P1, info->id, RES_STATE_ACTIVE);
+		break;
+	case REGULATOR_MODE_STANDBY:
+		message = MSG_SINGULAR(DEV_GRP_P1, info->id, RES_STATE_SLEEP);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Ensure the resource is associated with some group */
+	status = twl4030reg_grp(rdev);
+	if (status < 0)
+		return status;
+	if (!(status & (P3_GRP | P2_GRP | P1_GRP)))
+		return -EACCES;
+
+	status = twl4030_i2c_write_u8(TWL4030_MODULE_PM_MASTER,
+			message >> 8, 0x15 /* PB_WORD_MSB */ );
+	if (status >= 0)
+		return status;
+
+	return twl4030_i2c_write_u8(TWL4030_MODULE_PM_MASTER,
+			message, 0x16 /* PB_WORD_LSB */ );
+}
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * Support for adjustable-voltage LDOs uses a four bit (or less) voltage
+ * select field in its control register.   We use tables indexed by VSEL
+ * to record voltages in milliVolts.  (Accuracy is about three percent.)
+ *
+ * Note that VSEL values for VAUX2 changed in twl5030 and newer silicon;
+ * currently handled by listing two slightly different VAUX2 regulators,
+ * only one of which will be configured.
+ *
+ * VSEL values documented as "TI cannot support these values" are flagged
+ * in these tables as UNSUP() values; we normally won't assign them.
+ */
+#ifdef CONFIG_TWL4030_ALLOW_UNSUPPORTED
+#define UNSUP_MASK	0x0000
+#else
+#define UNSUP_MASK	0x8000
+#endif
+
+#define UNSUP(x)	(UNSUP_MASK | (x))
+#define IS_UNSUP(x)	(UNSUP_MASK & (x))
+#define LDO_MV(x)	(~UNSUP_MASK & (x))
+
+
+static const u16 VAUX1_VSEL_table[] = {
+	UNSUP(1500), UNSUP(1800), 2500, 2800,
+	3000, 3000, 3000, 3000,
+};
+static const u16 VAUX2_4030_VSEL_table[] = {
+	UNSUP(1000), UNSUP(1000), UNSUP(1200), 1300,
+	1500, 1800, UNSUP(1850), 2500,
+	UNSUP(2600), 2800, UNSUP(2850), UNSUP(3000),
+	UNSUP(3150), UNSUP(3150), UNSUP(3150), UNSUP(3150),
+};
+static const u16 VAUX2_VSEL_table[] = {
+	1700, 1700, 1900, 1300,
+	1500, 1800, 2000, 2500,
+	2100, 2800, 2200, 2300,
+	2400, 2400, 2400, 2400,
+};
+static const u16 VAUX3_VSEL_table[] = {
+	1500, 1800, 2500, 2800,
+	UNSUP(3000), UNSUP(3000), UNSUP(3000), UNSUP(3000),
+};
+static const u16 VAUX4_VSEL_table[] = {
+	700, 1000, 1200, UNSUP(1300),
+	1500, 1800, UNSUP(1850), 2500,
+};
+static const u16 VMMC1_VSEL_table[] = {
+	1850, 2850, 3000, 3150,
+};
+static const u16 VMMC2_VSEL_table[] = {
+	UNSUP(1000), UNSUP(1000), UNSUP(1200), UNSUP(1300),
+	UNSUP(1500), UNSUP(1800), 1850, UNSUP(2500),
+	2600, 2800, 2850, 3000,
+	3150, 3150, 3150, 3150,
+};
+static const u16 VPLL1_VSEL_table[] = {
+	1000, 1200, 1300, 1800,
+	UNSUP(2800), UNSUP(3000), UNSUP(3000), UNSUP(3000),
+};
+static const u16 VPLL2_VSEL_table[] = {
+	700, 1000, 1200, 1300,
+	UNSUP(1500), 1800, UNSUP(1850), UNSUP(2500),
+	UNSUP(2600), UNSUP(2800), UNSUP(2850), UNSUP(3000),
+	UNSUP(3150), UNSUP(3150), UNSUP(3150), UNSUP(3150),
+};
+static const u16 VSIM_VSEL_table[] = {
+	UNSUP(1000), UNSUP(1200), UNSUP(1300), 1800,
+	2800, 3000, 3000, 3000,
+};
+static const u16 VDAC_VSEL_table[] = {
+	1200, 1300, 1800, 1800,
+};
+
+
+static int
+twl4030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			vsel;
+
+	for (vsel = 0; vsel < info->table_len; vsel++) {
+		int mV = info->table[vsel];
+		int uV;
+
+		if (IS_UNSUP(mV))
+			continue;
+		uV = LDO_MV(mV) * 1000;
+
+		/* use the first in-range value */
+		if (min_uV <= uV && uV <= max_uV)
+			return twl4030reg_write(info, VREG_DEDICATED, vsel);
+	}
+
+	return -EDOM;
+}
+
+static int twl4030ldo_get_voltage(struct regulator_dev *rdev)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			vsel = twl4030reg_read(info, VREG_DEDICATED);
+
+	if (vsel < 0)
+		return vsel;
+
+	vsel &= info->table_len - 1;
+	return LDO_MV(info->table[vsel]) * 1000;
+}
+
+static struct regulator_ops twl4030ldo_ops = {
+	.set_voltage	= twl4030ldo_set_voltage,
+	.get_voltage	= twl4030ldo_get_voltage,
+
+	.enable		= twl4030reg_enable,
+	.disable	= twl4030reg_disable,
+	.is_enabled	= twl4030reg_is_enabled,
+
+	.set_mode	= twl4030reg_set_mode,
+
+	.get_status	= twl4030reg_get_status,
+};
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * Fixed voltage LDOs don't have a VSEL field to update.
+ */
+static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+
+	return info->min_mV * 1000;
+}
+
+static struct regulator_ops twl4030fixed_ops = {
+	.get_voltage	= twl4030fixed_get_voltage,
+
+	.enable		= twl4030reg_enable,
+	.disable	= twl4030reg_disable,
+	.is_enabled	= twl4030reg_is_enabled,
+
+	.set_mode	= twl4030reg_set_mode,
+
+	.get_status	= twl4030reg_get_status,
+};
+
+/*----------------------------------------------------------------------*/
+
+#define TWL_ADJUSTABLE_LDO(label, offset, num) { \
+	.base = offset, \
+	.id = num, \
+	.table_len = ARRAY_SIZE(label##_VSEL_table), \
+	.table = label##_VSEL_table, \
+	.desc = { \
+		.name = #label, \
+		.id = TWL4030_REG_##label, \
+		.ops = &twl4030ldo_ops, \
+		.type = REGULATOR_VOLTAGE, \
+		.owner = THIS_MODULE, \
+		}, \
+	}
+
+#define TWL_FIXED_LDO(label, offset, mVolts, num) { \
+	.base = offset, \
+	.id = num, \
+	.min_mV = mVolts, \
+	.max_mV = mVolts, \
+	.desc = { \
+		.name = #label, \
+		.id = TWL4030_REG_##label, \
+		.ops = &twl4030fixed_ops, \
+		.type = REGULATOR_VOLTAGE, \
+		.owner = THIS_MODULE, \
+		}, \
+	}
+
+/*
+ * We list regulators here if systems need some level of
+ * software control over them after boot.
+ */
+static struct twlreg_info twl4030_regs[] = {
+	TWL_ADJUSTABLE_LDO(VAUX1, 0x17, 1),
+	TWL_ADJUSTABLE_LDO(VAUX2_4030, 0x1b, 2),
+	TWL_ADJUSTABLE_LDO(VAUX2, 0x1b, 2),
+	TWL_ADJUSTABLE_LDO(VAUX3, 0x1f, 3),
+	TWL_ADJUSTABLE_LDO(VAUX4, 0x23, 4),
+	TWL_ADJUSTABLE_LDO(VMMC1, 0x27, 5),
+	TWL_ADJUSTABLE_LDO(VMMC2, 0x2b, 6),
+	/*
+	TWL_ADJUSTABLE_LDO(VPLL1, 0x2f, 7),
+	TWL_ADJUSTABLE_LDO(VPLL2, 0x33, 8),
+	*/
+	TWL_ADJUSTABLE_LDO(VSIM, 0x37, 9),
+	TWL_ADJUSTABLE_LDO(VDAC, 0x3b, 10),
+	/*
+	TWL_ADJUSTABLE_LDO(VINTANA1, 0x3f, 11),
+	TWL_ADJUSTABLE_LDO(VINTANA2, 0x43, 12),
+	TWL_ADJUSTABLE_LDO(VINTDIG, 0x47, 13),
+	TWL_SMPS(VIO, 0x4b, 14),
+	TWL_SMPS(VDD1, 0x55, 15),
+	TWL_SMPS(VDD2, 0x63, 16),
+	 */
+	TWL_FIXED_LDO(VUSB1V5, 0x71, 1500, 17),
+	TWL_FIXED_LDO(VUSB1V8, 0x74, 1800, 18),
+	TWL_FIXED_LDO(VUSB3V1, 0x77, 3100, 19),
+	/* VUSBCP is managed *only* by the USB subchip */
+};
+
+static int twl4030reg_probe(struct platform_device *pdev)
+{
+	int				i;
+	struct twlreg_info		*info;
+	struct regulator_init_data	*initdata;
+	struct regulation_constraints	*c;
+	struct regulator_dev		*rdev;
+	int				min_uV, max_uV;
+
+	for (i = 0, info = NULL; i < ARRAY_SIZE(twl4030_regs); i++) {
+		if (twl4030_regs[i].desc.id != pdev->id)
+			continue;
+		info = twl4030_regs + i;
+		min_uV = info->min_mV * 1000;
+		max_uV = info->max_mV * 1000;
+		break;
+	}
+	if (!info)
+		return -ENODEV;
+
+	initdata = pdev->dev.platform_data;
+	if (!initdata)
+		return -EINVAL;
+
+	/* Constrain board-specific capabilities according to what
+	 * this driver and the chip itself can actually do.
+	 */
+	c = &initdata->constraints;
+	if (!c->min_uV || c->min_uV < min_uV)
+		c->min_uV = min_uV;
+	if (!c->max_uV || c->max_uV > max_uV)
+		c->max_uV = max_uV;
+	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
+	c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE
+				| REGULATOR_CHANGE_MODE
+				| REGULATOR_CHANGE_STATUS;
+
+	rdev = regulator_register(&info->desc, &pdev->dev, info);
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "can't register %s, %ld\n",
+				info->desc.name, PTR_ERR(rdev));
+		return PTR_ERR(rdev);
+	}
+	platform_set_drvdata(pdev, rdev);
+
+	/* NOTE:  many regulators support short-circuit IRQs (presentable
+	 * as REGULATOR_OVER_CURRENT notifications?) configured via:
+	 *  - SC_CONFIG
+	 *  - SC_DETECT1 (vintana2, vmmc1/2, vaux1/2/3/4)
+	 *  - SC_DETECT2 (vusb, vdac, vio, vdd1/2, vpll2)
+	 *  - IT_CONFIG
+	 */
+
+	return 0;
+}
+
+static int __devexit twl4030reg_remove(struct platform_device *pdev)
+{
+	regulator_unregister(platform_get_drvdata(pdev));
+	return 0;
+}
+
+MODULE_ALIAS("platform:twl4030_reg");
+
+static struct platform_driver twl4030reg_driver = {
+	.probe		= twl4030reg_probe,
+	.remove		= __devexit_p(twl4030reg_remove),
+	/* NOTE: short name, to work around driver model truncation of
+	 * "twl4030_regulator.12" (and friends) to "twl4030_regulator.1".
+	 */
+	.driver.name	= "twl4030_reg",
+	.driver.owner	= THIS_MODULE,
+};
+
+static int __init twl4030reg_init(void)
+{
+	unsigned i, j;
+
+	/* determine min/max voltage constraints, taking into account
+	 * whether set_voltage() will use the "unsupported" settings
+	 */
+	for (i = 0; i < ARRAY_SIZE(twl4030_regs); i++) {
+		struct twlreg_info	*info = twl4030_regs + i;
+		const u16		*table;
+
+		/* fixed-voltage regulators */
+		if (!info->table_len)
+			continue;
+
+		/* LDO regulators: */
+		for (j = 0, table = info->table;
+				j < info->table_len;
+				j++, table++) {
+			u16		mV = *table;
+
+			if (IS_UNSUP(mV))
+				continue;
+			mV = LDO_MV(mV);
+
+			if (info->min_mV == 0 || info->min_mV > mV)
+				info->min_mV = mV;
+			if (info->max_mV < mV)
+				info->max_mV = mV;
+		}
+	}
+
+	return platform_driver_register(&twl4030reg_driver);
+}
+subsys_initcall(twl4030reg_init);
+
+static void __exit twl4030reg_exit(void)
+{
+	platform_driver_unregister(&twl4030reg_driver);
+}
+module_exit(twl4030reg_exit)
+
+MODULE_DESCRIPTION("TWL4030 regulator driver");
+MODULE_LICENSE("GPL");
--- a/include/linux/i2c/twl4030.h
+++ b/include/linux/i2c/twl4030.h
@@ -218,6 +218,53 @@ int twl4030_i2c_read(u8 mod_no, u8 *valu
 
 /*----------------------------------------------------------------------*/
 
+/* Power bus message definitions */
+
+#define DEV_GRP_NULL		0x0
+#define DEV_GRP_P1		0x1
+#define DEV_GRP_P2		0x2
+#define DEV_GRP_P3		0x4
+
+#define RES_GRP_RES		0x0
+#define RES_GRP_PP		0x1
+#define RES_GRP_RC		0x2
+#define RES_GRP_PP_RC		0x3
+#define RES_GRP_PR		0x4
+#define RES_GRP_PP_PR		0x5
+#define RES_GRP_RC_PR		0x6
+#define RES_GRP_ALL		0x7
+
+#define RES_TYPE2_R0		0x0
+
+#define RES_TYPE_ALL		0x7
+
+#define RES_STATE_WRST		0xF
+#define RES_STATE_ACTIVE	0xE
+#define RES_STATE_SLEEP		0x8
+#define RES_STATE_OFF		0x0
+
+/*
+ * Power Bus Message Format ... these can be sent individually by Linux,
+ * but are usually part of downloaded scripts that are run when various
+ * power events are triggered.
+ *
+ *  Broadcast Message (16 Bits):
+ *    DEV_GRP[15:13] MT[12]  RES_GRP[11:9]  RES_TYPE2[8:7] RES_TYPE[6:4]
+ *    RES_STATE[3:0]
+ *
+ *  Singular Message (16 Bits):
+ *    DEV_GRP[15:13] MT[12]  RES_ID[11:4]  RES_STATE[3:0]
+ */
+
+#define MSG_BROADCAST(devgrp, grp, type, type2, state) \
+	( (devgrp) << 13 | 1 << 12 | (grp) << 9 | (type2) << 7 \
+	| (type) << 4 | (state))
+
+#define MSG_SINGULAR(devgrp, id, state) \
+	((devgrp) << 13 | 0 << 12 | (id) << 4 | (state))
+
+/*----------------------------------------------------------------------*/
+
 struct twl4030_bci_platform_data {
 	int *battery_tmp_tbl;
 	unsigned int tblsize;

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Sun, Feb 08, 2009 at 10:37:06AM -0800, David Brownell wrote:

> +/*
> + * We list regulators here if systems need some level of
> + * software control over them after boot.
> + */
> +static struct twlreg_info twl4030_regs[] = {

> +	/*
> +	TWL_ADJUSTABLE_LDO(VPLL1, 0x2f, 7),
> +	TWL_ADJUSTABLE_LDO(VPLL2, 0x33, 8),
> +	*/

Minor nit, but I guess the comment would be a bit more accurate if the
comment said that the regulators that aren't likely to be changed have
been commented out (it required that little extra bit of thinking).

> +	/* Constrain board-specific capabilities according to what
> +	 * this driver and the chip itself can actually do.
> +	 */
> +	c = &initdata->constraints;
> +	if (!c->min_uV || c->min_uV < min_uV)
> +		c->min_uV = min_uV;

If we're going to do this I think it'd be better to push it into the
core and let the regulators pass in a set of constraints so that the
behaviour will be consistent between drivers.

I'd also expect to have the registration fail or at least issue a
warning if the code kicks in since that indicates that the board
constraints have been set up incorrectly.  There's a reasonable chance
that the fixed up constraints will still need to be changed for the
board to be configured properly and things may end up being driven out
of spec, potentially causing damage.

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Sunday 08 February 2009, Mark Brown wrote:
> > +     /* Constrain board-specific capabilities according to what
> > +      * this driver and the chip itself can actually do.
> > +      */
> > +     c = &initdata->constraints;
> > +     if (!c->min_uV || c->min_uV < min_uV)
> > +             c->min_uV = min_uV;
> 
> If we're going to do this I think it'd be better to push it into the
> core and let the regulators pass in a set of constraints so that the
> behaviour will be consistent between drivers.

Maybe, but there is no such mechanism right yet.
When it's created, then this could switch over.


> I'd also expect to have the registration fail or at least issue a
> warning if the code kicks in since that indicates that the board
> constraints have been set up incorrectly.

A warning might make sense in some cases ... that's
something I would expect to see from the regulator
core, though.  Example, I see no "max < min" checks
triggering registration errors.


> There's a reasonable chance 
> that the fixed up constraints will still need to be changed for the
> board to be configured properly and things may end up being driven out
> of spec, potentially causing damage.

I don't see that happening ... all that code does is
tighten existing constraints to match what the hardware
can do.  The result is just a subset of the range the
board already said was OK.  If no valid subset exists,
that's a board design bug ... albeit one the regulator
core could detect.  (E.g. those "max < min" checks that
don't currently exist.)

I can easily imagine having things partially set up, and
not really caring whether, on a particular board, those
initial constraints are really the most specific ones
applicable.  One component tolerates a range of 1V8..3V6
maybe, but on any given board it can be hooked up to any
supply that's even partially in-range.

If I hook such a component up to a supply supporting 1V2
through 2V5, it's really no worry that the 1V2..1V8 part
of that range won't be used; or if 2V5..3V6 could also
work.  Those options really don't matter at all.  The
only significant part is that only the 1V8..2V5 will be
software-accessible on that board.

- Dave

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Sunday 08 February 2009, Mark Brown wrote:
> > +     /* Constrain board-specific capabilities according to what
> > +      * this driver and the chip itself can actually do.
> > +      */
> > +     c = &initdata->constraints;
> > +     if (!c->min_uV || c->min_uV < min_uV)
> > +             c->min_uV = min_uV;
> 
> If we're going to do this I think it'd be better to push it into the
> core and let the regulators pass in a set of constraints so that the
> behaviour will be consistent between drivers.

Maybe, but there is no such mechanism right yet.
When it's created, then this could switch over.


> I'd also expect to have the registration fail or at least issue a
> warning if the code kicks in since that indicates that the board
> constraints have been set up incorrectly.

A warning might make sense in some cases ... that's
something I would expect to see from the regulator
core, though.  Example, I see no "max < min" checks
triggering registration errors.


> There's a reasonable chance 
> that the fixed up constraints will still need to be changed for the
> board to be configured properly and things may end up being driven out
> of spec, potentially causing damage.

I don't see that happening ... all that code does is
tighten existing constraints to match what the hardware
can do.  The result is just a subset of the range the
board already said was OK.  If no valid subset exists,
that's a board design bug ... albeit one the regulator
core could detect.  (E.g. those "max < min" checks that
don't currently exist.)

I can easily imagine having things partially set up, and
not really caring whether, on a particular board, those
initial constraints are really the most specific ones
applicable.  One component tolerates a range of 1V8..3V6
maybe, but on any given board it can be hooked up to any
supply that's even partially in-range.

If I hook such a component up to a supply supporting 1V2
through 2V5, it's really no worry that the 1V2..1V8 part
of that range won't be used; or if 2V5..3V6 could also
work.  Those options really don't matter at all.  The
only significant part is that only the 1V8..2V5 will be
software-accessible on that board.

- Dave

--
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: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Sun, Feb 08, 2009 at 04:04:29PM -0800, David Brownell wrote:
> On Sunday 08 February 2009, Mark Brown wrote:

> > If we're going to do this I think it'd be better to push it into the
> > core and let the regulators pass in a set of constraints so that the
> > behaviour will be consistent between drivers.

> Maybe, but there is no such mechanism right yet.
> When it's created, then this could switch over.

Since you appear to be writing the code already... :)

> > I'd also expect to have the registration fail or at least issue a
> > warning if the code kicks in since that indicates that the board
> > constraints have been set up incorrectly.

> A warning might make sense in some cases ... that's
> something I would expect to see from the regulator
> core, though.

That's what I'm suggesting should happen - that things like this should
be implemented in the core so that the behaviour of the API remains
consistent for users moving between platforms and everyone benefits from
new features.

>                Example, I see no "max < min" checks
> triggering registration errors.

Not a bad idea, though.  The core currently doesn't do much checking of
the constraints but that's as much because nobody spent the time on it
as anything else.  To the extent it's a deliberate design decision it's
because the constraints and consumer lists are where the information
about what's possible on a given system comes from and so the checking
that can be done by software is relatively minor.

> > There's a reasonable chance 
> > that the fixed up constraints will still need to be changed for the
> > board to be configured properly and things may end up being driven out
> > of spec, potentially causing damage.

> I don't see that happening ... all that code does is
> tighten existing constraints to match what the hardware
> can do.  The result is just a subset of the range the
> board already said was OK.  If no valid subset exists,

The trouble is that this code should only do anything if the board code
provided a configuration that can't be supported by the hardware, which
is a bit of a red flag.  I'd expect it'd end up catching things like the
user typing extra digits by mistake (this has definitely happened when
writing consumer drivers).

Your current patch does also set constraints for the voltages if they
weren't there previously (though this shouldn't matter since voltage
setting shouldn't be enabled without voltage value constraints).

> I can easily imagine having things partially set up, and
> not really caring whether, on a particular board, those
> initial constraints are really the most specific ones
> applicable.  One component tolerates a range of 1V8..3V6
> maybe, but on any given board it can be hooked up to any
> supply that's even partially in-range.

The pattern you're describing is very much that for consumer and
regulator drivers.  They should work with as wide a set of constraints
as possible to allow them to be used on different systems with different
capabilities and needs - allowing this is essential for the API to be
usable since so many chips are flexible about the supplies they can use.

It's different for the machine constraints since they are by definition
specific to a given system.  While it's expected that users (especially
those sensitive to power consumption) may want to optimise the
configuration of their system during development to get the best
performance out of it there is also an expectation that users will be
making decisions based on knowledge of the hardware design.

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 09 February 2009, Mark Brown wrote:
> On Sun, Feb 08, 2009 at 04:04:29PM -0800, David Brownell wrote:
> > On Sunday 08 February 2009, Mark Brown wrote:
> 
> > > If we're going to do this I think it'd be better to push it into the
> > > core and let the regulators pass in a set of constraints so that the
> > > behaviour will be consistent between drivers.
> 
> > Maybe, but there is no such mechanism right yet.
> > When it's created, then this could switch over.
> 
> Since you appear to be writing the code already... :)

Not for the regulator core, though.  Switching from
a simplistic constraint engine to one that starts to
handle a more realistic "union of constraints" model
would be doable, but is a bit more off-topic work than
I'd want to sign up for just now.


> > > I'd also expect to have the registration fail or at least issue a
> > > warning if the code kicks in since that indicates that the board
> > > constraints have been set up incorrectly.
> 
> > A warning might make sense in some cases ... that's
> > something I would expect to see from the regulator
> > core, though.
> 
> That's what I'm suggesting should happen - that things like this should
> be implemented in the core so that the behaviour of the API remains
> consistent for users moving between platforms and everyone benefits from
> new features.
> 
> >                Example, I see no "max < min" checks
> > triggering registration errors.
> 
> Not a bad idea, though.  The core currently doesn't do much checking of
> the constraints but that's as much because nobody spent the time on it
> as anything else.  To the extent it's a deliberate design decision it's
> because the constraints and consumer lists are where the information
> about what's possible on a given system comes from and so the checking
> that can be done by software is relatively minor.

Or as I put it earlier:  the current constraint model is
a bit simplistic.

Such things can *EASILY* be over-done; starting under-done
isn't a bad model.  Better to collect a few real examples of
how/why "under-done" needs to be improved, than overdesign
from the start.  (And I've seen some power "constraint"
frameworks that seemed way overdone.)

 
> > > There's a reasonable chance 
> > > that the fixed up constraints will still need to be changed for the
> > > board to be configured properly and things may end up being driven out
> > > of spec, potentially causing damage.
> 
> > I don't see that happening ... all that code does is
> > tighten existing constraints to match what the hardware
> > can do.  The result is just a subset of the range the
> > board already said was OK.  If no valid subset exists,
> 
> The trouble is that this code should only do anything if the board code
> provided a configuration that can't be supported by the hardware, which
> is a bit of a red flag.  I'd expect it'd end up catching things like the
> user typing extra digits by mistake (this has definitely happened when
> writing consumer drivers).

The model you're working with doesn't do a good job of
component-izing things ... "board" may be "board stack",
where notions like "the" hardware are fluid.

And in particular, "can't be supported" was NOT an issue
in the scenarios I gave.  The same mainboard could easily
support two regulators with different selections of output
voltages.  The problem was the model where the constraints
from the mainboard would be specific to one regulator,
instead of just to the mainboard.


> Your current patch does also set constraints for the voltages if they
> weren't there previously

I was thinking that boards which don't need constraints
shouldn't need to create any ... they could just pass on
the capabilities of the underlying regulator.


> > I can easily imagine having things partially set up, and
> > not really caring whether, on a particular board, those
> > initial constraints are really the most specific ones
> > applicable.  One component tolerates a range of 1V8..3V6
> > maybe, but on any given board it can be hooked up to any
> > supply that's even partially in-range.
> 
> The pattern you're describing is very much that for consumer and
> regulator drivers.  They should work with as wide a set of constraints
> as possible to allow them to be used on different systems with different
> capabilities and needs - allowing this is essential for the API to be
> usable since so many chips are flexible about the supplies they can use.
> 
> It's different for the machine constraints since they are by definition
> specific to a given system.

Only when that "system" is componentized that way.
Not all are.

Modular systems can have plug-in regulator boards;
constraints on a mainboard would necessarily overlap
with supported regulator boards ... but the regulators
themselves would naturally have different constraints.

One way to view this:  what you're calling "regulator"
constraints are really coming from the "machine".

Those few lines of code that seem to bother you are
only recognizing that they are, in fact, two very
different entities.

Without changing the regulator core, the only way
to handle contstraints which come from the actual
regulators is to force the machine constraints
to be in-range with respect to whatever regulator
driver ends up using them.

- Dave

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Mon, Feb 09, 2009 at 04:24:01PM -0800, David Brownell wrote:
> On Monday 09 February 2009, Mark Brown wrote:

> > > Maybe, but there is no such mechanism right yet.
> > > When it's created, then this could switch over.

> > Since you appear to be writing the code already... :)

> Not for the regulator core, though.  Switching from
> a simplistic constraint engine to one that starts to

There's nothing specific to the driver in this code - it's just doing
basic comparisons of two sets of constraints and acting on them and
would work just as well if it were pushed into the core.  You're going
to need this in multiple drivers to use it in a system anyway (it'll
need to be in at least the drivers which might be used on that system).

> handle a more realistic "union of constraints" model
> would be doable, but is a bit more off-topic work than
> I'd want to sign up for just now.

Solving all possible problems is, obviously, a bigger task but I don't
think we need to solve everything in order to avoid having this be
driver specific.

What I would suggest that you do for this is submit a patch allowing the
regulators to supply a set of constraints when they register (but not
doing anything with them), another with a TWL4030 driver using that API
and a third patch making the core do something with that data.  This
would result in something which maintains consistent behaviour over all
regulator drivers, which is my main concern here, and avoids tying up
the TWL4030 driver merge with any debate about how to use information
about the capabilities of the regulator.

I have some regulator API patches I need to test (should be sometime
this week) so I may be inspired to do at least the first patch myself.
I don't think there's any doubt that the core can do something useful
with the information.

> The model you're working with doesn't do a good job of
> component-izing things ... "board" may be "board stack",
> where notions like "the" hardware are fluid.

Most of the development work for the regulator API has been done on
reference platforms with many pluggable boards (often including the PMIC
board) so thought has been given as to how to handle these systems.

For daughtercards other than the primary PMIC (which are vastly more
common in production systems) one approach has been to represent the
daughtercards as a device that gets whatever supplies the daughtercard
has.  The distribution of power within the daughtercard is then be done
by adding child regulators.

> > Your current patch does also set constraints for the voltages if they
> > weren't there previously

> I was thinking that boards which don't need constraints
> shouldn't need to create any ... they could just pass on
> the capabilities of the underlying regulator.

For safety the regulator API won't do anything without being explicitly
told that it's OK to do so - if we were to do this we'd need to have an
explicit flag in the constraints which says that this is what to do.
Constraints are also permission grants.

> Only when that "system" is componentized that way.
> Not all are.

> Modular systems can have plug-in regulator boards;
> constraints on a mainboard would necessarily overlap
> with supported regulator boards ... but the regulators
> themselves would naturally have different constraints.

Indeed.  Like I say, a very large proportion of the development of the
regulator API has been done on reference designs which are built in this
fashion, including both pluggable PMIC boards and other daughtercards.
Normally the primary PMIC cards are handled with conditional code in the
machine file (either compile time or run time) or by registering drivers
for all the PMICs and allowing failed device probes to sort everything
out.  So far handling things this way hasn't been a big deal - there are
usually relatively few PMIC boards out there for a given platform and
the PMIC itself is rarely a major restriction.

This conditional code would normally do the setup that is specific to
each PMIC board, including matching the regulators on the PMIC board
with the rest of the system and any devices on the PMIC board itself
that require supplies.  A lot of the time there is a one to many mapping
between regulators on the PMIC board and power domains in the base
system and there are often additional devices on the PMIC board in
addition to the base board.  It's not unknown for the contraints applied
to not be straight combinations of supply constraints (normally due to a
lack of desire for runtime flexibility on supplies).

At the end of the day board code is still always going to have to at
some point know what regulators are fitted and how they are connected
up.  There are some things we could do to make handling runtime
decisions on this easier, like allowing multiple sets of constraints
and supplies to be supplied so the machine drivers can set up the
various combinations of supplies more easily when that's possible (which
is I think what you were getting at above), but they don't remove the
need for board code to know what's there.

> One way to view this:  what you're calling "regulator"
> constraints are really coming from the "machine".

Yes, of course.  The constraints are applied to the regulator by the
core, they are constraints for the regulator not constraints imposed by
the regulator.

> Those few lines of code that seem to bother you are
> only recognizing that they are, in fact, two very
> different entities.

What's really bothering me is the *location* of the code.  As I've said,
my primary concern is that this introduces what are effectively API
changes which will mean that this driver behaves differently to other
drivers.  Other concerns about precisely what we do with any information
from the regulator driver are very much less important.

> Without changing the regulator core, the only way
> to handle contstraints which come from the actual
> regulators is to force the machine constraints
> to be in-range with respect to whatever regulator
> driver ends up using them.

Modifying the core is, of course, an option.

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Tuesday 10 February 2009, Mark Brown wrote:

> What I would suggest that you do for this is submit a patch allowing the 
> regulators to supply a set of constraints when they register (but not
> doing anything with them),

That pretty much needs to allow a list of discrete
voltages, not just be a range ... and have a way
for clients to retrieve that list.  Else I have a
very hard time imagining how to plug in MMC supplies
without writing the type of regulator-specific code
that this framework is intended to supplant.


> another with a TWL4030 driver using that API 
> and a third patch making the core do something with that data.

Best IMO to switch the last two around.  Effectively
there'd be one patch "add new features to regulator
core", followed by the first of a set of "implement
those new features in the driver for regulator X".

And in fact that's what I've done with the two patches
I'll be sending in a moment.


> This 
> would result in something which maintains consistent behaviour over all
> regulator drivers,

It's already consistent, even without such patches!!

There is no driver which pays attention to regulator(_dev)
constraints that does it any differently than this one.


> > > Your current patch does also set constraints for the voltages if they
> > > weren't there previously
> 
> > I was thinking that boards which don't need constraints
> > shouldn't need to create any ... they could just pass on
> > the capabilities of the underlying regulator.
> 
> For safety the regulator API won't do anything without being explicitly
> told that it's OK to do so - if we were to do this we'd need to have an
> explicit flag in the constraints which says that this is what to do.
> Constraints are also permission grants.

I like to avoid flags unless they're absolutely required.
In this case my initial reaction is to say that hooking up
the components in the first place was the permission grant.

 
> > Only when that "system" is componentized that way.
> > Not all are.
> 
> > Modular systems can have plug-in regulator boards;
> > constraints on a mainboard would necessarily overlap
> > with supported regulator boards ... but the regulators
> > themselves would naturally have different constraints.
> 
> Indeed.  Like I say, a very large proportion of the development of the
> regulator API has been done on reference designs which are built in this
> fashion, including both pluggable PMIC boards and other daughtercards.

That doesn't seem to have escaped its development cage yet.  ;)


> Normally the primary PMIC cards are handled with conditional code in the
> machine file (either compile time or run time) or by registering drivers
> for all the PMICs and allowing failed device probes to sort everything
> out.  So far handling things this way hasn't been a big deal - there are
> usually relatively few PMIC boards out there for a given platform and
> the PMIC itself is rarely a major restriction.

Fair enough.  I'd de-emphasize "conditional", since that sounds
way too much like #ifdeffing.  The source code should just call
something like is_pmic_cardX(), and not care how that works.

 
> > One way to view this:  what you're calling "regulator"
> > constraints are really coming from the "machine".
> 
> Yes, of course.  The constraints are applied to the regulator by the
> core, they are constraints for the regulator not constraints imposed by
> the regulator.

Then what would you call constraints by/from the regulator?

I suggest updating your terminology.  "machine constraints"
would be much more clear for what's there now:  they relate
to the machine.  Other constraints (regulator, consumer)
relate to the other components ... the ones for which they
are an adjective.


> > Those few lines of code that seem to bother you are
> > only recognizing that they are, in fact, two very
> > different entities.
> 
> What's really bothering me is the *location* of the code.  As I've said,
> my primary concern is that this introduces what are effectively API
> changes which will mean that this driver behaves differently to other
> drivers.  Other concerns about precisely what we do with any information
> from the regulator driver are very much less important.

I don't mind moving it ... later, after the regulator
core has proper support for decoupling machine-specific
constraints from regulator-specific ones.  I view that
code as a workaround for a current limitation of that
core, so like all such workarounds it should vanish
when it's no longer relevant.

- Dave

[patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Add a basic mechanism for regulators to report the discrete
voltages they support:  one method to count how many voltages
are available, and another to enumerate them.

Use those methods to force machine-level constraints into bounds.
(Example:  regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board
constraints for that rail are 2.0V to 3.6V ... so the range of
voltages is then 2.4V to 3.3V on this board.)

Export those voltages to the regulator consumer interface, so for
example regulator hooked up to an MMC/SD/SDIO slot can report the
actual voltage options available to cards connected there.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
I'm not particularly pleased with these names; suggestions?
This could also be done with one fewer method by designating a
special list_voltage() return value, but I like this better.

 drivers/regulator/core.c           |  107 +++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |    2 
 include/linux/regulator/driver.h   |   10 +++
 3 files changed, 119 insertions(+)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -719,6 +719,44 @@ static int set_machine_constraints(struc
 	else
 		name = "regulator";
 
+	/* maybe force machine-wide voltage constraints to match the
+	 * voltages supported by this regulator.  use the regulator's
+	 * entire range for boards with no particular constraints.
+	 */
+	if (ops->list_voltage) {
+		int	count = ops->count_voltages(rdev);
+		int	i;
+		int	min_uV = INT_MAX;
+		int	max_uV = INT_MIN;
+		int	cmin = constraints->min_uV ? : INT_MIN;
+		int	cmax = constraints->max_uV ? : INT_MAX;
+
+		/* initial: [cmin..cmax] valid, [min_uV..max_uV] not */
+		for (i = 0; i < count; i++) {
+			int	value;
+
+			value = ops->list_voltage(rdev, i);
+			if (value <= 0)
+				continue;
+
+			/* maybe adjust [min_uV..max_uV] */
+			if (value >= cmin && value < min_uV)
+				min_uV = value;
+			if (value <= cmax && value > max_uV)
+				max_uV = value;
+		}
+
+		/* final: [min_uV..max_uV] valid iff constraints valid */
+		if (max_uV < min_uV) {
+			pr_err("%s: bad '%s' voltage constraints\n",
+				       __func__, name);
+			ret = -EINVAL;
+			goto out;
+		}
+		constraints->min_uV = min_uV;
+		constraints->max_uV = max_uV;
+	}
+
 	rdev->constraints = constraints;
 
 	/* do we need to apply the constraint voltage */
@@ -1245,6 +1283,75 @@ int regulator_is_enabled(struct regulato
 EXPORT_SYMBOL_GPL(regulator_is_enabled);
 
 /**
+ * regulator_count_voltages - count regulator_list_voltage() indices
+ * @regulator: regulator source
+ *
+ * Returns number of indices, or negative errno.
+ */
+int regulator_count_voltages(struct regulator *regulator)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_ops	*ops;
+	int			ret = -EINVAL;
+
+	mutex_lock(&rdev->mutex);
+
+	ops = rdev->desc->ops;
+	if (ops->count_voltages)
+		ret = ops->count_voltages(rdev);
+
+	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_count_voltages);
+
+/**
+ * regulator_list_voltage - enumerate supported voltages
+ * @regulator: regulator source
+ * @index: identify voltage to list
+ *
+ * Returns a voltage that can be passed to @regulator_set_voltage(),
+ * or negative fault code.
+ *
+ * Faults include passing in invalid index, and using an index
+ * corresponding to a voltage that can't be used on this system.
+ */
+int regulator_list_voltage(struct regulator *regulator, unsigned index)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_ops	*ops;
+	int			ret = -EINVAL;
+
+	mutex_lock(&rdev->mutex);
+
+	ops = rdev->desc->ops;
+	if (ops->count_voltages && ops->list_voltage)
+		ret = ops->count_voltages(rdev);
+
+	if (ret == 0)
+		ret = -EIO;
+	else if (ret > 0) {
+		if (index >= ret)
+			ret = -EDOM;
+		else
+			ret = ops->list_voltage(rdev, index);
+	}
+
+	if (ret >= 0) {
+		if (ret < rdev->constraints->min_uV)
+			ret = -ERANGE;
+		else if (ret > rdev->constraints->max_uV)
+			ret = -ERANGE;
+	}
+
+	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_list_voltage);
+
+/**
  * regulator_set_voltage - set regulator output voltage
  * @regulator: regulator source
  * @min_uV: Minimum required voltage in uV
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -140,6 +140,8 @@ int regulator_bulk_disable(int num_consu
 void regulator_bulk_free(int num_consumers,
 			 struct regulator_bulk_data *consumers);
 
+int regulator_count_voltages(struct regulator *regulator);
+int regulator_list_voltage(struct regulator *regulator, unsigned index);
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV);
 int regulator_get_voltage(struct regulator *regulator);
 int regulator_set_current_limit(struct regulator *regulator,
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -40,6 +40,12 @@ enum regulator_status {
  * @set_voltage: Set the voltage for the regulator within the range specified.
  *               The driver should select the voltage closest to min_uV.
  * @get_voltage: Return the currently configured voltage for the regulator.
+ * @count_voltages: Return the number of supported voltage indices which
+ *	may be passed to @list_voltage().  Some indices may correspond to
+ *	voltages that are not usable on this system.
+ * @list_voltage: Return one of the supported voltages, in microvolts;
+ *	or negative errno.  Indices range from zero to one less than
+ *	@count_voltages().  Voltages may be reported in any order.
  * @set_current_limit: Configure a limit for a current-limited regulator.
  * @get_current_limit: Get the configured limit for a current-limited regulator.
  * @set_mode: Set the operating mode for the regulator.
@@ -62,6 +68,10 @@ enum regulator_status {
  */
 struct regulator_ops {
 
+	/* enumerate supported voltages */
+	int (*count_voltages) (struct regulator_dev *);
+	int (*list_voltage) (struct regulator_dev *, unsigned index);
+
 	/* get/set regulator voltage */
 	int (*set_voltage) (struct regulator_dev *, int min_uV, int max_uV);
 	int (*get_voltage) (struct regulator_dev *);

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Mon, Feb 23, 2009 at 12:52:01PM -0800, David Brownell wrote:

> Use those methods to force machine-level constraints into bounds.
> (Example:  regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board
> constraints for that rail are 2.0V to 3.6V ... so the range of
> voltages is then 2.4V to 3.3V on this board.)

Being able to support this is definitely a win, thanks for looking at
this.

> +	/* maybe force machine-wide voltage constraints to match the
> +	 * voltages supported by this regulator.  use the regulator's
> +	 * entire range for boards with no particular constraints.
> +	 */

I'd really rather the second bit weren't here.  I'd like to see a
warning for the first part.

> + * @count_voltages: Return the number of supported voltage indices which
> + *	may be passed to @list_voltage().  Some indices may correspond to
> + *	voltages that are not usable on this system.
> + * @list_voltage: Return one of the supported voltages, in microvolts;
> + *	or negative errno.  Indices range from zero to one less than
> + *	@count_voltages().  Voltages may be reported in any order.

I'm having a hard time loving this interface for the driver.  It feels
fairly cumbersome to have to provide two functions to look up what I'd
expect to be static data - I'd be fairly surprised to see it change at
runtime.  I think I'd expect to see something more like the way ALSA
represents dB scales where the driver supplies a list of ranges that can
either be simple values or value ranges expressed as (start, step,
count).  That would cover both complicated cases like the TWL4030 and
the other common case with large regular ranges of voltages.

This mostly applies to the driver interface - on the consumer side it
feels a bit cumbersome to use but I can't think of anything that's
particularly better.  An interface that also allows consumers to ask if
particular ranges can be satisfied might help here - it'd be nice for
things like MMC that want to check for a relatively small set of
voltages or voltage ranges (which I'd expect is the common case).

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Tuesday 24 February 2009, Mark Brown wrote:
> On Mon, Feb 23, 2009 at 12:52:01PM -0800, David Brownell wrote:
> 
> > Use those methods to force machine-level constraints into bounds.
> > (Example:  regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board
> > constraints for that rail are 2.0V to 3.6V ... so the range of
> > voltages is then 2.4V to 3.3V on this board.)
> 
> Being able to support this is definitely a win, thanks for looking at
> this.

Likewise consumers being able to see what voltages are available...


> > +	/* maybe force machine-wide voltage constraints to match the
> > +	 * voltages supported by this regulator.  use the regulator's
> > +	 * entire range for boards with no particular constraints.
> > +	 */
> 
> I'd really rather the second bit weren't here.  I'd like to see a
> warning for the first part.

Fixable in an update.  I still think it's better to require less
manual configuration, not more ... manual configuration is by
definition error prone; requiring more manual configuration makes
systems be more fragile.  Which is why I wouldn't naturally want
to see a warning:  it implies manual configuration is desirable.


> > + * @count_voltages: Return the number of supported voltage indices which
> > + *	may be passed to @list_voltage().  Some indices may correspond to
> > + *	voltages that are not usable on this system.
> > + * @list_voltage: Return one of the supported voltages, in microvolts;
> > + *	or negative errno.  Indices range from zero to one less than
> > + *	@count_voltages().  Voltages may be reported in any order.
> 
> I'm having a hard time loving this interface for the driver.  It feels
> fairly cumbersome to have to provide two functions to look up what I'd
> expect to be static data - I'd be fairly surprised to see it change at
> runtime. 

It can be a function of configuration, and I didn't want to force
such seldom-used data into wasteful standardized-format tables.
Notice that with the twl4030 code, a functional interface takes
less space ... and is more flexible, since it copes with the
voltage options that are defined as "not supported".

Function accessors can use static const data when that's appropriate.
But going the other way around isn't very straightforward...

Consider also the TPS6235x regulators:  the voltages they support are
a simple linear function of an index 0..63, and that driver handles
seven such chips.  64 values * 4 bytes * 7 chips == about 2KB of
table data ... vs a few dozen bytes of function code.

Again, the tradeoff looks to me like a clear win:  this functional
interface is small, simple, clear, flexible.


> I think I'd expect to see something more like the way ALSA 
> represents dB scales where the driver supplies a list of ranges that can
> either be simple values or value ranges expressed as (start, step,
> count).  That would cover both complicated cases like the TWL4030 and
> the other common case with large regular ranges of voltages.

Another virtue of functional interfaces for enumeration is
they avoid the need for callers to see and handle a variety
of different models, like that!!

Do you really think it's easier to have to write code like

	if (regulator uses table model) {
		table iteration code {
			check(get(i))
		}
	else if (regulator uses start/step/count model) {
		start/step iteration {
			check(something)
		}
	else if ... another model-du-jour ... {
		...
	} else
		error

Instead of a one simple flexible model?

	limit = get_limit();
	for (i = 0; i < limit; i++)
		check(get(i));

 
> This mostly applies to the driver interface - on the consumer side it
> feels a bit cumbersome to use but I can't think of anything that's
> particularly better. 

There's a LONG history of functional iterator models, such as
the one I used.  I think they are clearly better, and don't
understand why you'd prefer that more cumbersome approach.


> An interface that also allows consumers to ask if 
> particular ranges can be satisfied might help here - it'd be nice for
> things like MMC that want to check for a relatively small set of
> voltages or voltage ranges (which I'd expect is the common case).

Umm, did you look at the MMC patch I sent?  It shows
how the $SUBJECT interface fits nicely with what the
MMC framework needs.  mmc_regulator_get_ocrmask() took
just thirty (ARM) instructions.

MMC doesn't want to "check" like that.  It wants to get
a mask of supported voltages, then AND it with the mask
reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to
pick a voltage supported by both host and card.

- Dave

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Tue, Feb 24, 2009 at 04:17:22PM -0800, David Brownell wrote:
> On Tuesday 24 February 2009, Mark Brown wrote:

> > > +	/* maybe force machine-wide voltage constraints to match the
> > > +	 * voltages supported by this regulator.  use the regulator's
> > > +	 * entire range for boards with no particular constraints.
> > > +	 */

> > I'd really rather the second bit weren't here.  I'd like to see a
> > warning for the first part.

> Fixable in an update.  I still think it's better to require less
> manual configuration, not more ... manual configuration is by
> definition error prone; requiring more manual configuration makes

This whole interface is structured around the idea that the consequences
of getting this wrong include things like lasting hardware damage.  This
hardware damage may not be immediately obvious but may develop over time
if components are kept running out of spec, either way it's not likely
to make users happy.

> systems be more fragile.  Which is why I wouldn't naturally want
> to see a warning:  it implies manual configuration is desirable.

If we weren't so reliant on the machine driver getting things right I'd
agree with you.  My thought here is that because there is room for human
error here it'd be good to use the information we do have to flag
potential problems to people, helping catch any mistakes that they make.

> > I'm having a hard time loving this interface for the driver.  It feels
> > fairly cumbersome to have to provide two functions to look up what I'd
> > expect to be static data - I'd be fairly surprised to see it change at
> > runtime. 

> It can be a function of configuration, and I didn't want to force
> such seldom-used data into wasteful standardized-format tables.
> Notice that with the twl4030 code, a functional interface takes
> less space ... and is more flexible, since it copes with the
> voltage options that are defined as "not supported".

Yeah, TWL4030 is pretty special here :/ .  The only gotcha I can see is
having to have a second table with only supported values in it for the
case where you're not allowing the out of spec values but TBH I don't
see that as a big deal.

> Consider also the TPS6235x regulators:  the voltages they support are
> a simple linear function of an index 0..63, and that driver handles
> seven such chips.  64 values * 4 bytes * 7 chips == about 2KB of
> table data ... vs a few dozen bytes of function code.

That's actually the most common case in the regulators I'd seen and is
why I'd suggested doing something like the ALSA dB scales which handle
this nicely - you don't need to actually have a table with all the
values, you just provide parmeters that expresses each sub range.  You
say things like

   DECLARE_TLV_DB_SCALE(mix_tlv, -1500, 300, 0);

As far as hardware requirements go I've seen regulators which provide:

 - A set of irregularly distributed values (usually fairly small).
 - A range of regularly distributed values.
 - A large range of values with several regular ranges in it (usually
   you get higher resolution at the low end).

Either way can be made to work for all of these, the concerns I have are
that the fact that it's a function based interface makes it look like
this might be dynamic data and that it's exposing a bit too much of the
implementation details (see below) which made that suggestion seem even
stronger.

[ALSA dB scale style]
> Another virtue of functional interfaces for enumeration is
> they avoid the need for callers to see and handle a variety
> of different models, like that!!

I'd expect the core to deal with unrolling the data rather than the
consumers, this is why...

> > This mostly applies to the driver interface - on the consumer side it
> > feels a bit cumbersome to use but I can't think of anything that's
> > particularly better. 

> There's a LONG history of functional iterator models, such as
> the one I used.  I think they are clearly better, and don't
> understand why you'd prefer that more cumbersome approach.

...I am, as I say, reasonably OK with it on the consumer side.

The only issue I have with it on the consumer side is that the invalid
slots in the array are visible to users since it feels icky to have
error conditions be part of the normal iteration process for what should
be well known constant data.  I worry that it's going to catch people
out since relatively few regulator drivers do that (the fact that it's
there is an implementation detail for drivers which have holes in the
range of register values they can set).

Thinking about it that could be hidden by mapping the invalid values to
zero or some value that is actually valid instead of returning an error
- not entirely nice but it keeps the pain away from the consumers.

> > An interface that also allows consumers to ask if 
> > particular ranges can be satisfied might help here - it'd be nice for
> > things like MMC that want to check for a relatively small set of
> > voltages or voltage ranges (which I'd expect is the common case).

> Umm, did you look at the MMC patch I sent?  It shows
> how the $SUBJECT interface fits nicely with what the
> MMC framework needs.  mmc_regulator_get_ocrmask() took
> just thirty (ARM) instructions.

> MMC doesn't want to "check" like that.  It wants to get
> a mask of supported voltages, then AND it with the mask
> reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to
> pick a voltage supported by both host and card.

You can either write the loop the way you have by iterating over the
voltages offered or you can write it by asking for voltage ranges that
the device might want.  It seems like it'd be useful for driver authors
if the core allowed either method so they can use whichever idiom fits
more comfortably with their needs.

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Wednesday 25 February 2009, Mark Brown wrote:
> On Tue, Feb 24, 2009 at 04:17:22PM -0800, David Brownell wrote:
> 
> > Fixable in an update.  I still think it's better to require less
> > manual configuration, not more ... manual configuration is by
> > definition error prone; requiring more manual configuration makes
> 
> This whole interface is structured around the idea that the consequences
> of getting this wrong include things like lasting hardware damage.  This
> hardware damage may not be immediately obvious but may develop over time
> if components are kept running out of spec, either way it's not likely
> to make users happy.

And as I noted:  *hardware* designers don't like to make
such goofage possible.  So that's not a common scenario.

I'll update the patch to be noisier about such situations,
since you insist.  And since braindamaged hardware designers
have actually been observed in the wild.  ;)


> > > 	 It feels
> > > fairly cumbersome to have to provide two functions to look up what I'd
> > > expect to be static data - I'd be fairly surprised to see it change at
> > > runtime. 
> 
> > It can be a function of configuration, and I didn't want to force
> > such seldom-used data into wasteful standardized-format tables.
> > Notice that with the twl4030 code, a functional interface takes
> > less space ... and is more flexible, since it copes with the
> > voltage options that are defined as "not supported".
> 
> Yeah, TWL4030 is pretty special here :/ .

Those "not supported" cases seem to be mostly fallout from using
four bit voltage selectors, and wanting future family members to
be able to add new "supported" cases.  A slightly more typical
model would be to have those bit values be "undefined".

Part of the space savings comes of course from tables being able
to use milliVolts, not microVolts ... two bytes for each entry
instead of four.  :)


> > Consider also the TPS6235x regulators:  the voltages they support are
> > a simple linear function of an index 0..63, and that driver handles
> > seven such chips.  64 values * 4 bytes * 7 chips == about 2KB of
> > table data ... vs a few dozen bytes of function code.
> 
> That's actually the most common case in the regulators I'd seen

Not for me.  I had seen two and three bit voltage selector fields,
defining fairly irregular sets of voltages.

I think the rationale has to do with getting better system-wide
efficiency, to stretch battery life.  Circuits generating the
reference voltages can be more efficient if they don't need to
be continually adjustable over some range(s).


> As far as hardware requirements go I've seen regulators which provide:
> 
>  - A set of irregularly distributed values (usually fairly small).
>  - A range of regularly distributed values.
>  - A large range of values with several regular ranges in it (usually
>    you get higher resolution at the low end).

All of which model nicely as a mapping { selector --> voltage }.

Hardware probably even has a register bitfield holding selector
values.  Maybe in that third case there's a second bitfield to
hold selector bits which specify the range.


> Either way can be made to work for all of these, the concerns I have are
> that the fact that it's a function based interface makes it look like
> this might be dynamic data and that it's exposing a bit too much of the
> implementation details (see below) which made that suggestion seem even
> stronger.

That still doesn't make sense to me.  It doesn't say a thing
about what it *is* ... just how to find the voltage associated
with a given index/selector.  

 
> [ALSA dB scale style]
> > Another virtue of functional interfaces for enumeration is
> > they avoid the need for callers to see and handle a variety
> > of different models, like that!!
> 
> I'd expect the core to deal with unrolling the data rather than the
> consumers, this is why...

I don't see why the core should "unroll" anything at all!
The regulator driver is already doing that for get_voltage:

	get_voltage() {
		read selector from hardware
		map selector to voltage
		return that voltage
	}

So it's trivial for similar code to take the selector as
a function parameter, and do the same thing.  Repackage
the existing code a bit; bzzt, done!


> > > This mostly applies to the driver interface - on the consumer side it
> > > feels a bit cumbersome to use but I can't think of anything that's
> > > particularly better. 
> 
> > There's a LONG history of functional iterator models, such as
> > the one I used.  I think they are clearly better, and don't
> > understand why you'd prefer that more cumbersome approach.
> 
> ...I am, as I say, reasonably OK with it on the consumer side.
> 
> The only issue I have with it on the consumer side is that the invalid
> slots in the array are visible to users since it feels icky to have
> error conditions be part of the normal iteration process for what should
> be well known constant data.

They are "fault" conditions (like "page fault"), not errors.  ;)


> I worry that it's going to catch people 
> out since relatively few regulator drivers do that (the fact that it's
> there is an implementation detail for drivers which have holes in the
> range of register values they can set).

It will be fairly common for the regulator to support voltages
that are disallowed by the machine constraints, though.  That
can produce "holes" too; and not necessarily only for the lowest
or highest selector codes.

IMO it's reasonable to expect users of a programming interface
to handle common cases like that.  :)

 
> Thinking about it that could be hidden by mapping the invalid values to
> zero or some value that is actually valid instead of returning an error
> - not entirely nice but it keeps the pain away from the consumers.

The test for an invalid voltage is "v <= 0" regardless.

I'll switch the return code for those out-of-range cases to
use zero, and update the spec for regulator_list_voltage()
to include that third outcome.


> > MMC doesn't want to "check" like that.  It wants to get
> > a mask of supported voltages, then AND it with the mask
> > reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to
> > pick a voltage supported by both host and card.
> 
> You can either write the loop the way you have by iterating over the
> voltages offered or you can write it by asking for voltage ranges that
> the device might want.

The MMC stack is written to work the way I described.
I don't see that model changing any time soon; most MMC
adapters don't seem to have a switchable Vdd rail, so
they've got a fixed mask reporting 3.2-3.4V support and
won't have any reason to use (or depend on) the regulator
framework.


True, *other* stacks might want something else:

> It seems like it'd be useful for driver authors 
> if the core allowed either method so they can use whichever idiom fits
> more comfortably with their needs.

A patch could be added later, when some system needs
some other model.  I'm not exactly sure what would be
returned by "asking for a voltage range".  That sounds
like it might be a different regulator capability model
than the "discrete voltage options" one.


My focus this time around was on exposing the regulator
capabilities to the core to support two simple use cases:
(a) validating machine constraints, (b) supporting MMC/SD
usage idiom, so the omap_hsmmc driver can fully decouple
from the twl4030-family regulators.

I'll send an updated version of this patch in a bit.

- Dave

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Wed, Feb 25, 2009 at 02:12:26PM -0800, David Brownell wrote:
> On Wednesday 25 February 2009, Mark Brown wrote:

> > This whole interface is structured around the idea that the consequences
> > of getting this wrong include things like lasting hardware damage.  This
> > hardware damage may not be immediately obvious but may develop over time
> > if components are kept running out of spec, either way it's not likely
> > to make users happy.

> And as I noted:  *hardware* designers don't like to make
> such goofage possible.  So that's not a common scenario.

That's often not possible with software controllable regulators - it's
usually hard to stop them being set to any value they support and the
desire for any additional protection needs to be traded off against
power consumption and space and BoM costs.

> Not for me.  I had seen two and three bit voltage selector fields,
> defining fairly irregular sets of voltages.

> I think the rationale has to do with getting better system-wide
> efficiency, to stretch battery life.  Circuits generating the
> reference voltages can be more efficient if they don't need to
> be continually adjustable over some range(s).

It's certainly not the common case for regulators that people are
contributing to the kernel (most of which seem to be intended to be
primary PMICs).  Make of that what you will :)

> > As far as hardware requirements go I've seen regulators which provide:

> >  - A set of irregularly distributed values (usually fairly small).
> >  - A range of regularly distributed values.
> >  - A large range of values with several regular ranges in it (usually
> >    you get higher resolution at the low end).

> All of which model nicely as a mapping { selector --> voltage }.

> Hardware probably even has a register bitfield holding selector
> values.  Maybe in that third case there's a second bitfield to
> hold selector bits which specify the range.

Yes, you can clearly always do selector->voltage since there's going to
be a finite number of register bits that it'll be possible to set.

> > Either way can be made to work for all of these, the concerns I have are
> > that the fact that it's a function based interface makes it look like
> > this might be dynamic data and that it's exposing a bit too much of the
> > implementation details (see below) which made that suggestion seem even
> > stronger.

> That still doesn't make sense to me.  It doesn't say a thing
> about what it *is* ... just how to find the voltage associated
> with a given index/selector.  

A function that return errors suggests something non-static to me.

> > I'd expect the core to deal with unrolling the data rather than the
> > consumers, this is why...

> I don't see why the core should "unroll" anything at all!
> The regulator driver is already doing that for get_voltage:

> 	get_voltage() {
> 		read selector from hardware
> 		map selector to voltage
> 		return that voltage
> 	}

> So it's trivial for similar code to take the selector as
> a function parameter, and do the same thing.  Repackage
> the existing code a bit; bzzt, done!

Yes, that's a reasonable point (though I'd still like to see the maximum
turn into a static value now I think about it).

> > I worry that it's going to catch people 
> > out since relatively few regulator drivers do that (the fact that it's
> > there is an implementation detail for drivers which have holes in the
> > range of register values they can set).

> It will be fairly common for the regulator to support voltages
> that are disallowed by the machine constraints, though.  That
> can produce "holes" too; and not necessarily only for the lowest
> or highest selector codes.

At present only continous ranges are possible, though.  I can't think of
any systems I've seen that'd want discontinous constraints, though I'm
sure there are some.

> > Thinking about it that could be hidden by mapping the invalid values to
> > zero or some value that is actually valid instead of returning an error
> > - not entirely nice but it keeps the pain away from the consumers.

> The test for an invalid voltage is "v <= 0" regardless.

If you're looking for a bound you'd just check for things within that
bound anyway.  It's if there's explicit "this is an error" return that
people start wanting to do something with it rather than silently ignore
it (we hope, anyway).

> > You can either write the loop the way you have by iterating over the
> > voltages offered or you can write it by asking for voltage ranges that
> > the device might want.

> The MMC stack is written to work the way I described.
...
> True, *other* stacks might want something else:

Indeed, I'm not talking about MMC in particular here - other things are
going to want to ask the same questions.

> > It seems like it'd be useful for driver authors 
> > if the core allowed either method so they can use whichever idiom fits
> > more comfortably with their needs.

> A patch could be added later, when some system needs
> some other model.  I'm not exactly sure what would be
> returned by "asking for a voltage range".  That sounds
> like it might be a different regulator capability model
> than the "discrete voltage options" one.

Yes, I'm suggesting something like:

  int regulator_available_voltage(min, max);

It's wrapping the discrete values for consumers that find that idiom
easier.

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Wednesday 25 February 2009, Mark Brown wrote:
> >       get_voltage() {
> >               read selector from hardware
> >               map selector to voltage
> >               return that voltage
> >       }
> 
> > So it's trivial for similar code to take the selector as
> > a function parameter, and do the same thing.  Repackage
> > the existing code a bit; bzzt, done!
> 
> Yes, that's a reasonable point (though I'd still like to see the maximum
> turn into a static value now I think about it).

At the regulator_desc level, that's trivial; I'll do that
in the patch you'll see.

In terms of the consumer interface, not -- "struct regulator"
is opaque to consumers, and everything is a functional accessor.
So I'll leave that as-is.


> > It will be fairly common for the regulator to support voltages
> > that are disallowed by the machine constraints, though.  That
> > can produce "holes" too; and not necessarily only for the lowest
> > or highest selector codes.
>
> At present only continous ranges are possible, though.  I can't think of
> any systems I've seen that'd want discontinous constraints, though I'm
> sure there are some.

Consider a regulator where voltage selectors 0..3 correspond to
voltages

	{ 3.3V, 1.8V, 4.2V, 5.0V }

With machine constraints that say voltages go from 3V to 4.5V ...

- Dave

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Wednesday 25 February 2009, Mark Brown wrote:
> >       get_voltage() {
> >               read selector from hardware
> >               map selector to voltage
> >               return that voltage
> >       }
> 
> > So it's trivial for similar code to take the selector as
> > a function parameter, and do the same thing.  Repackage
> > the existing code a bit; bzzt, done!
> 
> Yes, that's a reasonable point (though I'd still like to see the maximum
> turn into a static value now I think about it).

At the regulator_desc level, that's trivial; I'll do that
in the patch you'll see.

In terms of the consumer interface, not -- "struct regulator"
is opaque to consumers, and everything is a functional accessor.
So I'll leave that as-is.


> > It will be fairly common for the regulator to support voltages
> > that are disallowed by the machine constraints, though.  That
> > can produce "holes" too; and not necessarily only for the lowest
> > or highest selector codes.
>
> At present only continous ranges are possible, though.  I can't think of
> any systems I've seen that'd want discontinous constraints, though I'm
> sure there are some.

Consider a regulator where voltage selectors 0..3 correspond to
voltages

	{ 3.3V, 1.8V, 4.2V, 5.0V }

With machine constraints that say voltages go from 3V to 4.5V ...

- Dave
--
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: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Wed, Feb 25, 2009 at 03:47:52PM -0800, David Brownell wrote:
> On Wednesday 25 February 2009, Mark Brown wrote:

> In terms of the consumer interface, not -- "struct regulator"
> is opaque to consumers, and everything is a functional accessor.
> So I'll leave that as-is.

Yes, obviously.

> > At present only continous ranges are possible, though. ?I can't think of
> > any systems I've seen that'd want discontinous constraints, though I'm
> > sure there are some.

> Consider a regulator where voltage selectors 0..3 correspond to
> voltages

> 	{ 3.3V, 1.8V, 4.2V, 5.0V }

> With machine constraints that say voltages go from 3V to 4.5V ...

Continuous ranges of voltages, not continous indexes.  The indexes are
meaningless.  At the minute consumers and machines always supply
constraints as min,max pairs.

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Wednesday 25 February 2009, Mark Brown wrote:
> > Fixable in an update.  I still think it's better to require less
> > manual configuration, not more ... manual configuration is by
> > definition error prone; requiring more manual configuration makes
> 
> This whole interface is structured around the idea that the consequences
> of getting this wrong include things like lasting hardware damage.

Oh, one more comment.  Requiring manual configuration
of fixed-voltage regulators is pure time-wastage.

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Wednesday 25 February 2009, Mark Brown wrote:
> > Fixable in an update.  I still think it's better to require less
> > manual configuration, not more ... manual configuration is by
> > definition error prone; requiring more manual configuration makes
> 
> This whole interface is structured around the idea that the consequences
> of getting this wrong include things like lasting hardware damage.

Oh, one more comment.  Requiring manual configuration
of fixed-voltage regulators is pure time-wastage.

--
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: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Wed, Feb 25, 2009 at 05:02:03PM -0800, David Brownell wrote:

> Oh, one more comment.  Requiring manual configuration
> of fixed-voltage regulators is pure time-wastage.

Unless, of course, you happen to be using one of those consumers that
wants to know the voltage it's running at to configure itself.  Examples
I've seen include sensors and analogue components which can be
configured for better performance if they know the voltage they run at -
and yes, people do run these things off regulators that they vary at
runtime.

If you want to submit a patch making it optional that's fine.

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Thursday 26 February 2009, Mark Brown wrote:
> On Wed, Feb 25, 2009 at 05:02:03PM -0800, David Brownell wrote:
> 
> > Oh, one more comment.  Requiring manual configuration
> > of fixed-voltage regulators is pure time-wastage.
> 
> Unless, of course, you happen to be using one of those consumers that
> wants to know the voltage it's running at to configure itself.

In which case it can ask the regulator what voltage it's using.  :)

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Mark Brown]

On Thu, Feb 26, 2009 at 10:56:05AM -0800, David Brownell wrote:
> On Thursday 26 February 2009, Mark Brown wrote:

> > Unless, of course, you happen to be using one of those consumers that
> > wants to know the voltage it's running at to configure itself.

> In which case it can ask the regulator what voltage it's using.  :)

I suspect we're talking at cross purposes here - I was talking about the
fixed voltage regulator driver.  I suspect you were talking about
something else?

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Thursday 26 February 2009, Mark Brown wrote:
> On Thu, Feb 26, 2009 at 10:56:05AM -0800, David Brownell wrote:
> > On Thursday 26 February 2009, Mark Brown wrote:
> 
> > > Unless, of course, you happen to be using one of those consumers that
> > > wants to know the voltage it's running at to configure itself.
> 
> > In which case it can ask the regulator what voltage it's using.  :)
> 
> I suspect we're talking at cross purposes here - I was talking about the
> fixed voltage regulator driver.  I suspect you were talking about
> something else?

Yes ... e.g. the USB1V5, USB1V, and USB3V1 regulators
exported through the twl4030 regulator driver.


Semi-related:  someone with time to spend on it might
find and fix the bug causing the regulator framework
to oops when regulator/core.c::set_machine_constraints()
returns an error code.

- Dave

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[Liam Girdwood]

On Thu, 2009-02-26 at 11:38 -0800, David Brownell wrote:

> 
> Semi-related:  someone with time to spend on it might
> find and fix the bug causing the regulator framework
> to oops when regulator/core.c::set_machine_constraints()
> returns an error code.

Any chance you can post the oops. Is it happening within the
set_constraints() error path or sometime later on ?  

Thanks

Liam

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> > 
> > Semi-related:  someone with time to spend on it might
> > find and fix the bug causing the regulator framework
> > to oops when regulator/core.c::set_machine_constraints()
> > returns an error code.
> 
> Any chance you can post the oops. Is it happening within the
> set_constraints() error path or sometime later on ?  

Later, after it returns.  This particular oops went away
with the tweak eliminating the need for pointless init of
constraints for fixed voltage (1.8V in this case) regulators.

- Dave


<3>set_machine_constraints: invalid 'VUSB1V8' voltage constraints
<1>Unable to handle kernel NULL pointer dereference at virtual address 00000004
<1>pgd = c0004000
<1>[00000004] *pgd=00000000
Internal error: Oops: 805 [#1] PREEMPT
CPU: 0    Not tainted  (2.6.29-rc6-omap1 #517)
PC is at kobj_kset_leave+0x34/0x5c
LR is at _spin_lock+0x50/0x58
pc : [<c01748a0>]    lr : [<c0336e84>]    psr: 20000013
sp : c7821978  ip : c7821950  fp : c782198c
r10: c785f8a8  r9 : c7888a08  r8 : c78dade0
r7 : ffffffea  r6 : 00000000  r5 : c785f8a8  r4 : c785f908
r3 : 00000000  r2 : c7888a6c  r1 : c785f90c  r0 : c7815308
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387f  Table: 80004019  DAC: 00000017
Process swapper (pid: 1, stack limit = 0xc78202e8)
Stack: (0xc7821978 to 0xc7822000)
1960:                                                       c785f908 c785f8a8 
1980: 		... deletia ...
1fe0: 00000000 00000000 00000000 c7821ff8 c004d6c8 c0008668 8b77de58 01fbdd0a 
Backtrace: 
[<c017486c>] (kobj_kset_leave+0x0/0x5c) from [<c01748f4>] (kobject_del+0x2c/0x40)
 r5:c785f8a8 r4:c785f908
[<c01748c8>] (kobject_del+0x0/0x40) from [<c01a94fc>] (device_del+0x158/0x170)
 r5:c785f8a8 r4:c785f908
[<c01a93a4>] (device_del+0x0/0x170) from [<c01a9528>] (device_unregister+0x14/0x20)
 r7:ffffffea r6:c785f808 r5:c0437150 r4:c785f8a8
[<c01a9514>] (device_unregister+0x0/0x20) from [<c0188614>] (regulator_register+0x208/0x248)
 r5:c0437150 r4:c785f800
[<c018840c>] (regulator_register+0x0/0x248) from [<c0189894>] (twl4030reg_probe+0xa4/0x108)
[<c01897f0>] (twl4030reg_probe+0x0/0x108) from [<c01ac778>] (platform_drv_probe+0x20/0x24)
 r6:c04371b0 r5:c7888a08 r4:c04371b0
[<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148)
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c7888a68 r6:c7888a08 r5:c7888a08 r4:c04371b0
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14)
 r5:c01abab0 r4:00000000
[<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84)
[<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70)
 r6:00000000 r5:c7888aac r4:c7888a08
[<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64)
 r5:c7888a08 r4:c043b3f8
[<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284)
 r5:c7886820 r4:c7888a08
[<c01a96e8>] (device_add+0x0/0x284) from [<c01ace24>] (platform_device_add+0xf8/0x150)
 r7:c7888a8c r6:c7888a00 r5:00000000 r4:00000000
[<c01acd2c>] (platform_device_add+0x0/0x150) from [<c01b0714>] (add_numbered_child+0xe4/0x130)
 r7:00000120 r6:c7821bc0 r5:c7888a00 r4:00000000
[<c01b0630>] (add_numbered_child+0x0/0x130) from [<c01b07b0>] (add_regulator_linked+0x50/0x5c)
[<c01b0760>] (add_regulator_linked+0x0/0x5c) from [<c01b09f8>] (add_children+0x220/0x2d4)
[<c01b07d8>] (add_children+0x0/0x2d4) from [<c01b0eac>] (twl4030_probe+0x148/0x174)
 r8:c08010f4 r7:c041e848 r6:c7886200 r5:00000004 r4:00000178
[<c01b0d64>] (twl4030_probe+0x0/0x174) from [<c01cf6dc>] (i2c_device_probe+0x70/0x88)
[<c01cf66c>] (i2c_device_probe+0x0/0x88) from [<c01ab920>] (really_probe+0x9c/0x148)
 r7:c7886280 r6:c043b6f8 r5:c7886220 r4:c043b6f8
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c7886280 r6:c7886220 r5:c7886220 r4:c043b6f8
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14)
 r5:c01abab0 r4:00000000
[<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84)
[<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70)
 r6:00000000 r5:c78862c4 r4:c7886220
[<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64)
 r5:c7886220 r4:c043d558
[<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284)
 r5:c785ecf8 r4:c7886220
[<c01a96e8>] (device_add+0x0/0x284) from [<c01a9988>] (device_register+0x1c/0x20)
 r7:00000000 r6:c785ec50 r5:c7886200 r4:c7886220
[<c01a996c>] (device_register+0x0/0x20) from [<c01d069c>] (i2c_attach_client+0xc0/0x150)
 r5:c7886200 r4:c7886220
[<c01d05dc>] (i2c_attach_client+0x0/0x150) from [<c01d0d48>] (i2c_new_device+0x64/0x84)
 r7:c0420c90 r6:c785ec50 r5:c78064cc r4:c7886200
[<c01d0ce4>] (i2c_new_device+0x0/0x84) from [<c01d1388>] (i2c_scan_static_board_info+0x44/0x8c)
 r7:c0420c90 r6:00000000 r5:c785ec50 r4:c78064c0
[<c01d1344>] (i2c_scan_static_board_info+0x0/0x8c) from [<c01d145c>] (i2c_register_adapter+0x8c/0x144)
 r5:c785ec50 r4:00000000
[<c01d13d0>] (i2c_register_adapter+0x0/0x144) from [<c01d15b4>] (i2c_add_numbered_adapter+0xa0/0xb8)
 r5:c785ec50 r4:00000000
[<c01d1514>] (i2c_add_numbered_adapter+0x0/0xb8) from [<c0018c60>] (omap_i2c_probe+0x254/0x304)
 r5:c0420858 r4:c785ec00
[<c0018a0c>] (omap_i2c_probe+0x0/0x304) from [<c01ac778>] (platform_drv_probe+0x20/0x24)
 r8:00000001 r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8
[<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148)
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01aba8c>] (__driver_attach+0x68/0x8c)
 r5:c0420904 r4:c0420860
[<c01aba24>] (__driver_attach+0x0/0x8c) from [<c01ab170>] (bus_for_each_dev+0x4c/0x80)
 r7:c043b3f8 r6:c043d7c8 r5:c01aba24 r4:00000000
[<c01ab124>] (bus_for_each_dev+0x0/0x80) from [<c01ab79c>] (driver_attach+0x20/0x28)
 r6:c7887300 r5:00000000 r4:c043d7c8
[<c01ab77c>] (driver_attach+0x0/0x28) from [<c01aaaec>] (bus_add_driver+0xa4/0x170)
[<c01aaa48>] (bus_add_driver+0x0/0x170) from [<c01abd40>] (driver_register+0x98/0xcc)
 r7:c0018970 r6:00000000 r5:00000000 r4:c043d7c8
[<c01abca8>] (driver_register+0x0/0xcc) from [<c01accb0>] (platform_driver_register+0x6c/0x88)
 r5:00000000 r4:c0023fd8
[<c01acc44>] (platform_driver_register+0x0/0x88) from [<c0018984>] (omap_i2c_init_driver+0x14/0x1c)
[<c0018970>] (omap_i2c_init_driver+0x0/0x1c) from [<c002923c>] (__exception_text_end+0x5c/0x1a0)
[<c00291e0>] (__exception_text_end+0x0/0x1a0) from [<c000861c>] (do_initcalls+0x1c/0x38)
 r8:00000000 r7:00000000 r6:00000000 r5:00000000 r4:c0023fd8
[<c0008600>] (do_initcalls+0x0/0x38) from [<c0008658>] (do_basic_setup+0x20/0x24)
 r5:00000000 r4:c044b640
[<c0008638>] (do_basic_setup+0x0/0x24) from [<c00086b0>] (kernel_init+0x54/0xa8)
[<c000865c>] (kernel_init+0x0/0xa8) from [<c004d6c8>] (do_exit+0x0/0x254)
 r4:00000000
Code: e5942008 e5943004 e2841004 e5823000 (e5832004) 
<4>---[ end trace 1b75b31a2719ed1c ]---
<6>note: swapper[1] exited with preempt_count 2
<0>Kernel panic - not syncing: Attempted to kill init!

Re: [patch/rfc 2.6.29-rc6 1/2] regulator: enumerate voltages

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> > 
> > Semi-related:  someone with time to spend on it might
> > find and fix the bug causing the regulator framework
> > to oops when regulator/core.c::set_machine_constraints()
> > returns an error code.
> 
> Any chance you can post the oops. Is it happening within the
> set_constraints() error path or sometime later on ?  

Later, after it returns.  This particular oops went away
with the tweak eliminating the need for pointless init of
constraints for fixed voltage (1.8V in this case) regulators.

- Dave


<3>set_machine_constraints: invalid 'VUSB1V8' voltage constraints
<1>Unable to handle kernel NULL pointer dereference at virtual address 00000004
<1>pgd = c0004000
<1>[00000004] *pgd=00000000
Internal error: Oops: 805 [#1] PREEMPT
CPU: 0    Not tainted  (2.6.29-rc6-omap1 #517)
PC is at kobj_kset_leave+0x34/0x5c
LR is at _spin_lock+0x50/0x58
pc : [<c01748a0>]    lr : [<c0336e84>]    psr: 20000013
sp : c7821978  ip : c7821950  fp : c782198c
r10: c785f8a8  r9 : c7888a08  r8 : c78dade0
r7 : ffffffea  r6 : 00000000  r5 : c785f8a8  r4 : c785f908
r3 : 00000000  r2 : c7888a6c  r1 : c785f90c  r0 : c7815308
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387f  Table: 80004019  DAC: 00000017
Process swapper (pid: 1, stack limit = 0xc78202e8)
Stack: (0xc7821978 to 0xc7822000)
1960:                                                       c785f908 c785f8a8 
1980: 		... deletia ...
1fe0: 00000000 00000000 00000000 c7821ff8 c004d6c8 c0008668 8b77de58 01fbdd0a 
Backtrace: 
[<c017486c>] (kobj_kset_leave+0x0/0x5c) from [<c01748f4>] (kobject_del+0x2c/0x40)
 r5:c785f8a8 r4:c785f908
[<c01748c8>] (kobject_del+0x0/0x40) from [<c01a94fc>] (device_del+0x158/0x170)
 r5:c785f8a8 r4:c785f908
[<c01a93a4>] (device_del+0x0/0x170) from [<c01a9528>] (device_unregister+0x14/0x20)
 r7:ffffffea r6:c785f808 r5:c0437150 r4:c785f8a8
[<c01a9514>] (device_unregister+0x0/0x20) from [<c0188614>] (regulator_register+0x208/0x248)
 r5:c0437150 r4:c785f800
[<c018840c>] (regulator_register+0x0/0x248) from [<c0189894>] (twl4030reg_probe+0xa4/0x108)
[<c01897f0>] (twl4030reg_probe+0x0/0x108) from [<c01ac778>] (platform_drv_probe+0x20/0x24)
 r6:c04371b0 r5:c7888a08 r4:c04371b0
[<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148)
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c7888a68 r6:c7888a08 r5:c7888a08 r4:c04371b0
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14)
 r5:c01abab0 r4:00000000
[<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84)
[<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70)
 r6:00000000 r5:c7888aac r4:c7888a08
[<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64)
 r5:c7888a08 r4:c043b3f8
[<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284)
 r5:c7886820 r4:c7888a08
[<c01a96e8>] (device_add+0x0/0x284) from [<c01ace24>] (platform_device_add+0xf8/0x150)
 r7:c7888a8c r6:c7888a00 r5:00000000 r4:00000000
[<c01acd2c>] (platform_device_add+0x0/0x150) from [<c01b0714>] (add_numbered_child+0xe4/0x130)
 r7:00000120 r6:c7821bc0 r5:c7888a00 r4:00000000
[<c01b0630>] (add_numbered_child+0x0/0x130) from [<c01b07b0>] (add_regulator_linked+0x50/0x5c)
[<c01b0760>] (add_regulator_linked+0x0/0x5c) from [<c01b09f8>] (add_children+0x220/0x2d4)
[<c01b07d8>] (add_children+0x0/0x2d4) from [<c01b0eac>] (twl4030_probe+0x148/0x174)
 r8:c08010f4 r7:c041e848 r6:c7886200 r5:00000004 r4:00000178
[<c01b0d64>] (twl4030_probe+0x0/0x174) from [<c01cf6dc>] (i2c_device_probe+0x70/0x88)
[<c01cf66c>] (i2c_device_probe+0x0/0x88) from [<c01ab920>] (really_probe+0x9c/0x148)
 r7:c7886280 r6:c043b6f8 r5:c7886220 r4:c043b6f8
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c7886280 r6:c7886220 r5:c7886220 r4:c043b6f8
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14)
 r5:c01abab0 r4:00000000
[<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84)
[<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70)
 r6:00000000 r5:c78862c4 r4:c7886220
[<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64)
 r5:c7886220 r4:c043d558
[<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284)
 r5:c785ecf8 r4:c7886220
[<c01a96e8>] (device_add+0x0/0x284) from [<c01a9988>] (device_register+0x1c/0x20)
 r7:00000000 r6:c785ec50 r5:c7886200 r4:c7886220
[<c01a996c>] (device_register+0x0/0x20) from [<c01d069c>] (i2c_attach_client+0xc0/0x150)
 r5:c7886200 r4:c7886220
[<c01d05dc>] (i2c_attach_client+0x0/0x150) from [<c01d0d48>] (i2c_new_device+0x64/0x84)
 r7:c0420c90 r6:c785ec50 r5:c78064cc r4:c7886200
[<c01d0ce4>] (i2c_new_device+0x0/0x84) from [<c01d1388>] (i2c_scan_static_board_info+0x44/0x8c)
 r7:c0420c90 r6:00000000 r5:c785ec50 r4:c78064c0
[<c01d1344>] (i2c_scan_static_board_info+0x0/0x8c) from [<c01d145c>] (i2c_register_adapter+0x8c/0x144)
 r5:c785ec50 r4:00000000
[<c01d13d0>] (i2c_register_adapter+0x0/0x144) from [<c01d15b4>] (i2c_add_numbered_adapter+0xa0/0xb8)
 r5:c785ec50 r4:00000000
[<c01d1514>] (i2c_add_numbered_adapter+0x0/0xb8) from [<c0018c60>] (omap_i2c_probe+0x254/0x304)
 r5:c0420858 r4:c785ec00
[<c0018a0c>] (omap_i2c_probe+0x0/0x304) from [<c01ac778>] (platform_drv_probe+0x20/0x24)
 r8:00000001 r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8
[<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148)
[<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58)
 r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8
[<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01aba8c>] (__driver_attach+0x68/0x8c)
 r5:c0420904 r4:c0420860
[<c01aba24>] (__driver_attach+0x0/0x8c) from [<c01ab170>] (bus_for_each_dev+0x4c/0x80)
 r7:c043b3f8 r6:c043d7c8 r5:c01aba24 r4:00000000
[<c01ab124>] (bus_for_each_dev+0x0/0x80) from [<c01ab79c>] (driver_attach+0x20/0x28)
 r6:c7887300 r5:00000000 r4:c043d7c8
[<c01ab77c>] (driver_attach+0x0/0x28) from [<c01aaaec>] (bus_add_driver+0xa4/0x170)
[<c01aaa48>] (bus_add_driver+0x0/0x170) from [<c01abd40>] (driver_register+0x98/0xcc)
 r7:c0018970 r6:00000000 r5:00000000 r4:c043d7c8
[<c01abca8>] (driver_register+0x0/0xcc) from [<c01accb0>] (platform_driver_register+0x6c/0x88)
 r5:00000000 r4:c0023fd8
[<c01acc44>] (platform_driver_register+0x0/0x88) from [<c0018984>] (omap_i2c_init_driver+0x14/0x1c)
[<c0018970>] (omap_i2c_init_driver+0x0/0x1c) from [<c002923c>] (__exception_text_end+0x5c/0x1a0)
[<c00291e0>] (__exception_text_end+0x0/0x1a0) from [<c000861c>] (do_initcalls+0x1c/0x38)
 r8:00000000 r7:00000000 r6:00000000 r5:00000000 r4:c0023fd8
[<c0008600>] (do_initcalls+0x0/0x38) from [<c0008658>] (do_basic_setup+0x20/0x24)
 r5:00000000 r4:c044b640
[<c0008638>] (do_basic_setup+0x0/0x24) from [<c00086b0>] (kernel_init+0x54/0xa8)
[<c000865c>] (kernel_init+0x0/0xa8) from [<c004d6c8>] (do_exit+0x0/0x254)
 r4:00000000
Code: e5942008 e5943004 e2841004 e5823000 (e5832004) 
<4>---[ end trace 1b75b31a2719ed1c ]---
<6>note: swapper[1] exited with preempt_count 2
<0>Kernel panic - not syncing: Attempted to kill init!



--
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

[patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Add a basic mechanism for regulators to report the discrete
voltages they support:  list_voltage() enumerates them using
selectors numbered from 0 to an upper bound.

Use those methods to force machine-level constraints into bounds.
(Example:  regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board
constraints for that rail are 2.0V to 3.6V ... so the range of
voltages is then 2.4V to 3.3V on this board.)

Export those voltages to the regulator consumer interface, so for
example regulator hooked up to an MMC/SD/SDIO slot can report the
actual voltage options available to cards connected there.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Updates since previous version:  address feedback, simplify.

 drivers/regulator/core.c           |  113 +++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |    2 
 include/linux/regulator/driver.h   |    9 ++
 3 files changed, 124 insertions(+)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -719,6 +719,69 @@ static int set_machine_constraints(struc
 	else
 		name = "regulator";
 
+	/* constrain machine-level voltage specs to fit
+	 * the actual range supported by this regulator.
+	 */
+	if (ops->list_voltage && rdev->desc->n_voltages) {
+		int	count = rdev->desc->n_voltages;
+		int	i;
+		int	min_uV = INT_MAX;
+		int	max_uV = INT_MIN;
+		int	cmin = constraints->min_uV;
+		int	cmax = constraints->max_uV;
+
+		/* it's safe to autoconfigure fixed-voltage supplies */
+		if (count == 1 && !cmin) {
+			cmin = INT_MIN;
+			cmax = INT_MAX;
+		}
+
+		/* else require explicit machine-level constraints */
+		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
+			pr_err("%s: %s '%s' voltage constraints\n",
+				       __func__, "invalid", name);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* initial: [cmin..cmax] valid, [min_uV..max_uV] not */
+		for (i = 0; i < count; i++) {
+			int	value;
+
+			value = ops->list_voltage(rdev, i);
+			if (value <= 0)
+				continue;
+
+			/* maybe adjust [min_uV..max_uV] */
+			if (value >= cmin && value < min_uV)
+				min_uV = value;
+			if (value <= cmax && value > max_uV)
+				max_uV = value;
+		}
+
+		/* final: [min_uV..max_uV] valid iff constraints valid */
+		if (max_uV < min_uV) {
+			pr_err("%s: %s '%s' voltage constraints\n",
+				       __func__, "unsupportable", name);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* use regulator's subset of machine constraints */
+		if (constraints->min_uV < min_uV) {
+			pr_debug("%s: override '%s' %s, %d -> %d\n",
+				       __func__, name, "min_uV",
+					constraints->min_uV, min_uV);
+			constraints->min_uV = min_uV;
+		}
+		if (constraints->max_uV > max_uV) {
+			pr_debug("%s: override '%s' %s, %d -> %d\n",
+				       __func__, name, "max_uV",
+					constraints->max_uV, max_uV);
+			constraints->max_uV = max_uV;
+		}
+	}
+
 	rdev->constraints = constraints;
 
 	/* do we need to apply the constraint voltage */
@@ -1245,6 +1308,56 @@ int regulator_is_enabled(struct regulato
 EXPORT_SYMBOL_GPL(regulator_is_enabled);
 
 /**
+ * regulator_count_voltages - count regulator_list_voltage() selectors
+ * @regulator: regulator source
+ *
+ * Returns number of selectors, or negative errno.  Selectors are
+ * numbered starting at zero, and typically correspond to bitfields
+ * in hardware registers.
+ */
+int regulator_count_voltages(struct regulator *regulator)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+
+	return rdev->desc->n_voltages ? : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(regulator_count_voltages);
+
+/**
+ * regulator_list_voltage - enumerate supported voltages
+ * @regulator: regulator source
+ * @selector: identify voltage to list
+ * Context: can sleep
+ *
+ * Returns a voltage that can be passed to @regulator_set_voltage(),
+ * zero if this selector code can't be used on this sytem, or a
+ * negative errno.
+ */
+int regulator_list_voltage(struct regulator *regulator, unsigned selector)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_ops	*ops = rdev->desc->ops;
+	int			ret;
+
+	if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
+		return -EINVAL;
+
+	mutex_lock(&rdev->mutex);
+	ret = ops->list_voltage(rdev, selector);
+	mutex_unlock(&rdev->mutex);
+
+	if (ret > 0) {
+		if (ret < rdev->constraints->min_uV)
+			ret = 0;
+		else if (ret > rdev->constraints->max_uV)
+			ret = 0;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_list_voltage);
+
+/**
  * regulator_set_voltage - set regulator output voltage
  * @regulator: regulator source
  * @min_uV: Minimum required voltage in uV
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -140,6 +140,8 @@ int regulator_bulk_disable(int num_consu
 void regulator_bulk_free(int num_consumers,
 			 struct regulator_bulk_data *consumers);
 
+int regulator_count_voltages(struct regulator *regulator);
+int regulator_list_voltage(struct regulator *regulator, unsigned selector);
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV);
 int regulator_get_voltage(struct regulator *regulator);
 int regulator_set_current_limit(struct regulator *regulator,
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -40,6 +40,10 @@ enum regulator_status {
  * @set_voltage: Set the voltage for the regulator within the range specified.
  *               The driver should select the voltage closest to min_uV.
  * @get_voltage: Return the currently configured voltage for the regulator.
+ * @list_voltage: Return one of the supported voltages, in microvolts; zero
+ *	if the selector indicates a voltage that is unusable on this system;
+ *	or negative errno.  Selectors range from zero to one less than
+ *	regulator_desc.n_voltages.  Voltages may be reported in any order.
  * @set_current_limit: Configure a limit for a current-limited regulator.
  * @get_current_limit: Get the configured limit for a current-limited regulator.
  * @set_mode: Set the operating mode for the regulator.
@@ -62,6 +66,9 @@ enum regulator_status {
  */
 struct regulator_ops {
 
+	/* enumerate supported voltages */
+	int (*list_voltage) (struct regulator_dev *, unsigned selector);
+
 	/* get/set regulator voltage */
 	int (*set_voltage) (struct regulator_dev *, int min_uV, int max_uV);
 	int (*get_voltage) (struct regulator_dev *);
@@ -121,6 +128,7 @@ enum regulator_type {
  *
  * @name: Identifying name for the regulator.
  * @id: Numerical identifier for the regulator.
+ * @n_voltages: Number of selectors available for ops.list_voltage().
  * @ops: Regulator operations table.
  * @irq: Interrupt number for the regulator.
  * @type: Indicates if the regulator is a voltage or current regulator.
@@ -129,6 +137,7 @@ enum regulator_type {
 struct regulator_desc {
 	const char *name;
 	int id;
+	unsigned n_voltages;
 	struct regulator_ops *ops;
 	int irq;
 	enum regulator_type type;

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[Mark Brown]

On Thu, Feb 26, 2009 at 11:48:36AM -0800, David Brownell wrote:

> Updates since previous version:  address feedback, simplify.

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

This looks good to merge to me - coincidentally I've got a use case for
it lined up already.  Might it be worth merging the MMC client along
with this patch if the relevant maintainers are OK with that, could help
get it in faster?

Just two very minor points which might be nice to fix at some point:

> +			cmin = INT_MIN;
> +			cmax = INT_MAX;
> +		}
> +
> +		/* else require explicit machine-level constraints */
> +		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {

That indentation is going to catch some people out :)

> +		/* final: [min_uV..max_uV] valid iff constraints valid */
> +		if (max_uV < min_uV) {
> +			pr_err("%s: %s '%s' voltage constraints\n",
> +				       __func__, "unsupportable", name);
> +			ret = -EINVAL;

That style is going to hurt grepability for the error.

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[David Brownell]

On Thursday 26 February 2009, Mark Brown wrote:
> On Thu, Feb 26, 2009 at 11:48:36AM -0800, David Brownell wrote:
> 
> > Updates since previous version:  address feedback, simplify.
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> This looks good to merge to me - coincidentally I've got a use case for
> it lined up already.  Might it be worth merging the MMC client along
> with this patch if the relevant maintainers are OK with that, could help
> get it in faster?

You mean, that example MMC code I sent?  I think it's a bit
early to merge to mainline ... only the "generate ocr_mask"
call has really been verified.  I'll send the updated version
along though.

I had thought about sending that with patches to convert the
omap_hsmmc driver over to the regulator framework.  No skin
off my back if it goes with a different set of patches though.


> Just two very minor points which might be nice to fix at some point:
> 
> > +			cmin = INT_MIN;
> > +			cmax = INT_MAX;
> > +		}
> > +
> > +		/* else require explicit machine-level constraints */
> > +		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
> 
> That indentation is going to catch some people out :)

Maybe.


> > +		/* final: [min_uV..max_uV] valid iff constraints valid */
> > +		if (max_uV < min_uV) {
> > +			pr_err("%s: %s '%s' voltage constraints\n",
> > +				       __func__, "unsupportable", name);
> > +			ret = -EINVAL;
> 
> That style is going to hurt grepability for the error.

"grep unsupportable" ... :)

Sharing the primary string saves about three dozen bytes,
and I'm not keen on needless bloat.

[patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Add optional glue between MMC and regulator stacks, using a new
regulator interface to learn what voltages are available.

This is intended to be selected and driven by MMC host adapters.
It only handles reusable parts of the regulator-to-MMC glue; the
adapter drivers will have access to details that affect how this
is used.  Examples include when to use multiple voltage rails or
configure (internal or external) level shifters.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Changes from previous version:  adapter must select this, and
callers now pass in the regulator.  mmc_regulator_set_ocr()
is still not tested, mmc_regulator_get_ocrmask() passed sanity
testing.

Pierre:  Mark may have a need for this soonish.  The omap_hsmmc
code will want it at some point.

 drivers/mmc/core/Kconfig |    7 +++
 drivers/mmc/core/core.c  |   84 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    5 ++
 3 files changed, 96 insertions(+)

--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -14,3 +14,10 @@ config MMC_UNSAFE_RESUME
 	  This option is usually just for embedded systems which use
 	  a MMC/SD card for rootfs. Most people should say N here.
 
+config MMC_REGULATOR
+	bool
+	depends on REGULATOR
+	help
+	  Select this if your MMC host adapter driver wants helper
+	  utilities for accessing power rails.
+
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,89 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_MMC_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @host: mmc host whose supply will be consulted
+ * @supply: regulator to use
+ *
+ * This returns either a negative errno, or a mask of voltages that
+ * can be provided to MMC/SD/SDIO devices using the specified voltage
+ * regulator.  This would normally be called before registering the
+ * MMC host adapter.
+ */
+int mmc_regulator_get_ocrmask(struct mmc_host *host, struct regulator *supply)
+{
+	int			result = 0;
+	int			count;
+	int			i;
+
+	count = regulator_count_voltages(supply);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(supply, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @host: mmc host whose supply voltage will be changed
+ * @supply: regulator to use
+ *
+ * MMC host drivers may use this to enable or disable a regulator using
+ * a particular supply voltage.  This would normally be called from the
+ * set_ios() method.
+ */
+int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
+{
+	int			result = 0;
+	int			min_mV, max_mV;
+	int			enabled;
+
+	enabled = regulator_is_enabled(supply);
+	if (enabled < 0)
+		return enabled;
+
+	if (host->ios.vdd) {
+		int		tmp;
+
+		tmp = host->ios.vdd - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_mV = 1650;
+			max_mV = 1950;
+		} else {
+			min_mV = 2000 + tmp * 100;
+			max_mV = min_mV + 100;
+		}
+
+		result = regulator_set_voltage(supply,
+				min_mV * 1000, max_mV * 1000);
+		if (result == 0 && !enabled)
+			result = regulator_enable(supply);
+	} else if (enabled) {
+		result = regulator_disable(supply);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,10 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+struct regulator;
+
+int mmc_regulator_get_ocrmask(struct mmc_host *host, struct regulator *supply);
+int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply);
+
 #endif
 

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Pierre Ossman]

On Thu, 26 Feb 2009 13:48:30 -0800
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Add optional glue between MMC and regulator stacks, using a new
> regulator interface to learn what voltages are available.
> 
> This is intended to be selected and driven by MMC host adapters.
> It only handles reusable parts of the regulator-to-MMC glue; the
> adapter drivers will have access to details that affect how this
> is used.  Examples include when to use multiple voltage rails or
> configure (internal or external) level shifters.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> Changes from previous version:  adapter must select this, and
> callers now pass in the regulator.  mmc_regulator_set_ocr()
> is still not tested, mmc_regulator_get_ocrmask() passed sanity
> testing.
> 
> Pierre:  Mark may have a need for this soonish.  The omap_hsmmc
> code will want it at some point.
> 

I have no insight into the regulator stuff, so I'm going to have to
trust you on this. :)

Some nitpicking though:

> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -14,3 +14,10 @@ config MMC_UNSAFE_RESUME
>  	  This option is usually just for embedded systems which use
>  	  a MMC/SD card for rootfs. Most people should say N here.
>  
> +config MMC_REGULATOR
> +	bool
> +	depends on REGULATOR
> +	help
> +	  Select this if your MMC host adapter driver wants helper
> +	  utilities for accessing power rails.
> +

Is there a need for a special Kconfig for this? Can't we just build
these two whenever REGULATOR is defined? Or always, provided the
regulator API is present even when the code isn't.

> +/**
> + * mmc_regulator_set_ocr - set regulator to match host->ios voltage
> + * @host: mmc host whose supply voltage will be changed
> + * @supply: regulator to use
> + *
> + * MMC host drivers may use this to enable or disable a regulator using
> + * a particular supply voltage.  This would normally be called from the
> + * set_ios() method.
> + */
> +int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
> +{

Why not pass the vdd directly? Saves a few dereferences if nothing else.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

On Monday 02 March 2009, Pierre Ossman wrote:
> On Thu, 26 Feb 2009 13:48:30 -0800
> David Brownell <david-b@pacbell.net> wrote:
> 
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Add optional glue between MMC and regulator stacks, using a new
> > regulator interface to learn what voltages are available.
> > 
> > This is intended to be selected and driven by MMC host adapters.
> > It only handles reusable parts of the regulator-to-MMC glue; the
> > adapter drivers will have access to details that affect how this
> > is used.  Examples include when to use multiple voltage rails or
> > configure (internal or external) level shifters.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > ---
> > Changes from previous version:  adapter must select this, and
> > callers now pass in the regulator.  mmc_regulator_set_ocr()
> > is still not tested, mmc_regulator_get_ocrmask() passed sanity
> > testing.
> > 
> > Pierre:  Mark may have a need for this soonish.  The omap_hsmmc
> > code will want it at some point.
> > 
> 
> I have no insight into the regulator stuff, so I'm going to have to
> trust you on this. :)

Works for me.  ;)


> Some nitpicking though:
> 
> > --- a/drivers/mmc/core/Kconfig
> > +++ b/drivers/mmc/core/Kconfig
> > @@ -14,3 +14,10 @@ config MMC_UNSAFE_RESUME
> >  	  This option is usually just for embedded systems which use
> >  	  a MMC/SD card for rootfs. Most people should say N here.
> >  
> > +config MMC_REGULATOR
> > +	bool
> > +	depends on REGULATOR
> > +	help
> > +	  Select this if your MMC host adapter driver wants helper
> > +	  utilities for accessing power rails.
> > +
> 
> Is there a need for a special Kconfig for this? Can't we just build
> these two whenever REGULATOR is defined? Or always, provided the
> regulator API is present even when the code isn't.

The first patch had a "default y" there, nobody commented.
I'll simplify that, and use #ifdef CONFIG_REGULATOR instead.

 
> > +/**
> > + * mmc_regulator_set_ocr - set regulator to match host->ios voltage
> > + * @host: mmc host whose supply voltage will be changed
> > + * @supply: regulator to use
> > + *
> > + * MMC host drivers may use this to enable or disable a regulator using
> > + * a particular supply voltage.  This would normally be called from the
> > + * set_ios() method.
> > + */
> > +int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
> > +{
> 
> Why not pass the vdd directly? Saves a few dereferences if nothing else.

This call syntax is simpler, which is usually a win.
Passing a third parameter would create fault paths
of the "pass *wrong* parameter" flavor.

In terms of object code, when I've looked at such things
the dereferences generally cost the same as a ref to a
parameter, but passing an extra parameter isn't free.

- Dave

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Pierre Ossman]

On Mon, 2 Mar 2009 13:27:13 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Monday 02 March 2009, Pierre Ossman wrote:
> > On Thu, 26 Feb 2009 13:48:30 -0800
> > David Brownell <david-b@pacbell.net> wrote:
> > 
> > > + */
> > > +int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
> > > +{
> > 
> > Why not pass the vdd directly? Saves a few dereferences if nothing else.
> 
> This call syntax is simpler, which is usually a win.
> Passing a third parameter would create fault paths
> of the "pass *wrong* parameter" flavor.
> 
> In terms of object code, when I've looked at such things
> the dereferences generally cost the same as a ref to a
> parameter, but passing an extra parameter isn't free.
> 

I couldn't see host being used in there, so I was thinking more of a
replacement, not an addition.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

On Monday 02 March 2009, Pierre Ossman wrote:
> > > > +int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
> > > > +{
> > > 
> > > Why not pass the vdd directly? Saves a few dereferences if nothing else.
> > 
> > This call syntax is simpler, which is usually a win.
> > Passing a third parameter would create fault paths
> > of the "pass *wrong* parameter" flavor.
> > 
> > In terms of object code, when I've looked at such things
> > the dereferences generally cost the same as a ref to a
> > parameter, but passing an extra parameter isn't free.
> > 
> 
> I couldn't see host being used in there, so I was thinking more of a
> replacement, not an addition.

Oh, I see.  That'd make sense.  Just pass host->ios.vdd.

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Glue between MMC and regulator stacks ... compiles, and
mmc_regulator_get_ocrmask() passed sanity testing.

These calls are intended to be used by MMC host adapters
using at least one regulator per host.  Examples include
slots with regulators supporting multiple voltages and
ones using multiple voltage rails (e.g. DAT4..DAT7 using a
separate supply, or a split rail chip like certain SDIO
WLAN or eMMC solutions).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Changes from previous version:  automagically available if the
regulator calls are available; callers now pass in host->ios.vdd.
mmc_regulator_set_ocr() still untested.

 drivers/mmc/core/core.c  |   86 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    5 ++
 2 files changed, 91 insertions(+)

--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,91 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @host: mmc host whose supply will be consulted
+ * @supply: regulator to use
+ *
+ * This returns either a negative errno, or a mask of voltages that
+ * can be provided to MMC/SD/SDIO devices using the specified voltage
+ * regulator.  This would normally be called before registering the
+ * MMC host adapter.
+ */
+int mmc_regulator_get_ocrmask(struct mmc_host *host, struct regulator *supply)
+{
+	int			result = 0;
+	int			count;
+	int			i;
+
+	count = regulator_count_voltages(supply);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(supply, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
+ * @supply: regulator to use
+ *
+ * Returns zero on success, else negative errno.
+ *
+ * MMC host drivers may use this to enable or disable a regulator using
+ * a particular supply voltage.  This would normally be called from the
+ * set_ios() method.
+ */
+int mmc_regulator_set_ocr(unsigned short vdd_bit, struct regulator *supply)
+{
+	int			result = 0;
+	int			min_mV, max_mV;
+	int			enabled;
+
+	enabled = regulator_is_enabled(supply);
+	if (enabled < 0)
+		return enabled;
+
+	if (vdd_bit) {
+		int		tmp;
+
+		tmp = vdd_bit - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_mV = 1650;
+			max_mV = 1950;
+		} else {
+			min_mV = 2000 + tmp * 100;
+			max_mV = min_mV + 100;
+		}
+
+		result = regulator_set_voltage(supply,
+				min_mV * 1000, max_mV * 1000);
+		if (result == 0 && !enabled)
+			result = regulator_enable(supply);
+	} else if (enabled) {
+		result = regulator_disable(supply);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,10 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+struct regulator;
+
+int mmc_regulator_get_ocrmask(struct mmc_host *host, struct regulator *supply);
+int mmc_regulator_set_ocr(unsigned short vdd_bit, struct regulator *supply);
+
 #endif
 

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Pierre Ossman]

On Tue, 3 Mar 2009 19:18:06 -0800
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Glue between MMC and regulator stacks ... compiles, and
> mmc_regulator_get_ocrmask() passed sanity testing.
> 
> These calls are intended to be used by MMC host adapters
> using at least one regulator per host.  Examples include
> slots with regulators supporting multiple voltages and
> ones using multiple voltage rails (e.g. DAT4..DAT7 using a
> separate supply, or a split rail chip like certain SDIO
> WLAN or eMMC solutions).
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> Changes from previous version:  automagically available if the
> regulator calls are available; callers now pass in host->ios.vdd.
> mmc_regulator_set_ocr() still untested.
> 

Looks good. Now what should I do with it? Merge it in the next window
good enough?

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

On Sunday 08 March 2009, Pierre Ossman wrote:
> Looks good. Now what should I do with it? Merge it in the next window
> good enough?

Well, after testing the previously-untested call ... here's
another update.

Since these depend on new calls in the regulator framework,
it can't merge until after they merge (in the next window).
Least hassle for you would be if this merges through the
regulator framework (with your ack), I suspect.

- Dave


======== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

Glue between MMC and regulator stacks ... verified with
some OMAP3 boards using adjustable and configured-as-fixed
regulators on several MMC controllers.

These calls are intended to be used by MMC host adapters
using at least one regulator per host.  Examples include
slots with regulators supporting multiple voltages and
ones using multiple voltage rails (e.g. DAT4..DAT7 using a
separate supply, or a split rail chip like certain SDIO
WLAN or eMMC solutions).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Changes since previous version:  simplified set_ocr() calling
convention, fixed an off-by-100mA error in that code, and don't
set voltage on regulators that don't need (and may disallow) it.

 drivers/mmc/core/core.c  |  100 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    5 ++
 2 files changed, 105 insertions(+)

--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,105 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @supply: regulator to use
+ *
+ * This returns either a negative errno, or a mask of voltages that
+ * can be provided to MMC/SD/SDIO devices using the specified voltage
+ * regulator.  This would normally be called before registering the
+ * MMC host adapter.
+ */
+int mmc_regulator_get_ocrmask(struct regulator *supply)
+{
+	int			result = 0;
+	int			count;
+	int			i;
+
+	count = regulator_count_voltages(supply);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(supply, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
+ * @supply: regulator to use
+ *
+ * Returns zero on success, else negative errno.
+ *
+ * MMC host drivers may use this to enable or disable a regulator using
+ * a particular supply voltage.  This would normally be called from the
+ * set_ios() method.
+ */
+int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
+{
+	int			result = 0;
+	int			min_uV, max_uV;
+	int			enabled;
+
+	enabled = regulator_is_enabled(supply);
+	if (enabled < 0)
+		return enabled;
+
+	if (vdd_bit) {
+		int		tmp;
+		int		voltage;
+
+		/* REVISIT mmc_vddrange_to_ocrmask() may have set some
+		 * bits this regulator doesn't quite support ... don't
+		 * be too picky, most cards and regulators are OK with
+		 * a 0.1V range goof (it's a small error percentage).
+		 */
+		tmp = vdd_bit - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_uV = 1650 * 1000;
+			max_uV = 1950 * 1000;
+		} else {
+			min_uV = 1900 * 1000 + tmp * 100 * 1000;
+			max_uV = min_uV + 100 * 1000;
+		}
+
+		/* avoid needless changes to this voltage; the regulator
+		 * might not allow this operation
+		 */
+		voltage = regulator_get_voltage(supply);
+		if (voltage < 0)
+			result = voltage;
+		else if (voltage < min_uV || voltage > max_uV)
+			result = regulator_set_voltage(supply, min_uV, max_uV);
+		else
+			result = 0;
+
+		if (result == 0 && !enabled)
+			result = regulator_enable(supply);
+	} else if (enabled) {
+		result = regulator_disable(supply);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,10 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+struct regulator;
+
+int mmc_regulator_get_ocrmask(struct regulator *supply);
+int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit);
+
 #endif
 

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Pierre Ossman]

On Sun, 8 Mar 2009 12:34:25 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Sunday 08 March 2009, Pierre Ossman wrote:
> > Looks good. Now what should I do with it? Merge it in the next window
> > good enough?
> 
> Well, after testing the previously-untested call ... here's
> another update.
> 
> Since these depend on new calls in the regulator framework,
> it can't merge until after they merge (in the next window).
> Least hassle for you would be if this merges through the
> regulator framework (with your ack), I suspect.
> 
> - Dave
> 
> 
> ======== CUT HERE
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Glue between MMC and regulator stacks ... verified with
> some OMAP3 boards using adjustable and configured-as-fixed
> regulators on several MMC controllers.
> 
> These calls are intended to be used by MMC host adapters
> using at least one regulator per host.  Examples include
> slots with regulators supporting multiple voltages and
> ones using multiple voltage rails (e.g. DAT4..DAT7 using a
> separate supply, or a split rail chip like certain SDIO
> WLAN or eMMC solutions).
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---

Acked-by: Pierre Ossman <drzeus@drzeus.cx>

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Liam Girdwood]

On Sun, 2009-03-08 at 22:49 +0100, Pierre Ossman wrote:
> On Sun, 8 Mar 2009 12:34:25 -0800
> David Brownell <david-b@pacbell.net> wrote:
> 
> > On Sunday 08 March 2009, Pierre Ossman wrote:
> > > Looks good. Now what should I do with it? Merge it in the next window
> > > good enough?
> > 
> > Well, after testing the previously-untested call ... here's
> > another update.
> > 
> > Since these depend on new calls in the regulator framework,
> > it can't merge until after they merge (in the next window).
> > Least hassle for you would be if this merges through the
> > regulator framework (with your ack), I suspect.
> > 
> > - Dave
> > 
> > 
> > ======== CUT HERE
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Glue between MMC and regulator stacks ... verified with
> > some OMAP3 boards using adjustable and configured-as-fixed
> > regulators on several MMC controllers.
> > 
> > These calls are intended to be used by MMC host adapters
> > using at least one regulator per host.  Examples include
> > slots with regulators supporting multiple voltages and
> > ones using multiple voltage rails (e.g. DAT4..DAT7 using a
> > separate supply, or a split rail chip like certain SDIO
> > WLAN or eMMC solutions).
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > ---
> 
> Acked-by: Pierre Ossman <drzeus@drzeus.cx>
> 

Applied.

Thanks

Liam

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell]

On Monday 09 March 2009, Liam Girdwood wrote:
> > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > 
> > > Glue between MMC and regulator stacks ... verified with
> > > some OMAP3 boards using adjustable and configured-as-fixed
> > > regulators on several MMC controllers.
> > > 
> > > ...
> > 
> > Acked-by: Pierre Ossman <drzeus@drzeus.cx>
> > 
> 
> Applied.

... actually you applied an old version, not the one that
was verified, with mmc_regulator_set_ocr() fixes and with
Pierre's ACK.  The comment is in your GIT tree is wrong,
for starters...

Please use this one instead.

- Dave

========== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

Glue between MMC and regulator stacks ... verified with
some OMAP3 boards using adjustable and configured-as-fixed
regulators on several MMC controllers.

These calls are intended to be used by MMC host adapters
using at least one regulator per host.  Examples include
slots with regulators supporting multiple voltages and
ones using multiple voltage rails (e.g. DAT4..DAT7 using a
separate supply, or a split rail chip like certain SDIO
WLAN or eMMC solutions).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Pierre Ossman <drzeus@drzeus.cx>
---
Changes since previous version:  simplified set_ocr() calling
convention, fixed an off-by-100mA error in that code, and don't
set voltage on regulators that don't need (and may disallow) it.

 drivers/mmc/core/core.c  |  100 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    5 ++
 2 files changed, 105 insertions(+)

--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,105 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @supply: regulator to use
+ *
+ * This returns either a negative errno, or a mask of voltages that
+ * can be provided to MMC/SD/SDIO devices using the specified voltage
+ * regulator.  This would normally be called before registering the
+ * MMC host adapter.
+ */
+int mmc_regulator_get_ocrmask(struct regulator *supply)
+{
+	int			result = 0;
+	int			count;
+	int			i;
+
+	count = regulator_count_voltages(supply);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(supply, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
+ * @supply: regulator to use
+ *
+ * Returns zero on success, else negative errno.
+ *
+ * MMC host drivers may use this to enable or disable a regulator using
+ * a particular supply voltage.  This would normally be called from the
+ * set_ios() method.
+ */
+int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
+{
+	int			result = 0;
+	int			min_uV, max_uV;
+	int			enabled;
+
+	enabled = regulator_is_enabled(supply);
+	if (enabled < 0)
+		return enabled;
+
+	if (vdd_bit) {
+		int		tmp;
+		int		voltage;
+
+		/* REVISIT mmc_vddrange_to_ocrmask() may have set some
+		 * bits this regulator doesn't quite support ... don't
+		 * be too picky, most cards and regulators are OK with
+		 * a 0.1V range goof (it's a small error percentage).
+		 */
+		tmp = vdd_bit - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_uV = 1650 * 1000;
+			max_uV = 1950 * 1000;
+		} else {
+			min_uV = 1900 * 1000 + tmp * 100 * 1000;
+			max_uV = min_uV + 100 * 1000;
+		}
+
+		/* avoid needless changes to this voltage; the regulator
+		 * might not allow this operation
+		 */
+		voltage = regulator_get_voltage(supply);
+		if (voltage < 0)
+			result = voltage;
+		else if (voltage < min_uV || voltage > max_uV)
+			result = regulator_set_voltage(supply, min_uV, max_uV);
+		else
+			result = 0;
+
+		if (result == 0 && !enabled)
+			result = regulator_enable(supply);
+	} else if (enabled) {
+		result = regulator_disable(supply);
+	}
+
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,10 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+struct regulator;
+
+int mmc_regulator_get_ocrmask(struct regulator *supply);
+int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit);
+
 #endif
 

Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[Liam Girdwood]

On Wed, 2009-03-11 at 03:30 -0800, David Brownell wrote:
> On Monday 09 March 2009, Liam Girdwood wrote:
> > > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > > 
> > > > Glue between MMC and regulator stacks ... verified with
> > > > some OMAP3 boards using adjustable and configured-as-fixed
> > > > regulators on several MMC controllers.
> > > > 
> > > > ...
> > > 
> > > Acked-by: Pierre Ossman <drzeus@drzeus.cx>
> > > 
> > 
> > Applied.
> 
> ... actually you applied an old version, not the one that
> was verified, with mmc_regulator_set_ocr() fixes and with
> Pierre's ACK.  The comment is in your GIT tree is wrong,
> for starters...
> 
> Please use this one instead.

I've removed the old patch now and applied the correct one. In the
future could you start a new thread or change the thread subject a
little (i.e. patch V2) when a patch changes during discussion. This
should avoid similar confusion in the future.

Thanks

Liam

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[Liam Girdwood]

On Thu, 2009-02-26 at 11:48 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Add a basic mechanism for regulators to report the discrete
> voltages they support:  list_voltage() enumerates them using
> selectors numbered from 0 to an upper bound.
> 
> Use those methods to force machine-level constraints into bounds.
> (Example:  regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board
> constraints for that rail are 2.0V to 3.6V ... so the range of
> voltages is then 2.4V to 3.3V on this board.)
> 
> Export those voltages to the regulator consumer interface, so for
> example regulator hooked up to an MMC/SD/SDIO slot can report the
> actual voltage options available to cards connected there.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---

Applied with git-am merge conflicts. It builds ok, can you check against
your tree.

Thanks

Liam

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> Applied with git-am merge conflicts. It builds ok, can you check against
> your tree.

What were the conflicts -- just offsets?

Your "-next" regulator tree seems to be missing a doc patch
you had asked for, maybe that's an issue.

- Dave

=========== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>
Subject: regulator: get_status() grows kerneldoc

Add kerneldoc for the new get_status() message.  Fix the existing
kerneldoc for that struct in two ways:

 (a) Syntax, making sure parameter descriptions immediately
     follow the one-line struct description and that the first
     blank lines is before any more expansive description;
 (b) Presentation for a few points, to highlight the fact that
     the previous "get" methods exist only to report the current
     configuration, not to display actual status.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 include/linux/regulator/driver.h |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -34,26 +34,20 @@ enum regulator_status {
 
 /**
  * struct regulator_ops - regulator operations.
- *
- * This struct describes regulator operations which can be implemented by
- * regulator chip drivers.
- *
- * @enable: Enable the regulator.
- * @disable: Disable the regulator.
+ * @enable: Configure the regulator as enabled.
+ * @disable: Configure the regulator as disabled.
  * @is_enabled: Return 1 if the regulator is enabled, 0 otherwise.
- *
  * @set_voltage: Set the voltage for the regulator within the range specified.
  *               The driver should select the voltage closest to min_uV.
  * @get_voltage: Return the currently configured voltage for the regulator.
- *
  * @set_current_limit: Configure a limit for a current-limited regulator.
- * @get_current_limit: Get the limit for a current-limited regulator.
- *
+ * @get_current_limit: Get the configured limit for a current-limited regulator.
  * @set_mode: Set the operating mode for the regulator.
- * @get_mode: Get the current operating mode for the regulator.
+ * @get_mode: Get the configured operating mode for the regulator.
+ * @get_status: Return actual (not as-configured) status of regulator, as a
+ *	REGULATOR_STATUS value (or negative errno)
  * @get_optimum_mode: Get the most efficient operating mode for the regulator
  *                    when running with the specified parameters.
- *
  * @set_suspend_voltage: Set the voltage for the regulator when the system
  *                       is suspended.
  * @set_suspend_enable: Mark the regulator as enabled when the system is
@@ -62,6 +56,9 @@ enum regulator_status {
  *                       suspended.
  * @set_suspend_mode: Set the operating mode for the regulator when the
  *                    system is suspended.
+ *
+ * This struct describes regulator operations which can be implemented by
+ * regulator chip drivers.
  */
 struct regulator_ops {
 
@@ -86,6 +83,7 @@ struct regulator_ops {
 	/* report regulator status ... most other accessors report
 	 * control inputs, this reports results of combining inputs
 	 * from Linux (and other sources) with the actual load.
+	 * returns REGULATOR_STATUS_* or negative errno.
 	 */
 	int (*get_status)(struct regulator_dev *);
 

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[Liam Girdwood]

On Thu, 2009-02-26 at 13:28 -0800, David Brownell wrote:
> On Thursday 26 February 2009, Liam Girdwood wrote:
> > Applied with git-am merge conflicts. It builds ok, can you check against
> > your tree.
> 
> What were the conflicts -- just offsets?
> 
> Your "-next" regulator tree seems to be missing a doc patch
> you had asked for, maybe that's an issue.
> 
> - Dave
> 
> =========== CUT HERE
> From: David Brownell <dbrownell@users.sourceforge.net>
> Subject: regulator: get_status() grows kerneldoc
> 
> Add kerneldoc for the new get_status() message.  Fix the existing
> kerneldoc for that struct in two ways:
> 
>  (a) Syntax, making sure parameter descriptions immediately
>      follow the one-line struct description and that the first
>      blank lines is before any more expansive description;
>  (b) Presentation for a few points, to highlight the fact that
>      the previous "get" methods exist only to report the current
>      configuration, not to display actual status.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  include/linux/regulator/driver.h |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

Thanks.

Fixed.

Liam

Re: [patch 2.6.29-rc6 1/2] regulator: enumerate voltages (v2)

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> 
> > What were the conflicts -- just offsets?
> > 
> > Your "-next" regulator tree seems to be missing a doc patch
> > you had asked for, maybe that's an issue.

And you seem to only have merged the header part of
the $SUBJECT patch, not the core.c parts ...

Everything gets kind of broken without the core.c
change!

- Dave

[patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Update previously-posted twl4030 regulator driver to export
supported voltages to upper layers using a new mechanism.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/twl4030-regulator.c |   72 ++++++++++++++------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

--- a/drivers/regulator/twl4030-regulator.c
+++ b/drivers/regulator/twl4030-regulator.c
@@ -42,7 +42,6 @@ struct twlreg_info {
 
 	/* chip constraints on regulator behavior */
 	u16			min_mV;
-	u16			max_mV;
 
 	/* used by regulator core */
 	struct regulator_desc	desc;
@@ -262,6 +261,24 @@ static const u16 VDAC_VSEL_table[] = {
 };
 
 
+static int twl4030ldo_count_voltages(struct regulator_dev *rdev)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+
+	return info->table_len ? : 1;
+}
+
+static int twl4030ldo_list_voltage(struct regulator_dev *rdev, unsigned index)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			mV;
+
+	mV = info->table[index];
+	if (IS_UNSUP(mV))
+		return -ESRCH;
+	return LDO_MV(mV) * 1000;
+}
+
 static int
 twl4030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV)
 {
@@ -276,6 +293,8 @@ twl4030ldo_set_voltage(struct regulator_
 			continue;
 		uV = LDO_MV(mV) * 1000;
 
+		/* REVISIT for VAUX2, first match may not be best/lowest */
+
 		/* use the first in-range value */
 		if (min_uV <= uV && uV <= max_uV)
 			return twl4030reg_write(info, VREG_DEDICATED, vsel);
@@ -297,6 +316,9 @@ static int twl4030ldo_get_voltage(struct
 }
 
 static struct regulator_ops twl4030ldo_ops = {
+	.count_voltages	= twl4030ldo_count_voltages,
+	.list_voltage	= twl4030ldo_list_voltage,
+
 	.set_voltage	= twl4030ldo_set_voltage,
 	.get_voltage	= twl4030ldo_get_voltage,
 
@@ -314,6 +336,13 @@ static struct regulator_ops twl4030ldo_o
 /*
  * Fixed voltage LDOs don't have a VSEL field to update.
  */
+static int twl4030fixed_list_voltage(struct regulator_dev *rdev, unsigned index)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+
+	return info->min_mV * 1000;
+}
+
 static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -322,6 +351,9 @@ static int twl4030fixed_get_voltage(stru
 }
 
 static struct regulator_ops twl4030fixed_ops = {
+	.count_voltages	= twl4030ldo_count_voltages,
+	.list_voltage	= twl4030fixed_list_voltage,
+
 	.get_voltage	= twl4030fixed_get_voltage,
 
 	.enable		= twl4030reg_enable,
@@ -353,7 +385,6 @@ static struct regulator_ops twl4030fixed
 	.base = offset, \
 	.id = num, \
 	.min_mV = mVolts, \
-	.max_mV = mVolts, \
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
@@ -402,14 +433,11 @@ static int twl4030reg_probe(struct platf
 	struct regulator_init_data	*initdata;
 	struct regulation_constraints	*c;
 	struct regulator_dev		*rdev;
-	int				min_uV, max_uV;
 
 	for (i = 0, info = NULL; i < ARRAY_SIZE(twl4030_regs); i++) {
 		if (twl4030_regs[i].desc.id != pdev->id)
 			continue;
 		info = twl4030_regs + i;
-		min_uV = info->min_mV * 1000;
-		max_uV = info->max_mV * 1000;
 		break;
 	}
 	if (!info)
@@ -423,10 +451,6 @@ static int twl4030reg_probe(struct platf
 	 * this driver and the chip itself can actually do.
 	 */
 	c = &initdata->constraints;
-	if (!c->min_uV || c->min_uV < min_uV)
-		c->min_uV = min_uV;
-	if (!c->max_uV || c->max_uV > max_uV)
-		c->max_uV = max_uV;
 	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
 	c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE
 				| REGULATOR_CHANGE_MODE
@@ -471,36 +495,6 @@ static struct platform_driver twl4030reg
 
 static int __init twl4030reg_init(void)
 {
-	unsigned i, j;
-
-	/* determine min/max voltage constraints, taking into account
-	 * whether set_voltage() will use the "unsupported" settings
-	 */
-	for (i = 0; i < ARRAY_SIZE(twl4030_regs); i++) {
-		struct twlreg_info	*info = twl4030_regs + i;
-		const u16		*table;
-
-		/* fixed-voltage regulators */
-		if (!info->table_len)
-			continue;
-
-		/* LDO regulators: */
-		for (j = 0, table = info->table;
-				j < info->table_len;
-				j++, table++) {
-			u16		mV = *table;
-
-			if (IS_UNSUP(mV))
-				continue;
-			mV = LDO_MV(mV);
-
-			if (info->min_mV == 0 || info->min_mV > mV)
-				info->min_mV = mV;
-			if (info->max_mV < mV)
-				info->max_mV = mV;
-		}
-	}
-
 	return platform_driver_register(&twl4030reg_driver);
 }
 subsys_initcall(twl4030reg_init);

[patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Update previously-posted twl4030 regulator driver to export
supported voltages to upper layers using a new mechanism.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Updates since previous version:  match updated [1/2] interfaces.

Note that the twl4030 regulator patch referred to will need a
minor patch to work with the -next tree, because of interface
change in the regulator framework.

 drivers/regulator/twl4030-regulator.c |   62 +++++++++++---------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

--- a/drivers/regulator/twl4030-regulator.c
+++ b/drivers/regulator/twl4030-regulator.c
@@ -42,7 +42,6 @@ struct twlreg_info {
 
 	/* chip constraints on regulator behavior */
 	u16			min_mV;
-	u16			max_mV;
 
 	/* used by regulator core */
 	struct regulator_desc	desc;
@@ -262,6 +261,14 @@ static const u16 VDAC_VSEL_table[] = {
 };
 
 
+static int twl4030ldo_list_voltage(struct regulator_dev *rdev, unsigned index)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+	int			mV = info->table[index];
+
+	return IS_UNSUP(mV) ? 0 : (LDO_MV(mV) * 1000);
+}
+
 static int
 twl4030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV)
 {
@@ -276,6 +283,8 @@ twl4030ldo_set_voltage(struct regulator_
 			continue;
 		uV = LDO_MV(mV) * 1000;
 
+		/* REVISIT for VAUX2, first match may not be best/lowest */
+
 		/* use the first in-range value */
 		if (min_uV <= uV && uV <= max_uV)
 			return twl4030reg_write(info, VREG_DEDICATED, vsel);
@@ -297,6 +306,8 @@ static int twl4030ldo_get_voltage(struct
 }
 
 static struct regulator_ops twl4030ldo_ops = {
+	.list_voltage	= twl4030ldo_list_voltage,
+
 	.set_voltage	= twl4030ldo_set_voltage,
 	.get_voltage	= twl4030ldo_get_voltage,
 
@@ -314,6 +325,13 @@ static struct regulator_ops twl4030ldo_o
 /*
  * Fixed voltage LDOs don't have a VSEL field to update.
  */
+static int twl4030fixed_list_voltage(struct regulator_dev *rdev, unsigned index)
+{
+	struct twlreg_info	*info = rdev_get_drvdata(rdev);
+
+	return info->min_mV * 1000;
+}
+
 static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -322,6 +340,8 @@ static int twl4030fixed_get_voltage(stru
 }
 
 static struct regulator_ops twl4030fixed_ops = {
+	.list_voltage	= twl4030fixed_list_voltage,
+
 	.get_voltage	= twl4030fixed_get_voltage,
 
 	.enable		= twl4030reg_enable,
@@ -343,6 +363,7 @@ static struct regulator_ops twl4030fixed
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
+		.n_voltages = ARRAY_SIZE(label##_VSEL_table), \
 		.ops = &twl4030ldo_ops, \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
@@ -353,10 +374,10 @@ static struct regulator_ops twl4030fixed
 	.base = offset, \
 	.id = num, \
 	.min_mV = mVolts, \
-	.max_mV = mVolts, \
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
+		.n_voltages = 1, \
 		.ops = &twl4030fixed_ops, \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
@@ -402,14 +423,11 @@ static int twl4030reg_probe(struct platf
 	struct regulator_init_data	*initdata;
 	struct regulation_constraints	*c;
 	struct regulator_dev		*rdev;
-	int				min_uV, max_uV;
 
 	for (i = 0, info = NULL; i < ARRAY_SIZE(twl4030_regs); i++) {
 		if (twl4030_regs[i].desc.id != pdev->id)
 			continue;
 		info = twl4030_regs + i;
-		min_uV = info->min_mV * 1000;
-		max_uV = info->max_mV * 1000;
 		break;
 	}
 	if (!info)
@@ -423,10 +441,6 @@ static int twl4030reg_probe(struct platf
 	 * this driver and the chip itself can actually do.
 	 */
 	c = &initdata->constraints;
-	if (!c->min_uV || c->min_uV < min_uV)
-		c->min_uV = min_uV;
-	if (!c->max_uV || c->max_uV > max_uV)
-		c->max_uV = max_uV;
 	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
 	c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE
 				| REGULATOR_CHANGE_MODE
@@ -471,36 +485,6 @@ static struct platform_driver twl4030reg
 
 static int __init twl4030reg_init(void)
 {
-	unsigned i, j;
-
-	/* determine min/max voltage constraints, taking into account
-	 * whether set_voltage() will use the "unsupported" settings
-	 */
-	for (i = 0; i < ARRAY_SIZE(twl4030_regs); i++) {
-		struct twlreg_info	*info = twl4030_regs + i;
-		const u16		*table;
-
-		/* fixed-voltage regulators */
-		if (!info->table_len)
-			continue;
-
-		/* LDO regulators: */
-		for (j = 0, table = info->table;
-				j < info->table_len;
-				j++, table++) {
-			u16		mV = *table;
-
-			if (IS_UNSUP(mV))
-				continue;
-			mV = LDO_MV(mV);
-
-			if (info->min_mV == 0 || info->min_mV > mV)
-				info->min_mV = mV;
-			if (info->max_mV < mV)
-				info->max_mV = mV;
-		}
-	}
-
 	return platform_driver_register(&twl4030reg_driver);
 }
 subsys_initcall(twl4030reg_init);

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[Mark Brown]

On Thu, Feb 26, 2009 at 11:50:14AM -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>

> Update previously-posted twl4030 regulator driver to export
> supported voltages to upper layers using a new mechanism.

> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Looks good, thanks!

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

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[Liam Girdwood]

On Thu, 2009-02-26 at 11:50 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Update previously-posted twl4030 regulator driver to export
> supported voltages to upper layers using a new mechanism.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> Updates since previous version:  match updated [1/2] interfaces.
> 
> Note that the twl4030 regulator patch referred to will need a
> minor patch to work with the -next tree, because of interface
> change in the regulator framework.
> 
>  drivers/regulator/twl4030-regulator.c |   62 +++++++++++---------------------
>  1 file changed, 23 insertions(+), 39 deletions(-)

Applied.

Thanks

Liam

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> > 
> > Note that the twl4030 regulator patch referred to will need a
> > minor patch to work with the -next tree, because of interface
> > change in the regulator framework.
> > 
> >  drivers/regulator/twl4030-regulator.c |   62 +++++++++++---------------------
> >  1 file changed, 23 insertions(+), 39 deletions(-)
> 
> Applied.

..... and here's that "minor patch".

====== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

Catch up the twl4030 regulator driver to the regulator
interface change adding another parameter.  Also, fix
some comments, and take this opportunity to shrink the
associated per-regulator memory usage by a word.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/twl4030-regulator.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

--- a/drivers/regulator/twl4030-regulator.c
+++ b/drivers/regulator/twl4030-regulator.c
@@ -36,13 +36,13 @@ struct twlreg_info {
 	/* twl4030 resource ID, for resource control state machine */
 	u8			id;
 
+	/* FIXED_LDO voltage */
+	u8			deciV;
+
 	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
 	u8			table_len;
 	const u16		*table;
 
-	/* chip constraints on regulator behavior */
-	u16			min_mV;
-
 	/* used by regulator core */
 	struct regulator_desc	desc;
 };
@@ -329,14 +329,14 @@ static int twl4030fixed_list_voltage(str
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static struct regulator_ops twl4030fixed_ops = {
@@ -373,7 +373,7 @@ static struct regulator_ops twl4030fixed
 #define TWL_FIXED_LDO(label, offset, mVolts, num) { \
 	.base = offset, \
 	.id = num, \
-	.min_mV = mVolts, \
+	.deciV = mVolts / 100 , \
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
@@ -385,7 +385,7 @@ static struct regulator_ops twl4030fixed
 	}
 
 /*
- * We list regulators here if systems need some level of
+ * We expose regulators here if systems need some level of
  * software control over them after boot.
  */
 static struct twlreg_info twl4030_regs[] = {
@@ -439,6 +439,7 @@ static int twl4030reg_probe(struct platf
 
 	/* Constrain board-specific capabilities according to what
 	 * this driver and the chip itself can actually do.
+	 * (Regulator core now does this for voltage constraints.)
 	 */
 	c = &initdata->constraints;
 	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
@@ -446,7 +447,7 @@ static int twl4030reg_probe(struct platf
 				| REGULATOR_CHANGE_MODE
 				| REGULATOR_CHANGE_STATUS;
 
-	rdev = regulator_register(&info->desc, &pdev->dev, info);
+	rdev = regulator_register(&info->desc, &pdev->dev, initdata, info);
 	if (IS_ERR(rdev)) {
 		dev_err(&pdev->dev, "can't register %s, %ld\n",
 				info->desc.name, PTR_ERR(rdev));

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[David Brownell]

On Thursday 26 February 2009, Liam Girdwood wrote:
> > 
> > Note that the twl4030 regulator patch referred to will need a
> > minor patch to work with the -next tree, because of interface
> > change in the regulator framework.
> > 
> >  drivers/regulator/twl4030-regulator.c |   62 +++++++++++---------------------
> >  1 file changed, 23 insertions(+), 39 deletions(-)
> 
> Applied.

.... and here's that "minor patch".

====== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

Catch up the twl4030 regulator driver to the regulator
interface change adding another parameter.  Also, fix
some comments, and take this opportunity to shrink the
associated per-regulator memory usage by a word.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/twl4030-regulator.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

--- a/drivers/regulator/twl4030-regulator.c
+++ b/drivers/regulator/twl4030-regulator.c
@@ -36,13 +36,13 @@ struct twlreg_info {
 	/* twl4030 resource ID, for resource control state machine */
 	u8			id;
 
+	/* FIXED_LDO voltage */
+	u8			deciV;
+
 	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
 	u8			table_len;
 	const u16		*table;
 
-	/* chip constraints on regulator behavior */
-	u16			min_mV;
-
 	/* used by regulator core */
 	struct regulator_desc	desc;
 };
@@ -329,14 +329,14 @@ static int twl4030fixed_list_voltage(str
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static struct regulator_ops twl4030fixed_ops = {
@@ -373,7 +373,7 @@ static struct regulator_ops twl4030fixed
 #define TWL_FIXED_LDO(label, offset, mVolts, num) { \
 	.base = offset, \
 	.id = num, \
-	.min_mV = mVolts, \
+	.deciV = mVolts / 100 , \
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
@@ -385,7 +385,7 @@ static struct regulator_ops twl4030fixed
 	}
 
 /*
- * We list regulators here if systems need some level of
+ * We expose regulators here if systems need some level of
  * software control over them after boot.
  */
 static struct twlreg_info twl4030_regs[] = {
@@ -439,6 +439,7 @@ static int twl4030reg_probe(struct platf
 
 	/* Constrain board-specific capabilities according to what
 	 * this driver and the chip itself can actually do.
+	 * (Regulator core now does this for voltage constraints.)
 	 */
 	c = &initdata->constraints;
 	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
@@ -446,7 +447,7 @@ static int twl4030reg_probe(struct platf
 				| REGULATOR_CHANGE_MODE
 				| REGULATOR_CHANGE_STATUS;
 
-	rdev = regulator_register(&info->desc, &pdev->dev, info);
+	rdev = regulator_register(&info->desc, &pdev->dev, initdata, info);
 	if (IS_ERR(rdev)) {
 		dev_err(&pdev->dev, "can't register %s, %ld\n",
 				info->desc.name, PTR_ERR(rdev));



--
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: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[Liam Girdwood]

On Thu, 2009-02-26 at 16:02 -0800, David Brownell wrote:
> On Thursday 26 February 2009, Liam Girdwood wrote:
> > > 
> > > Note that the twl4030 regulator patch referred to will need a
> > > minor patch to work with the -next tree, because of interface
> > > change in the regulator framework.
> > > 
> > >  drivers/regulator/twl4030-regulator.c |   62 +++++++++++---------------------
> > >  1 file changed, 23 insertions(+), 39 deletions(-)
> > 
> > Applied.
> 
> .... and here's that "minor patch".
> 
> ====== CUT HERE
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Catch up the twl4030 regulator driver to the regulator
> interface change adding another parameter.  Also, fix
> some comments, and take this opportunity to shrink the
> associated per-regulator memory usage by a word.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/regulator/twl4030-regulator.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)


Sorry, this didn't apply. It looks like I'm missing an earlier patch(s)
here. Could you regenerate this and your core patch against latest
for-next.

Thanks

Liam

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[David Brownell]

On Friday 27 February 2009, Liam Girdwood wrote:
> On Thu, 2009-02-26 at 16:02 -0800, David Brownell wrote:
> > On Thursday 26 February 2009, Liam Girdwood wrote:
> > > > 
> > > > Note that the twl4030 regulator patch referred to will need a
> > > > minor patch to work with the -next tree, because of interface
> > > > change in the regulator framework.
> > > 
> > > Applied.
> > 
> > .... and here's that "minor patch".
> > 
> > ...
> 
> Sorry, this didn't apply. It looks like I'm missing an earlier patch(s)
> here. Could you regenerate this and your core patch against latest
> for-next.

The regulator -next tree seems to be missing a bunch of stuff...
I generated this patch against a "twl4030-regulator.c" which I
extracted *from that tree* yesterday.  But today it's different.

In this case, the current code doesn't have the $SUBJECT patch,
which at that time you had applied.  But it does have a small
snippet from that "minor patch"...


Color me confused.  Are you asking for a "v3" of $SUBJECT, or
is the "v2" going to re-appear?  And when will that -next tree
acquire the rest of

 http://marc.info/?l=linux-kernel&m=123567791402469&w=2

Having only the driver.h part of that patch breaks things
(your 0ae0e667c8a2bacfe066b90f8f2ee3b4a83a120d).

- Dave

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[Liam Girdwood]

On Fri, 2009-02-27 at 12:39 -0800, David Brownell wrote:
> On Friday 27 February 2009, Liam Girdwood wrote:

> > Sorry, this didn't apply. It looks like I'm missing an earlier patch(s)
> > here. Could you regenerate this and your core patch against latest
> > for-next.
> 
> The regulator -next tree seems to be missing a bunch of stuff...
> I generated this patch against a "twl4030-regulator.c" which I
> extracted *from that tree* yesterday.  But today it's different.
> 
> In this case, the current code doesn't have the $SUBJECT patch,
> which at that time you had applied.  But it does have a small
> snippet from that "minor patch"...
> 
> 
> Color me confused.  Are you asking for a "v3" of $SUBJECT, or
> is the "v2" going to re-appear? 

Applied the "v2".

>  And when will that -next tree
> acquire the rest of
> 
>  http://marc.info/?l=linux-kernel&m=123567791402469&w=2
> 

Applied and re-based this one into original patch.

Thanks

Liam

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[David Brownell]

On Friday 27 February 2009, Liam Girdwood wrote:
> Sorry, this didn't apply. It looks like I'm missing an earlier patch(s)
> here. Could you regenerate this and your core patch against latest
> for-next.

Here you go.

- Dave


========== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

Minor cleanups to the twl403 regulator driver, mostly enabled
by other recent changes:  comments, shrink memory usage, add
definition for one bit.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/twl4030-regulator.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

--- a/drivers/regulator/twl4030-regulator.c
+++ b/drivers/regulator/twl4030-regulator.c
@@ -36,13 +36,13 @@ struct twlreg_info {
 	/* twl4030 resource ID, for resource control state machine */
 	u8			id;
 
+	/* FIXED_LDO voltage */
+	u8			deciV;
+
 	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
 	u8			table_len;
 	const u16		*table;
 
-	/* chip constraints on regulator behavior */
-	u16			min_mV;
-
 	/* used by regulator core */
 	struct regulator_desc	desc;
 };
@@ -93,6 +93,7 @@ static int twl4030reg_grp(struct regulat
 #define P3_GRP		BIT(7)		/* "peripherals" */
 #define P2_GRP		BIT(6)		/* secondary processor, modem, etc */
 #define P1_GRP		BIT(5)		/* CPU/Linux */
+#define WARM_CFG	BIT(4)
 
 static int twl4030reg_is_enabled(struct regulator_dev *rdev)
 {
@@ -325,14 +326,14 @@ static int twl4030fixed_list_voltage(str
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static int twl4030fixed_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 
-	return info->min_mV * 1000;
+	return info->deciV * 100 * 1000;
 }
 
 static struct regulator_ops twl4030fixed_ops = {
@@ -369,7 +370,7 @@ static struct regulator_ops twl4030fixed
 #define TWL_FIXED_LDO(label, offset, mVolts, num) { \
 	.base = offset, \
 	.id = num, \
-	.min_mV = mVolts, \
+	.deciV = mVolts / 100 , \
 	.desc = { \
 		.name = #label, \
 		.id = TWL4030_REG_##label, \
@@ -381,7 +382,7 @@ static struct regulator_ops twl4030fixed
 	}
 
 /*
- * We list regulators here if systems need some level of
+ * We expose regulators here if systems need some level of
  * software control over them after boot.
  */
 static struct twlreg_info twl4030_regs[] = {
@@ -435,6 +436,7 @@ static int twl4030reg_probe(struct platf
 
 	/* Constrain board-specific capabilities according to what
 	 * this driver and the chip itself can actually do.
+	 * (Regulator core now does this for voltage constraints.)
 	 */
 	c = &initdata->constraints;
 	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;

Re: [patch/rfc 2.6.29-rc6 2/2] regulator: twl4030 voltage enumeration (v2)

[Liam Girdwood]

On Tue, 2009-03-03 at 14:59 -0800, David Brownell wrote:
> On Friday 27 February 2009, Liam Girdwood wrote:
> > Sorry, this didn't apply. It looks like I'm missing an earlier patch(s)
> > here. Could you regenerate this and your core patch against latest
> > for-next.
> 
> Here you go.
> 
> - Dave
> 
> 
> ========== CUT HERE
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Minor cleanups to the twl403 regulator driver, mostly enabled
> by other recent changes:  comments, shrink memory usage, add
> definition for one bit.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/regulator/twl4030-regulator.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 

Applied.

Thanks

Liam

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Mon, Feb 23, 2009 at 12:45:44PM -0800, David Brownell wrote:

> > another with a TWL4030 driver using that API 
> > and a third patch making the core do something with that data.

> Best IMO to switch the last two around.  Effectively
> there'd be one patch "add new features to regulator
> core", followed by the first of a set of "implement
> those new features in the driver for regulator X".

> And in fact that's what I've done with the two patches
> I'll be sending in a moment.

The reason I'm suggesting splitting things up the way I am is that it
separates out the TWL4030 driver (which looks very mergable to me right
now) from the behaviour changes.  Ordering things that way makes it
clear what the dependency is.  Another way of splitting it out would be
to remove the new API from the TWL4030, make that the first patch, then
have further patches adding the new API and the TWL4030 code to use it.

I don't see any reason why the TWL4030 regulator support needs to be
blocked on adding the new API and it makes review easier to keep them
separate.

> > This 
> > would result in something which maintains consistent behaviour over all
> > regulator drivers,

> It's already consistent, even without such patches!!

> There is no driver which pays attention to regulator(_dev)
> constraints that does it any differently than this one.

That's because nobody's doing it at all; once we add a custom
implementation in one driver we then need to police each driver
individually for consistency with the new interface.  If the logic is
factored out then that problem doesn't exist and drivers don't need to
reimplement any of it.

> > > > Your current patch does also set constraints for the voltages if they
> > > > weren't there previously

> > > I was thinking that boards which don't need constraints
> > > shouldn't need to create any ... they could just pass on
> > > the capabilities of the underlying regulator.

> > For safety the regulator API won't do anything without being explicitly
> > told that it's OK to do so - if we were to do this we'd need to have an
> > explicit flag in the constraints which says that this is what to do.
> > Constraints are also permission grants.

> I like to avoid flags unless they're absolutely required.
> In this case my initial reaction is to say that hooking up
> the components in the first place was the permission grant.

The trouble is we don't know what's connected to the regulator without
being told - even if some of the components hanging off the supply are
visible to software that's no guarantee that all of them are.  Keeping
the responsibility in the hands of the machine driver helps ensure that
users have made a concious decision that their settings are OK for their
design.

> > Indeed.  Like I say, a very large proportion of the development of the
> > regulator API has been done on reference designs which are built in this
> > fashion, including both pluggable PMIC boards and other daughtercards.

> That doesn't seem to have escaped its development cage yet.  ;)

There's a couple on their way to mainline right now, should make it in
the next merge window hopefully.  Unfortunately they're for systems that
aren't very completely supported in mainline right now which makes them
less useful as examples than they might be.  There's also none of them
where there's any immediate prospect of more than one of the PMIC board
options going into mainline.

> > Normally the primary PMIC cards are handled with conditional code in the
> > machine file (either compile time or run time) or by registering drivers

> Fair enough.  I'd de-emphasize "conditional", since that sounds
> way too much like #ifdeffing.  The source code should just call
> something like is_pmic_cardX(), and not care how that works.

It's normally a combination of the two - normally you don't get any form
of board ID readback and you get things like mutually exclusive options
for I2C or SPI control due to shared multi function pin setup that can't
be autoprobed entirely safely, for example.  Once you've decided on the
control bus it's normally safe to do autoprobing, though.

> Then what would you call constraints by/from the regulator?

They're subsumed within the constraints supplied by the machine driver
at the minute.

> I suggest updating your terminology.  "machine constraints"
> would be much more clear for what's there now:  they relate
> to the machine.  Other constraints (regulator, consumer)
> relate to the other components ... the ones for which they
> are an adjective.

Yeah, I kind of agree.  To avoid confusion from changing the names I'd
be tempted to go for something like "regulator driver constraints" but
it's not desparately nice.

> I don't mind moving it ... later, after the regulator
> core has proper support for decoupling machine-specific
> constraints from regulator-specific ones.  I view that
> code as a workaround for a current limitation of that
> core, so like all such workarounds it should vanish
> when it's no longer relevant.

I'd strongly suggest that if you're adding workarounds like this it's
worth at least calling them out in the changelog.  To be honest I'm not
100% clear why this new feature is essential to supporting the TWL4030 -
I can see that it could be useful but I'm not clear on what makes it
essential for this driver.

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, Mark Brown wrote:
> On Mon, Feb 23, 2009 at 12:45:44PM -0800, David Brownell wrote:
> 
> > > another with a TWL4030 driver using that API 
> > > and a third patch making the core do something with that data.
> 
> > Best IMO to switch the last two around.  Effectively
> > there'd be one patch "add new features to regulator
> > core", followed by the first of a set of "implement
> > those new features in the driver for regulator X".
> 
> > And in fact that's what I've done with the two patches
> > I'll be sending in a moment.
> 
> The reason I'm suggesting splitting things up the way I am is that it
> separates out the TWL4030 driver (which looks very mergable to me right
> now) from the behaviour changes.  Ordering things that way makes it
> clear what the dependency is.  Another way of splitting it out would be
> to remove the new API from the TWL4030, make that the first patch, then
> have further patches adding the new API and the TWL4030 code to use it.
> 
> I don't see any reason why the TWL4030 regulator support needs to be
> blocked on adding the new API and it makes review easier to keep them
> separate.

I think we're talking past each other.  I agree the twl4030 driver
is very mergeable right now; that's why it was submitted.  You could
do so, and then apply the two patches on top ... very clear what the
dependency is, and as I understand it the result would be more or less
to your liking.

My comment was more along the lines of "avoid adding unused hooks,
just merge the create-hook and use-hook patches".  Having "create"
separate from "use" is often troublesome.


> > Then what would you call constraints by/from the regulator?
> 
> They're subsumed within the constraints supplied by the machine driver
> at the minute.

That is, they are not named.  :)


> > I suggest updating your terminology.  "machine constraints"
> > would be much more clear for what's there now:  they relate
> > to the machine.  Other constraints (regulator, consumer)
> > relate to the other components ... the ones for which they
> > are an adjective.
> 
> Yeah, I kind of agree.  To avoid confusion from changing the names I'd
> be tempted to go for something like "regulator driver constraints" but
> it's not desparately nice.

Hence my suggestion:  {regulator,machine,consumer} constraints,
going from bottom up.  They aren't driver constraints; they
are hardware constraints:  regulators can't supply arbitrary
voltages.


> 	 To be honest I'm not
> 100% clear why this new feature is essential to supporting the TWL4030 -
> I can see that it could be useful but I'm not clear on what makes it
> essential for this driver.

I never said it was "essential".  However it does simplify
the core driver code a lot by moving a lot of error checks
to the init code; the checks need to live someplace.  You're
the one asking for them to be packaged as a new framework
feature.

- Dave

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Mon, Feb 23, 2009 at 02:43:16PM -0800, David Brownell wrote:
> On Monday 23 February 2009, Mark Brown wrote:

> > Yeah, I kind of agree.  To avoid confusion from changing the names I'd
> > be tempted to go for something like "regulator driver constraints" but
> > it's not desparately nice.

> Hence my suggestion:  {regulator,machine,consumer} constraints,
> going from bottom up.  They aren't driver constraints; they
> are hardware constraints:  regulators can't supply arbitrary
> voltages.

Trouble is that the term regulator gets very overloaded and causes
confusion.  There's also fun and games to be had with accuracy once you
start looking too closely at the discrete voltages.

> > 	 To be honest I'm not
> > 100% clear why this new feature is essential to supporting the TWL4030 -
> > I can see that it could be useful but I'm not clear on what makes it
> > essential for this driver.

> I never said it was "essential".  However it does simplify

Apologies, you didn't actually say that, no - I was reading between the
lines there.

> the core driver code a lot by moving a lot of error checks
> to the init code; the checks need to live someplace.  You're

I'm not sure I see that for the constraint tightening, and indeed the
TWL4030 set_voltage() operation does have a range check in it.  Unless
I'm missing something if the tightened constraint is usable then it
should flop out of the range based requests with the constraints left
unmodified.  The reason the TWL4030 driver still has the range check in
the set_voltage() operation is that it is checking to make sure that the
requests that come in can be satisfied from the discrete values the
regulator is able to produce which is a good thing to do.

> the one asking for them to be packaged as a new framework
> feature.

The change to add voltage range constraints if none were supplied is a
noticable policy change from the existing framework standard - it allows
machines to enable voltage changes without specifying what the valid
values are.  I'm not convinced that this is a good idea in the first
place and it will result in the opposite behaviour to the current core
code (which should end up erroring out in constraint checking at runtime).

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, Mark Brown wrote:
> The change to add voltage range constraints if none were supplied is a
> noticable policy change from the existing framework standard - it allows
> machines to enable voltage changes without specifying what the valid
> values are. 

"Whatever the hardware handles" *is* a specification.

And there's no more assurance it's right than any
other specification would be ... except that, as a
rule, hardware designers like to avoid assemblies
subject to trivial misconfiguration mistakes (like
firing up a 2.5V-max rail at 5V).


> I'm not convinced that this is a good idea in the first 
> place and it will result in the opposite behaviour to the current core
> code (which should end up erroring out in constraint checking at runtime).

Well, if you really dislike it so much, that can
easily be removed.  Got any comments on the
framework patch I sent?  I'll take that as the
first one, even though it's a different thread.

- Dave

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Mark Brown]

On Mon, Feb 23, 2009 at 06:03:49PM -0800, David Brownell wrote:
> On Monday 23 February 2009, Mark Brown wrote:

> > The change to add voltage range constraints if none were supplied is a
> > noticable policy change from the existing framework standard - it allows
> > machines to enable voltage changes without specifying what the valid
> > values are. 

> "Whatever the hardware handles" *is* a specification.

> And there's no more assurance it's right than any
> other specification would be ... except that, as a

You're right that we can't guarantee that users get everything correct,
the goal is more to ensure that some machine specific checks have been
done and that the regulator API won't go off by itself and start doing
things since it has no idea at all what's supportable.

> rule, hardware designers like to avoid assemblies
> subject to trivial misconfiguration mistakes (like
> firing up a 2.5V-max rail at 5V).

The issue is what happens when software starts changing the
configuration, for static configurations it doesn't make any difference.

> > I'm not convinced that this is a good idea in the first 
> > place and it will result in the opposite behaviour to the current core
> > code (which should end up erroring out in constraint checking at runtime).

> Well, if you really dislike it so much, that can
> easily be removed.  Got any comments on the
> framework patch I sent?  I'll take that as the
> first one, even though it's a different thread.

Yes, I wanted to sleep on it.  I'll reply at some point today.

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, Mark Brown wrote:
> > > Yeah, I kind of agree.  To avoid confusion from changing the names I'd
> > > be tempted to go for something like "regulator driver constraints" but
> > > it's not desparately nice.
> 
> > Hence my suggestion:  {regulator,machine,consumer} constraints,
> > going from bottom up.  They aren't driver constraints; they
> > are hardware constraints:  regulators can't supply arbitrary
> > voltages.
> 
> Trouble is that the term regulator gets very overloaded and causes
> confusion.

That's why one of the *first* jobs in architecture is to
ensure the terminology doesn't easily overload ... there's
always trouble when it isn't.  Despite early groans from
peanut galleries about "language lawyering" and "wanting
to code in C not English", etc.  ;)

Thing is, there's already overloading here.  Two types
of regulator -- "struct regulator_dev" wrapping hardware,
and the "struct regulator" is a client/consumer handle.
(And thus confusing to me, since it sounds like the more
fundamental concept, but instead it's a few layers up.)

If you're concerned about overloading terminology, then
best to address it sooner than later.  I've noticed you
using the "consumer", "machine", and "regulator" terms
more or less like I suggested, and those make sense; but
the current struct names don't encourage that usage.  It's
possible to adjust usage before changing names in "C".
Thankfully, in Linux global name struct changes are not
rejected out of hand.


> There's also fun and games to be had with accuracy once you 
> start looking too closely at the discrete voltages.

Yes; the patch I sent is explicitly making those available.

But I ignored issues like "+/- 3% accurate output" for LDOs
(or switchers) ... if anyone really needs to address them,
patches will be needed.  For now I only care that a 3.1 Volt
output can match both MMC_VDD_30_31 and MMC_VDD_31_32! ;)

- Dave

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, Mark Brown wrote:
> > > Yeah, I kind of agree.  To avoid confusion from changing the names I'd
> > > be tempted to go for something like "regulator driver constraints" but
> > > it's not desparately nice.
> 
> > Hence my suggestion:  {regulator,machine,consumer} constraints,
> > going from bottom up.  They aren't driver constraints; they
> > are hardware constraints:  regulators can't supply arbitrary
> > voltages.
> 
> Trouble is that the term regulator gets very overloaded and causes
> confusion.

That's why one of the *first* jobs in architecture is to
ensure the terminology doesn't easily overload ... there's
always trouble when it isn't.  Despite early groans from
peanut galleries about "language lawyering" and "wanting
to code in C not English", etc.  ;)

Thing is, there's already overloading here.  Two types
of regulator -- "struct regulator_dev" wrapping hardware,
and the "struct regulator" is a client/consumer handle.
(And thus confusing to me, since it sounds like the more
fundamental concept, but instead it's a few layers up.)

If you're concerned about overloading terminology, then
best to address it sooner than later.  I've noticed you
using the "consumer", "machine", and "regulator" terms
more or less like I suggested, and those make sense; but
the current struct names don't encourage that usage.  It's
possible to adjust usage before changing names in "C".
Thankfully, in Linux global name struct changes are not
rejected out of hand.


> There's also fun and games to be had with accuracy once you 
> start looking too closely at the discrete voltages.

Yes; the patch I sent is explicitly making those available.

But I ignored issues like "+/- 3% accurate output" for LDOs
(or switchers) ... if anyone really needs to address them,
patches will be needed.  For now I only care that a 3.1 Volt
output can match both MMC_VDD_30_31 and MMC_VDD_31_32! ;)

- Dave



--
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: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, David Brownell wrote:
> > There's also fun and games to be had with accuracy once you 
> > start looking too closely at the discrete voltages.
> 
> Yes; the patch I sent is explicitly making those available.
> 
> But I ignored issues like "+/- 3% accurate output" for LDOs
> (or switchers) ... if anyone really needs to address them,
> patches will be needed.  For now I only care that a 3.1 Volt
> output can match both MMC_VDD_30_31 and MMC_VDD_31_32! ;)

And -- for kicks -- here's one notion of what it might look
like to have the MMC stack support the regulator framework.

- Dave


=================
Prototype glue between MMC and regulator stacks ... compiles,
and mmc_regulator_get_ocrmask() passed sanity testing.

NOTES:

 - The MMC core does't call mmc_regulator_set_ocr() because hosts
   may need to do that in conjunction with updating I/O voltage.

   Case in point, MMC1 on omap_hsmmc ... where the host driver
   must update MMC1_HCTL.SDVS and PBIAS registers in addition to
   the regulator, supporting 1.8V or 3.0V voltage ranges.  (MMC2
   and MMC3 use external level shifting for Vdd != 1.8V.)

   Likewise, using eMMC "managed NAND" solutions, powerup includes
   not both Vcc ("vdd" to Linux, e.g. 3.0V) and an I/O interface
   rail VccQ (e.g. 1.8V).  The JEDEC spec for eMMC requires VccQ
   powerup after Vcc, and powerdown before it.

 - The "vdd" supply name isn't fixed, since platforms may need
   to use more than one I/O supply.

   Case in point, MMC1 on omap_hsmmc (again) ... where a second
   supply is needed to kick in 8-bit I/O using DAT4..DAT7 signals.
   That would not be handled quite like VccQ, since it's only
   used for 8-bit I/O widths (MMCplus cards, some eMMC, etc).
 
---
 drivers/mmc/core/Kconfig |    8 +++
 drivers/mmc/core/core.c  |   98 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    3 +
 3 files changed, 109 insertions(+)

--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -14,3 +14,11 @@ config MMC_UNSAFE_RESUME
 	  This option is usually just for embedded systems which use
 	  a MMC/SD card for rootfs. Most people should say N here.
 
+config MMC_REGULATOR
+	bool
+	depends on REGULATOR
+	default y
+	help
+	  Select this to provide some helper utilities to access the
+	  "vdd" (card) voltage supply associated with an MMC/SD slot.
+
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,103 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_MMC_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @host: mmc host whose supply will be consulted
+ * @supply: supply voltage to use; "vdd" if NULL
+ *
+ * This returns either a negative errno, or a mask of voltages
+ * that can be provided to MMC/SD/SDIO devices using the specified
+ * host's "vdd" supply.
+ */
+int mmc_regulator_get_ocrmask(struct mmc_host *host, const char *supply)
+{
+	int			result = 0;
+	struct regulator	*reg;
+	int			count;
+	int			i;
+
+	reg = regulator_get(host->parent, supply ? : "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	count = regulator_count_voltages(reg);
+	if (count < 0) {
+		result = count;
+		goto done;
+	}
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(reg, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+done:
+	regulator_put(reg);
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @host: mmc host whose supply voltage will be changed
+ * @supply: supply voltage to use; "vdd" if NULL
+ *
+ * MMC host drivers may use this to enable or disable a regulator
+ * using a particular supply voltage.  This would normally be
+ * called from the set_ios() method, possibly as part of updating
+ * digital interfaces to support that voltage.
+ */
+int mmc_regulator_set_ocr(struct mmc_host *host, const char *supply)
+{
+	int			result = 0;
+	struct regulator	*reg;
+	int			min_mV, max_mV;
+	int			enabled;
+
+	reg = regulator_get(host->parent, supply ? : "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+	enabled = regulator_is_enabled(reg);
+	if (WARN(enabled < 0, "%s: regulator_is_enabled --> %d\n",
+			mmc_hostname(host), enabled))
+		enabled = !host->ios.vdd;
+
+	if (host->ios.vdd) {
+		int		tmp;
+
+		tmp = host->ios.vdd - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_mV = 1650;
+			max_mV = 1950;
+		} else {
+			min_mV = 2000 + tmp * 100;
+			max_mV = min_mV + 100;
+		}
+
+		result = regulator_set_voltage(reg, min_mV * 1000, max_mV * 1000);
+		if (result == 0 && !enabled)
+			result = regulator_enable(reg);
+	} else if (enabled) {
+		result = regulator_disable(reg);
+	}
+
+	regulator_put(reg);
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,8 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+int mmc_regulator_get_ocrmask(struct mmc_host *host, const char *supply);
+int mmc_regulator_set_ocr(struct mmc_host *host, const char *supply);
+
 #endif
 

Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[David Brownell]

On Monday 23 February 2009, David Brownell wrote:
> > There's also fun and games to be had with accuracy once you 
> > start looking too closely at the discrete voltages.
> 
> Yes; the patch I sent is explicitly making those available.
> 
> But I ignored issues like "+/- 3% accurate output" for LDOs
> (or switchers) ... if anyone really needs to address them,
> patches will be needed.  For now I only care that a 3.1 Volt
> output can match both MMC_VDD_30_31 and MMC_VDD_31_32! ;)

And -- for kicks -- here's one notion of what it might look
like to have the MMC stack support the regulator framework.

- Dave


=================
Prototype glue between MMC and regulator stacks ... compiles,
and mmc_regulator_get_ocrmask() passed sanity testing.

NOTES:

 - The MMC core does't call mmc_regulator_set_ocr() because hosts
   may need to do that in conjunction with updating I/O voltage.

   Case in point, MMC1 on omap_hsmmc ... where the host driver
   must update MMC1_HCTL.SDVS and PBIAS registers in addition to
   the regulator, supporting 1.8V or 3.0V voltage ranges.  (MMC2
   and MMC3 use external level shifting for Vdd != 1.8V.)

   Likewise, using eMMC "managed NAND" solutions, powerup includes
   not both Vcc ("vdd" to Linux, e.g. 3.0V) and an I/O interface
   rail VccQ (e.g. 1.8V).  The JEDEC spec for eMMC requires VccQ
   powerup after Vcc, and powerdown before it.

 - The "vdd" supply name isn't fixed, since platforms may need
   to use more than one I/O supply.

   Case in point, MMC1 on omap_hsmmc (again) ... where a second
   supply is needed to kick in 8-bit I/O using DAT4..DAT7 signals.
   That would not be handled quite like VccQ, since it's only
   used for 8-bit I/O widths (MMCplus cards, some eMMC, etc).
 
---
 drivers/mmc/core/Kconfig |    8 +++
 drivers/mmc/core/core.c  |   98 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    3 +
 3 files changed, 109 insertions(+)

--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -14,3 +14,11 @@ config MMC_UNSAFE_RESUME
 	  This option is usually just for embedded systems which use
 	  a MMC/SD card for rootfs. Most people should say N here.
 
+config MMC_REGULATOR
+	bool
+	depends on REGULATOR
+	default y
+	help
+	  Select this to provide some helper utilities to access the
+	  "vdd" (card) voltage supply associated with an MMC/SD slot.
+
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -21,6 +21,7 @@
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -523,6 +524,103 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min,
 }
 EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
 
+#ifdef CONFIG_MMC_REGULATOR
+
+/**
+ * mmc_regulator_get_ocrmask - return mask of supported voltages
+ * @host: mmc host whose supply will be consulted
+ * @supply: supply voltage to use; "vdd" if NULL
+ *
+ * This returns either a negative errno, or a mask of voltages
+ * that can be provided to MMC/SD/SDIO devices using the specified
+ * host's "vdd" supply.
+ */
+int mmc_regulator_get_ocrmask(struct mmc_host *host, const char *supply)
+{
+	int			result = 0;
+	struct regulator	*reg;
+	int			count;
+	int			i;
+
+	reg = regulator_get(host->parent, supply ? : "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	count = regulator_count_voltages(reg);
+	if (count < 0) {
+		result = count;
+		goto done;
+	}
+
+	for (i = 0; i < count; i++) {
+		int		vdd_uV;
+		int		vdd_mV;
+
+		vdd_uV = regulator_list_voltage(reg, i);
+		if (vdd_uV <= 0)
+			continue;
+
+		vdd_mV = vdd_uV / 1000;
+		result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV);
+	}
+
+done:
+	regulator_put(reg);
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
+
+/**
+ * mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @host: mmc host whose supply voltage will be changed
+ * @supply: supply voltage to use; "vdd" if NULL
+ *
+ * MMC host drivers may use this to enable or disable a regulator
+ * using a particular supply voltage.  This would normally be
+ * called from the set_ios() method, possibly as part of updating
+ * digital interfaces to support that voltage.
+ */
+int mmc_regulator_set_ocr(struct mmc_host *host, const char *supply)
+{
+	int			result = 0;
+	struct regulator	*reg;
+	int			min_mV, max_mV;
+	int			enabled;
+
+	reg = regulator_get(host->parent, supply ? : "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+	enabled = regulator_is_enabled(reg);
+	if (WARN(enabled < 0, "%s: regulator_is_enabled --> %d\n",
+			mmc_hostname(host), enabled))
+		enabled = !host->ios.vdd;
+
+	if (host->ios.vdd) {
+		int		tmp;
+
+		tmp = host->ios.vdd - ilog2(MMC_VDD_165_195);
+		if (tmp == 0) {
+			min_mV = 1650;
+			max_mV = 1950;
+		} else {
+			min_mV = 2000 + tmp * 100;
+			max_mV = min_mV + 100;
+		}
+
+		result = regulator_set_voltage(reg, min_mV * 1000, max_mV * 1000);
+		if (result == 0 && !enabled)
+			result = regulator_enable(reg);
+	} else if (enabled) {
+		result = regulator_disable(reg);
+	}
+
+	regulator_put(reg);
+	return result;
+}
+EXPORT_SYMBOL(mmc_regulator_set_ocr);
+
+#endif
+
 /*
  * Mask off any voltages we don't support and select
  * the lowest voltage
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -192,5 +192,8 @@ static inline void mmc_signal_sdio_irq(s
 	wake_up_process(host->sdio_irq_thread);
 }
 
+int mmc_regulator_get_ocrmask(struct mmc_host *host, const char *supply);
+int mmc_regulator_set_ocr(struct mmc_host *host, const char *supply);
+
 #endif
 
--
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: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

[Liam Girdwood]

On Sun, 2009-02-08 at 10:37 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Support most of the LDO regulators in the twl4030 family chips.
> In the case of LDOs supporting MMC/SD, the voltage controls are
> used; but in most other cases, the regulator framework is only
> used to enable/disable a supplies, conserving power when a given
> voltage rail is not needed.
> 
> The drivers/mfd/twl4030-core.c code already sets up the various
> regulators according to board-specific configuration, and knows
> that some chips don't provide the full set of voltage rails.
> 
> The omitted regulators are intended to be under hardware control,
> such as during the hardware-mediated system powerup, powerdown,
> and suspend states.  Unless/until software hooks are known to
> be safe, they won't be exported here.
> 
> These regulators implement the new get_status() operation, but
> can't realistically implement get_mode(); the status output is
> effectively the result of a vote, with the relevant hardware
> inputs not exposed.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Applied.

Thanks

Liam