[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[MyungJoo Ham]

Two issues are addressed for max8998_set_voltage function.

1. Min/Max Voltage.

	max8998_set_voltage had been using the voltage value of

	min ( voltage[i] >= max_vol , i )

	This is corrected to use:

	min ( voltage[i] >= min_vol , i )

2. Ramp Up Delay.

	max8998_set_voltage should provide delay for BUCK1/2
if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
RAMP delay length. However, when max8998_set_voltage's new voltage is
lower than the previous, we don't care because it does not deteriorate
the stability.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/max8998.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index 1cca8b5..99c5279 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/mutex.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/mfd/max8998.h>
@@ -302,10 +303,13 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
 	struct i2c_client *i2c = max8998->iodev->i2c;
 	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
+	int previous_vol = 0;
 	const struct voltage_map_desc *desc;
 	int ldo = max8998_get_ldo(rdev);
 	int reg, shift = 0, mask, ret;
 	int i = 0;
+	u8 val;
+	bool en_ramp = false;
 
 	if (ldo > ARRAY_SIZE(ldo_voltage_map))
 		return -EINVAL;
@@ -317,15 +321,37 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 	if (max_vol < desc->min || min_vol > desc->max)
 		return -EINVAL;
 
-	while (desc->min + desc->step*i < max_vol &&
+	while (desc->min + desc->step*i < min_vol &&
 	       desc->min + desc->step*i < desc->max)
 		i++;
 
+	if (desc->min + desc->step*i > max_vol)
+		return -EINVAL;
+
 	ret = max8998_get_voltage_register(rdev, &reg, &shift, &mask);
 	if (ret)
 		return ret;
 
-	return max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
+	/* wait for RAMP_UP_DELAY if rdev is BUCK1/2 and
+	 * ENRAMP is ON */
+	if (ldo == MAX8998_BUCK1 || ldo == MAX8998_BUCK2) {
+		max8998_read_reg(i2c, MAX8998_REG_ONOFF4, &val);
+		if (val & (1 << 4)) {
+			en_ramp = true;
+			previous_vol = max8998_get_voltage(rdev);
+		}
+	}
+
+	ret = max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
+
+	if (en_ramp == true) {
+		int difference = desc->min + desc->step * i
+			- previous_vol / 1000;
+		if (difference > 0)
+			udelay(difference / ((val & 0x0f) + 1));
+	}
+
+	return ret;
 }
 
 static struct regulator_ops max8998_ldo_ops = {
-- 
1.6.3.3

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[MyungJoo Ham]

Two issues are addressed for max8998_set_voltage function.

1. Min/Max Voltage.

	max8998_set_voltage had been using the voltage value of

	min ( voltage[i] >= max_vol , i )

	This is corrected to use:

	min ( voltage[i] >= min_vol , i )

2. Ramp Up Delay.

	max8998_set_voltage should provide delay for BUCK1/2
if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
RAMP delay length. However, when max8998_set_voltage's new voltage is
lower than the previous, we don't care because it does not deteriorate
the stability.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
I've omitted some CC in the previous email, adding them here. Sorry for the duplicated submission.
---
 drivers/regulator/max8998.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index 1cca8b5..99c5279 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/mutex.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/mfd/max8998.h>
@@ -302,10 +303,13 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
 	struct i2c_client *i2c = max8998->iodev->i2c;
 	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
+	int previous_vol = 0;
 	const struct voltage_map_desc *desc;
 	int ldo = max8998_get_ldo(rdev);
 	int reg, shift = 0, mask, ret;
 	int i = 0;
+	u8 val;
+	bool en_ramp = false;
 
 	if (ldo > ARRAY_SIZE(ldo_voltage_map))
 		return -EINVAL;
@@ -317,15 +321,37 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 	if (max_vol < desc->min || min_vol > desc->max)
 		return -EINVAL;
 
-	while (desc->min + desc->step*i < max_vol &&
+	while (desc->min + desc->step*i < min_vol &&
 	       desc->min + desc->step*i < desc->max)
 		i++;
 
+	if (desc->min + desc->step*i > max_vol)
+		return -EINVAL;
+
 	ret = max8998_get_voltage_register(rdev, &reg, &shift, &mask);
 	if (ret)
 		return ret;
 
-	return max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
+	/* wait for RAMP_UP_DELAY if rdev is BUCK1/2 and
+	 * ENRAMP is ON */
+	if (ldo == MAX8998_BUCK1 || ldo == MAX8998_BUCK2) {
+		max8998_read_reg(i2c, MAX8998_REG_ONOFF4, &val);
+		if (val & (1 << 4)) {
+			en_ramp = true;
+			previous_vol = max8998_get_voltage(rdev);
+		}
+	}
+
+	ret = max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
+
+	if (en_ramp == true) {
+		int difference = desc->min + desc->step * i
+			- previous_vol / 1000;
+		if (difference > 0)
+			udelay(difference / ((val & 0x0f) + 1));
+	}
+
+	return ret;
 }
 
 static struct regulator_ops max8998_ldo_ops = {
-- 
1.6.3.3

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[MyungJoo Ham]

Please ignore this one. I'm sending again with CCs omitted here.
Sorry for duplicated mails.

On Fri, Jul 16, 2010 at 4:46 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Two issues are addressed for max8998_set_voltage function.
>
> 1. Min/Max Voltage.
>
> ? ? ? ?max8998_set_voltage had been using the voltage value of
>
> ? ? ? ?min ( voltage[i] >= max_vol , i )
>
> ? ? ? ?This is corrected to use:
>
> ? ? ? ?min ( voltage[i] >= min_vol , i )
>
> 2. Ramp Up Delay.
>
> ? ? ? ?max8998_set_voltage should provide delay for BUCK1/2
> if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
> RAMP delay length. However, when max8998_set_voltage's new voltage is
> lower than the previous, we don't care because it does not deteriorate
> the stability.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> ?drivers/regulator/max8998.c | ? 30 ++++++++++++++++++++++++++++--
> ?1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
> index 1cca8b5..99c5279 100644
> --- a/drivers/regulator/max8998.c
> +++ b/drivers/regulator/max8998.c
> @@ -28,6 +28,7 @@
> ?#include <linux/slab.h>
> ?#include <linux/interrupt.h>
> ?#include <linux/mutex.h>
> +#include <linux/delay.h>
> ?#include <linux/platform_device.h>
> ?#include <linux/regulator/driver.h>
> ?#include <linux/mfd/max8998.h>
> @@ -302,10 +303,13 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
> ? ? ? ?struct max8998_data *max8998 = rdev_get_drvdata(rdev);
> ? ? ? ?struct i2c_client *i2c = max8998->iodev->i2c;
> ? ? ? ?int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
> + ? ? ? int previous_vol = 0;
> ? ? ? ?const struct voltage_map_desc *desc;
> ? ? ? ?int ldo = max8998_get_ldo(rdev);
> ? ? ? ?int reg, shift = 0, mask, ret;
> ? ? ? ?int i = 0;
> + ? ? ? u8 val;
> + ? ? ? bool en_ramp = false;
>
> ? ? ? ?if (ldo > ARRAY_SIZE(ldo_voltage_map))
> ? ? ? ? ? ? ? ?return -EINVAL;
> @@ -317,15 +321,37 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
> ? ? ? ?if (max_vol < desc->min || min_vol > desc->max)
> ? ? ? ? ? ? ? ?return -EINVAL;
>
> - ? ? ? while (desc->min + desc->step*i < max_vol &&
> + ? ? ? while (desc->min + desc->step*i < min_vol &&
> ? ? ? ? ? ? ? desc->min + desc->step*i < desc->max)
> ? ? ? ? ? ? ? ?i++;
>
> + ? ? ? if (desc->min + desc->step*i > max_vol)
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> ? ? ? ?ret = max8998_get_voltage_register(rdev, &reg, &shift, &mask);
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?return ret;
>
> - ? ? ? return max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
> + ? ? ? /* wait for RAMP_UP_DELAY if rdev is BUCK1/2 and
> + ? ? ? ?* ENRAMP is ON */
> + ? ? ? if (ldo == MAX8998_BUCK1 || ldo == MAX8998_BUCK2) {
> + ? ? ? ? ? ? ? max8998_read_reg(i2c, MAX8998_REG_ONOFF4, &val);
> + ? ? ? ? ? ? ? if (val & (1 << 4)) {
> + ? ? ? ? ? ? ? ? ? ? ? en_ramp = true;
> + ? ? ? ? ? ? ? ? ? ? ? previous_vol = max8998_get_voltage(rdev);
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? ret = max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
> +
> + ? ? ? if (en_ramp == true) {
> + ? ? ? ? ? ? ? int difference = desc->min + desc->step * i
> + ? ? ? ? ? ? ? ? ? ? ? - previous_vol / 1000;
> + ? ? ? ? ? ? ? if (difference > 0)
> + ? ? ? ? ? ? ? ? ? ? ? udelay(difference / ((val & 0x0f) + 1));
> + ? ? ? }
> +
> + ? ? ? return ret;
> ?}
>
> ?static struct regulator_ops max8998_ldo_ops = {
> --
> 1.6.3.3
>
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Mark Brown]

On Fri, Jul 16, 2010 at 05:09:20PM +0900, MyungJoo Ham wrote:
> Two issues are addressed for max8998_set_voltage function.

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

Ideally you should've submitted this as two separate patches.

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Marek Szyprowski]

Hello,

On Friday, July 16, 2010 10:09 AM MyungJoo Ham wrote:

> Two issues are addressed for max8998_set_voltage function.
> 
> 1. Min/Max Voltage.
> 
> 	max8998_set_voltage had been using the voltage value of
> 
> 	min ( voltage[i] >= max_vol , i )
> 
> 	This is corrected to use:
> 
> 	min ( voltage[i] >= min_vol , i )
> 
> 2. Ramp Up Delay.
> 
> 	max8998_set_voltage should provide delay for BUCK1/2
> if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
> RAMP delay length. However, when max8998_set_voltage's new voltage is
> lower than the previous, we don't care because it does not deteriorate
> the stability.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> --
> I've omitted some CC in the previous email, adding them here. Sorry for the
> duplicated submission.
> ---
>  drivers/regulator/max8998.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
> index 1cca8b5..99c5279 100644
> --- a/drivers/regulator/max8998.c
> +++ b/drivers/regulator/max8998.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/mutex.h>
> +#include <linux/delay.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/mfd/max8998.h>
> @@ -302,10 +303,13 @@ static int max8998_set_voltage(struct regulator_dev
> *rdev,
>  	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
>  	struct i2c_client *i2c = max8998->iodev->i2c;
>  	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
> +	int previous_vol = 0;
>  	const struct voltage_map_desc *desc;
>  	int ldo = max8998_get_ldo(rdev);
>  	int reg, shift = 0, mask, ret;
>  	int i = 0;
> +	u8 val;
> +	bool en_ramp = false;
> 
>  	if (ldo > ARRAY_SIZE(ldo_voltage_map))
>  		return -EINVAL;
> @@ -317,15 +321,37 @@ static int max8998_set_voltage(struct regulator_dev
> *rdev,
>  	if (max_vol < desc->min || min_vol > desc->max)
>  		return -EINVAL;
> 
> -	while (desc->min + desc->step*i < max_vol &&
> +	while (desc->min + desc->step*i < min_vol &&
>  	       desc->min + desc->step*i < desc->max)
>  		i++;
> 
> +	if (desc->min + desc->step*i > max_vol)
> +		return -EINVAL;
> +
>  	ret = max8998_get_voltage_register(rdev, &reg, &shift, &mask);
>  	if (ret)
>  		return ret;
> 
> -	return max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
> +	/* wait for RAMP_UP_DELAY if rdev is BUCK1/2 and
> +	 * ENRAMP is ON */
> +	if (ldo == MAX8998_BUCK1 || ldo == MAX8998_BUCK2) {
> +		max8998_read_reg(i2c, MAX8998_REG_ONOFF4, &val);
> +		if (val & (1 << 4)) {
> +			en_ramp = true;
> +			previous_vol = max8998_get_voltage(rdev);
> +		}
> +	}
> +
> +	ret = max8998_update_reg(i2c, reg, i<<shift, mask<<shift);
> +
> +	if (en_ramp == true) {
> +		int difference = desc->min + desc->step * i
> +			- previous_vol / 1000;
> +		if (difference > 0)
> +			udelay(difference / ((val & 0x0f) + 1));
> +	}
> +
> +	return ret;
>  }
> 
>  static struct regulator_ops max8998_ldo_ops = {
> --
> 1.6.3.3


Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Liam Girdwood]

On Fri, 2010-07-16 at 17:09 +0900, MyungJoo Ham wrote:
> Two issues are addressed for max8998_set_voltage function.
> 
> 1. Min/Max Voltage.
> 
> 	max8998_set_voltage had been using the voltage value of
> 
> 	min ( voltage[i] >= max_vol , i )
> 
> 	This is corrected to use:
> 
> 	min ( voltage[i] >= min_vol , i )
> 
> 2. Ramp Up Delay.
> 
> 	max8998_set_voltage should provide delay for BUCK1/2
> if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
> RAMP delay length. However, when max8998_set_voltage's new voltage is
> lower than the previous, we don't care because it does not deteriorate
> the stability.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> --
> I've omitted some CC in the previous email, adding them here. Sorry for the duplicated submission.

This doesn't apply. Can you re-create against my regulator for-next
branch.

git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6.git for-next

Thanks

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

[PATCH v2] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Marek Szyprowski]

From: MyungJoo Ham <myungjoo.ham@samsung.com>

Two issues are addressed for max8998_set_voltage function.

1. Min/Max Voltage.

	max8998_set_voltage had been using the voltage value of

	min ( voltage[i] >= max_vol , i )

	This is corrected to use:

	min ( voltage[i] >= min_vol , i )

2. Ramp Up Delay.

	max8998_set_voltage should provide delay for BUCK1/2
if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
RAMP delay length. However, when max8998_set_voltage's new voltage is
lower than the previous, we don't care because it does not deteriorate
the stability.

Changes since v1:
- rebased onto latest regulator-for-next tree

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/max8998.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index 174fd19..78aea26 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/mutex.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/mfd/max8998.h>
@@ -297,10 +298,13 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 {
 	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
 	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
+	int previous_vol = 0;
 	const struct voltage_map_desc *desc;
 	int ldo = max8998_get_ldo(rdev);
 	int reg, shift = 0, mask, ret;
 	int i = 0;
+	u8 val;
+	bool en_ramp = false;
 
 	if (ldo > ARRAY_SIZE(ldo_voltage_map))
 		return -EINVAL;
@@ -312,15 +316,36 @@ static int max8998_set_voltage(struct regulator_dev *rdev,
 	if (max_vol < desc->min || min_vol > desc->max)
 		return -EINVAL;
 
-	while (desc->min + desc->step*i < max_vol &&
+	while (desc->min + desc->step*i < min_vol &&
 	       desc->min + desc->step*i < desc->max)
 		i++;
 
+	if (desc->min + desc->step*i > max_vol)
+		return -EINVAL;
+
 	ret = max8998_get_voltage_register(rdev, &reg, &shift, &mask);
 	if (ret)
 		return ret;
 
-	return max8998_update_reg(max8998->iodev, reg, i<<shift, mask<<shift);
+	/* wait for RAMP_UP_DELAY if rdev is BUCK1/2 and
+	 * ENRAMP is ON */
+	if (ldo == MAX8998_BUCK1 || ldo == MAX8998_BUCK2) {
+		max8998_read_reg(max8998->iodev, MAX8998_REG_ONOFF4, &val);
+		if (val & (1 << 4)) {
+			en_ramp = true;
+			previous_vol = max8998_get_voltage(rdev);
+		}
+	}
+ 
+	ret = max8998_update_reg(max8998->iodev, reg, i<<shift, mask<<shift);
+ 
+	if (en_ramp == true) {
+		int difference = desc->min + desc->step*i - previous_vol/1000;
+		if (difference > 0)
+			udelay(difference / ((val & 0x0f) + 1));
+	}
+
+	return ret;
 }
 
 static struct regulator_ops max8998_ldo_ops = {
-- 
1.7.1.569.g6f426

[PATCH v2] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Mark Brown]

On Tue, Jul 20, 2010 at 08:07:07AM +0200, Marek Szyprowski wrote:
> From: MyungJoo Ham <myungjoo.ham@samsung.com>
> 
> Two issues are addressed for max8998_set_voltage function.

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

but as IIRC I said last time it would be nicer to fix one issue per
patch.

[PATCH v2] regulator: MAX8998: set_voltage bugfix. ramp_up delay and min/max voltage

[Liam Girdwood]

On Tue, 2010-07-20 at 08:07 +0200, Marek Szyprowski wrote:
> From: MyungJoo Ham <myungjoo.ham@samsung.com>
> 
> Two issues are addressed for max8998_set_voltage function.
> 
> 1. Min/Max Voltage.
> 
> 	max8998_set_voltage had been using the voltage value of
> 
> 	min ( voltage[i] >= max_vol , i )
> 
> 	This is corrected to use:
> 
> 	min ( voltage[i] >= min_vol , i )
> 
> 2. Ramp Up Delay.
> 
> 	max8998_set_voltage should provide delay for BUCK1/2
> if ENRAMP is on. It reads RAMP value from ONOFF4 register to determine
> RAMP delay length. However, when max8998_set_voltage's new voltage is
> lower than the previous, we don't care because it does not deteriorate
> the stability.
> 
> Changes since v1:
> - rebased onto latest regulator-for-next tree

Applied. Btw, please run checkpatch and/or cleanpatch before submitting
next time as this patch contained some trailing white space.

Thanks

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