[PATCH] regulator: Update consumer state only after set voltage succeeds.

[PATCH] regulator: Update consumer state only after set voltage succeeds.

[Saravana Kannan]

If the consumer state is updated but an error is returned due to
consumer constraint issues, then all set voltages by all consumers on
the regulator will fail till the "bad" consumer updates its voltage
range with a compatible value.  A bad consumer request that's rejected
shouldn't prevent other consumers from changing the voltage.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/regulator/core.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ab419f8..590ae0c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -148,11 +148,14 @@ static int regulator_check_voltage(struct regulator_dev *rdev,
  * regulator consumers
  */
 static int regulator_check_consumers(struct regulator_dev *rdev,
+				     struct regulator *ignore,
 				     int *min_uV, int *max_uV)
 {
 	struct regulator *regulator;
 
 	list_for_each_entry(regulator, &rdev->consumer_list, list) {
+		if (regulator  == ignore)
+			continue;
 		if (*max_uV > regulator->max_uV)
 			*max_uV = regulator->max_uV;
 		if (*min_uV < regulator->min_uV)
@@ -1719,14 +1722,15 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
 	if (ret < 0)
 		goto out;
-	regulator->min_uV = min_uV;
-	regulator->max_uV = max_uV;
-
-	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	ret = regulator_check_consumers(rdev, regulator, &min_uV, &max_uV);
 	if (ret < 0)
 		goto out;
 
 	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+	if (!ret) {
+		regulator->min_uV = min_uV;
+		regulator->max_uV = max_uV;
+	}
 
 out:
 	mutex_unlock(&rdev->mutex);
@@ -1769,7 +1773,7 @@ int regulator_sync_voltage(struct regulator *regulator)
 	if (ret < 0)
 		goto out;
 
-	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	ret = regulator_check_consumers(rdev, NULL, &min_uV, &max_uV);
 	if (ret < 0)
 		goto out;
 
-- 
1.7.1.1
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] regulator: Update consumer state only after set voltage succeeds.

[Mark Brown]

On Fri, Dec 17, 2010 at 02:44:28PM -0800, Saravana Kannan wrote:

>  static int regulator_check_consumers(struct regulator_dev *rdev,
> +				     struct regulator *ignore,
>  				     int *min_uV, int *max_uV)

This feels really invasive, and prone to robustness issues as we're just
randomly not checking one of the consumers on a single call, meaning we
skip some checking some of the time.  It's not going to make the code
more maintainable.

> -	regulator->min_uV = min_uV;
> -	regulator->max_uV = max_uV;
> -
> -	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
> +	ret = regulator_check_consumers(rdev, regulator, &min_uV, &max_uV);
>  	if (ret < 0)
>  		goto out;
>  
>  	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
> +	if (!ret) {
> +		regulator->min_uV = min_uV;
> +		regulator->max_uV = max_uV;
> +	}

If you're going to do something probably unwinding the assignment on
error would cover it.

Re: [PATCH] regulator: Update consumer state only after set voltage succeeds.

[Saravana Kannan]

On 12/20/10 04:39, Mark Brown wrote:
> On Fri, Dec 17, 2010 at 02:44:28PM -0800, Saravana Kannan wrote:
>
>>   static int regulator_check_consumers(struct regulator_dev *rdev,
>> +				     struct regulator *ignore,
>>   				     int *min_uV, int *max_uV)
>
> This feels really invasive, and prone to robustness issues as we're just
> randomly not checking one of the consumers on a single call, meaning we
> skip some checking some of the time.  It's not going to make the code
> more maintainable.

I agree it looks a bit odd and I'm willing to do the code reorg if there 
is a better way. But I definitely wouldn't call this as randomly 
ignoring a consumer. We are just avoiding the consumer that's changing 
the range from "voting twice". We already send the new request thru 
min/max params.

We will also need the option of not including the calling consumer when 
computing the min/max for the next patch. See below.

Do you have any suggestions for a better way to compute the min/max 
while leaving out a single consumer? I'm very much open to do that.

Would something like below be better?

regulator_check_consumers_except(rdev, ignore, min, max)
{
  ...
}

regulator_check_consumers(rdev, min, max)
{
  regulator_check_consumer(rdev, NULL, min, max);
}

>
>> -	regulator->min_uV = min_uV;
>> -	regulator->max_uV = max_uV;
>> -
>> -	ret = regulator_check_consumers(rdev,&min_uV,&max_uV);
>> +	ret = regulator_check_consumers(rdev, regulator,&min_uV,&max_uV);
>>   	if (ret<  0)
>>   		goto out;
>>
>>   	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
>> +	if (!ret) {
>> +		regulator->min_uV = min_uV;
>> +		regulator->max_uV = max_uV;
>> +	}
>
> If you're going to do something probably unwinding the assignment on
> error would cover it.

It would, but the next patch was going to be to optimize out the call to 
the regulator driver if the votes of the calling consumer doesn't make a 
difference. To do that, we will need to compute the voltage range with 
and without the calling consumer's min/max and then figure out if the 
change in the calling consumer's min/max makes a difference.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] regulator: Update consumer state only after set voltage succeeds.

[Mark Brown]

On Mon, Dec 20, 2010 at 02:27:40PM -0800, Saravana Kannan wrote:

> I agree it looks a bit odd and I'm willing to do the code reorg if
> there is a better way. But I definitely wouldn't call this as

See the suggestion I made in the previous mail - try doing it by making
the change, then backing it out again if the attempt fails.

> randomly ignoring a consumer. We are just avoiding the consumer
> that's changing the range from "voting twice". We already send the
> new request thru min/max params.

It's a code clarity issue rather than a correctness issue.  When you're
in the code doing the check it's not terribly obvious why you're
ignoring it, and if you make any changes to the structure here or check
from other places you need to worry about which consumer to ignore.  If
we ever end up wanting to ignore two it'd be fun also...

> Do you have any suggestions for a better way to compute the min/max
> while leaving out a single consumer? I'm very much open to do that.

It seems better to arrange things so we don't ignore a consumer so the
check function doesn't need to worry about what it's checking, it just
goes and does it.