Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
On Wed, May 23, 2012 at 9:46 AM, Yadwinder Singh Brar yadi.bra...@gmail.com wrote: (adding Kyungmin Park and Samuel Ortiz) Hi, Yes, It happened unintentionally. I didn't know about your patch before submitting the initial version of my patches. I agree with you, I will review your patches and will try to incorporate extra features from your patches. Now I have seen your patches for mfd and regulator drivers. Apparently, it seems that mostly we same features in our patches. Their is no extra feature to be incorporated form your patches. Rather I found device tree support is additional in our patches and mainly their are some differences related to DVS_GPIO and opmode stuff in our patches: 1- Since we are not implementing and using DVS feature through GPIOs, so all(incomplete) stuff related to dvs_gpio is not required currently in our mfd driver presently. 2- Since presently, we are not implementing suspend_enable/disable callbacks in regulator driver, So we don't need opmode related stuff now because I think regulators should come up in normal mode only through .enable callback function. On Wed, May 23, 2012 at 7:10 AM, jonghwa3@samsung.com wrote: Hi, Yadwinder, As you know, both of us, recently, had competition for one driver whether you intend or not. And now, i think it is time to stop this and to find appropriate goal. From now on, i won't update this driver no more. I recommend you to review my patch and apply feature that you can apply. And also check comments that i wrote below. Thanks, Yadwinder. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
Hi, Yadwinder,
As you know, both of us, recently, had competition for one driver
whether you intend or not. And now, i think it is time to stop this and
to find appropriate goal. From now on, i won't update this driver no
more. I recommend you to review my patch and apply feature that you can
apply. And also check comments that i wrote below.
On 2012년 05월 22일 14:57, yadi.bra...@gmail.com wrote:
From: Yadwinder Singh Brar yadi.b...@samsung.com
Add support for PMIC/regulator portion of MAX77686 multifunction device.
MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
which supports setting and getting the voltage of a regulator with I2C
interface.
Signed-off-by: Yadwinder Singh Brar yadi.b...@samsung.com
---
drivers/regulator/Kconfig|9 +
drivers/regulator/Makefile |1 +
drivers/regulator/max77686.c | 387
++
3 files changed, 397 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/max77686.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c86b886..e8f9417 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,15 @@ 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 voltage regulator via I2C
+ bus. The provided regulator is suitable for Exynos5 chips to
+ control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
+ and BUCKs[1-9].
+
config REGULATOR_PCAP
tristate Motorola PCAP2 regulator driver
depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 977fd46..d854453 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 000..98dbd50
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,387 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics Co. Ltd.
+ * Chiwoong Byun woong.b...@samsung.com
+ * Yadwinder Singh Brar yadi.b...@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/module.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/regulator/of_regulator.h
+#include linux/mfd/max77686.h
+#include linux/mfd/max77686-private.h
+
+struct max77686_data {
+ struct device *dev;
+ struct max77686_dev *iodev;
+ int num_regulators;
+ struct regulator_dev **rdev;
+ int ramp_delay; /* index of ramp_delay */
+
+ /*GPIO-DVS feature is not enabled with the
+ *current version of MAX77686 driver.*/
+};
+
+static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
+ unsigned int old_sel,
+ unsigned int new_sel)
+{
+ struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+ int ramp[] = {13, 27, 55, 100}; /* ramp_rate in mV/uS */
+
+ return DIV_ROUND_UP(rdev-desc-uV_step *
+ abs(new_sel - old_sel),
+ ramp[max77686-ramp_delay]);
+}
+
+static int max77686_voltage_time_sel(struct regulator_dev *rdev,
+ unsigned int old_sel,
+ unsigned int new_sel)
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
Hi, again.
On 2012년 05월 22일 14:57, yadi.bra...@gmail.com wrote:
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+
+ for (i = 0; i pdata-num_regulators; i++) {
+ config.dev = max77686-dev;
+ config.init_data = pdata-regulators[i].init_data;
+ config.driver_data = max77686;
+ config.regmap = iodev-regmap;
+
+ rdev[i] = regulator_register(regulators[i], config);
I'm sorry that i missed one. You have to register all regulators
unconditionally. Mark brown commented about this to my former patch.
'No, you should unconditionally register all regulators the device
physically has. This is useful for debug and simplifies the code.'
- from Mark Brown
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
(adding Kyungmin Park and Samuel Ortiz) Hi, Yes, It happened unintentionally. I didn't know about your patch before submitting the initial version of my patches. I agree with you, I will review your patches and will try to incorporate extra features from your patches. On Wed, May 23, 2012 at 7:10 AM, jonghwa3@samsung.com wrote: Hi, Yadwinder, As you know, both of us, recently, had competition for one driver whether you intend or not. And now, i think it is time to stop this and to find appropriate goal. From now on, i won't update this driver no more. I recommend you to review my patch and apply feature that you can apply. And also check comments that i wrote below. On 2012년 05월 22일 14:57, yadi.bra...@gmail.com wrote: From: Yadwinder Singh Brar yadi.b...@samsung.com Add support for PMIC/regulator portion of MAX77686 multifunction device. MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv which supports setting and getting the voltage of a regulator with I2C interface. + return DIV_ROUND_UP(rdev-desc-uV_step * + abs(new_sel - old_sel), + 100); +} + Does LDO also need waiting for voltage change? I afraid it's not. Yes, according to technical reference manual which I have, ramp rate for LDOs is also 100mV/us. + + if (pdata-ramp_delay MAX77686_RAMP_RATE_13MV || + pdata-ramp_delay MAX77686_RAMP_RATE_100MV) + pdata-ramp_delay = MAX77686_RAMP_RATE_27MV; /* default */ If pdata doesn't have proper ramp_delay, it will get value MAX77686_RAMP_RATE_27MV. + + max77686-ramp_delay = pdata-ramp_delay - 1; I think it is better to check pdata-ramp_delay is available. If pdata doesn't have ramp_delay member it might be error. Yes, we have taken care of this case above before setting value of max77686-ramp_delay. + max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + Why do you use i2c client still? If you registered regmap you can use its API. I recommend you to use regmap_update_bits() directly. Yes, we are using regmap_update_bits(). max77686_update_reg() is just a wrapper over it. + platform_set_drvdata(pdev, max77686); + MAX77686 has crystal oscillator in it. And original version of this driver which was written by Chiwoon Byun, registers it as a regulator. As Mark says, we have to change it to use generic clock API. Where do you think should we put them into? In my opinion, it is proper that just leave them in regulator driver because this driver is almost core of PMIC. I already applied generic API in my local repository but i couldn't test yet. Because it crashed with SOC's private clock API. Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE() will help you out which defined in linux/clk-private.h. Yes, I was also thinking about where to put it. I am not sure whether this is a proper place to put them. Anyway I will again think about it. Thanks, Yadwinder. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
Hi,
On Wed, May 23, 2012 at 7:20 AM, jonghwa3@samsung.com wrote:
Hi, again.
On 2012년 05월 22일 14:57, yadi.bra...@gmail.com wrote:
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+
+ for (i = 0; i pdata-num_regulators; i++) {
+ config.dev = max77686-dev;
+ config.init_data = pdata-regulators[i].init_data;
+ config.driver_data = max77686;
+ config.regmap = iodev-regmap;
+
+ rdev[i] = regulator_register(regulators[i], config);
I'm sorry that i missed one. You have to register all regulators
unconditionally. Mark brown commented about this to my former patch.
'No, you should unconditionally register all regulators the device
physically has. This is useful for debug and simplifies the code.'
- from Mark Brown
Yes, we are registering all regulators here.
As pdata-num_regulators will be equal to ARRAY_SIZE(regulators)
Thanks,
Yadwinder.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote: + max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + Why do you use i2c client still? If you registered regmap you can use its API. I recommend you to use regmap_update_bits() directly. Yes, we are using regmap_update_bits(). max77686_update_reg() is just a wrapper over it. Yes, i know what you mean. However it doesn't need max77686_update_reg() any more since it uses regmap API. Why don't you just pass iodev-regmap to regmap_update_bits(). It is clear that there is no reason for using i2c client as a medium. Please check regulator and mfd driver of my previous patch. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
On Wed, May 23, 2012 at 10:10 AM, jonghwa3@samsung.com wrote: On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote: + max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + Why do you use i2c client still? If you registered regmap you can use its API. I recommend you to use regmap_update_bits() directly. Yes, we are using regmap_update_bits(). max77686_update_reg() is just a wrapper over it. Yes, i know what you mean. However it doesn't need max77686_update_reg() any more since it uses regmap API. Why don't you just pass iodev-regmap to regmap_update_bits(). It is clear that there is no reason for using i2c client as a medium. Please check regulator and mfd driver of my previous patch. I agree with you we can use directly regmap API. But I preferred max77686_update_reg() because its a common practice to use common read/write API which we define in mfd driver to access that particular mfd device from other drivers. Regards, Yadwinder. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] regulator: Add support for MAX77686.
On 2012년 05월 23일 14:23, Yadwinder Singh Brar wrote: On Wed, May 23, 2012 at 10:10 AM, jonghwa3@samsung.com wrote: On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote: + max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1, + max77686-ramp_delay 6, RAMP_MASK); + Why do you use i2c client still? If you registered regmap you can use its API. I recommend you to use regmap_update_bits() directly. Yes, we are using regmap_update_bits(). max77686_update_reg() is just a wrapper over it. Yes, i know what you mean. However it doesn't need max77686_update_reg() any more since it uses regmap API. Why don't you just pass iodev-regmap to regmap_update_bits(). It is clear that there is no reason for using i2c client as a medium. Please check regulator and mfd driver of my previous patch. I agree with you we can use directly regmap API. But I preferred max77686_update_reg() because its a common practice to use common read/write API which we define in mfd driver to access that particular mfd device from other drivers. Regards, Yadwinder. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ I inform you my mfd driver has been confirmed by Samuel Oritz and there is no mfd private API. This situation looks unusual that we registers mfd driver and regulator driver separately. But how should we do? For corporation , i'm asking you to consider my suggestion. Thanks. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] regulator: Add support for MAX77686.
From: Yadwinder Singh Brar yadi.b...@samsung.com
Add support for PMIC/regulator portion of MAX77686 multifunction device.
MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
which supports setting and getting the voltage of a regulator with I2C
interface.
Signed-off-by: Yadwinder Singh Brar yadi.b...@samsung.com
---
drivers/regulator/Kconfig|9 +
drivers/regulator/Makefile |1 +
drivers/regulator/max77686.c | 387 ++
3 files changed, 397 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/max77686.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c86b886..e8f9417 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,15 @@ 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 voltage regulator via I2C
+ bus. The provided regulator is suitable for Exynos5 chips to
+ control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
+ and BUCKs[1-9].
+
config REGULATOR_PCAP
tristate Motorola PCAP2 regulator driver
depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 977fd46..d854453 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 000..98dbd50
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,387 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics Co. Ltd.
+ * Chiwoong Byun woong.b...@samsung.com
+ * Yadwinder Singh Brar yadi.b...@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/module.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/regulator/of_regulator.h
+#include linux/mfd/max77686.h
+#include linux/mfd/max77686-private.h
+
+struct max77686_data {
+ struct device *dev;
+ struct max77686_dev *iodev;
+ int num_regulators;
+ struct regulator_dev **rdev;
+ int ramp_delay; /* index of ramp_delay */
+
+ /*GPIO-DVS feature is not enabled with the
+*current version of MAX77686 driver.*/
+};
+
+static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
+ unsigned int old_sel,
+ unsigned int new_sel)
+{
+ struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+ int ramp[] = {13, 27, 55, 100}; /* ramp_rate in mV/uS */
+
+ return DIV_ROUND_UP(rdev-desc-uV_step *
+ abs(new_sel - old_sel),
+ ramp[max77686-ramp_delay]);
+}
+
+static int max77686_voltage_time_sel(struct regulator_dev *rdev,
+ unsigned int old_sel,
+ unsigned int new_sel)
+{
+ return DIV_ROUND_UP(rdev-desc-uV_step *
+ abs(new_sel - old_sel),
+ 100);
+}
+
+static struct regulator_ops max77686_ops = {
+ .map_voltage= regulator_map_voltage_linear,
+ .list_voltage = regulator_list_voltage_linear,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable = regulator_enable_regmap,
+ .disable
