regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

PATCH In review Axel Lin 2 participants 5 messages Jun 26, 2012

Subsystems (2)

Voltage and current regulator framework Wolfson microelectronics drivers

lore

Share & tools

lore

Axel Lin

author

2012-06-26 08:01 UTC #0 lore raw

PATCH 1/1 regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

ARIZONA_MICSUPP_MAX_SELECTOR is 0x1f

When selector is 0 ... 0x1e, we use below equation:

        (selector * 50000) + 1700000;

so the voltage range is 1700000 ... 3200000 uV.

When selector is 0x1f:

        voltage = 3300000 uV.

Thus in the map_voltage callback:

If min_uV > 3300000, we should return -EINVAL.
If min_uV > 3200000, selector should be ARIZONA_MICSUPP_MAX_SELECTOR.

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

---
 drivers/regulator/arizona-micsupp.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/regulator/arizona-micsupp.c +4 -1

index 97d85b0..a27421e 100644
@@ -54,10 +54,13 @@ static int arizona_micsupp_map_voltage(struct regulator_dev *rdev,
 	unsigned int voltage;
 	int selector;
 
+	if (min_uV > 3300000)
+		return -EINVAL;
+
 	if (min_uV < 1700000)
 		min_uV = 1700000;
 
-	if (min_uV >= 3300000)
+	if (min_uV > 3200000)
 		selector = ARIZONA_MICSUPP_MAX_SELECTOR;
 	else
 		selector = DIV_ROUND_UP(min_uV - 1700000, 50000);

Signature · 1 line

1.7.9.5

Mark Brown

2012-06-26 09:08 UTC #1 lore raw

drivers/regulator/arizona-micsupp.c static int arizona_micsupp_map_voltage(struct regulator_dev *rdev, · line 59

Quoted 3 lines · Axel Lin

drivers/regulator/arizona-micsupp.c +3 -0
+	if (min_uV > 3300000)
+		return -EINVAL;
+

This is OK but I think we want to factor this out into the caller as we're implementing this limits check in a lot of places.

drivers/regulator/arizona-micsupp.c static int arizona_micsupp_map_voltage(struct regulator_dev *rdev, · line 66

Quoted 5 lines

drivers/regulator/arizona-micsupp.c +1 -1

-	if (min_uV >= 3300000)
+	if (min_uV > 3200000)
 		selector = ARIZONA_MICSUPP_MAX_SELECTOR;
 	else
 		selector = DIV_ROUND_UP(min_uV - 1700000, 50000);

This doesn't change anything; with version of the if statement will give 3.3V for a voltage between 3.2V and 3.3V as there's no gaps in the selector space so if we're over 3.2V we'll round up to 3.3V.

Axel Lin

author

2012-06-26 09:27 UTC #2 lore raw

2012/6/26 Mark Brown <[email protected]>:

Quoted 8 lines · Axel Lin

Quoted 4 lines
+ if (min_uV > 3300000)
+ return -EINVAL;
+
This is OK but I think we want to factor this out into the caller as we're implementing this limits check in a lot of places.

It seems most of the new code are calling list_voltage() in map_voltage to ensure
the selected voltage are still in bound.
But in this case, current actually set selector to ARIZONA_MICSUPP_MAX_SELECTOR in map_voltage() if
min_uV >= 3300000. calling list_voltage() still returns valid voltage for this case looks wrong to me.

Quoted 10 lines

Quoted 6 lines
- if (min_uV >= 3300000)
+ if (min_uV > 3200000)
selector = ARIZONA_MICSUPP_MAX_SELECTOR; else
selector = DIV_ROUND_UP(min_uV - 1700000, 50000);
This doesn't change anything; with version of the if statement will give 3.3V for a voltage between 3.2V and 3.3V as there's no gaps in the selector space so if we're over 3.2V we'll round up to 3.3V.

If min_uV is in the range of: 3250001~3269999,
current code uses the equation: selector = DIV_ROUND_UP(min_uV - 1700000, 50000);
Then selector will be 32.
Then arizona_micsupp_list_voltage returns -EINVAL for this case.

With this patch, the selector will be 31 for this case.

Mark Brown

2012-06-26 09:52 UTC #3 lore raw

On Tue, Jun 26, 2012 at 05:27:26PM +0800, Axel Lin wrote:

Your mailer is doing something *really* odd with word wrapping, please fix it. It looks like it's just randomly wrapping lines rather than flowing paragraphs.

Quoted 5 lines

2012/6/26 Mark Brown <[email protected]>:
Quoted 2 lines
This is OK but I think we want to factor this out into the caller as we're implementing this limits check in a lot of places.

Quoted 7 lines

It seems most of the new code are calling list_voltage() in map_voltage to ensure
the selected voltage are still in bound.
for this case looks wrong to me.
But in this case, current actually set selector to ARIZONA_MICSUPP_MAX_SELECTOR in map_voltage() if
min_uV >= 3300000. calling list_voltage() still returns valid voltage

Which we then immediately check against min_uV so as far as I can tell we're fine here even with no code modifications.

Quoted 5 lines

If min_uV is in the range of: 3250001~3269999,
current code uses the equation: selector = DIV_ROUND_UP(min_uV - 1700000, 50000);
Then selector will be 32.
Then arizona_micsupp_list_voltage returns -EINVAL for this case.

OK, please submit a separate change for this. It would sometimes help if your changelog entries were clearer, while you do normally provide a lot of detail but you often don't highlight which are the important details or miss critical ones about why your change is important. Your original changelog for this makes it look like this is just a change being made for taste reasons since it doesn't mention the error case.

Axel Lin

author

2012-06-26 10:32 UTC #4 lore raw

2012/6/26 Mark Brown <[email protected]>:

Quoted 5 lines · Axel Lin

Your mailer is doing something *really* odd with word wrapping, please fix it. It looks like it's just randomly wrapping lines rather than flowing paragraphs.

I've no idea why gmail do the odd word wrapping...

Quoted 23 lines

Quoted 6 lines
2012/6/26 Mark Brown <[email protected]>:
Quoted 2 lines
This is OK but I think we want to factor this out into the caller as we're implementing this limits check in a lot of places.
Quoted 7 lines
It seems most of the new code are calling list_voltage() in map_voltage to ensure
the selected voltage are still in bound.
for this case looks wrong to me.
But in this case, current actually set selector to ARIZONA_MICSUPP_MAX_SELECTOR in map_voltage() if
min_uV >= 3300000. calling list_voltage() still returns valid voltage
Which we then immediately check against min_uV so as far as I can tell we're fine here even with no code modifications.
Quoted 5 lines
If min_uV is in the range of: 3250001~3269999,
current code uses the equation: selector = DIV_ROUND_UP(min_uV - 1700000, 50000);
Then selector will be 32.
Then arizona_micsupp_list_voltage returns -EINVAL for this case.

Quoted 6 lines

OK, please submit a separate change for this. It would sometimes help if your changelog entries were clearer, while you do normally provide a lot of detail but you often don't highlight which are the important details or miss critical ones about why your change is important. Your original changelog for this makes it look like this is just a change being made for taste reasons since it doesn't mention the error case.

I will send the patch with better changelog.

Regards,
Axel