[PATCH] regulator: max8973: add regulator driver support

[PATCH] regulator: max8973: add regulator driver support

[Laxman Dewangan]

The MAXIM MAX8973 high-efficiency, three phase, DC-DC step-down
switching regulator delievers up to 9A of output current. Each
phase operates at a 2MHz fixed frequency with a 120 deg shift
from the adjacent phase, allowing the use of small magnetic
component.

Add regulator driver for this device.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/Kconfig                   |   10 +
 drivers/regulator/Makefile                  |    1 +
 drivers/regulator/max8973-regulator.c       |  505 +++++++++++++++++++++++++++
 include/linux/regulator/max8973-regulator.h |   72 ++++
 4 files changed, 588 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/max8973-regulator.c
 create mode 100644 include/linux/regulator/max8973-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index cbc685d..ddd8869 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -204,6 +204,16 @@ config REGULATOR_MAX8952
 	  via I2C bus. Maxim 8952 has one voltage output and supports 4 DVS
 	  modes ranging from 0.77V to 1.40V by 0.01V steps.
 
+config REGULATOR_MAX8973
+	tristate "Maxim MAX8973 voltage regulator "
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  The MAXIM MAX8973 high-efficiency. three phase, DC-DC step-down
+	  switching regulator delievers up to 9A of output current. Each
+	  phase operates at a 2MHz fixed frequency with a 120 deg shift
+	  from the adjacent phase, allowing the use of small magnetic component.
+
 config REGULATOR_MAX8997
 	tristate "Maxim 8997/8966 regulator"
 	depends on MFD_MAX8997
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index db274dc..ac093f3 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o
 obj-$(CONFIG_REGULATOR_MAX8907) += max8907-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
+obj-$(CONFIG_REGULATOR_MAX8973) += max8973-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
 obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
 obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
new file mode 100644
index 0000000..e6328f9
--- /dev/null
+++ b/drivers/regulator/max8973-regulator.c
@@ -0,0 +1,505 @@
+/*
+ * max8973-regulator.c -- Maxim max8973
+ *
+ * Regulator driver for MAXIM 8973 DC-DC step-down switching regulator.
+ *
+ * Copyright (c) 2012, NVIDIA Corporation.
+ *
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+ * 02111-1307, USA
+ */
+
+#include <linux/kernel.h>
+#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/regulator/max8973-regulator.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+
+/* Register definitions */
+#define MAX8973_VOUT					0x0
+#define MAX8973_VOUT_DVS				0x1
+#define MAX8973_CONTROL1				0x2
+#define MAX8973_CONTROL2				0x3
+#define MAX8973_CHIPID1					0x4
+#define MAX8973_CHIPID2					0x5
+
+#define MAX8973_MAX_VOUT_REG				2
+
+/* MAX8973_VOUT */
+#define MAX8973_VOUT_ENABLE				BIT(7)
+#define MAX8973_VOUT_MASK				0x7F
+
+/* MAX8973_VOUT_DVS */
+#define MAX8973_DVS_VOUT_MASK				0x7F
+
+/* MAX8973_CONTROL1 */
+#define MAX8973_SNS_ENABLE				BIT(7)
+#define MAX8973_FPWM_EN_M				BIT(6)
+#define MAX8973_NFSR_ENABLE				BIT(5)
+#define MAX8973_AD_ENABLE				BIT(4)
+#define MAX8973_BIAS_ENABLE				BIT(3)
+#define MAX8973_FREQSHIFT_9PER				BIT(2)
+
+#define MAX8973_RAMP_12mV_PER_US			0x0
+#define MAX8973_RAMP_25mV_PER_US			0x1
+#define MAX8973_RAMP_50mV_PER_US			0x2
+#define MAX8973_RAMP_200mV_PER_US			0x3
+
+/* MAX8973_CONTROL2 */
+#define MAX8973_WDTMR_ENABLE				BIT(6)
+#define MAX8973_DISCH_ENBABLE				BIT(5)
+#define MAX8973_FT_ENABLE				BIT(4)
+
+#define MAX8973_CKKADV_TRIP_DISABLE			0xC
+#define MAX8973_CKKADV_TRIP_75mV_PER_US			0x0
+#define MAX8973_CKKADV_TRIP_150mV_PER_US		0x4
+#define MAX8973_CKKADV_TRIP_75mV_PER_US_HIST_DIS	0x8
+#define MAX8973_CONTROL_CLKADV_TRIP_MASK		0x00030000
+
+#define MAX8973_INDUCTOR_MIN_30_PER			0x0
+#define MAX8973_INDUCTOR_NOMINAL			0x1
+#define MAX8973_INDUCTOR_PLUS_30_PER			0x2
+#define MAX8973_INDUCTOR_PLUS_60_PER			0x3
+#define MAX8973_CONTROL_INDUCTOR_VALUE_MASK		0x00300000
+
+#define MAX8973_MIN_VOLATGE				606250
+#define MAX8973_MAX_VOLATGE				1400000
+#define MAX8973_VOLATGE_STEP				6250
+#define MAX8973_BUCK_N_VOLTAGE				0x80
+
+/* Maxim 8973 chip information */
+struct max8973_chip {
+	struct device *dev;
+	struct regulator_desc desc;
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+	bool enable_external_control;
+	int dvs_gpio;
+	int lru_index[MAX8973_MAX_VOUT_REG];
+	int curr_vout_val[MAX8973_MAX_VOUT_REG];
+	int curr_vout_reg;
+	int curr_gpio_val;
+	bool valid_dvs_gpio;
+};
+
+/*
+ * find_voltage_set_register: Find new voltage configuration register (VOUT).
+ * The finding of the new VOUT register will be based on the LRU mechanism.
+ * Each VOUT register will have different voltage configured . This
+ * Function will look if any of the VOUT register have requested voltage set
+ * or not.
+ *     - If it is already there then it will make that register as most
+ *       recently used and return as found so that caller need not to set
+ *       the VOUT register but need to set the proper gpios to select this
+ *       VOUT register.
+ *     - If requested voltage is not found then it will use the least
+ *       recently mechanism to get new VOUT register for new configuration
+ *       and will return not_found so that caller need to set new VOUT
+ *       register and then gpios (both).
+ */
+static bool find_voltage_set_register(struct max8973_chip *tps,
+		int req_vsel, int *vout_reg, int *gpio_val)
+{
+	int i;
+	bool found = false;
+	int new_vout_reg = tps->lru_index[MAX8973_MAX_VOUT_REG - 1];
+	int found_index = MAX8973_MAX_VOUT_REG - 1;
+
+	for (i = 0; i < MAX8973_MAX_VOUT_REG; ++i) {
+		if (tps->curr_vout_val[tps->lru_index[i]] == req_vsel) {
+			new_vout_reg = tps->lru_index[i];
+			found_index = i;
+			found = true;
+			goto update_lru_index;
+		}
+	}
+
+update_lru_index:
+	for (i = found_index; i > 0; i--)
+		tps->lru_index[i] = tps->lru_index[i - 1];
+
+	tps->lru_index[0] = new_vout_reg;
+	*gpio_val = new_vout_reg;
+	*vout_reg = MAX8973_VOUT + new_vout_reg;
+	return found;
+}
+
+static int max8973_dcdc_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct max8973_chip *max = rdev_get_drvdata(rdev);
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(max->regmap, max->curr_vout_reg, &data);
+	if (ret < 0) {
+		dev_err(max->dev, "register %d read failed, err = %d\n",
+			max->curr_vout_reg, ret);
+		return ret;
+	}
+	return data & MAX8973_VOUT_MASK;
+}
+
+static int max8973_dcdc_set_voltage_sel(struct regulator_dev *rdev,
+	     unsigned vsel)
+{
+	struct max8973_chip *max = rdev_get_drvdata(rdev);
+	int ret;
+	bool found = false;
+	int vout_reg = max->curr_vout_reg;
+	int gpio_val = max->curr_gpio_val;
+
+	/*
+	 * If gpios are available to select the VOUT register then least
+	 * recently used register for new configuration.
+	 */
+	if (max->valid_dvs_gpio)
+		found = find_voltage_set_register(max, vsel,
+					&vout_reg, &gpio_val);
+
+	if (!found) {
+		ret = regmap_update_bits(max->regmap, vout_reg,
+					MAX8973_VOUT_MASK, vsel);
+		if (ret < 0) {
+			dev_err(max->dev, "register %d update failed, err %d\n",
+				 vout_reg, ret);
+			return ret;
+		}
+		max->curr_vout_reg = vout_reg;
+		max->curr_vout_val[gpio_val] = vsel;
+	}
+
+	/* Select proper VOUT register vio gpios */
+	if (max->valid_dvs_gpio) {
+		gpio_set_value_cansleep(max->dvs_gpio, gpio_val & 0x1);
+		max->curr_gpio_val = gpio_val;
+	}
+	return 0;
+}
+
+static int max8973_dcdc_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	struct max8973_chip *max = rdev_get_drvdata(rdev);
+	int ret;
+	int pwm;
+
+	/* Enable force PWM mode in FAST mode only. */
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		pwm = MAX8973_FPWM_EN_M;
+		break;
+
+	case REGULATOR_MODE_NORMAL:
+		pwm = 0;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(max->regmap, MAX8973_CONTROL1,
+				MAX8973_FPWM_EN_M, pwm);
+	if (ret < 0)
+		dev_err(max->dev, "register %d update failed, err %d\n",
+				MAX8973_CONTROL1, ret);
+	return ret;
+}
+
+static unsigned int max8973_dcdc_get_mode(struct regulator_dev *rdev)
+{
+	struct max8973_chip *max = rdev_get_drvdata(rdev);
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(max->regmap, MAX8973_CONTROL1, &data);
+	if (ret < 0) {
+		dev_err(max->dev, "register %d read failed, err %d\n",
+				MAX8973_CONTROL1, ret);
+		return ret;
+	}
+	return (data & MAX8973_FPWM_EN_M) ?
+		REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
+}
+
+static struct regulator_ops max8973_dcdc_ops = {
+	.get_voltage_sel	= max8973_dcdc_get_voltage_sel,
+	.set_voltage_sel	= max8973_dcdc_set_voltage_sel,
+	.list_voltage		= regulator_list_voltage_linear,
+	.set_mode		= max8973_dcdc_set_mode,
+	.get_mode		= max8973_dcdc_get_mode,
+};
+
+static int __devinit max8973_init_dcdc(struct max8973_chip *max,
+		struct max8973_regulator_platform_data *pdata)
+{
+	int ret;
+	uint8_t	control1 = 0;
+	uint8_t control2 = 0;
+
+	if (pdata->control_flags & MAX8973_CONTROL_REMOTE_SENSE_ENABLE)
+		control1 |= MAX8973_SNS_ENABLE;
+
+	if (!(pdata->control_flags & MAX8973_CONTROL_FALLING_SLEW_RATE_ENABLE))
+		control1 |= MAX8973_NFSR_ENABLE;
+
+	if (pdata->control_flags & MAX8973_CONTROL_OUTPUT_ACTIVE_DISCH_ENABLE)
+		control1 |= MAX8973_AD_ENABLE;
+
+	if (pdata->control_flags & MAX8973_CONTROL_BIAS_ENABLE)
+		control1 |= MAX8973_BIAS_ENABLE;
+
+	if (pdata->control_flags & MAX8973_CONTROL_FREQ_SHIFT_9PER_ENABLE)
+		control1 |= MAX8973_FREQSHIFT_9PER;
+
+	/* Set ramp delay */
+	if (pdata->reg_init_data &&
+			pdata->reg_init_data->constraints.ramp_delay) {
+		if (pdata->reg_init_data->constraints.ramp_delay < 25000)
+			control1 = MAX8973_RAMP_12mV_PER_US;
+		else if (pdata->reg_init_data->constraints.ramp_delay < 50000)
+			control1 = MAX8973_RAMP_25mV_PER_US;
+		else if (pdata->reg_init_data->constraints.ramp_delay < 200000)
+			control1 = MAX8973_RAMP_50mV_PER_US;
+		else
+			control1 = MAX8973_RAMP_200mV_PER_US;
+	} else {
+		control1 = MAX8973_RAMP_12mV_PER_US;
+		max->desc.ramp_delay = 12500;
+	}
+
+	if (!(pdata->control_flags & MAX8973_CONTROL_PULL_DOWN_ENABLE))
+		control2 |= MAX8973_DISCH_ENBABLE;
+
+	/*  Clock advance trip configuration */
+	switch (pdata->control_flags & MAX8973_CONTROL_CLKADV_TRIP_MASK) {
+	case MAX8973_CONTROL_CLKADV_TRIP_DISABLED:
+		control2 |= MAX8973_CKKADV_TRIP_DISABLE;
+		break;
+
+	case MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US:
+		control2 |= MAX8973_CKKADV_TRIP_75mV_PER_US;
+		break;
+
+	case MAX8973_CONTROL_CLKADV_TRIP_150mV_PER_US:
+		control2 |= MAX8973_CKKADV_TRIP_150mV_PER_US;
+		break;
+
+	case MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US_HIST_DIS:
+		control2 |= MAX8973_CKKADV_TRIP_75mV_PER_US_HIST_DIS;
+		break;
+	}
+
+	/* Configure inductor value */
+	switch (pdata->control_flags & MAX8973_CONTROL_INDUCTOR_VALUE_MASK) {
+	case MAX8973_CONTROL_INDUCTOR_VALUE_NOMINAL:
+		control2 |= MAX8973_INDUCTOR_NOMINAL;
+		break;
+
+	case MAX8973_CONTROL_INDUCTOR_VALUE_MINUS_30_PER:
+		control2 |= MAX8973_INDUCTOR_MIN_30_PER;
+		break;
+
+	case MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_30_PER:
+		control2 |= MAX8973_INDUCTOR_PLUS_30_PER;
+		break;
+
+	case MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_60_PER:
+		control2 |= MAX8973_INDUCTOR_PLUS_60_PER;
+		break;
+	}
+
+	ret = regmap_write(max->regmap, MAX8973_CONTROL1, control1);
+	if (ret < 0) {
+		dev_err(max->dev, "register %d write failed, err = %d",
+				MAX8973_CONTROL1, ret);
+		return ret;
+	}
+
+	ret = regmap_write(max->regmap, MAX8973_CONTROL2, control2);
+	if (ret < 0) {
+		dev_err(max->dev, "register %d write failed, err = %d",
+				MAX8973_CONTROL2, ret);
+		return ret;
+	}
+
+	/* If external control is enabled then disable EN bit */
+	if (max->enable_external_control) {
+		ret = regmap_update_bits(max->regmap, MAX8973_VOUT,
+						MAX8973_VOUT_ENABLE, 0);
+		if (ret < 0)
+			dev_err(max->dev, "register %d update failed, err = %d",
+				MAX8973_VOUT, ret);
+	}
+	return ret;
+}
+
+static const struct regmap_config max8973_regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= MAX8973_CHIPID2,
+	.cache_type		= REGCACHE_RBTREE,
+};
+
+static int __devinit max8973_probe(struct i2c_client *client,
+				     const struct i2c_device_id *id)
+{
+	struct max8973_regulator_platform_data *pdata;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct max8973_chip *max;
+	int ret;
+
+	pdata = client->dev.platform_data;
+	if (!pdata) {
+		dev_err(&client->dev, "No Platform data");
+		return -EIO;
+	}
+
+	max = devm_kzalloc(&client->dev, sizeof(*max), GFP_KERNEL);
+	if (!max) {
+		dev_err(&client->dev, "Memory allocation for max failed\n");
+		return -ENOMEM;
+	}
+
+	max->regmap = devm_regmap_init_i2c(client, &max8973_regmap_config);
+	if (IS_ERR(max->regmap)) {
+		ret = PTR_ERR(max->regmap);
+		dev_err(&client->dev, "regmap init failed, err %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, max);
+	max->dev = &client->dev;
+	max->desc.name = id->name;
+	max->desc.id = 0;
+	max->desc.ops = &max8973_dcdc_ops;
+	max->desc.type = REGULATOR_VOLTAGE;
+	max->desc.owner = THIS_MODULE;
+	max->desc.min_uV = MAX8973_MIN_VOLATGE;
+	max->desc.uV_step = MAX8973_VOLATGE_STEP;
+	max->desc.n_voltages = MAX8973_BUCK_N_VOLTAGE;
+
+	if (pdata->enable_ext_control) {
+		max->desc.enable_reg = MAX8973_VOUT;
+		max->desc.enable_mask = MAX8973_VOUT_ENABLE;
+		max8973_dcdc_ops.enable = regulator_is_enabled_regmap;
+		max8973_dcdc_ops.disable = regulator_disable_regmap;
+		max8973_dcdc_ops.is_enabled = regulator_is_enabled_regmap;
+	}
+
+	max->enable_external_control = pdata->enable_ext_control;
+	max->dvs_gpio = pdata->dvs_gpio;
+	max->curr_gpio_val = pdata->dvs_def_state;
+	max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
+	max->lru_index[0] = max->curr_vout_reg;
+	max->valid_dvs_gpio = false;
+
+	if (gpio_is_valid(max->dvs_gpio)) {
+		int gpio_flags;
+		int i;
+
+		gpio_flags = (pdata->dvs_def_state) ?
+				GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
+		ret = devm_gpio_request_one(&client->dev, max->dvs_gpio,
+				gpio_flags, "max8973-dvs");
+		if (ret) {
+			dev_err(&client->dev,
+				"gpio_request for gpio %d failed, err = %d\n",
+				max->dvs_gpio, ret);
+			return ret;
+		}
+		max->valid_dvs_gpio = true;
+
+		/*
+		 * Initialize the lru index with vout_reg id
+		 * The index 0 will be most recently used and
+		 * set with the max->curr_vout_reg */
+		for (i = 0; i < MAX8973_MAX_VOUT_REG; ++i)
+			max->lru_index[i] = i;
+		max->lru_index[0] = max->curr_vout_reg;
+		max->lru_index[max->curr_vout_reg] = 0;
+	}
+
+	ret = max8973_init_dcdc(max, pdata);
+	if (ret < 0) {
+		dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret);
+		return ret;
+	}
+
+	config.dev = &client->dev;
+	config.init_data = pdata->reg_init_data;
+	config.driver_data = max;
+	config.of_node = client->dev.of_node;
+	config.regmap = max->regmap;
+
+	/* Register the regulators */
+	rdev = regulator_register(&max->desc, &config);
+	if (IS_ERR(rdev)) {
+		ret = PTR_ERR(rdev);
+		dev_err(max->dev, "regulator register failed, err %d\n", ret);
+		return ret;
+	}
+
+	max->rdev = rdev;
+	return 0;
+}
+
+static int __devexit max8973_remove(struct i2c_client *client)
+{
+	struct max8973_chip *max = i2c_get_clientdata(client);
+
+	regulator_unregister(max->rdev);
+	return 0;
+}
+
+static const struct i2c_device_id max8973_id[] = {
+	{.name = "max8973",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, max8973_id);
+
+static struct i2c_driver max8973_i2c_driver = {
+	.driver = {
+		.name = "max8973",
+		.owner = THIS_MODULE,
+	},
+	.probe = max8973_probe,
+	.remove = __devexit_p(max8973_remove),
+	.id_table = max8973_id,
+};
+
+static int __init max8973_init(void)
+{
+	return i2c_add_driver(&max8973_i2c_driver);
+}
+subsys_initcall(max8973_init);
+
+static void __exit max8973_cleanup(void)
+{
+	i2c_del_driver(&max8973_i2c_driver);
+}
+module_exit(max8973_cleanup);
+
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_DESCRIPTION("MAX8973 voltage regulator driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/max8973-regulator.h b/include/linux/regulator/max8973-regulator.h
new file mode 100644
index 0000000..f8acc05
--- /dev/null
+++ b/include/linux/regulator/max8973-regulator.h
@@ -0,0 +1,72 @@
+/*
+ * max8973-regulator.h -- MAXIM 8973 regulator
+ *
+ * Interface for regulator driver for MAXIM 8973 DC-DC step-down
+ * switching regulator.
+ *
+ * Copyright (C) 2012 NVIDIA Corporation
+
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA	02110-1301, USA.
+ *
+ */
+
+#ifndef __LINUX_REGULATOR_MAX8973_H
+#define __LINUX_REGULATOR_MAX8973_H
+
+/*
+ * Control flags for configuration of the device.
+ * Client need to pass this information with ORed
+ */
+#define MAX8973_CONTROL_REMOTE_SENSE_ENABLE			0x00000001
+#define MAX8973_CONTROL_FALLING_SLEW_RATE_ENABLE		0x00000002
+#define MAX8973_CONTROL_OUTPUT_ACTIVE_DISCH_ENABLE		0x00000004
+#define MAX8973_CONTROL_BIAS_ENABLE				0x00000008
+#define MAX8973_CONTROL_PULL_DOWN_ENABLE			0x00000010
+#define MAX8973_CONTROL_FREQ_SHIFT_9PER_ENABLE			0x00000020
+
+#define MAX8973_CONTROL_CLKADV_TRIP_DISABLED			0x00000000
+#define MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US			0x00010000
+#define MAX8973_CONTROL_CLKADV_TRIP_150mV_PER_US		0x00020000
+#define MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US_HIST_DIS	0x00030000
+
+#define MAX8973_CONTROL_INDUCTOR_VALUE_NOMINAL			0x00000000
+#define MAX8973_CONTROL_INDUCTOR_VALUE_MINUS_30_PER		0x00100000
+#define MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_30_PER		0x00200000
+#define MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_60_PER		0x00300000
+
+/*
+ * struct max8973_regulator_platform_data - max8973 regulator platform data.
+ *
+ * @reg_init_data: The regulator init data.
+ * @control_flags: Control flags which are ORed value of above flags to
+ *		configure device.
+ * @enable_ext_control: Enable the voltage enable/disable through external
+ *		control signal from EN input pin. If it is false then
+ *		voltage output will be enabled/disabled through EN bit of
+ *		device register.
+ * @dvs_gpio: GPIO for dvs. It should be -1 if this is tied with fixed logic.
+ * @dvs_def_state: Default state of dvs. 1 if it is high else 0.
+ */
+struct max8973_regulator_platform_data {
+	struct regulator_init_data *reg_init_data;
+	unsigned long control_flags;
+	bool enable_ext_control;
+	int dvs_gpio;
+	unsigned dvs_def_state:1;
+};
+
+#endif /* __LINUX_REGULATOR_MAX8973_H */
-- 
1.7.1.1

Re: [PATCH] regulator: max8973: add regulator driver support

[Mark Brown]

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

On Mon, Nov 19, 2012 at 06:58:29AM +0530, Laxman Dewangan wrote:
> The MAXIM MAX8973 high-efficiency, three phase, DC-DC step-down
> switching regulator delievers up to 9A of output current. Each
> phase operates at a 2MHz fixed frequency with a 120 deg shift
> from the adjacent phase, allowing the use of small magnetic
> component.

Applied, thanks.

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

Re: [PATCH] regulator: max8973: add regulator driver support

[Guennadi Liakhovetski]

Hi Laxman

On Mon, 19 Nov 2012, Laxman Dewangan wrote:

> The MAXIM MAX8973 high-efficiency, three phase, DC-DC step-down
> switching regulator delievers up to 9A of output current. Each
> phase operates at a 2MHz fixed frequency with a 120 deg shift
> from the adjacent phase, allowing the use of small magnetic
> component.

Thanks for submitting this driver! The notion of DVS regulators was new to 
me, so, I checked http://www.ti.com/lit/an/sbva020/sbva020.pdf for a short 
description. After that I had a look at a couple of existing DVS regulator 
drivers in the tree. Well, I came to two conclusions so far: (1) The 
current regulator API is not very well suitable for such regulators. I 
would imagine, one would need two methods: for setting the "normal" and 
the DVS voltage. Instead of this drivers are trying to be smart at 
guessing, which voltage the user is trying to set now... (2) Drivers do 
this in different ways and at least out of the 2 drivers I looked at both 
have bugs and different ones at that. I'll send a separate email, 
describing what I found suspicious in them.

Of course, all the above was just my DVS-newbie impression, which can very 
well be absolutely wrong.

> 
> Add regulator driver for this device.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/regulator/Kconfig                   |   10 +
>  drivers/regulator/Makefile                  |    1 +
>  drivers/regulator/max8973-regulator.c       |  505 +++++++++++++++++++++++++++
>  include/linux/regulator/max8973-regulator.h |   72 ++++
>  4 files changed, 588 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/max8973-regulator.c
>  create mode 100644 include/linux/regulator/max8973-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index cbc685d..ddd8869 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -204,6 +204,16 @@ config REGULATOR_MAX8952
>  	  via I2C bus. Maxim 8952 has one voltage output and supports 4 DVS
>  	  modes ranging from 0.77V to 1.40V by 0.01V steps.
>  
> +config REGULATOR_MAX8973
> +	tristate "Maxim MAX8973 voltage regulator "
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  The MAXIM MAX8973 high-efficiency. three phase, DC-DC step-down
> +	  switching regulator delievers up to 9A of output current. Each
> +	  phase operates at a 2MHz fixed frequency with a 120 deg shift
> +	  from the adjacent phase, allowing the use of small magnetic component.
> +
>  config REGULATOR_MAX8997
>  	tristate "Maxim 8997/8966 regulator"
>  	depends on MFD_MAX8997
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index db274dc..ac093f3 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o
>  obj-$(CONFIG_REGULATOR_MAX8907) += max8907-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
> +obj-$(CONFIG_REGULATOR_MAX8973) += max8973-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
>  obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
>  obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
> diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
> new file mode 100644
> index 0000000..e6328f9
> --- /dev/null
> +++ b/drivers/regulator/max8973-regulator.c
> @@ -0,0 +1,505 @@
> +/*
> + * max8973-regulator.c -- Maxim max8973
> + *
> + * Regulator driver for MAXIM 8973 DC-DC step-down switching regulator.
> + *
> + * Copyright (c) 2012, NVIDIA Corporation.
> + *
> + * Author: Laxman Dewangan <ldewangan@nvidia.com>
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + * 02111-1307, USA
> + */
> +
> +#include <linux/kernel.h>
> +#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/regulator/max8973-regulator.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +
> +/* Register definitions */
> +#define MAX8973_VOUT					0x0
> +#define MAX8973_VOUT_DVS				0x1

You're lucky, that your first voltage-set register address is 0. This is used 
multiple times in the driver and is very confusing. Repeatedly offsets and 
addresses are mixed up. So, if you had a non-0 address it would not work. 
Also the fact, that the DVS voltage set register has address == 1 is used 
implicitly... To reduce confusion you could introduce a macro, say,
#define MAX8973_VOUT_BASE 0
and use it explicitly to differentiate between addresses and offsets.

> +#define MAX8973_CONTROL1				0x2
> +#define MAX8973_CONTROL2				0x3
> +#define MAX8973_CHIPID1					0x4
> +#define MAX8973_CHIPID2					0x5
> +
> +#define MAX8973_MAX_VOUT_REG				2

MAX8973_MAX_VOUT_REG is the number of voltage-set registers

> +
> +/* MAX8973_VOUT */
> +#define MAX8973_VOUT_ENABLE				BIT(7)
> +#define MAX8973_VOUT_MASK				0x7F
> +
> +/* MAX8973_VOUT_DVS */
> +#define MAX8973_DVS_VOUT_MASK				0x7F
> +
> +/* MAX8973_CONTROL1 */
> +#define MAX8973_SNS_ENABLE				BIT(7)
> +#define MAX8973_FPWM_EN_M				BIT(6)
> +#define MAX8973_NFSR_ENABLE				BIT(5)
> +#define MAX8973_AD_ENABLE				BIT(4)
> +#define MAX8973_BIAS_ENABLE				BIT(3)
> +#define MAX8973_FREQSHIFT_9PER				BIT(2)
> +
> +#define MAX8973_RAMP_12mV_PER_US			0x0
> +#define MAX8973_RAMP_25mV_PER_US			0x1
> +#define MAX8973_RAMP_50mV_PER_US			0x2
> +#define MAX8973_RAMP_200mV_PER_US			0x3
> +
> +/* MAX8973_CONTROL2 */
> +#define MAX8973_WDTMR_ENABLE				BIT(6)
> +#define MAX8973_DISCH_ENBABLE				BIT(5)
> +#define MAX8973_FT_ENABLE				BIT(4)
> +
> +#define MAX8973_CKKADV_TRIP_DISABLE			0xC
> +#define MAX8973_CKKADV_TRIP_75mV_PER_US			0x0
> +#define MAX8973_CKKADV_TRIP_150mV_PER_US		0x4
> +#define MAX8973_CKKADV_TRIP_75mV_PER_US_HIST_DIS	0x8
> +#define MAX8973_CONTROL_CLKADV_TRIP_MASK		0x00030000
> +
> +#define MAX8973_INDUCTOR_MIN_30_PER			0x0
> +#define MAX8973_INDUCTOR_NOMINAL			0x1
> +#define MAX8973_INDUCTOR_PLUS_30_PER			0x2
> +#define MAX8973_INDUCTOR_PLUS_60_PER			0x3
> +#define MAX8973_CONTROL_INDUCTOR_VALUE_MASK		0x00300000
> +
> +#define MAX8973_MIN_VOLATGE				606250
> +#define MAX8973_MAX_VOLATGE				1400000
> +#define MAX8973_VOLATGE_STEP				6250
> +#define MAX8973_BUCK_N_VOLTAGE				0x80
> +
> +/* Maxim 8973 chip information */
> +struct max8973_chip {
> +	struct device *dev;
> +	struct regulator_desc desc;
> +	struct regulator_dev *rdev;
> +	struct regmap *regmap;
> +	bool enable_external_control;
> +	int dvs_gpio;
> +	int lru_index[MAX8973_MAX_VOUT_REG];

lru_index[] is an array with voltage-set register address offsets, i.e. its 
members can take values MAX8973_VOUT - MAX8973_VOUT_BASE and
MAX8973_VOUT_DVS - MAX8973_VOUT_BASE

> +	int curr_vout_val[MAX8973_MAX_VOUT_REG];

curr_vout_val[] is an array with voltage selectors for above registers

> +	int curr_vout_reg;

The above is used both as an offset and as a register address. Let's say it 
is intended to be a register address

> +	int curr_gpio_val;

This one is confusing too. Judging by the name it seems it should mean a 
GPIO value, so, should take values "high" and "low."

> +	bool valid_dvs_gpio;
> +};
> +
> +/*
> + * find_voltage_set_register: Find new voltage configuration register (VOUT).
> + * The finding of the new VOUT register will be based on the LRU mechanism.
> + * Each VOUT register will have different voltage configured . This
> + * Function will look if any of the VOUT register have requested voltage set
> + * or not.
> + *     - If it is already there then it will make that register as most
> + *       recently used and return as found so that caller need not to set
> + *       the VOUT register but need to set the proper gpios to select this
> + *       VOUT register.
> + *     - If requested voltage is not found then it will use the least
> + *       recently mechanism to get new VOUT register for new configuration
> + *       and will return not_found so that caller need to set new VOUT
> + *       register and then gpios (both).
> + */
> +static bool find_voltage_set_register(struct max8973_chip *tps,
> +		int req_vsel, int *vout_reg, int *gpio_val)
> +{
> +	int i;
> +	bool found = false;
> +	int new_vout_reg = tps->lru_index[MAX8973_MAX_VOUT_REG - 1];

new_vout_reg is a register address offset

> +	int found_index = MAX8973_MAX_VOUT_REG - 1;
> +
> +	for (i = 0; i < MAX8973_MAX_VOUT_REG; ++i) {
> +		if (tps->curr_vout_val[tps->lru_index[i]] == req_vsel) {
> +			new_vout_reg = tps->lru_index[i];
> +			found_index = i;
> +			found = true;
> +			goto update_lru_index;

Just use "break"

> +		}
> +	}
> +
> +update_lru_index:
> +	for (i = found_index; i > 0; i--)
> +		tps->lru_index[i] = tps->lru_index[i - 1];
> +
> +	tps->lru_index[0] = new_vout_reg;
> +	*gpio_val = new_vout_reg;

Hm, what does a register address offset have to do with a GPIO value??

> +	*vout_reg = MAX8973_VOUT + new_vout_reg;
> +	return found;
> +}
> +
> +static int max8973_dcdc_get_voltage_sel(struct regulator_dev *rdev)
> +{
> +	struct max8973_chip *max = rdev_get_drvdata(rdev);
> +	unsigned int data;
> +	int ret;
> +
> +	ret = regmap_read(max->regmap, max->curr_vout_reg, &data);
> +	if (ret < 0) {
> +		dev_err(max->dev, "register %d read failed, err = %d\n",
> +			max->curr_vout_reg, ret);
> +		return ret;
> +	}
> +	return data & MAX8973_VOUT_MASK;
> +}
> +
> +static int max8973_dcdc_set_voltage_sel(struct regulator_dev *rdev,
> +	     unsigned vsel)
> +{
> +	struct max8973_chip *max = rdev_get_drvdata(rdev);
> +	int ret;
> +	bool found = false;
> +	int vout_reg = max->curr_vout_reg;
> +	int gpio_val = max->curr_gpio_val;
> +
> +	/*
> +	 * If gpios are available to select the VOUT register then least
> +	 * recently used register for new configuration.
> +	 */
> +	if (max->valid_dvs_gpio)
> +		found = find_voltage_set_register(max, vsel,
> +					&vout_reg, &gpio_val);
> +
> +	if (!found) {
> +		ret = regmap_update_bits(max->regmap, vout_reg,
> +					MAX8973_VOUT_MASK, vsel);
> +		if (ret < 0) {
> +			dev_err(max->dev, "register %d update failed, err %d\n",
> +				 vout_reg, ret);
> +			return ret;
> +		}
> +		max->curr_vout_reg = vout_reg;
> +		max->curr_vout_val[gpio_val] = vsel;

Why is the gpio_val used as an index into the curr_vout_val[] array, which 
should actually be indexed by voltage set register address offsets?

> +	}
> +
> +	/* Select proper VOUT register vio gpios */
> +	if (max->valid_dvs_gpio) {
> +		gpio_set_value_cansleep(max->dvs_gpio, gpio_val & 0x1);

What does odd / even have to do with the GPIO level selection? I think, an 
attempt of a generic approach with multiple voltage set registers and a 
generic multiplexor is confused here with its special case of just two such 
registers, in which case the multiplexor is controlled by just one GPIO. 
You can keep some parts of the code generic, although I'm not sure it's 
worth doing here, since DVS in general and this regulator specifically do 
only deal with two voltage values. So, I would probably just explicitly go 
for either voltage #1 or voltage #2, not even trying to pretend, that the 
driver can handle arbitrary mane of them.

> +		max->curr_gpio_val = gpio_val;
> +	}
> +	return 0;
> +}
> +
> +static int max8973_dcdc_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> +	struct max8973_chip *max = rdev_get_drvdata(rdev);
> +	int ret;
> +	int pwm;
> +
> +	/* Enable force PWM mode in FAST mode only. */
> +	switch (mode) {
> +	case REGULATOR_MODE_FAST:
> +		pwm = MAX8973_FPWM_EN_M;
> +		break;
> +
> +	case REGULATOR_MODE_NORMAL:
> +		pwm = 0;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(max->regmap, MAX8973_CONTROL1,
> +				MAX8973_FPWM_EN_M, pwm);
> +	if (ret < 0)
> +		dev_err(max->dev, "register %d update failed, err %d\n",
> +				MAX8973_CONTROL1, ret);
> +	return ret;
> +}
> +
> +static unsigned int max8973_dcdc_get_mode(struct regulator_dev *rdev)
> +{
> +	struct max8973_chip *max = rdev_get_drvdata(rdev);
> +	unsigned int data;
> +	int ret;
> +
> +	ret = regmap_read(max->regmap, MAX8973_CONTROL1, &data);
> +	if (ret < 0) {
> +		dev_err(max->dev, "register %d read failed, err %d\n",
> +				MAX8973_CONTROL1, ret);
> +		return ret;
> +	}
> +	return (data & MAX8973_FPWM_EN_M) ?
> +		REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
> +}
> +
> +static struct regulator_ops max8973_dcdc_ops = {
> +	.get_voltage_sel	= max8973_dcdc_get_voltage_sel,
> +	.set_voltage_sel	= max8973_dcdc_set_voltage_sel,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.set_mode		= max8973_dcdc_set_mode,
> +	.get_mode		= max8973_dcdc_get_mode,
> +};
> +
> +static int __devinit max8973_init_dcdc(struct max8973_chip *max,
> +		struct max8973_regulator_platform_data *pdata)
> +{
> +	int ret;
> +	uint8_t	control1 = 0;
> +	uint8_t control2 = 0;
> +
> +	if (pdata->control_flags & MAX8973_CONTROL_REMOTE_SENSE_ENABLE)
> +		control1 |= MAX8973_SNS_ENABLE;
> +
> +	if (!(pdata->control_flags & MAX8973_CONTROL_FALLING_SLEW_RATE_ENABLE))
> +		control1 |= MAX8973_NFSR_ENABLE;
> +
> +	if (pdata->control_flags & MAX8973_CONTROL_OUTPUT_ACTIVE_DISCH_ENABLE)
> +		control1 |= MAX8973_AD_ENABLE;
> +
> +	if (pdata->control_flags & MAX8973_CONTROL_BIAS_ENABLE)
> +		control1 |= MAX8973_BIAS_ENABLE;
> +
> +	if (pdata->control_flags & MAX8973_CONTROL_FREQ_SHIFT_9PER_ENABLE)
> +		control1 |= MAX8973_FREQSHIFT_9PER;
> +
> +	/* Set ramp delay */
> +	if (pdata->reg_init_data &&
> +			pdata->reg_init_data->constraints.ramp_delay) {
> +		if (pdata->reg_init_data->constraints.ramp_delay < 25000)
> +			control1 = MAX8973_RAMP_12mV_PER_US;
> +		else if (pdata->reg_init_data->constraints.ramp_delay < 50000)
> +			control1 = MAX8973_RAMP_25mV_PER_US;
> +		else if (pdata->reg_init_data->constraints.ramp_delay < 200000)
> +			control1 = MAX8973_RAMP_50mV_PER_US;
> +		else
> +			control1 = MAX8973_RAMP_200mV_PER_US;
> +	} else {
> +		control1 = MAX8973_RAMP_12mV_PER_US;
> +		max->desc.ramp_delay = 12500;
> +	}
> +
> +	if (!(pdata->control_flags & MAX8973_CONTROL_PULL_DOWN_ENABLE))
> +		control2 |= MAX8973_DISCH_ENBABLE;
> +
> +	/*  Clock advance trip configuration */
> +	switch (pdata->control_flags & MAX8973_CONTROL_CLKADV_TRIP_MASK) {
> +	case MAX8973_CONTROL_CLKADV_TRIP_DISABLED:
> +		control2 |= MAX8973_CKKADV_TRIP_DISABLE;
> +		break;
> +
> +	case MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US:
> +		control2 |= MAX8973_CKKADV_TRIP_75mV_PER_US;
> +		break;
> +
> +	case MAX8973_CONTROL_CLKADV_TRIP_150mV_PER_US:
> +		control2 |= MAX8973_CKKADV_TRIP_150mV_PER_US;
> +		break;
> +
> +	case MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US_HIST_DIS:
> +		control2 |= MAX8973_CKKADV_TRIP_75mV_PER_US_HIST_DIS;
> +		break;
> +	}
> +
> +	/* Configure inductor value */
> +	switch (pdata->control_flags & MAX8973_CONTROL_INDUCTOR_VALUE_MASK) {
> +	case MAX8973_CONTROL_INDUCTOR_VALUE_NOMINAL:
> +		control2 |= MAX8973_INDUCTOR_NOMINAL;
> +		break;
> +
> +	case MAX8973_CONTROL_INDUCTOR_VALUE_MINUS_30_PER:
> +		control2 |= MAX8973_INDUCTOR_MIN_30_PER;
> +		break;
> +
> +	case MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_30_PER:
> +		control2 |= MAX8973_INDUCTOR_PLUS_30_PER;
> +		break;
> +
> +	case MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_60_PER:
> +		control2 |= MAX8973_INDUCTOR_PLUS_60_PER;
> +		break;
> +	}
> +
> +	ret = regmap_write(max->regmap, MAX8973_CONTROL1, control1);
> +	if (ret < 0) {
> +		dev_err(max->dev, "register %d write failed, err = %d",
> +				MAX8973_CONTROL1, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(max->regmap, MAX8973_CONTROL2, control2);
> +	if (ret < 0) {
> +		dev_err(max->dev, "register %d write failed, err = %d",
> +				MAX8973_CONTROL2, ret);
> +		return ret;
> +	}
> +
> +	/* If external control is enabled then disable EN bit */
> +	if (max->enable_external_control) {
> +		ret = regmap_update_bits(max->regmap, MAX8973_VOUT,
> +						MAX8973_VOUT_ENABLE, 0);
> +		if (ret < 0)
> +			dev_err(max->dev, "register %d update failed, err = %d",
> +				MAX8973_VOUT, ret);
> +	}
> +	return ret;
> +}
> +
> +static const struct regmap_config max8973_regmap_config = {
> +	.reg_bits		= 8,
> +	.val_bits		= 8,
> +	.max_register		= MAX8973_CHIPID2,
> +	.cache_type		= REGCACHE_RBTREE,
> +};
> +
> +static int __devinit max8973_probe(struct i2c_client *client,
> +				     const struct i2c_device_id *id)
> +{
> +	struct max8973_regulator_platform_data *pdata;
> +	struct regulator_config config = { };
> +	struct regulator_dev *rdev;
> +	struct max8973_chip *max;
> +	int ret;
> +
> +	pdata = client->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&client->dev, "No Platform data");
> +		return -EIO;
> +	}
> +
> +	max = devm_kzalloc(&client->dev, sizeof(*max), GFP_KERNEL);
> +	if (!max) {
> +		dev_err(&client->dev, "Memory allocation for max failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	max->regmap = devm_regmap_init_i2c(client, &max8973_regmap_config);
> +	if (IS_ERR(max->regmap)) {
> +		ret = PTR_ERR(max->regmap);
> +		dev_err(&client->dev, "regmap init failed, err %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, max);
> +	max->dev = &client->dev;
> +	max->desc.name = id->name;
> +	max->desc.id = 0;

Don't you have to be able to process multiple such devices?

> +	max->desc.ops = &max8973_dcdc_ops;
> +	max->desc.type = REGULATOR_VOLTAGE;
> +	max->desc.owner = THIS_MODULE;
> +	max->desc.min_uV = MAX8973_MIN_VOLATGE;
> +	max->desc.uV_step = MAX8973_VOLATGE_STEP;
> +	max->desc.n_voltages = MAX8973_BUCK_N_VOLTAGE;
> +
> +	if (pdata->enable_ext_control) {
> +		max->desc.enable_reg = MAX8973_VOUT;
> +		max->desc.enable_mask = MAX8973_VOUT_ENABLE;
> +		max8973_dcdc_ops.enable = regulator_is_enabled_regmap;
> +		max8973_dcdc_ops.disable = regulator_disable_regmap;
> +		max8973_dcdc_ops.is_enabled = regulator_is_enabled_regmap;
> +	}
> +
> +	max->enable_external_control = pdata->enable_ext_control;
> +	max->dvs_gpio = pdata->dvs_gpio;
> +	max->curr_gpio_val = pdata->dvs_def_state;
> +	max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
> +	max->lru_index[0] = max->curr_vout_reg;

Here you actually need an offset within your register address space, so, 
should be

+	max->lru_index[0] = pdata->dvs_def_state;

> +	max->valid_dvs_gpio = false;
> +
> +	if (gpio_is_valid(max->dvs_gpio)) {
> +		int gpio_flags;
> +		int i;
> +
> +		gpio_flags = (pdata->dvs_def_state) ?
> +				GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
> +		ret = devm_gpio_request_one(&client->dev, max->dvs_gpio,
> +				gpio_flags, "max8973-dvs");
> +		if (ret) {
> +			dev_err(&client->dev,
> +				"gpio_request for gpio %d failed, err = %d\n",
> +				max->dvs_gpio, ret);
> +			return ret;
> +		}
> +		max->valid_dvs_gpio = true;
> +
> +		/*
> +		 * Initialize the lru index with vout_reg id
> +		 * The index 0 will be most recently used and
> +		 * set with the max->curr_vout_reg */
> +		for (i = 0; i < MAX8973_MAX_VOUT_REG; ++i)
> +			max->lru_index[i] = i;
> +		max->lru_index[0] = max->curr_vout_reg;

This is already done above

> +		max->lru_index[max->curr_vout_reg] = 0;

Sorry, what does this mean? Is "current" at index 0 or is it at index, 
corresponding to curr_vout_reg? This whole Least Recently Used algorithm 
is pretty, but seems unneeded here, if you do decide to limit yourself to 
hard-coded 2 values: DVS and "normal." But that is coming back to the 
general question at the top of this mail.

> +	}
> +
> +	ret = max8973_init_dcdc(max, pdata);
> +	if (ret < 0) {
> +		dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	config.dev = &client->dev;
> +	config.init_data = pdata->reg_init_data;
> +	config.driver_data = max;
> +	config.of_node = client->dev.of_node;
> +	config.regmap = max->regmap;
> +
> +	/* Register the regulators */
> +	rdev = regulator_register(&max->desc, &config);
> +	if (IS_ERR(rdev)) {
> +		ret = PTR_ERR(rdev);
> +		dev_err(max->dev, "regulator register failed, err %d\n", ret);
> +		return ret;
> +	}
> +
> +	max->rdev = rdev;
> +	return 0;
> +}
> +
> +static int __devexit max8973_remove(struct i2c_client *client)
> +{
> +	struct max8973_chip *max = i2c_get_clientdata(client);
> +
> +	regulator_unregister(max->rdev);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max8973_id[] = {
> +	{.name = "max8973",},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, max8973_id);
> +
> +static struct i2c_driver max8973_i2c_driver = {
> +	.driver = {
> +		.name = "max8973",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = max8973_probe,
> +	.remove = __devexit_p(max8973_remove),
> +	.id_table = max8973_id,
> +};
> +
> +static int __init max8973_init(void)
> +{
> +	return i2c_add_driver(&max8973_i2c_driver);
> +}
> +subsys_initcall(max8973_init);
> +
> +static void __exit max8973_cleanup(void)
> +{
> +	i2c_del_driver(&max8973_i2c_driver);
> +}
> +module_exit(max8973_cleanup);
> +
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
> +MODULE_DESCRIPTION("MAX8973 voltage regulator driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/regulator/max8973-regulator.h b/include/linux/regulator/max8973-regulator.h
> new file mode 100644
> index 0000000..f8acc05
> --- /dev/null
> +++ b/include/linux/regulator/max8973-regulator.h
> @@ -0,0 +1,72 @@
> +/*
> + * max8973-regulator.h -- MAXIM 8973 regulator
> + *
> + * Interface for regulator driver for MAXIM 8973 DC-DC step-down
> + * switching regulator.
> + *
> + * Copyright (C) 2012 NVIDIA Corporation
> +
> + * Author: Laxman Dewangan <ldewangan@nvidia.com>
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA	02110-1301, USA.
> + *
> + */
> +
> +#ifndef __LINUX_REGULATOR_MAX8973_H
> +#define __LINUX_REGULATOR_MAX8973_H
> +
> +/*
> + * Control flags for configuration of the device.
> + * Client need to pass this information with ORed
> + */
> +#define MAX8973_CONTROL_REMOTE_SENSE_ENABLE			0x00000001
> +#define MAX8973_CONTROL_FALLING_SLEW_RATE_ENABLE		0x00000002
> +#define MAX8973_CONTROL_OUTPUT_ACTIVE_DISCH_ENABLE		0x00000004
> +#define MAX8973_CONTROL_BIAS_ENABLE				0x00000008
> +#define MAX8973_CONTROL_PULL_DOWN_ENABLE			0x00000010
> +#define MAX8973_CONTROL_FREQ_SHIFT_9PER_ENABLE			0x00000020
> +
> +#define MAX8973_CONTROL_CLKADV_TRIP_DISABLED			0x00000000
> +#define MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US			0x00010000
> +#define MAX8973_CONTROL_CLKADV_TRIP_150mV_PER_US		0x00020000
> +#define MAX8973_CONTROL_CLKADV_TRIP_75mV_PER_US_HIST_DIS	0x00030000
> +
> +#define MAX8973_CONTROL_INDUCTOR_VALUE_NOMINAL			0x00000000
> +#define MAX8973_CONTROL_INDUCTOR_VALUE_MINUS_30_PER		0x00100000
> +#define MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_30_PER		0x00200000
> +#define MAX8973_CONTROL_INDUCTOR_VALUE_PLUS_60_PER		0x00300000
> +
> +/*
> + * struct max8973_regulator_platform_data - max8973 regulator platform data.
> + *
> + * @reg_init_data: The regulator init data.
> + * @control_flags: Control flags which are ORed value of above flags to
> + *		configure device.
> + * @enable_ext_control: Enable the voltage enable/disable through external
> + *		control signal from EN input pin. If it is false then
> + *		voltage output will be enabled/disabled through EN bit of
> + *		device register.
> + * @dvs_gpio: GPIO for dvs. It should be -1 if this is tied with fixed logic.
> + * @dvs_def_state: Default state of dvs. 1 if it is high else 0.
> + */
> +struct max8973_regulator_platform_data {
> +	struct regulator_init_data *reg_init_data;
> +	unsigned long control_flags;
> +	bool enable_ext_control;
> +	int dvs_gpio;
> +	unsigned dvs_def_state:1;
> +};
> +
> +#endif /* __LINUX_REGULATOR_MAX8973_H */
> -- 
> 1.7.1.1
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] regulator: max8973: add regulator driver support

[Mark Brown]

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

On Mon, Nov 19, 2012 at 11:52:42AM +0100, Guennadi Liakhovetski wrote:

> Thanks for submitting this driver! The notion of DVS regulators was new to 
> me, so, I checked http://www.ti.com/lit/an/sbva020/sbva020.pdf for a short 
> description. After that I had a look at a couple of existing DVS regulator 

Do you just mean regulators that have a quick voltage change ability/

> drivers in the tree. Well, I came to two conclusions so far: (1) The 
> current regulator API is not very well suitable for such regulators. I 
> would imagine, one would need two methods: for setting the "normal" and 
> the DVS voltage. Instead of this drivers are trying to be smart at 
> guessing, which voltage the user is trying to set now... (2) Drivers do 
> this in different ways and at least out of the 2 drivers I looked at both 
> have bugs and different ones at that. I'll send a separate email, 
> describing what I found suspicious in them.

The thing I'd like to see factored out here is the LRU mechanism,
otherwise I think the situation is pretty good.  Some of the older
devices should use a different scheme to modern ones as the hardware
they have to interoperate is different.

> Of course, all the above was just my DVS-newbie impression, which can very 
> well be absolutely wrong.
> 
> > 
> > Add regulator driver for this device.

*ALWAYS* delete irrelevant text when replying.

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

Re: [PATCH] regulator: max8973: add regulator driver support

[Laxman Dewangan]

On Monday 19 November 2012 04:22 PM, Guennadi Liakhovetski wrote:
>
> Hi Laxman
>
> drivers in the tree. Well, I came to two conclusions so far: (1) The
> current regulator API is not very well suitable for such regulators. I
> would imagine, one would need two methods: for setting the "normal" and
> the DVS voltage. Instead of this drivers are trying to be smart at
> guessing, which voltage the user is trying to set now... (2) Drivers do
> this in different ways and at least out of the 2 drivers I looked at both
> have bugs and different ones at that. I'll send a separate email,
> describing what I found suspicious in them.
>
> Of course, all the above was just my DVS-newbie impression, which can very
> well be absolutely wrong.
>

If there is multipel VOUT register for single vout then these registers 
are generally selected by the input pin of device.
In a given system, you can connect the gpios pin to this input pins to 
select the proper VOUT register.
The register update through i2c consume more time and changing the gpio 
state is comparatively less.
So if you have let say 4 voltages 1.10, 1.11, 1.12, 1.13 and having 4 
VOUT register. Then program these vout val to vout reg like 1.10 to 
vout_reg0, 1.11 to vout_reg1 etc.
Now for changing voltage between these will just require to change the 
gpio pin state, not the register update and so it will be faster. You 
can achive the voltage change by gpio pin state change.

Now if your DVS have more volatge scaling then you can use the LRU 
mechanism to use the vout register for this new value.



>> +     max->dev =&client->dev;
>> +     max->desc.name = id->name;
>> +     max->desc.id = 0;
> Don't you have to be able to process multiple such devices?

Not really require as device have only one output. The different devices 
will have different registrations and so does not matter here.




>> +     max->enable_external_control = pdata->enable_ext_control;
>> +     max->dvs_gpio = pdata->dvs_gpio;
>> +     max->curr_gpio_val = pdata->dvs_def_state;
>> +     max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
>> +     max->lru_index[0] = max->curr_vout_reg;
> Here you actually need an offset within your register address space, so,
> should be
>
> +       max->lru_index[0] = pdata->dvs_def_state;


Yaah, seems some issue if vout_base is not zero. But really dont require 
here as MAX8973_VOUT is 0 in this case.

Re: [PATCH] regulator: max8973: add regulator driver support

[Guennadi Liakhovetski]

On Tue, 20 Nov 2012, Mark Brown wrote:

> On Mon, Nov 19, 2012 at 11:52:42AM +0100, Guennadi Liakhovetski wrote:
> 
> > Thanks for submitting this driver! The notion of DVS regulators was new to 
> > me, so, I checked http://www.ti.com/lit/an/sbva020/sbva020.pdf for a short 
> > description. After that I had a look at a couple of existing DVS regulator 
> 
> Do you just mean regulators that have a quick voltage change ability/

Yes, regulators with an input pin, that can be used to switch output 
voltage on one of outputs.

> > drivers in the tree. Well, I came to two conclusions so far: (1) The 
> > current regulator API is not very well suitable for such regulators. I 
> > would imagine, one would need two methods: for setting the "normal" and 
> > the DVS voltage. Instead of this drivers are trying to be smart at 
> > guessing, which voltage the user is trying to set now... (2) Drivers do 
> > this in different ways and at least out of the 2 drivers I looked at both 
> > have bugs and different ones at that. I'll send a separate email, 
> > describing what I found suspicious in them.
> 
> The thing I'd like to see factored out here is the LRU mechanism,
> otherwise I think the situation is pretty good.  Some of the older
> devices should use a different scheme to modern ones as the hardware
> they have to interoperate is different.

So, do you consider the LRU algorithm to be the preferred way to configure 
such regulators? I realise that in practice it will work well in most 
cases, usually users do only want to preconfigure such a regulator to 2 
fixed voltages and switch between them at runtime, right? OTOH, do you 
think it is too unlikely, that someone will want to switch, say, between 3 
voltages: X-Y-Z-X-Y-Z-X...? In this case the LRU will just lead to 
constantly reprogramming the regulator. Whereas if the user had a way to 
say "configure context A to X," "B to Y," and then only reprogram B 
between voltages Y and Z, we'd save 1/3 of re-configuration accesses? 
Maybe even in some such case, quickly switching to voltage X is more 
important than to voltage Y or Z.

> > Of course, all the above was just my DVS-newbie impression, which can very 
> > well be absolutely wrong.
> > 
> > > 
> > > Add regulator driver for this device.
> 
> *ALWAYS* delete irrelevant text when replying.

Not sure what you mean, sorry. If you mean all the text, that followed the 
above line, then it wasn't all irrelevant, there were more comments down 
there. OTOH, if you just meant, that I could have deleted even more text, 
than what I've done, then right, sorry, there's always a balance between 
deleting too little and too much, and the decision is subjective. I 
usually tend to keep somewhat more, tnan most would consider required, I 
think, it is easier to hit "Page Down" a couple more times, than to have 
to guess what the missing context was. But I'll try to reduce unneeded 
context next time.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] regulator: max8973: add regulator driver support

[Mark Brown]

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

On Tue, Nov 20, 2012 at 08:55:47AM +0100, Guennadi Liakhovetski wrote:
> On Tue, 20 Nov 2012, Mark Brown wrote:

> > The thing I'd like to see factored out here is the LRU mechanism,
> > otherwise I think the situation is pretty good.  Some of the older
> > devices should use a different scheme to modern ones as the hardware
> > they have to interoperate is different.

> So, do you consider the LRU algorithm to be the preferred way to configure 
> such regulators? I realise that in practice it will work well in most 

Well, there's not really many other options.

> cases, usually users do only want to preconfigure such a regulator to 2 
> fixed voltages and switch between them at runtime, right? OTOH, do you 
> think it is too unlikely, that someone will want to switch, say, between 3 
> voltages: X-Y-Z-X-Y-Z-X...? In this case the LRU will just lead to 
> constantly reprogramming the regulator. Whereas if the user had a way to 
> say "configure context A to X," "B to Y," and then only reprogram B 
> between voltages Y and Z, we'd save 1/3 of re-configuration accesses? 
> Maybe even in some such case, quickly switching to voltage X is more 
> important than to voltage Y or Z.

Modern devices tend to use multiple GPIOs for this control for a jolly
good reason.  If you've only got two levels then the wm831x algorithm is
probably the most sensible.

> > > > Add regulator driver for this device.

> > *ALWAYS* delete irrelevant text when replying.

> Not sure what you mean, sorry. If you mean all the text, that followed the 
> above line, then it wasn't all irrelevant, there were more comments down 
> there. OTOH, if you just meant, that I could have deleted even more text, 
> than what I've done, then right, sorry, there's always a balance between 

I actually thought you'd just quoted the entire mail and just deleted
the rest after a couple of screenfuls so a bit of both.  

> deleting too little and too much, and the decision is subjective. I 
> usually tend to keep somewhat more, tnan most would consider required, I 
> think, it is easier to hit "Page Down" a couple more times, than to have 
> to guess what the missing context was. But I'll try to reduce unneeded 
> context next time.

The extra content is profoundly unhelpful to people reading on phones,
and to people on slow connections (I spend an awful lot of time in
hotels with dodgy internet access for example).  It also (as happened to
me) makes it hard to find new comments in the middle of reams of stuff
you're paging down through.

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

Re: [PATCH] regulator: max8973: add regulator driver support

[Guennadi Liakhovetski]

On Tue, 20 Nov 2012, Mark Brown wrote:

> On Tue, Nov 20, 2012 at 08:55:47AM +0100, Guennadi Liakhovetski wrote:
> > On Tue, 20 Nov 2012, Mark Brown wrote:
> 
> > > The thing I'd like to see factored out here is the LRU mechanism,
> > > otherwise I think the situation is pretty good.  Some of the older
> > > devices should use a different scheme to modern ones as the hardware
> > > they have to interoperate is different.
> 
> > So, do you consider the LRU algorithm to be the preferred way to configure 
> > such regulators? I realise that in practice it will work well in most 
> 
> Well, there's not really many other options.
> 
> > cases, usually users do only want to preconfigure such a regulator to 2 
> > fixed voltages and switch between them at runtime, right? OTOH, do you 
> > think it is too unlikely, that someone will want to switch, say, between 3 
> > voltages: X-Y-Z-X-Y-Z-X...? In this case the LRU will just lead to 
> > constantly reprogramming the regulator. Whereas if the user had a way to 
> > say "configure context A to X," "B to Y," and then only reprogram B 
> > between voltages Y and Z, we'd save 1/3 of re-configuration accesses? 
> > Maybe even in some such case, quickly switching to voltage X is more 
> > important than to voltage Y or Z.
> 
> Modern devices tend to use multiple GPIOs for this control for a jolly
> good reason.  If you've only got two levels then the wm831x algorithm is
> probably the most sensible.

Ok, I see, but other my comments still hold.

> > > > > Add regulator driver for this device.
> 
> > > *ALWAYS* delete irrelevant text when replying.
> 
> > Not sure what you mean, sorry. If you mean all the text, that followed the 
> > above line, then it wasn't all irrelevant, there were more comments down 
> > there. OTOH, if you just meant, that I could have deleted even more text, 
> > than what I've done, then right, sorry, there's always a balance between 
> 
> I actually thought you'd just quoted the entire mail and just deleted
> the rest after a couple of screenfuls so a bit of both.  
> 
> > deleting too little and too much, and the decision is subjective. I 
> > usually tend to keep somewhat more, tnan most would consider required, I 
> > think, it is easier to hit "Page Down" a couple more times, than to have 
> > to guess what the missing context was. But I'll try to reduce unneeded 
> > context next time.
> 
> The extra content is profoundly unhelpful to people reading on phones,
> and to people on slow connections (I spend an awful lot of time in
> hotels with dodgy internet access for example).  It also (as happened to
> me) makes it hard to find new comments in the middle of reams of stuff
> you're paging down through.

Understand.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] regulator: max8973: add regulator driver support

[Mark Brown]

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

On Tue, Nov 20, 2012 at 09:23:41AM +0100, Guennadi Liakhovetski wrote:
> On Tue, 20 Nov 2012, Mark Brown wrote:

> > Modern devices tend to use multiple GPIOs for this control for a jolly
> > good reason.  If you've only got two levels then the wm831x algorithm is
> > probably the most sensible.

> Ok, I see, but other my comments still hold.

Not sure what those were?

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

Re: [PATCH] regulator: max8973: add regulator driver support

[Guennadi Liakhovetski]

Hi Laxman

On Tue, 20 Nov 2012, Laxman Dewangan wrote:

> On Monday 19 November 2012 04:22 PM, Guennadi Liakhovetski wrote:
> > 
> > Hi Laxman
> > 
> > drivers in the tree. Well, I came to two conclusions so far: (1) The
> > current regulator API is not very well suitable for such regulators. I
> > would imagine, one would need two methods: for setting the "normal" and
> > the DVS voltage. Instead of this drivers are trying to be smart at
> > guessing, which voltage the user is trying to set now... (2) Drivers do
> > this in different ways and at least out of the 2 drivers I looked at both
> > have bugs and different ones at that. I'll send a separate email,
> > describing what I found suspicious in them.
> > 
> > Of course, all the above was just my DVS-newbie impression, which can very
> > well be absolutely wrong.
> > 
> 
> If there is multipel VOUT register for single vout then these registers are
> generally selected by the input pin of device.
> In a given system, you can connect the gpios pin to this input pins to select
> the proper VOUT register.
> The register update through i2c consume more time and changing the gpio state
> is comparatively less.
> So if you have let say 4 voltages 1.10, 1.11, 1.12, 1.13 and having 4 VOUT
> register. Then program these vout val to vout reg like 1.10 to vout_reg0, 1.11
> to vout_reg1 etc.
> Now for changing voltage between these will just require to change the gpio
> pin state, not the register update and so it will be faster. You can achive
> the voltage change by gpio pin state change.

Right, that's also how I understood it, thanks for confirming.

> Now if your DVS have more volatge scaling then you can use the LRU mechanism
> to use the vout register for this new value.

Well, as I commented to Mark, you certainly can do that, but it might or 
might not work very well. But we can keep it if it works well for you and, 
probably, it will also work for most other users. I just find the way you 
interface the generic LRU for arbitrary many elements and your specific 
2-element case not very cleanly. In your case the LRU selection can only 
return 0 or 1, anything else would be a bug. So, why don't you just 
BUG_ON() / WARN_ON() / dev_err() if returned index > 1 instead of taking 
the lowest bit?

> > > +     max->dev =&client->dev;
> > > +     max->desc.name = id->name;
> > > +     max->desc.id = 0;
> > Don't you have to be able to process multiple such devices?
> 
> Not really require as device have only one output. The different devices will
> have different registrations and so does not matter here.

Right, I thought it had to be unique.

> > > +     max->enable_external_control = pdata->enable_ext_control;
> > > +     max->dvs_gpio = pdata->dvs_gpio;
> > > +     max->curr_gpio_val = pdata->dvs_def_state;
> > > +     max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
> > > +     max->lru_index[0] = max->curr_vout_reg;
> > Here you actually need an offset within your register address space, so,
> > should be
> > 
> > +       max->lru_index[0] = pdata->dvs_def_state;
> 
> 
> Yaah, seems some issue if vout_base is not zero. But really dont require here
> as MAX8973_VOUT is 0 in this case.

I think cleanly differentiating between register addresses, offsets and 
indices is important.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] regulator: max8973: add regulator driver support

[Laxman Dewangan]

On Tuesday 20 November 2012 03:12 PM, Guennadi Liakhovetski wrote:
> Hi Laxman
>
> On Tue, 20 Nov 2012, Laxman Dewangan wrote:
>
>>>
>>> Here you actually need an offset within your register address space, so,
>>> should be
>>>
>>> +       max->lru_index[0] = pdata->dvs_def_state;
>>
>> Yaah, seems some issue if vout_base is not zero. But really dont require here
>> as MAX8973_VOUT is 0 in this case.
> I think cleanly differentiating between register addresses, offsets and
> indices is important.
>


Ok, I will push the patch. Seems same fixing is also require for 
tps62360 regulator.