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.
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html