regulator: pcf50633: Don't write to reserved bits of AUTO output voltage select register

PATCH Superseded Axel Lin 2 participants 4 messages Mar 16, 2012 Voltage and current regulator framework

Share & tools

Axel Lin

author

2012-03-16 13:35 UTC #0 lore raw

PATCH 1/2 regulator: pcf50633: Don't write to reserved bits of AUTO output voltage select register

The datasheet says 00000000 to 00101110 are reserved, and the min value of the voltage setting is 1.8 V.
Thus don't write 0 to AUTO output voltage select register (address 1Ah).

Table 50. AUTOOUT - AUTO output voltage select register (address 1Ah) bit description[1] Bit Symbol Access Description
7:0 auto_out R/W VO(prog) = 0.625 + auto_out × 0.025 V
eg. 00000000 to 00101110: reserved
00101111: 1.8 V (min)
01010011: 2.7 V
01101010: 3.275 V
01101011: 3.300 V
01101100: 3.325 V
01111111 : 3.800 V (max)
..... .....
11111110 : 3.800 V
11111111 : 3.800 V

This patch also fixes a bug in pcf50633_regulator_list_voltage.
It is wrong to do "index += 0x2f" for PCF50633_REGULATOR_AUTO in pcf50633_regulator_list_voltage. The purpose of adding 0x2f to index is because current code return 0 in auto_voltage_bits when millivolts < 1800. For millivolts > 1800, adding 0x2f to index is wrong.

We should handle this by
return -EINVAL if the selector is in the reserved range of AUTOOUT.

Signed-off-by Axel Lin <[email protected]>

---
 drivers/regulator/pcf50633-regulator.c |   23 +++++------------------
 1 files changed, 5 insertions(+), 18 deletions(-)

drivers/regulator/pcf50633-regulator.c +5 ~~-18~~

index 6db46c6..3cefc63 100644
@@ -52,7 +52,7 @@ static const u8 pcf50633_regulator_registers[PCF50633_NUM_REGULATORS] = {
 static u8 auto_voltage_bits(unsigned int millivolts)
 {
 	if (millivolts < 1800)
-		return 0;
+		return 0x2f;
 	if (millivolts > 3800)
 		return 0xff;

@@ -87,9 +87,6 @@ static u8 ldo_voltage_bits(unsigned int millivolts)
 /* Obtain voltage value from bits */
 static unsigned int auto_voltage_value(u8 bits)
 {
-	if (bits < 0x2f)
-		return 0;
-
 	return 625 + (bits * 25);
 }

@@ -161,6 +158,9 @@ static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
 
 	switch (id) {
 	case PCF50633_REGULATOR_AUTO:
+		/* AUTOOUT: 00000000 to 00101110 are reserved */
+		if (bits < 0x2f)
+			return -EINVAL;
 		millivolts = auto_voltage_value(bits);
 		break;
 	case PCF50633_REGULATOR_DOWN1:

@@ -208,20 +208,7 @@ static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev)
 static int pcf50633_regulator_list_voltage(struct regulator_dev *rdev,
 						unsigned int index)
 {
-	struct pcf50633 *pcf;
-	int regulator_id;
-
-	pcf = rdev_get_drvdata(rdev);
-
-	regulator_id = rdev_get_id(rdev);
-
-	switch (regulator_id) {
-	case PCF50633_REGULATOR_AUTO:
-		index += 0x2f;
-		break;
-	default:
-		break;
-	}
+	int regulator_id = rdev_get_id(rdev);
 
 	return pcf50633_regulator_voltage_value(regulator_id, index);
 }

Signature · 1 line

1.7.5.4

Axel Lin

author

2012-03-16 13:36 UTC #1 lore raw

PATCH 2/2 regulator: Convert pcf50633 to get_voltage_sel

Convert pcf50633 to get_voltage_sel and then we can remove pcf50633_regulator_voltage_value function and move its implementation to pcf50633_regulator_list_voltage.

Signed-off-by Axel Lin <[email protected]>

---
 drivers/regulator/pcf50633-regulator.c |   65 ++++++++++++++-----------------
 1 files changed, 29 insertions(+), 36 deletions(-)

drivers/regulator/pcf50633-regulator.c +29 ~~-36~~

index 3cefc63..5c712e5 100644
@@ -151,23 +151,43 @@ static int pcf50633_regulator_set_voltage(struct regulator_dev *rdev,
 	return pcf50633_reg_write(pcf, regnr, volt_bits);
 }
 
-static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
-						u8 bits)
+static int pcf50633_regulator_get_voltage_sel(struct regulator_dev *rdev)
 {
+	struct pcf50633 *pcf;
+	int regulator_id;
+	u8 volt_bits, regnr;
+
+	pcf = rdev_get_drvdata(rdev);
+
+	regulator_id = rdev_get_id(rdev);
+	if (regulator_id >= PCF50633_NUM_REGULATORS)
+		return -EINVAL;
+
+	regnr = pcf50633_regulator_registers[regulator_id];
+
+	volt_bits = pcf50633_reg_read(pcf, regnr);
+
+	return volt_bits;
+}
+
+static int pcf50633_regulator_list_voltage(struct regulator_dev *rdev,
+						unsigned int index)
+{
+	int regulator_id = rdev_get_id(rdev);
 	int millivolts;
 
-	switch (id) {
+	switch (regulator_id) {
 	case PCF50633_REGULATOR_AUTO:
 		/* AUTOOUT: 00000000 to 00101110 are reserved */
-		if (bits < 0x2f)
+		if (index < 0x2f)
 			return -EINVAL;
-		millivolts = auto_voltage_value(bits);
+		millivolts = auto_voltage_value(index);
 		break;
 	case PCF50633_REGULATOR_DOWN1:
-		millivolts = down_voltage_value(bits);
+		millivolts = down_voltage_value(index);
 		break;
 	case PCF50633_REGULATOR_DOWN2:
-		millivolts = down_voltage_value(bits);
+		millivolts = down_voltage_value(index);
 		break;
 	case PCF50633_REGULATOR_LDO1:
 	case PCF50633_REGULATOR_LDO2:

@@ -177,7 +197,7 @@ static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
 	case PCF50633_REGULATOR_LDO6:
 	case PCF50633_REGULATOR_HCLDO:
 	case PCF50633_REGULATOR_MEMLDO:
-		millivolts = ldo_voltage_value(bits);
+		millivolts = ldo_voltage_value(index);
 		break;
 	default:
 		return -EINVAL;

@@ -186,33 +206,6 @@ static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
 	return millivolts * 1000;
 }
 
-static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev)
-{
-	struct pcf50633 *pcf;
-	int regulator_id;
-	u8 volt_bits, regnr;
-
-	pcf = rdev_get_drvdata(rdev);
-
-	regulator_id = rdev_get_id(rdev);
-	if (regulator_id >= PCF50633_NUM_REGULATORS)
-		return -EINVAL;
-
-	regnr = pcf50633_regulator_registers[regulator_id];
-
-	volt_bits = pcf50633_reg_read(pcf, regnr);
-
-	return pcf50633_regulator_voltage_value(regulator_id, volt_bits);
-}
-
-static int pcf50633_regulator_list_voltage(struct regulator_dev *rdev,
-						unsigned int index)
-{
-	int regulator_id = rdev_get_id(rdev);
-
-	return pcf50633_regulator_voltage_value(regulator_id, index);
-}
-
 static int pcf50633_regulator_enable(struct regulator_dev *rdev)
 {
 	struct pcf50633 *pcf = rdev_get_drvdata(rdev);

@@ -265,7 +258,7 @@ static int pcf50633_regulator_is_enabled(struct regulator_dev *rdev)
 
 static struct regulator_ops pcf50633_regulator_ops = {
 	.set_voltage = pcf50633_regulator_set_voltage,
-	.get_voltage = pcf50633_regulator_get_voltage,
+	.get_voltage_sel = pcf50633_regulator_get_voltage_sel,
 	.list_voltage = pcf50633_regulator_list_voltage,
 	.enable = pcf50633_regulator_enable,
 	.disable = pcf50633_regulator_disable,

Signature · 1 line

1.7.5.4

Lars-Peter Clausen

2012-03-16 13:56 UTC #2 lore raw

In reply to Axel Lin

Quoted 57 lines · Axel Lin

The datasheet says 00000000 to 00101110 are reserved, and the min value of the voltage setting is 1.8 V.
Thus don't write 0 to AUTO output voltage select register (address 1Ah).
Table 50. AUTOOUT - AUTO output voltage select register (address 1Ah) bit description[1] Bit Symbol Access Description
7:0 auto_out R/W VO(prog) = 0.625 + auto_out × 0.025 V
eg. 00000000 to 00101110: reserved
00101111: 1.8 V (min)
01010011: 2.7 V
01101010: 3.275 V
01101011: 3.300 V
01101100: 3.325 V
01111111 : 3.800 V (max)
..... .....
11111110 : 3.800 V
11111111 : 3.800 V
This patch also fixes a bug in pcf50633_regulator_list_voltage.
It is wrong to do "index += 0x2f" for PCF50633_REGULATOR_AUTO in pcf50633_regulator_list_voltage. The purpose of adding 0x2f to index is because current code return 0 in auto_voltage_bits when millivolts < 1800. For millivolts > 1800, adding 0x2f to index is wrong.
We should handle this by
return -EINVAL if the selector is in the reserved range of AUTOOUT.
Signed-off-by Axel Lin <[email protected]>
---
 drivers/regulator/pcf50633-regulator.c |   23 +++++------------------
 1 files changed, 5 insertions(+), 18 deletions(-)
drivers/regulator/pcf50633-regulator.c +3 -4
index 6db46c6..3cefc63 100644
@@ -52,7 +52,7 @@ static const u8 pcf50633_regulator_registers[PCF50633_NUM_REGULATORS] = {
[...]
@@ -161,6 +158,9 @@ static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
 
 	switch (id) {
 	case PCF50633_REGULATOR_AUTO:
+		/* AUTOOUT: 00000000 to 00101110 are reserved */
+		if (bits < 0x2f)
+			return -EINVAL;
 		millivolts = auto_voltage_value(bits);
 		break;
 	case PCF50633_REGULATOR_DOWN1:
@@ -208,20 +208,7 @@ static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev)
 static int pcf50633_regulator_list_voltage(struct regulator_dev *rdev,
 						unsigned int index)
 {
-	struct pcf50633 *pcf;
-	int regulator_id;
-
-	pcf = rdev_get_drvdata(rdev);

drivers/regulator/pcf50633-regulator.c static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev) · line 224

Quoted 10 lines

drivers/regulator/pcf50633-regulator.c +0 ~~-10~~

-
-	regulator_id = rdev_get_id(rdev);
-
-	switch (regulator_id) {
-	case PCF50633_REGULATOR_AUTO:
-		index += 0x2f;
-		break;
-	default:
-		break;
-	}

Does this make sense? Now we return -EINVAL if index is less than 0x2f

Axel Lin

author

2012-03-16 23:55 UTC #3 lore raw

Quoted 27 lines

drivers/regulator/pcf50633-regulator.c +0 -0

@@ -52,7 +52,7 @@ static const u8 pcf50633_regulator_registers[PCF50633_NUM_REGULATORS] = {
[...]

@@ -161,6 +158,9 @@ static int pcf50633_regulator_voltage_value(enum pcf50633_regulator_id id,
 
       switch (id) {

case PCF50633_REGULATOR_AUTO:
+ /* AUTOOUT: 00000000 to 00101110 are reserved */
+ if (bits < 0x2f)
+ return -EINVAL;
millivolts = auto_voltage_value(bits); break;
case PCF50633_REGULATOR_DOWN1:

drivers/regulator/pcf50633-regulator.c +0 -8

@@ -208,20 +208,7 @@ static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev)
  static int pcf50633_regulator_list_voltage(struct regulator_dev *rdev,
                                               unsigned int index)
  {
-     struct pcf50633 *pcf;
-     int regulator_id;
-
-     pcf = rdev_get_drvdata(rdev);
-
-     regulator_id = rdev_get_id(rdev);
-
-     switch (regulator_id) {

Quoted 10 lines

drivers/regulator/pcf50633-regulator.c static int pcf50633_regulator_get_voltage(struct regulator_dev *rdev) · line 224
Quoted 6 lines
drivers/regulator/pcf50633-regulator.c +0 -6
-     case PCF50633_REGULATOR_AUTO:
-             index += 0x2f;
-             break;
-     default:
-             break;
-     }
Does this make sense? Now we return -EINVAL if index is less than 0x2f

I check the code and regulator core again.
I think for this case we should return 0 instead of -EINVAL.

/**
* regulator_list_voltage - enumerate supported voltages
* @regulator: regulator source
* @selector: identify voltage to list
* Context: can sleep
*
* Returns a voltage that can be passed to @regulator_set_voltage(),
* zero if this selector code can't be used on this system, or a
* negative errno.
*/

I'll send a V2 soon.
Thanks for the review.
Axel