Skip to contentSkip to conversation

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

PATCH New MyungJoo Ham 4 participants 7 messages Jul 16, 2010 – Jul 22, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

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.

--
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(-)
drivers/regulator/max8998.c +28 -2
index 1cca8b5..99c5279 100644
@@ -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 = {
Signature · 1 line
1.6.3.3
#1 lore raw

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

Quoted 1 line · MyungJoo Ham

Two issues are addressed for max8998_set_voltage function.

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

#2 lore raw

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

In reply to MyungJoo Ham

Hello,

Quoted 14 lines · 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.

Quoted 8 lines

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.

Quoted 74 lines

--
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(-)
drivers/regulator/max8998.c +4 -0
index 1cca8b5..99c5279 100644
@@ -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))
drivers/regulator/max8998.c +24 -2
 		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;
 }
 
Quoted 3 lines

static struct regulator_ops max8998_ldo_ops = {
--
1.6.3.3

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

#3 lore raw

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

In reply to MyungJoo Ham
Quoted 20 lines · 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.

Quoted 4 lines

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

Signature · 3 lines
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk
#4 lore raw

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

From: MyungJoo Ham <[email protected]>

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

---
 drivers/regulator/max8998.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)
drivers/regulator/max8998.c +27 -2
index 174fd19..78aea26 100644
@@ -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 = {
Signature · 1 line
1.7.1.569.g6f426
#5 lore raw

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

Quoted 3 lines · Marek Szyprowski

From: MyungJoo Ham <[email protected]>

Two issues are addressed for max8998_set_voltage function.

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

#6 lore raw

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

In reply to Marek Szyprowski
Quoted 16 lines · Marek Szyprowski

From: MyungJoo Ham <[email protected]>

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.

Quoted 8 lines

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

Signature · 3 lines
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk