Skip to contentSkip to conversation

regulator: max8997: Use uV in voltage_map_desc

PATCH Applied Axel Lin 2 participants 6 messages Dec 28, 2012 – Jan 8, 2013 Voltage and current regulator framework

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/3 regulator: max8997: Use uV in voltage_map_desc

Current code does integer division (min_vol = min_uV / 1000) before pass min_vol to max8997_get_voltage_proper_val().
So it is possible min_vol is truncated to a smaller value.

For example, if the request min_uV is 800900 for ldo.
min_vol = 800900 / 1000 = 800 (mV)
Then max8997_get_voltage_proper_val returns 800 mV for this case which is lower than the requested voltage.

Use uV rather than mV in voltage_map_desc to prevent truncation by integer division.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/regulator/max8997.c |   36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)
drivers/regulator/max8997.c +17 -19
index c30424a..1a1ae99 100644
@@ -78,26 +78,26 @@ struct voltage_map_desc {
 	int step;
 };
 
-/* Voltage maps in mV */
+/* Voltage maps in uV */
 static const struct voltage_map_desc ldo_voltage_map_desc = {
-	.min = 800,	.max = 3950,	.step = 50,
+	.min = 800000,	.max = 3950000,	.step = 50000,
 }; /* LDO1 ~ 18, 21 all */
 
 static const struct voltage_map_desc buck1245_voltage_map_desc = {
-	.min = 650,	.max = 2225,	.step = 25,
+	.min = 650000,	.max = 2225000,	.step = 25000,
 }; /* Buck1, 2, 4, 5 */
 
 static const struct voltage_map_desc buck37_voltage_map_desc = {
-	.min = 750,	.max = 3900,	.step = 50,
+	.min = 750000,	.max = 3900000,	.step = 50000,
 }; /* Buck3, 7 */
 
-/* current map in mA */
+/* current map in uA */
 static const struct voltage_map_desc charger_current_map_desc = {
-	.min = 200,	.max = 950,	.step = 50,
+	.min = 200000,	.max = 950000,	.step = 50000,
 };
 
 static const struct voltage_map_desc topoff_current_map_desc = {
-	.min = 50,	.max = 200,	.step = 10,
+	.min = 50000,	.max = 200000,	.step = 10000,
 };
 
 static const struct voltage_map_desc *reg_voltage_map[] = {

@@ -178,7 +178,7 @@ static int max8997_list_voltage(struct regulator_dev *rdev,
 	if (val > desc->max)
 		return -EINVAL;
 
-	return val * 1000;
+	return val;
 }
 
 static int max8997_get_enable_register(struct regulator_dev *rdev,

@@ -469,7 +469,6 @@ static int max8997_set_voltage_ldobuck(struct regulator_dev *rdev,
 {
 	struct max8997_data *max8997 = rdev_get_drvdata(rdev);
 	struct i2c_client *i2c = max8997->iodev->i2c;
-	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
 	const struct voltage_map_desc *desc;
 	int rid = rdev_get_id(rdev);
 	int i, reg, shift, mask, ret;

@@ -493,7 +492,7 @@ static int max8997_set_voltage_ldobuck(struct regulator_dev *rdev,
 
 	desc = reg_voltage_map[rid];
 
-	i = max8997_get_voltage_proper_val(desc, min_vol, max_vol);
+	i = max8997_get_voltage_proper_val(desc, min_uV, max_uV);
 	if (i < 0)
 		return i;
 

@@ -541,7 +540,7 @@ static int max8997_set_voltage_buck_time_sel(struct regulator_dev *rdev,
 	case MAX8997_BUCK4:
 	case MAX8997_BUCK5:
 		return DIV_ROUND_UP(desc->step * (new_selector - old_selector),
-				    max8997->ramp_delay);
+				    max8997->ramp_delay * 1000);
 	}
 
 	return 0;

@@ -640,7 +639,6 @@ static int max8997_set_voltage_buck(struct regulator_dev *rdev,
 	const struct voltage_map_desc *desc;
 	int new_val, new_idx, damage, tmp_val, tmp_idx, tmp_dmg;
 	bool gpio_dvs_mode = false;
-	int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
 
 	if (rid < MAX8997_BUCK1 || rid > MAX8997_BUCK7)
 		return -EINVAL;

@@ -665,7 +663,7 @@ static int max8997_set_voltage_buck(struct regulator_dev *rdev,
 						selector);
 
 	desc = reg_voltage_map[rid];
-	new_val = max8997_get_voltage_proper_val(desc, min_vol, max_vol);
+	new_val = max8997_get_voltage_proper_val(desc, min_uV, max_uV);
 	if (new_val < 0)
 		return new_val;
 

@@ -1080,8 +1078,8 @@ static int max8997_pmic_probe(struct platform_device *pdev)
 		max8997->buck1_vol[i] = ret =
 			max8997_get_voltage_proper_val(
 					&buck1245_voltage_map_desc,
-					pdata->buck1_voltage[i] / 1000,
-					pdata->buck1_voltage[i] / 1000 +
+					pdata->buck1_voltage[i],
+					pdata->buck1_voltage[i] +
 					buck1245_voltage_map_desc.step);
 		if (ret < 0)
 			goto err_out;

@@ -1089,8 +1087,8 @@ static int max8997_pmic_probe(struct platform_device *pdev)
 		max8997->buck2_vol[i] = ret =
 			max8997_get_voltage_proper_val(
 					&buck1245_voltage_map_desc,
-					pdata->buck2_voltage[i] / 1000,
-					pdata->buck2_voltage[i] / 1000 +
+					pdata->buck2_voltage[i],
+					pdata->buck2_voltage[i] +
 					buck1245_voltage_map_desc.step);
 		if (ret < 0)
 			goto err_out;

@@ -1098,8 +1096,8 @@ static int max8997_pmic_probe(struct platform_device *pdev)
 		max8997->buck5_vol[i] = ret =
 			max8997_get_voltage_proper_val(
 					&buck1245_voltage_map_desc,
-					pdata->buck5_voltage[i] / 1000,
-					pdata->buck5_voltage[i] / 1000 +
+					pdata->buck5_voltage[i],
+					pdata->buck5_voltage[i] +
 					buck1245_voltage_map_desc.step);
 		if (ret < 0)
 			goto err_out;
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 2/3 regulator: max8998: Use uV in voltage_map_desc

Integer division may truncate.
This happens when pdata->buckx_voltagex setting is not align with 1000 uV. Thus use uV in voltage_map_desc, this ensures the selected voltage won't less than pdata buckx_voltagex settings.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/regulator/max8998.c |   42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)
drivers/regulator/max8998.c +21 -21
index b821d08..06be8cc 100644
@@ -51,39 +51,39 @@ struct voltage_map_desc {
 	int step;
 };
 
-/* Voltage maps */
+/* Voltage maps in uV*/
 static const struct voltage_map_desc ldo23_voltage_map_desc = {
-	.min = 800,	.step = 50,	.max = 1300,
+	.min = 800000,	.step = 50000,	.max = 1300000,
 };
 static const struct voltage_map_desc ldo456711_voltage_map_desc = {
-	.min = 1600,	.step = 100,	.max = 3600,
+	.min = 1600000,	.step = 100000,	.max = 3600000,
 };
 static const struct voltage_map_desc ldo8_voltage_map_desc = {
-	.min = 3000,	.step = 100,	.max = 3600,
+	.min = 3000000,	.step = 100000,	.max = 3600000,
 };
 static const struct voltage_map_desc ldo9_voltage_map_desc = {
-	.min = 2800,	.step = 100,	.max = 3100,
+	.min = 2800000,	.step = 100000,	.max = 3100000,
 };
 static const struct voltage_map_desc ldo10_voltage_map_desc = {
-	.min = 950,	.step = 50,	.max = 1300,
+	.min = 95000,	.step = 50000,	.max = 1300000,
 };
 static const struct voltage_map_desc ldo1213_voltage_map_desc = {
-	.min = 800,	.step = 100,	.max = 3300,
+	.min = 800000,	.step = 100000,	.max = 3300000,
 };
 static const struct voltage_map_desc ldo1415_voltage_map_desc = {
-	.min = 1200,	.step = 100,	.max = 3300,
+	.min = 1200000,	.step = 100000,	.max = 3300000,
 };
 static const struct voltage_map_desc ldo1617_voltage_map_desc = {
-	.min = 1600,	.step = 100,	.max = 3600,
+	.min = 1600000,	.step = 100000,	.max = 3600000,
 };
 static const struct voltage_map_desc buck12_voltage_map_desc = {
-	.min = 750,	.step = 25,	.max = 1525,
+	.min = 750000,	.step = 25000,	.max = 1525000,
 };
 static const struct voltage_map_desc buck3_voltage_map_desc = {
-	.min = 1600,	.step = 100,	.max = 3600,
+	.min = 1600000,	.step = 100000,	.max = 3600000,
 };
 static const struct voltage_map_desc buck4_voltage_map_desc = {
-	.min = 800,	.step = 100,	.max = 2300,
+	.min = 800000,	.step = 100000,	.max = 2300000,
 };
 
 static const struct voltage_map_desc *ldo_voltage_map[] = {

@@ -445,7 +445,7 @@ static int max8998_set_voltage_buck_time_sel(struct regulator_dev *rdev,
 	if (max8998->iodev->type == TYPE_MAX8998 && !(val & MAX8998_ENRAMP))
 		return 0;
 
-	difference = (new_selector - old_selector) * desc->step;
+	difference = (new_selector - old_selector) * desc->step / 1000;
 	if (difference > 0)
 		return difference / ((val & 0x0f) + 1);
 

@@ -702,7 +702,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck1_voltage1 / 1000))
+		       < pdata->buck1_voltage1)
 			i++;
 		max8998->buck1_vol[0] = i;
 		ret = max8998_write_reg(i2c, MAX8998_REG_BUCK1_VOLTAGE1, i);

@@ -713,7 +713,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck1_voltage2 / 1000))
+		       < pdata->buck1_voltage2)
 			i++;
 
 		max8998->buck1_vol[1] = i;

@@ -725,7 +725,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck1_voltage3 / 1000))
+		       < pdata->buck1_voltage3)
 			i++;
 
 		max8998->buck1_vol[2] = i;

@@ -737,7 +737,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck1_voltage4 / 1000))
+		       < pdata->buck1_voltage4)
 			i++;
 
 		max8998->buck1_vol[3] = i;

@@ -763,7 +763,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck2_voltage1 / 1000))
+		       < pdata->buck2_voltage1)
 			i++;
 		max8998->buck2_vol[0] = i;
 		ret = max8998_write_reg(i2c, MAX8998_REG_BUCK2_VOLTAGE1, i);

@@ -774,7 +774,7 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 		i = 0;
 		while (buck12_voltage_map_desc.min +
 		       buck12_voltage_map_desc.step*i
-		       < (pdata->buck2_voltage2 / 1000))
+		       < pdata->buck2_voltage2)
 			i++;
 		max8998->buck2_vol[1] = i;
 		ret = max8998_write_reg(i2c, MAX8998_REG_BUCK2_VOLTAGE2, i);

@@ -792,8 +792,8 @@ static int max8998_pmic_probe(struct platform_device *pdev)
 			int count = (desc->max - desc->min) / desc->step + 1;
 
 			regulators[index].n_voltages = count;
-			regulators[index].min_uV = desc->min * 1000;
-			regulators[index].uV_step = desc->step * 1000;
+			regulators[index].min_uV = desc->min;
+			regulators[index].uV_step = desc->step;
 		}
 
 		config.dev = max8998->dev;
Signature · 1 line
1.7.9.5
Applied
#2 lore raw

Re: [PATCH 2/3] regulator: max8998: Use uV in voltage_map_desc

Quoted 4 lines · Axel Lin

Integer division may truncate.
This happens when pdata->buckx_voltagex setting is not align with 1000 uV. Thus use uV in voltage_map_desc, this ensures the selected voltage won't less than pdata buckx_voltagex settings.

Applied, thanks.

author
#3 lore raw

PATCH 3/3 regulator: max8998: Return enough delay time for max8998_set_voltage_buck_time_sel

In reply to Axel Lin

Use DIV_ROUND_UP to prevent truncation by integer division issue. This ensures we return enough delay time.

Since the delay is required only if the voltage is increasing, and we know both old_selector and new_selector.
We can check it earlier, for linear mapping, by simply compare new_selector with old_selector.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/regulator/max8998.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/regulator/max8998.c +7 -9
index 06be8cc..26e5915 100644
@@ -425,16 +425,17 @@ static int max8998_set_voltage_buck_time_sel(struct regulator_dev *rdev,
 {
 	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
 	struct i2c_client *i2c = max8998->iodev->i2c;
-	const struct voltage_map_desc *desc;
 	int buck = rdev_get_id(rdev);
 	u8 val = 0;
-	int difference, ret;
+	int ret;
+
+	/* Delay is required only if the voltage is increasing */
+	if (new_selector <= old_selector)
+		return 0;
 
 	if (buck < MAX8998_BUCK1 || buck > MAX8998_BUCK4)
 		return -EINVAL;
 
-	desc = ldo_voltage_map[buck];
-
 	/* Voltage stabilization */
 	ret = max8998_read_reg(i2c, MAX8998_REG_ONOFF4, &val);
 	if (ret)

@@ -445,11 +446,8 @@ static int max8998_set_voltage_buck_time_sel(struct regulator_dev *rdev,
 	if (max8998->iodev->type == TYPE_MAX8998 && !(val & MAX8998_ENRAMP))
 		return 0;
 
-	difference = (new_selector - old_selector) * desc->step / 1000;
-	if (difference > 0)
-		return difference / ((val & 0x0f) + 1);
-
-	return 0;
+	return DIV_ROUND_UP((new_selector - old_selector) * rdev->desc->uV_step,
+			    ((val & 0x0f) + 1) * 1000);
 }
 
 static struct regulator_ops max8998_ldo_ops = {
Signature · 1 line
1.7.9.5
#4 lore raw

Re: [PATCH 3/3] regulator: max8998: Return enough delay time for max8998_set_voltage_buck_time_sel

Quoted 2 lines · Axel Lin

Use DIV_ROUND_UP to prevent truncation by integer division issue. This ensures we return enough delay time.

This bit is OK.

Quoted 4 lines

Since the delay is required only if the voltage is increasing, and we know both old_selector and new_selector.
We can check it earlier, for linear mapping, by simply compare new_selector with old_selector.

This is generally a reasonable assumption but it's not a bug fix, it's an optimisation, and we should be doing it in core code not in a specific driver.

Applied
#5 lore raw
In reply to Axel Lin
Quoted 3 lines · Axel Lin

Current code does integer division (min_vol = min_uV / 1000) before pass min_vol to max8997_get_voltage_proper_val().
So it is possible min_vol is truncated to a smaller value.

Applied, thanks.