[PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

[PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

[Jonghwa Lee]

Add driver for support max77686 regulator.
MAX77686 provides LDOs[1~26] and BUCKs[1~9]. It support to set or get the
volatege of regulator on max77686 chip with using regmap.

v5
- Remove unnecessary initializing and variable.

v4
- Register Crystal oscillator's clocks with generic clock API.

v3
- Remove voltage_map_desc structure and replace minimum voltage and voltage
  step into regulator_desc.
- Use only regulator common APIs for regulator_ops.
  (No more use driver's own funcions)

This patch is based on broonie/regulator/for-next branch.

v2
- Convert get_voltage to get_voltage_sel
- Convert set_voltage to set_voltage_sel
- Implement set_voltage_time_sel
- Register regulators unconditionally
- kzalloc -> devm_kzalloc
- Remove unneccessary printk
- Keep probing whether pdata exists or not
- Use regmap API to access PMIC register.

Signed-off-by: Chiwoong Byun <woong.byun@samsung.com>
Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/Kconfig    |    8 +
 drivers/regulator/Makefile   |    1 +
 drivers/regulator/max77686.c |  491 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 500 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/max77686.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 36db5a4..8e2ebad 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,14 @@ config REGULATOR_MAX8998
 	  via I2C bus. The provided regulator is suitable for S3C6410
 	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
 
+config REGULATOR_MAX77686
+	tristate "Maxim 77686 regulator"
+	depends on MFD_MAX77686
+	help
+	  This driver controls a Maxim 77686 regulator
+	  via I2C bus. The provided regulator is suitable for
+	  Exynos-4 chips to control VARM and VINT voltages.
+
 config REGULATOR_PCAP
 	tristate "Motorola PCAP2 regulator driver"
 	depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 94b5274..008931b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
 obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
 obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
+obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
new file mode 100644
index 0000000..2d23f68
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,491 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Chiwoong Byun <woong.byun@smasung.com>
+ * Jonghwa Lee <jonghwa3.lee@samsung.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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * This driver is based on max8997.c
+ */
+
+#include <linux/kernel.h>
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/max77686.h>
+#include <linux/mfd/max77686-private.h>
+
+#ifdef CONFIG_COMMON_CLK
+#include <linux/clk-private.h>
+#endif
+
+#define MAX77686_LDO_MINUV	800000
+#define MAX77686_LDO_UVSTEP	50000
+#define MAX77686_LDO_LOW_MINUV	800000
+#define MAX77686_LDO_LOW_UVSTEP	25000
+#define MAX77686_BUCK_MINUV	750000
+#define MAX77686_BUCK_UVSTEP	50000
+#define MAX77686_DVS_MINUV	600000
+#define MAX77686_DVS_UVSTEP	12500
+
+#define MAX77686_OPMODE_SHIFT	6
+#define MAX77686_OPMODE_BUCK234_SHIFT	4
+#define MAX77686_OPMODE_MASK	0x3
+
+#define MAX77686_VSEL_MASK	0x3f
+#define MAX77686_DVS_VSEL_MASK	0xff
+
+#define MAX77686_RAMP_RATE	100
+
+#define MAX77686_REGULATORS	MAX77686_REG_MAX
+#define MAX77686_LDOS		26
+
+struct max77686_data {
+	struct device *dev;
+	struct max77686_dev *iodev;
+	int num_regulators;
+	struct regulator_dev **rdev;
+	int ramp_delay; /* in mV/us */
+
+#ifdef CONFIG_COMMON_CLK
+	struct clk clk32khz_ap;
+	struct clk clk32khz_cp;
+	struct clk clk32khz_pmic;
+#endif
+	u8 buck2_vol[8];
+	u8 buck3_vol[8];
+	u8 buck4_vol[8];
+};
+
+#ifdef CONFIG_COMMON_CLK
+
+#define to_max77686_data(__name)	container_of(hw->clk,		\
+					struct max77686_data, __name)
+
+static struct clk_gate clk32khz_ap_hw = {
+		.ret = MAX77686_REG_32KH,
+		.bit_idx = 0,
+		.flags = 0,
+		.lock = clk_lock,
+};
+
+static struct clk_gate clk32khz_cp_hw = {
+		.ret = MAX77686_REG_32KH,
+		.bit_idx = 1,
+		.flags = 0,
+		.lock = clk_lock,
+};
+
+static struct clk_gate clk32khz_pmic_hw = {
+		.ret = MAX77686_REG_32KH,
+		.bit_idx = 2,
+		.flags = 0,
+		.lock = clk_lock,
+};
+
+static int *clk_enable(struct clk_hw *hw);
+static void *clk_disable(struct clk_hw *hw);
+static int *clk_is_enable(struct clk_hw *hw);
+
+static struct clk_ops clk32khz_ops {
+	.enable		= clk_enable,
+	.disable	= clk_disable,
+	.is_enable	= cli_is_enable,
+};
+#endif
+
+static int max77686_set_voltage_time_sel(struct regulator_dev *rdev,
+			unsigned int old_selector, unsigned int new_selector)
+{
+	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int rid = rdev_get_id(rdev);
+
+	switch (rid) {
+	case MAX77686_BUCK2 ... MAX77686_BUCK4:
+		return (DIV_ROUND_UP(rdev->desc->uV_step
+			* abs(new_selector - old_selector),
+			max77686->ramp_delay * 1000));
+	}
+	/* Unconditionally 100 mV/us */
+	return (DIV_ROUND_UP(rdev->desc->uV_step
+		 * abs(new_selector - old_selector), 100000));
+}
+
+static struct regulator_ops max77686_ldo_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_set_voltage_time_sel,
+};
+
+static struct regulator_ops max77686_buck_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_set_voltage_time_sel,
+};
+
+#define regulator_desc_ldo(num)		{				\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ldo_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= MAX77686_LDO_MINUV,				\
+	.uV_step	= MAX77686_LDO_UVSTEP,				\
+	.n_voltages	= MAX77686_VSEL_MASK + 1,			\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= MAX77686_VSEL_MASK,				\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= MAX77686_OPMODE_MASK				\
+			<< MAX77686_OPMODE_SHIFT,			\
+}
+#define regulator_desc_ldo_low(num)		{			\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ldo_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= MAX77686_LDO_LOW_MINUV,			\
+	.uV_step	= MAX77686_LDO_LOW_UVSTEP,			\
+	.n_voltages	= MAX77686_VSEL_MASK + 1,			\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= MAX77686_VSEL_MASK,				\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= MAX77686_OPMODE_MASK				\
+			<< MAX77686_OPMODE_SHIFT,			\
+}
+#define regulator_desc_buck(num)		{			\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_buck_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= MAX77686_BUCK_MINUV,				\
+	.uV_step	= MAX77686_BUCK_UVSTEP,				\
+	.n_voltages	= MAX77686_VSEL_MASK + 1,			\
+	.vsel_reg	= MAX77686_REG_BUCK5OUT + (num - 5) * 2,	\
+	.vsel_mask	= MAX77686_VSEL_MASK,				\
+	.enable_reg	= MAX77686_REG_BUCK5CTRL + (num - 5) * 2,	\
+	.enable_mask	= MAX77686_OPMODE_MASK,				\
+}
+#define regulator_desc_buck1(num)		{			\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_buck_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= MAX77686_BUCK_MINUV,				\
+	.uV_step	= MAX77686_BUCK_UVSTEP,				\
+	.n_voltages	= MAX77686_VSEL_MASK + 1,			\
+	.vsel_reg	= MAX77686_REG_BUCK1OUT,			\
+	.vsel_mask	= MAX77686_VSEL_MASK,				\
+	.enable_reg	= MAX77686_REG_BUCK1CTRL,			\
+	.enable_mask	= MAX77686_OPMODE_MASK,				\
+}
+#define regulator_desc_buck_dvs(num)		{			\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_buck_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= MAX77686_DVS_MINUV,				\
+	.uV_step	= MAX77686_DVS_UVSTEP,				\
+	.n_voltages	= MAX77686_DVS_VSEL_MASK + 1,			\
+	.vsel_reg	= MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,	\
+	.vsel_mask	= MAX77686_DVS_VSEL_MASK,			\
+	.enable_reg	= MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10,	\
+	.enable_mask	= MAX77686_OPMODE_MASK				\
+			<< MAX77686_OPMODE_BUCK234_SHIFT,		\
+}
+
+static struct regulator_desc regulators[] = {
+	regulator_desc_ldo_low(1),
+	regulator_desc_ldo_low(2),
+	regulator_desc_ldo(3),
+	regulator_desc_ldo(4),
+	regulator_desc_ldo(5),
+	regulator_desc_ldo_low(6),
+	regulator_desc_ldo_low(7),
+	regulator_desc_ldo_low(8),
+	regulator_desc_ldo(9),
+	regulator_desc_ldo(10),
+	regulator_desc_ldo(11),
+	regulator_desc_ldo(12),
+	regulator_desc_ldo(13),
+	regulator_desc_ldo(14),
+	regulator_desc_ldo_low(15),
+	regulator_desc_ldo(16),
+	regulator_desc_ldo(17),
+	regulator_desc_ldo(18),
+	regulator_desc_ldo(19),
+	regulator_desc_ldo(20),
+	regulator_desc_ldo(21),
+	regulator_desc_ldo(22),
+	regulator_desc_ldo(23),
+	regulator_desc_ldo(24),
+	regulator_desc_ldo(25),
+	regulator_desc_ldo(26),
+	regulator_desc_buck1(1),
+	regulator_desc_buck_dvs(2),
+	regulator_desc_buck_dvs(3),
+	regulator_desc_buck_dvs(4),
+	regulator_desc_buck(5),
+	regulator_desc_buck(6),
+	regulator_desc_buck(7),
+	regulator_desc_buck(8),
+	regulator_desc_buck(9),
+};
+
+#ifdef CONFIG_COMMON_CLK
+static int *clk_enable(struct clk_hw *hw)
+{
+	struct max77686_data *max77686 = NULL;
+	struct regmap *regmap;
+	int mask;
+
+	if (hw == &clk32khz_ap_hw) {
+		max77686 = to_max77686_dev(clk32khz_ap);
+		mask = (1 << MAX77686_32KH_AP);
+	} else if (hw == &clk32khz_cp_hw) {
+		max77686 = to_max77686_dev(clk32khz_cp);
+		mask = (1 << MAX77686_32KH_CP);
+	} else if (hw == &clk32khz_pmic_hw) {
+		max77686 = to_max77686_dev(clk32khz_pmic);
+		maske = (1 << MAX77686_P32KH);
+	}
+
+	if (!max77686)
+		return -ENOMEM;
+
+	return regmap_update_bits(regmap, MAX77686_REG_32KHZ, mask, 1);
+}
+
+static void *clk_disable(struct clk_hw *hw)
+{
+	struct max77686_data *max77686 = NULL;
+	struct regmap *regmap;
+	int mask;
+
+	if (hw == &clk32khz_ap_hw) {
+		max77686 = to_max77686_dev(clk32khz_ap);
+		mask = (1 << MAX77686_32KH_AP);
+	} else if (hw == &clk32khz_cp_hw) {
+		max77686 = to_max77686_dev(clk32khz_cp);
+		mask = (1 << MAX77686_32KH_CP);
+	} else if (hw == &clk32khz_pmic_hw) {
+		max77686 = to_max77686_dev(clk32khz_pmic);
+		maske = (1 << MAX77686_P32KH);
+	}
+
+	if (!max77686)
+		return -ENOMEM;
+
+	return regmap_update_bits(regmap, MAX77686_REG_32KHZ, mask, 0);
+}
+
+static int *clk_is_enable(struct clk_hw *hw)
+{
+	struct max77686_data *max77686 = NULL;
+	struct regmap *regmap;
+	int mask, ret;
+	u8 val;
+
+	if (hw == &clk32khz_ap_hw) {
+		max77686 = to_max77686_dev(clk32khz_ap);
+		mask = (1 << MAX77686_32KH_AP);
+	} else if (hw == &clk32khz_cp_hw) {
+		max77686 = to_max77686_dev(clk32khz_cp);
+		mask = (1 << MAX77686_32KH_CP);
+	} else if (hw == &clk32khz_pmic_hw) {
+		max77686 = to_max77686_dev(clk32khz_pmic);
+		maske = (1 << MAX77686_P32KH);
+	}
+
+	if (!max77686)
+		return -ENOMEM;
+
+	ret = regmap_read(regamp, MAX77686_REG_32KHZ, &val);
+	if (ret < 0)
+		return -EINVAL;
+
+	return val & mask;
+}
+
+static int set_max77686_clk_init(int cid, struct clk *clk)
+{
+	switch (cid) {
+	case MAX77686_32KH_AP:
+		clk->name	= "32khap";
+		clk->hw		= &clk32khz_ap_hw;
+		break;
+	case MAX77686_32KH_CP:
+		clk->name	= "32khcp";
+		clk->hw		= &clk32khz_cp_hw;
+		break;
+	case MAX77686_P32KH:
+		clk->name	= "p32kh";
+		clk->hw		= &clk32khz_pmic_hw;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	clk->ops	= &clk_ops;
+	clk->flags	= CLK_IS_ROOT;
+	clk->hw->clk	= clk;
+	return 0;
+}
+#endif /* CONFIG_COMMON_CLK */
+
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+	struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev);
+	struct regulator_dev **rdev;
+	struct max77686_data *max77686;
+	struct regulator_init_data **init_data;
+	int i, size;
+	int ret = 0;
+	struct regulator_config config;
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
+				GFP_KERNEL);
+	if (!max77686)
+		return -ENOMEM;
+
+	size = sizeof(struct regulator_dev *) * MAX77686_REGULATORS;
+	max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!max77686->rdev)
+		return -ENOMEM;
+
+	rdev = max77686->rdev;
+	max77686->dev = &pdev->dev;
+	max77686->iodev = iodev;
+	if (pdata)
+		max77686->num_regulators = pdata->num_regulators;
+	platform_set_drvdata(pdev, max77686);
+	init_data = devm_kzalloc(&pdev->dev,
+				sizeof(struct regulator_init_data *)
+					 * MAX77686_REGULATORS, GFP_KERNEL);
+	if (!init_data)
+		return -ENOMEM;
+
+	config.dev = max77686->dev;
+	config.regmap = iodev->regmap;
+	config.driver_data = max77686;
+
+	max77686->ramp_delay = MAX77686_RAMP_RATE; /* Set 0x3 for RAMP */
+	regmap_update_bits(max77686->iodev->regmap,
+					 MAX77686_REG_BUCK2CTRL1, 0xC0, 0xC0);
+	regmap_update_bits(max77686->iodev->regmap,
+					 MAX77686_REG_BUCK3CTRL1, 0xC0, 0xC0);
+	regmap_update_bits(max77686->iodev->regmap,
+					 MAX77686_REG_BUCK4CTRL1, 0xC0, 0xC0);
+
+	for (i = 0; i < MAX77686_REGULATORS; i++) {
+		if (pdata)
+			init_data[pdata->regulators[i].id] =
+						 pdata->regulators[i].initdata;
+
+		config.init_data = init_data[i];
+		rdev[i] = regulator_register(&regulators[i], &config);
+
+		if (IS_ERR(rdev[i])) {
+			ret = PTR_ERR(rdev[i]);
+			dev_err(max77686->dev,
+				"regulator init failed for %d\n", i);
+			rdev[i] = NULL;
+			goto err;
+		}
+	}
+
+#ifdef CONFIG_COMMON_CLK
+	set_max77686_clk_init(MAX77686_32KH_AP, &max77686->clk32khz_ap);
+	set_max77686_clk_init(MAX77686_32KH_CP, &max77686->clk32khz_cp);
+	set_max77686_clk_init(MAX77686_P32KH, &max77686->clk32khz_pmic);
+	__clk_init(&pdev->dev, &max77686->clk32khz_ap);
+	__clk_init(&pdev->dev, &max77686->clk32khz_cp);
+	__clk_init(&pdev->dev, &max77686->clk32khz_pmic);
+#endif
+
+	return 0;
+err:
+	for (i = 0; i < MAX77686_REGULATORS; i++) {
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+	}
+	return ret;
+}
+
+static int __devexit max77686_pmic_remove(struct platform_device *pdev)
+{
+	struct max77686_data *max77686 = platform_get_drvdata(pdev);
+	struct regulator_dev **rdev = max77686->rdev;
+	int i;
+
+	for (i = 0; i < MAX77686_REGULATORS; i++)
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+
+	return 0;
+}
+
+static const struct platform_device_id max77686_pmic_id[] = {
+	{ "max77686-pmic", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
+
+static struct platform_driver max77686_pmic_driver = {
+	.driver = {
+		.name = "max77686-pmic",
+		.owner = THIS_MODULE,
+	},
+	.probe = max77686_pmic_probe,
+	.remove = __devexit_p(max77686_pmic_remove),
+	.id_table = max77686_pmic_id,
+};
+
+static int __init max77686_pmic_init(void)
+{
+	return platform_driver_register(&max77686_pmic_driver);
+}
+subsys_initcall(max77686_pmic_init);
+
+static void __exit max77686_pmic_cleanup(void)
+{
+	platform_driver_unregister(&max77686_pmic_driver);
+}
+module_exit(max77686_pmic_cleanup);
+
+MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
+MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.4.1

Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

[Mark Brown]

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

On Tue, May 29, 2012 at 11:20:51AM +0900, Jonghwa Lee wrote:
> Add driver for support max77686 regulator.
> MAX77686 provides LDOs[1~26] and BUCKs[1~9]. It support to set or get the
> volatege of regulator on max77686 chip with using regmap.
> 
> v5
> - Remove unnecessary initializing and variable.
> 

Don't put stuff like this in the changelog, it's useless noise in git.
Include it after the cut as documented in SubmittingPatches.

> +#ifdef CONFIG_COMMON_CLK
> +	struct clk clk32khz_ap;
> +	struct clk clk32khz_cp;
> +	struct clk clk32khz_pmic;
> +#endif

This should be a clock driver in drivers/clock.

> +static int max77686_set_voltage_time_sel(struct regulator_dev *rdev,
> +			unsigned int old_selector, unsigned int new_selector)
> +{
> +	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> +	int rid = rdev_get_id(rdev);
> +
> +	switch (rid) {
> +	case MAX77686_BUCK2 ... MAX77686_BUCK4:
> +		return (DIV_ROUND_UP(rdev->desc->uV_step
> +			* abs(new_selector - old_selector),
> +			max77686->ramp_delay * 1000));
> +	}
> +	/* Unconditionally 100 mV/us */
> +	return (DIV_ROUND_UP(rdev->desc->uV_step
> +		 * abs(new_selector - old_selector), 100000));
> +}

Just do separate functions.  The above is pretty illegible.

> +	max77686->ramp_delay = MAX77686_RAMP_RATE; /* Set 0x3 for RAMP */
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK2CTRL1, 0xC0, 0xC0);
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK3CTRL1, 0xC0, 0xC0);
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK4CTRL1, 0xC0, 0xC0);

This still appears to not be referencing the ramp rate that's being set?

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

Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

[jonghwa3.lee]

Hi, Mark,
On 2012년 05월 31일 02:26, Mark Brown wrote:

> On Tue, May 29, 2012 at 11:20:51AM +0900, Jonghwa Lee wrote:
>> Add driver for support max77686 regulator.
>> MAX77686 provides LDOs[1~26] and BUCKs[1~9]. It support to set or get the
>> volatege of regulator on max77686 chip with using regmap.
>>
>> v5
>> - Remove unnecessary initializing and variable.
>>
> 
> Don't put stuff like this in the changelog, it's useless noise in git.
> Include it after the cut as documented in SubmittingPatches.
> 


Okay, I'll do that,

>> +#ifdef CONFIG_COMMON_CLK
>> +	struct clk clk32khz_ap;
>> +	struct clk clk32khz_cp;
>> +	struct clk clk32khz_pmic;
>> +#endif
> 
> This should be a clock driver in drivers/clock.
> 


Isn't it drivers/clk ? Could you explain more about this?


>> +static int max77686_set_voltage_time_sel(struct regulator_dev *rdev,
>> +			unsigned int old_selector, unsigned int new_selector)
>> +{
>> +	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
>> +	int rid = rdev_get_id(rdev);
>> +
>> +	switch (rid) {
>> +	case MAX77686_BUCK2 ... MAX77686_BUCK4:
>> +		return (DIV_ROUND_UP(rdev->desc->uV_step
>> +			* abs(new_selector - old_selector),
>> +			max77686->ramp_delay * 1000));
>> +	}
>> +	/* Unconditionally 100 mV/us */
>> +	return (DIV_ROUND_UP(rdev->desc->uV_step
>> +		 * abs(new_selector - old_selector), 100000));
>> +}
> 
> Just do separate functions.  The above is pretty illegible.
> 


Okay I'll separate it into two functions. One for DVS BUCKs and another
for remains.

>> +	max77686->ramp_delay = MAX77686_RAMP_RATE; /* Set 0x3 for RAMP */
>> +	regmap_update_bits(max77686->iodev->regmap,
>> +					 MAX77686_REG_BUCK2CTRL1, 0xC0, 0xC0);
>> +	regmap_update_bits(max77686->iodev->regmap,
>> +					 MAX77686_REG_BUCK3CTRL1, 0xC0, 0xC0);
>> +	regmap_update_bits(max77686->iodev->regmap,
>> +					 MAX77686_REG_BUCK4CTRL1, 0xC0, 0xC0);
> 
> This still appears to not be referencing the ramp rate that's being set?


Okay, I'll remove hard coding, and will use ramp rate for setting.

Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

[Mark Brown]

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

On Thu, May 31, 2012 at 03:36:17PM +0900, jonghwa3.lee@samsung.com wrote:
> On 2012년 05월 31일 02:26, Mark Brown wrote:
> > On Tue, May 29, 2012 at 11:20:51AM +0900, Jonghwa Lee wrote:

> >> +#ifdef CONFIG_COMMON_CLK
> >> +	struct clk clk32khz_ap;
> >> +	struct clk clk32khz_cp;
> >> +	struct clk clk32khz_pmic;
> >> +#endif

> > This should be a clock driver in drivers/clock.

> Isn't it drivers/clk ? Could you explain more about this?

It's using the drivers/clk API, yes - what I'm saying is that you should
make a new MFD child driver drivers/clk/clk-max77686.c (or whatever) to
contain this code.

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