[PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Axel Lin]

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 <axel.lin@gmail.com>
---
 drivers/regulator/arizona-micsupp.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/arizona-micsupp.c b/drivers/regulator/arizona-micsupp.c
index 97d85b0..a27421e 100644
--- a/drivers/regulator/arizona-micsupp.c
+++ b/drivers/regulator/arizona-micsupp.c
@@ -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);
-- 
1.7.9.5

Re: [PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Tue, Jun 26, 2012 at 04:01:47PM +0800, Axel Lin wrote:

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Axel Lin]

2012/6/26 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Tue, Jun 26, 2012 at 04:01:47PM +0800, Axel Lin wrote:
>
>> +     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.

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

Re: [PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

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.

> 2012/6/26 Mark Brown <broonie@opensource.wolfsonmicro.com>:

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Axel Lin]

2012/6/26 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> 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.
I've no idea why gmail do the odd word wrapping...

>
>> 2012/6/26 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>
>> > 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.
>> 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.
>
>> 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.
I will send the patch with better changelog.

Regards,
Axel

[PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Axel Lin]

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 which is wrong.

This patch fixes this issue:
If min_uV > 3200000, selector should be ARIZONA_MICSUPP_MAX_SELECTOR.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 drivers/regulator/arizona-micsupp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/arizona-micsupp.c b/drivers/regulator/arizona-micsupp.c
index 97d85b0..fdd7473 100644
--- a/drivers/regulator/arizona-micsupp.c
+++ b/drivers/regulator/arizona-micsupp.c
@@ -57,7 +57,7 @@ static int arizona_micsupp_map_voltage(struct regulator_dev *rdev,
 	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);
-- 
1.7.9.5

Re: [PATCH] regulator: arizona-micsupp: Fix choosing selector in arizona_micsupp_map_voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

On Tue, Jun 26, 2012 at 06:37:58PM +0800, Axel Lin wrote:
> If min_uV is in the range of: 3250001~3269999,
> current code uses the equation:

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]