[PATCH] regulator: Call into regulator driver only when voltage min/max really changes.

[PATCH] regulator: Call into regulator driver only when voltage min/max really changes.

[Saravana Kannan]

Even in cases where the consumer driver calls the regulator core with
different voltage min/max values, the application of the various
voltage constraints could result in the min/max voltage values passed
to the regulator driver to be unchanged since the previous invocation.

Optimize these cases by not calling into the regulator driver and not
sending incorrect/unnecessary voltage change notifications.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ba521f0..c2e67bb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1640,12 +1640,18 @@ 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;
+
+	if (regulator->min_uV == min_uV && regulator->max_uV == max_uV)
+		goto unlock;
 	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV);
+	if (!ret) {
+		regulator->min_uV = min_uV;
+		regulator->max_uV = max_uV;
+	}
 
 out:
 	_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE, NULL);
+unlock:
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
-- 
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: Call into regulator driver only when voltage min/max really changes.

[Mark Brown]

On Sun, Dec 12, 2010 at 02:55:40AM -0800, Saravana Kannan wrote:
> Even in cases where the consumer driver calls the regulator core with
> different voltage min/max values, the application of the various
> voltage constraints could result in the min/max voltage values passed
> to the regulator driver to be unchanged since the previous invocation.

Out of interest do we have any examples of consumers that do this
sufficiently often and/or in paths sufficiently performance critical for
it to be an issue?  Sounds like there might be room for optimisation in
those consumers.

> Optimize these cases by not calling into the regulator driver and not
> sending incorrect/unnecessary voltage change notifications.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

The down side of doing this is that if the regulator state changes
underneath us we've now got no way of recovering from that situation.
This is something that's only partially supported by the API at the
minute but it's nice to have a story about how drivers can work with
this.  I'll send a patch adding an explicit sync API.

Re: [PATCH] regulator: Call into regulator driver only when voltage min/max really changes.

[skannan]

> On Sun, Dec 12, 2010 at 02:55:40AM -0800, Saravana Kannan wrote:
>> Even in cases where the consumer driver calls the regulator core with
>> different voltage min/max values, the application of the various
>> voltage constraints could result in the min/max voltage values passed
>> to the regulator driver to be unchanged since the previous invocation.
>
> Out of interest do we have any examples of consumers that do this
> sufficiently often and/or in paths sufficiently performance critical for
> it to be an issue?  Sounds like there might be room for optimisation in
> those consumers.

Wouldn't it be better to optimize in one location (the core), instead of
optimizing in several consumers?

I probably should have mentioned this too -- The other case this optimizes
is the consumer calling with the same values. It can happen during CPU
frequency scaling where multiple frequencies might have the same voltage
level.

>> Optimize these cases by not calling into the regulator driver and not
>> sending incorrect/unnecessary voltage change notifications.
>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>
> The down side of doing this is that if the regulator state changes
> underneath us we've now got no way of recovering from that situation.
> This is something that's only partially supported by the API at the
> minute but it's nice to have a story about how drivers can work with
> this.  I'll send a patch adding an explicit sync API.

Would this be a problem irrespective of this optimization?

Btw, after sending this patch, I just realized there might be another
"bug" in the existing code (not my changes). What's the meaning of the
"voltage change" event? It appears to be sent even if the regulator driver
fails to set the voltage. It would be correct if the event just means
"someone called set_voltage", but that seems like an unlikely definition.
Let me know if I should send in a patch for that too.

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: Call into regulator driver only when voltage min/max really changes.

[Mark Brown]

On Sun, Dec 12, 2010 at 04:41:04AM -0800, skannan@codeaurora.org wrote:
> > On Sun, Dec 12, 2010 at 02:55:40AM -0800, Saravana Kannan wrote:

> > Out of interest do we have any examples of consumers that do this
> > sufficiently often and/or in paths sufficiently performance critical for
> > it to be an issue?  Sounds like there might be room for optimisation in
> > those consumers.

> Wouldn't it be better to optimize in one location (the core), instead of
> optimizing in several consumers?

It's certainly good to optimise in the core, which is why I acked your
patch, but it strikes me that if these consumers are regularly setting
the regulator voltage to the same thing over and over again they may
well also be doing whatever else goes along with setting the voltage
over and over again and there's room for optimisation there also.

> > The down side of doing this is that if the regulator state changes
> > underneath us we've now got no way of recovering from that situation.
> > This is something that's only partially supported by the API at the
> > minute but it's nice to have a story about how drivers can work with
> > this.  I'll send a patch adding an explicit sync API.

> Would this be a problem irrespective of this optimization?

At the minute this can be handled at the consumer level since if the
consumer knows that the voltage got changed underneath it (and if it
cares about the voltage then it probably ought to know about this
otherwise something is going to get upset) it can reconfigure the
voltage by calling set_voltage() again.

> Btw, after sending this patch, I just realized there might be another
> "bug" in the existing code (not my changes). What's the meaning of the
> "voltage change" event? It appears to be sent even if the regulator driver
> fails to set the voltage. It would be correct if the event just means
> "someone called set_voltage", but that seems like an unlikely definition.

It's an indication to other consumers that they might want to reread the
voltage.  Overnotification doesn't really hurt here, though it'd be nice
to avoid it.

> Let me know if I should send in a patch for that too.

It'd probably coollide with the update I'm doing for syncing voltages.

Re: [PATCH] regulator: Call into regulator driver only when voltage min/max really changes.

[Mark Brown]

On Sun, Dec 12, 2010 at 02:55:40AM -0800, Saravana Kannan wrote:
> Even in cases where the consumer driver calls the regulator core with
> different voltage min/max values, the application of the various
> voltage constraints could result in the min/max voltage values passed
> to the regulator driver to be unchanged since the previous invocation.

So, I tried to apply this against my local tree to avoid collisions with
it but it doesn't apply so I suspect Liam will have trouble also, there
are several updates in there that overlap with your change.  Please
regenerate against -next or Liam's tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6

You should always submit changes against the development versions of
code, generally -next will have these merged into it.

Re: [PATCH] regulator: Call into regulator driver only when voltage min/max really changes.

[Saravana Kannan]

> So, I tried to apply this against my local tree to avoid collisions with
> it but it doesn't apply so I suspect Liam will have trouble also, there
> are several updates in there that overlap with your change.  Please
> regenerate against -next or Liam's tree:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6
>
> You should always submit changes against the development versions of
> code, generally -next will have these merged into it.
>
Will do. From a quick git log made it looked like for-linus was the
latest. Looks like I was mistaken.

-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: Call into regulator driver only when voltage min/max really changes.

[Liam Girdwood]

On Sun, 2010-12-12 at 02:55 -0800, Saravana Kannan wrote:
> Even in cases where the consumer driver calls the regulator core with
> different voltage min/max values, the application of the various
> voltage constraints could result in the min/max voltage values passed
> to the regulator driver to be unchanged since the previous invocation.
> 
> Optimize these cases by not calling into the regulator driver and not
> sending incorrect/unnecessary voltage change notifications.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>

This doesn't apply. Any chance you could regenerate against the
regulator for-next branch ?

Thanks

Liam  
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk