[PATCH 0/9] Simplify regulator_set_optimum_mode

[PATCH 0/9] Simplify regulator_set_optimum_mode

[Bjorn Andersson]

This patch series pushes the get_optimum_mode/set_mode pair from
regulator_set_optimum_mode into the individual drivers.

The first patch consolidates the two drms updating paths, to reduce code
duplication.

The second patch introduces the set_optimum_mode call, the following patches
moves drivers over to the new api and get_optimum_mode is finally dropped.

With this in place we can implement set_optimum_mode in the Qualcomm RPM
driver.


When we talked about this you (Mark) said that you wished that we simplified
this api, but how far would you like to take that?

* Drop input_uV/output_uV from set_optimum_mode? (there are no users today)
* Rename the op to set_current() (without in/out uV, this is basically what we have)
* s/regulator_set_optimum_mode/regulator_set_current/ ?
* Remove regulator_set_mode (consumers should know their expected load, not the
  expected operating mode of whatever regulator they are supplied by)

Bjorn Andersson (9):
  regulator: core: Consolidate drms update handling
  regulator: core: Introduce set_optimum_mode op
  regulator: ab8500: move to set_optimum_mode
  regulator: wm831x-ldo: move to set_optimum_mode
  regulator: wm8350: move to set_optimum_mode
  regulator: hi6421: move to set_optimum_mode
  regulator: wm8400: move to set_optimum_mode
  regulator: Drop now unused get_optimum_mode
  regulator: qcom-rpm: Implement set_optimum_mode

 drivers/regulator/ab8500.c             |  48 +++++++-------
 drivers/regulator/core.c               | 112 +++++++++------------------------
 drivers/regulator/hi6421-regulator.c   |  15 +++--
 drivers/regulator/qcom_rpm-regulator.c |  28 +++++++++
 drivers/regulator/wm831x-ldo.c         |  21 ++++---
 drivers/regulator/wm8350-regulator.c   |  12 ++--
 drivers/regulator/wm8400-regulator.c   |  10 +--
 include/linux/regulator/driver.h       |   8 +--
 8 files changed, 118 insertions(+), 136 deletions(-)

-- 
1.9.1

[PATCH 1/9] regulator: core: Consolidate drms update handling

[Bjorn Andersson]

Refactor drms_uA_update() slightly to allow regulator_set_optimum_mode()
to utilize the same logic instead of duplicating it.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/core.c | 112 +++++++++++++++--------------------------------
 1 file changed, 35 insertions(+), 77 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e225711..0e0d829 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -658,23 +658,32 @@ static struct class regulator_class = {
 
 /* Calculate the new optimum regulator operating mode based on the new total
  * consumer load. All locks held by caller */
-static void drms_uA_update(struct regulator_dev *rdev)
+static int drms_uA_update(struct regulator_dev *rdev)
 {
 	struct regulator *sibling;
 	int current_uA = 0, output_uV, input_uV, err;
 	unsigned int mode;
 
+	/*
+	 * first check to see if we can set modes at all, otherwise just
+	 * tell the consumer everything is OK.
+	 */
 	err = regulator_check_drms(rdev);
-	if (err < 0 || !rdev->desc->ops->get_optimum_mode ||
-	    (!rdev->desc->ops->get_voltage &&
-	     !rdev->desc->ops->get_voltage_sel) ||
-	    !rdev->desc->ops->set_mode)
-		return;
+	if (err < 0)
+		return 0;
+
+	if (!rdev->desc->ops->get_optimum_mode)
+		return 0;
+
+	if (!rdev->desc->ops->set_mode)
+		return -EINVAL;
 
 	/* get output voltage */
 	output_uV = _regulator_get_voltage(rdev);
-	if (output_uV <= 0)
-		return;
+	if (output_uV <= 0) {
+		rdev_err(rdev, "invalid output voltage found\n");
+		return -EINVAL;
+	}
 
 	/* get input voltage */
 	input_uV = 0;
@@ -682,8 +691,10 @@ static void drms_uA_update(struct regulator_dev *rdev)
 		input_uV = regulator_get_voltage(rdev->supply);
 	if (input_uV <= 0)
 		input_uV = rdev->constraints->input_uV;
-	if (input_uV <= 0)
-		return;
+	if (input_uV <= 0) {
+		rdev_err(rdev, "invalid input voltage found\n");
+		return -EINVAL;
+	}
 
 	/* calc total requested load */
 	list_for_each_entry(sibling, &rdev->consumer_list, list)
@@ -695,8 +706,17 @@ static void drms_uA_update(struct regulator_dev *rdev)
 
 	/* check the new mode is allowed */
 	err = regulator_mode_constrain(rdev, &mode);
-	if (err == 0)
-		rdev->desc->ops->set_mode(rdev, mode);
+	if (err < 0) {
+		rdev_err(rdev, "failed to get optimum mode @ %d uA %d -> %d uV\n",
+			 current_uA, input_uV, output_uV);
+		return err;
+	}
+
+	err = rdev->desc->ops->set_mode(rdev, mode);
+	if (err < 0)
+		rdev_err(rdev, "failed to set optimum mode %x\n", mode);
+
+	return err;
 }
 
 static int suspend_set_state(struct regulator_dev *rdev,
@@ -3024,75 +3044,13 @@ EXPORT_SYMBOL_GPL(regulator_get_mode);
 int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
 {
 	struct regulator_dev *rdev = regulator->rdev;
-	struct regulator *consumer;
-	int ret, output_uV, input_uV = 0, total_uA_load = 0;
-	unsigned int mode;
-
-	if (rdev->supply)
-		input_uV = regulator_get_voltage(rdev->supply);
+	int ret;
 
 	mutex_lock(&rdev->mutex);
-
-	/*
-	 * first check to see if we can set modes at all, otherwise just
-	 * tell the consumer everything is OK.
-	 */
 	regulator->uA_load = uA_load;
-	ret = regulator_check_drms(rdev);
-	if (ret < 0) {
-		ret = 0;
-		goto out;
-	}
-
-	if (!rdev->desc->ops->get_optimum_mode)
-		goto out;
-
-	/*
-	 * we can actually do this so any errors are indicators of
-	 * potential real failure.
-	 */
-	ret = -EINVAL;
-
-	if (!rdev->desc->ops->set_mode)
-		goto out;
-
-	/* get output voltage */
-	output_uV = _regulator_get_voltage(rdev);
-	if (output_uV <= 0) {
-		rdev_err(rdev, "invalid output voltage found\n");
-		goto out;
-	}
-
-	/* No supply? Use constraint voltage */
-	if (input_uV <= 0)
-		input_uV = rdev->constraints->input_uV;
-	if (input_uV <= 0) {
-		rdev_err(rdev, "invalid input voltage found\n");
-		goto out;
-	}
-
-	/* calc total requested load for this regulator */
-	list_for_each_entry(consumer, &rdev->consumer_list, list)
-		total_uA_load += consumer->uA_load;
-
-	mode = rdev->desc->ops->get_optimum_mode(rdev,
-						 input_uV, output_uV,
-						 total_uA_load);
-	ret = regulator_mode_constrain(rdev, &mode);
-	if (ret < 0) {
-		rdev_err(rdev, "failed to get optimum mode @ %d uA %d -> %d uV\n",
-			 total_uA_load, input_uV, output_uV);
-		goto out;
-	}
-
-	ret = rdev->desc->ops->set_mode(rdev, mode);
-	if (ret < 0) {
-		rdev_err(rdev, "failed to set optimum mode %x\n", mode);
-		goto out;
-	}
-	ret = mode;
-out:
+	ret = drms_uA_update(rdev);
 	mutex_unlock(&rdev->mutex);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_optimum_mode);
-- 
1.9.1

Re: [PATCH 1/9] regulator: core: Consolidate drms update handling

[Mark Brown]

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

On Tue, Jan 27, 2015 at 06:46:31PM -0800, Bjorn Andersson wrote:
> Refactor drms_uA_update() slightly to allow regulator_set_optimum_mode()
> to utilize the same logic instead of duplicating it.

Applied, thanks.

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

[PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Bjorn Andersson]

Expose the requested load directly to the regulator implementation for
hardware that does not support the normal enum based set_mode().

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Note that the majority of the change to drms_uA_update is cancelled out in
patch 8, but I added this to keep it bisectable.

 drivers/regulator/core.c         | 42 ++++++++++++++++++++++++++--------------
 include/linux/regulator/driver.h |  5 +++++
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0e0d829..2a53515 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -672,10 +672,12 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	if (err < 0)
 		return 0;
 
-	if (!rdev->desc->ops->get_optimum_mode)
+	if (!rdev->desc->ops->get_optimum_mode &&
+	    !rdev->desc->ops->set_optimum_mode)
 		return 0;
 
-	if (!rdev->desc->ops->set_mode)
+	if (!rdev->desc->ops->set_mode &&
+	    !rdev->desc->ops->set_optimum_mode)
 		return -EINVAL;
 
 	/* get output voltage */
@@ -700,22 +702,32 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	list_for_each_entry(sibling, &rdev->consumer_list, list)
 		current_uA += sibling->uA_load;
 
-	/* now get the optimum mode for our new total regulator load */
-	mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
-						  output_uV, current_uA);
+	if (rdev->desc->ops->set_optimum_mode) {
+		/* set the optimum mode for our new total regulator load */
+		err = rdev->desc->ops->set_optimum_mode(rdev,
+							input_uV, output_uV,
+							current_uA);
+		if (err < 0)
+			rdev_err(rdev, "failed to set optimum mode @ %d uA %d -> %d uV\n",
+				 current_uA, input_uV, output_uV);
+	} else {
+		/* now get the optimum mode for our new total regulator load */
+		mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
+							 output_uV, current_uA);
+
+		/* check the new mode is allowed */
+		err = regulator_mode_constrain(rdev, &mode);
+		if (err < 0) {
+			rdev_err(rdev, "failed to get optimum mode @ %d uA %d -> %d uV\n",
+				 current_uA, input_uV, output_uV);
+			return err;
+		}
 
-	/* check the new mode is allowed */
-	err = regulator_mode_constrain(rdev, &mode);
-	if (err < 0) {
-		rdev_err(rdev, "failed to get optimum mode @ %d uA %d -> %d uV\n",
-			 current_uA, input_uV, output_uV);
-		return err;
+		err = rdev->desc->ops->set_mode(rdev, mode);
+		if (err < 0)
+			rdev_err(rdev, "failed to set optimum mode %x\n", mode);
 	}
 
-	err = rdev->desc->ops->set_mode(rdev, mode);
-	if (err < 0)
-		rdev_err(rdev, "failed to set optimum mode %x\n", mode);
-
 	return err;
 }
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 5f1e9ca..837addb 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -97,6 +97,8 @@ struct regulator_linear_range {
  *	REGULATOR_STATUS value (or negative errno)
  * @get_optimum_mode: Get the most efficient operating mode for the regulator
  *                    when running with the specified parameters.
+ * @set_optimum_mode: Set the most efficient operating mode for the regulator
+ *                    when running with the specified parameters.
  *
  * @set_bypass: Set the regulator in bypass mode.
  * @get_bypass: Get the regulator bypass mode state.
@@ -166,6 +168,9 @@ struct regulator_ops {
 	/* get most efficient regulator operating mode for load */
 	unsigned int (*get_optimum_mode) (struct regulator_dev *, int input_uV,
 					  int output_uV, int load_uA);
+	/* set most efficient regulator operating mode for load */
+	int (*set_optimum_mode)(struct regulator_dev *, int input_uV,
+				int output_uV, int load_uA);
 
 	/* control and report on bypass mode */
 	int (*set_bypass)(struct regulator_dev *dev, bool enable);
-- 
1.9.1

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Mark Brown]

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

On Tue, Jan 27, 2015 at 06:46:32PM -0800, Bjorn Andersson wrote:

> +	/* set most efficient regulator operating mode for load */
> +	int (*set_optimum_mode)(struct regulator_dev *, int input_uV,
> +				int output_uV, int load_uA);

This is basically fine but can you please rename this to be something
that more directly reflects the fact that we're just informing the
driver about the operating parameters - there are other things a driver
could usefully do with this, for example set current limits so that if
something starts to consume excessive current the device will flag this
as an error.

It'd also be better to split the voltage specs out into a separate
function, especially for the output voltage where obviously we have a
separate range based interface for setting that.

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

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Bjorn Andersson]

On Wed 28 Jan 11:52 PST 2015, Mark Brown wrote:

> On Tue, Jan 27, 2015 at 06:46:32PM -0800, Bjorn Andersson wrote:
> 
> > +	/* set most efficient regulator operating mode for load */
> > +	int (*set_optimum_mode)(struct regulator_dev *, int input_uV,
> > +				int output_uV, int load_uA);
> 
> This is basically fine but can you please rename this to be something
> that more directly reflects the fact that we're just informing the
> driver about the operating parameters - there are other things a driver
> could usefully do with this, for example set current limits so that if
> something starts to consume excessive current the device will flag this
> as an error.
> 

The purpose of the series was to be able to implement patch 9, which
will utilize the load_uA to set the "mode" of the Qualcomm regulators.

So I would like it to be a "setter of current consumption".

I'm not sure what to name the function to have it cover these additional
cases.

> It'd also be better to split the voltage specs out into a separate
> function, especially for the output voltage where obviously we have a
> separate range based interface for setting that.

I was fishing for a statement regarding this in the cover letter, but
here we go.

The current implementors of get_optimum_mode all ignore the voltages, so
we could effectively simplify the interface to:

 get_optimum_mode(rdev, load);

Question is if there are any implementations where we don't know the
output voltage in the regulator driver (as locking prevents us from
using the standard interface of querying this). Input voltage is just a
query away.


Having drms_uA_update() request an appropriate mode for the given load
and then calling set_mode directly (the current implementation) gives us
a single point of entry to the regulator drivers related to setting
modes (regulator_set_mode and drms_uA_update calls set_mode). But seen
from a consumer there's no consistency; the last call to
regulator_set_mode() and regulator_set_optimum_mode() will win.

Further more, get_optimum_mode is not exposed to the consumers and there
are no other users in the regulator framework. So it could be completely
internal to the regulator drivers, without loss of functionality.


Looking at the consumers, in my mind they should not be aware of the
operating performance characteristics of the regulator supplying them.
So I think they should all use regulator_set_optimum_mode() rather than
"guessing" what performance mode the regulator should run in. (Maybe
some board code could use set_mode?).

With this in mind I was going to follow up after this series with a
rename of regulator_set_optimum_mode() to regulator_set_current() and
set_optimum_mode() to set_current().

I was also going to suggest dropping regulator_set_mode() and set_mode
to make the consumer facing API consistent.


I think this covers your concern about patch 3-7 as well, please let me
know what you think.

Regards,
Bjorn

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Mark Brown]

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

On Thu, Jan 29, 2015 at 04:07:42PM -0800, Bjorn Andersson wrote:
> On Wed 28 Jan 11:52 PST 2015, Mark Brown wrote:

> > This is basically fine but can you please rename this to be something
> > that more directly reflects the fact that we're just informing the
> > driver about the operating parameters - there are other things a driver
> > could usefully do with this, for example set current limits so that if
> > something starts to consume excessive current the device will flag this
> > as an error.

> The purpose of the series was to be able to implement patch 9, which
> will utilize the load_uA to set the "mode" of the Qualcomm regulators.

> So I would like it to be a "setter of current consumption".

> I'm not sure what to name the function to have it cover these additional
> cases.

notify_load() or something?  That's what it's doing, what the driver
does with it is a separate thing.

> > It'd also be better to split the voltage specs out into a separate
> > function, especially for the output voltage where obviously we have a
> > separate range based interface for setting that.

> The current implementors of get_optimum_mode all ignore the voltages, so
> we could effectively simplify the interface to:

>  get_optimum_mode(rdev, load);

> Question is if there are any implementations where we don't know the
> output voltage in the regulator driver (as locking prevents us from
> using the standard interface of querying this). Input voltage is just a
> query away.

We can always fix the locking to let people query the voltage if they
need to.

> Having drms_uA_update() request an appropriate mode for the given load
> and then calling set_mode directly (the current implementation) gives us
> a single point of entry to the regulator drivers related to setting
> modes (regulator_set_mode and drms_uA_update calls set_mode). But seen
> from a consumer there's no consistency; the last call to
> regulator_set_mode() and regulator_set_optimum_mode() will win.

That's fine, consumers shouldn't be using both simultaneously anyway.
If a consumer is actually setting modes actively at runtime by name it
needs to be fairly closely tied to a specific system and regulator so
it's not clear if there's much use case anyway.

> I think this covers your concern about patch 3-7 as well, please let me
> know what you think.

Possibly.

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

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Bjorn Andersson]

On Fri, Jan 30, 2015 at 4:27 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jan 29, 2015 at 04:07:42PM -0800, Bjorn Andersson wrote:
>> On Wed 28 Jan 11:52 PST 2015, Mark Brown wrote:
>
>> > This is basically fine but can you please rename this to be something
>> > that more directly reflects the fact that we're just informing the
>> > driver about the operating parameters - there are other things a driver
>> > could usefully do with this, for example set current limits so that if
>> > something starts to consume excessive current the device will flag this
>> > as an error.
>
>> The purpose of the series was to be able to implement patch 9, which
>> will utilize the load_uA to set the "mode" of the Qualcomm regulators.
>
>> So I would like it to be a "setter of current consumption".
>
>> I'm not sure what to name the function to have it cover these additional
>> cases.
>
> notify_load() or something?  That's what it's doing, what the driver
> does with it is a separate thing.
>

I changed it to set_load() in my tree, maybe "notify" is better as
it's more of a hint to the driver.

You said something when we talked that I interpreted to
regulator_set_optimum_mode() is not a good name for the consumer API;
was that a correct interpretation or was your comment limited to the
driver facing API?

Should we go ahead and rename it to regulator_notify_load()
(regulator_set_load() perhaps?) before we get more consumers as well?
(would be a separate patch set)

>> > It'd also be better to split the voltage specs out into a separate
>> > function, especially for the output voltage where obviously we have a
>> > separate range based interface for setting that.
>
>> The current implementors of get_optimum_mode all ignore the voltages, so
>> we could effectively simplify the interface to:
>
>>  get_optimum_mode(rdev, load);
>
>> Question is if there are any implementations where we don't know the
>> output voltage in the regulator driver (as locking prevents us from
>> using the standard interface of querying this). Input voltage is just a
>> query away.
>
> We can always fix the locking to let people query the voltage if they
> need to.
>

Sounds good, drms_uA_update() becomes quite minimal with that.

But if we're only considering load in drms_uA_update() does it make
sense to check this against the valid ranges of min_uA, max_uA and
hence instead of checking REGULATOR_CHANGE_DRMS just check for
REGULATOR_CHANGE_CURRENT?
We have reduced the interface to not necessarily be dynamic _mode_ setting.

This would allow us to use existing properties in DT and
of_get_regulation_constraints() would provide us those limits and flag
that this code path is valid.

>> Having drms_uA_update() request an appropriate mode for the given load
>> and then calling set_mode directly (the current implementation) gives us
>> a single point of entry to the regulator drivers related to setting
>> modes (regulator_set_mode and drms_uA_update calls set_mode). But seen
>> from a consumer there's no consistency; the last call to
>> regulator_set_mode() and regulator_set_optimum_mode() will win.
>
> That's fine, consumers shouldn't be using both simultaneously anyway.
> If a consumer is actually setting modes actively at runtime by name it
> needs to be fairly closely tied to a specific system and regulator so
> it's not clear if there's much use case anyway.
>

Indeed, as there's no MAX() or similar of the modes requested that
seems to be a sure way to get into problems otherwise.

>> I think this covers your concern about patch 3-7 as well, please let me
>> know what you think.
>
> Possibly.

Well, my question is still there:

Unless we replace the get_optimum_mode()/set_mode() pair with
notify_load() the driver API logic will become confusing; so for the
regulators that to day implement that combo I think we need patch 3-7
as well.

Regards,
Bjorn

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Mark Brown]

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

On Mon, Feb 09, 2015 at 02:08:41PM -0800, Bjorn Andersson wrote:

> You said something when we talked that I interpreted to
> regulator_set_optimum_mode() is not a good name for the consumer API;
> was that a correct interpretation or was your comment limited to the
> driver facing API?

> Should we go ahead and rename it to regulator_notify_load()
> (regulator_set_load() perhaps?) before we get more consumers as well?
> (would be a separate patch set)

This can be _set_load() I think since we are actually telling the
framework about the expected load but yes, we should rename it to
reflect what we're actually doing.

> But if we're only considering load in drms_uA_update() does it make
> sense to check this against the valid ranges of min_uA, max_uA and
> hence instead of checking REGULATOR_CHANGE_DRMS just check for
> REGULATOR_CHANGE_CURRENT?
> We have reduced the interface to not necessarily be dynamic _mode_ setting.

No, _CHANGE_CURRENT is about current reglators which are a different
thing to voltage regulators.

> >> I think this covers your concern about patch 3-7 as well, please let me
> >> know what you think.

> > Possibly.

> Well, my question is still there:

> Unless we replace the get_optimum_mode()/set_mode() pair with
> notify_load() the driver API logic will become confusing; so for the
> regulators that to day implement that combo I think we need patch 3-7
> as well.

That's not a question, that's a statement...  Since we don't currently
have a notify_load() interface we obviously don't have any drivers using
it so I'm not sure what drivers will become confusing here or how this
addresses the part about keeping the operations split for the common
pattern?

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

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Bjorn Andersson]

On Thu, Jan 29, 2015 at 4:07 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Wed 28 Jan 11:52 PST 2015, Mark Brown wrote:
>
>> On Tue, Jan 27, 2015 at 06:46:32PM -0800, Bjorn Andersson wrote:
>>
>> > +   /* set most efficient regulator operating mode for load */
>> > +   int (*set_optimum_mode)(struct regulator_dev *, int input_uV,
>> > +                           int output_uV, int load_uA);
>>
>> This is basically fine but can you please rename this to be something
>> that more directly reflects the fact that we're just informing the
>> driver about the operating parameters - there are other things a driver
>> could usefully do with this, for example set current limits so that if
>> something starts to consume excessive current the device will flag this
>> as an error.
>>
>
> The purpose of the series was to be able to implement patch 9, which
> will utilize the load_uA to set the "mode" of the Qualcomm regulators.
>
> So I would like it to be a "setter of current consumption".
>
> I'm not sure what to name the function to have it cover these additional
> cases.
>
>> It'd also be better to split the voltage specs out into a separate
>> function, especially for the output voltage where obviously we have a
>> separate range based interface for setting that.
>
> I was fishing for a statement regarding this in the cover letter, but
> here we go.
>
> The current implementors of get_optimum_mode all ignore the voltages, so
> we could effectively simplify the interface to:
>
>  get_optimum_mode(rdev, load);
>
> Question is if there are any implementations where we don't know the
> output voltage in the regulator driver (as locking prevents us from
> using the standard interface of querying this). Input voltage is just a
> query away.
>
>
> Having drms_uA_update() request an appropriate mode for the given load
> and then calling set_mode directly (the current implementation) gives us
> a single point of entry to the regulator drivers related to setting
> modes (regulator_set_mode and drms_uA_update calls set_mode). But seen
> from a consumer there's no consistency; the last call to
> regulator_set_mode() and regulator_set_optimum_mode() will win.
>
> Further more, get_optimum_mode is not exposed to the consumers and there
> are no other users in the regulator framework. So it could be completely
> internal to the regulator drivers, without loss of functionality.
>
>
> Looking at the consumers, in my mind they should not be aware of the
> operating performance characteristics of the regulator supplying them.
> So I think they should all use regulator_set_optimum_mode() rather than
> "guessing" what performance mode the regulator should run in. (Maybe
> some board code could use set_mode?).
>
> With this in mind I was going to follow up after this series with a
> rename of regulator_set_optimum_mode() to regulator_set_current() and
> set_optimum_mode() to set_current().
>
> I was also going to suggest dropping regulator_set_mode() and set_mode
> to make the consumer facing API consistent.
>
>
> I think this covers your concern about patch 3-7 as well, please let me
> know what you think.
>

Mark, any suggestions on this?

I need a way to pass the expected current consumption from the mmc
framework through our regulator driver to the RPM to make our eMMC
stable.
We can discuss the proposed "cleanups" next time we meet if you prefer.

Regards,
Bjorn

Re: [PATCH 2/9] regulator: core: Introduce set_optimum_mode op

[Mark Brown]

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

On Thu, Feb 05, 2015 at 02:16:54PM -0800, Bjorn Andersson wrote:

> > I think this covers your concern about patch 3-7 as well, please let me
> > know what you think.

> Mark, any suggestions on this?

> I need a way to pass the expected current consumption from the mmc
> framework through our regulator driver to the RPM to make our eMMC
> stable.
> We can discuss the proposed "cleanups" next time we meet if you prefer.

Could you be a little more specific about what suggestions you're
looking for?  You've quoted a rather large e-mail there where you
seemed to say you were going to send a new version of the series so I
think I was expecting something to review here.

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

[PATCH 3/9] regulator: ab8500: move to set_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/ab8500.c | 48 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 0f97514..c1588c1 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -320,27 +320,6 @@ static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
 		return 0;
 }
 
-static unsigned int ab8500_regulator_get_optimum_mode(
-		struct regulator_dev *rdev, int input_uV,
-		int output_uV, int load_uA)
-{
-	unsigned int mode;
-
-	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
-
-	if (info == NULL) {
-		dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
-		return -EINVAL;
-	}
-
-	if (load_uA <= info->load_lp_uA)
-		mode = REGULATOR_MODE_IDLE;
-	else
-		mode = REGULATOR_MODE_NORMAL;
-
-	return mode;
-}
-
 static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
 				     unsigned int mode)
 {
@@ -430,6 +409,27 @@ out_unlock:
 	return ret;
 }
 
+static int ab8500_regulator_set_optimum_mode(
+		struct regulator_dev *rdev, int input_uV,
+		int output_uV, int load_uA)
+{
+	unsigned int mode;
+
+	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
+
+	if (info == NULL) {
+		dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
+		return -EINVAL;
+	}
+
+	if (load_uA <= info->load_lp_uA)
+		mode = REGULATOR_MODE_IDLE;
+	else
+		mode = REGULATOR_MODE_NORMAL;
+
+	return ab8500_regulator_set_mode(rdev, mode);
+}
+
 static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
 {
 	struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
@@ -645,7 +645,7 @@ static struct regulator_ops ab8500_regulator_volt_mode_ops = {
 	.enable			= ab8500_regulator_enable,
 	.disable		= ab8500_regulator_disable,
 	.is_enabled		= ab8500_regulator_is_enabled,
-	.get_optimum_mode	= ab8500_regulator_get_optimum_mode,
+	.set_optimum_mode	= ab8500_regulator_set_optimum_mode,
 	.set_mode		= ab8500_regulator_set_mode,
 	.get_mode		= ab8500_regulator_get_mode,
 	.get_voltage_sel 	= ab8500_regulator_get_voltage_sel,
@@ -656,7 +656,7 @@ static struct regulator_ops ab8500_regulator_volt_mode_ops = {
 static struct regulator_ops ab8540_aux3_regulator_volt_mode_ops = {
 	.enable		= ab8500_regulator_enable,
 	.disable	= ab8500_regulator_disable,
-	.get_optimum_mode	= ab8500_regulator_get_optimum_mode,
+	.set_optimum_mode	= ab8500_regulator_set_optimum_mode,
 	.set_mode	= ab8500_regulator_set_mode,
 	.get_mode	= ab8500_regulator_get_mode,
 	.is_enabled	= ab8500_regulator_is_enabled,
@@ -678,7 +678,7 @@ static struct regulator_ops ab8500_regulator_mode_ops = {
 	.enable			= ab8500_regulator_enable,
 	.disable		= ab8500_regulator_disable,
 	.is_enabled		= ab8500_regulator_is_enabled,
-	.get_optimum_mode	= ab8500_regulator_get_optimum_mode,
+	.set_optimum_mode	= ab8500_regulator_set_optimum_mode,
 	.set_mode		= ab8500_regulator_set_mode,
 	.get_mode		= ab8500_regulator_get_mode,
 	.list_voltage		= regulator_list_voltage_table,
-- 
1.9.1

Re: [PATCH 3/9] regulator: ab8500: move to set_optimum_mode

[Mark Brown]

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

On Tue, Jan 27, 2015 at 06:46:33PM -0800, Bjorn Andersson wrote:
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  drivers/regulator/ab8500.c | 48 +++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)

These refactorings are surprising and don't seem like an obvious
advantage - they mean that we now have two different paths to setting
the mode (one explict and the other via this).  For regulators that do
want to just do a lookup of a mode based on parameters I'd expect to
keep the operations separate in the same way that we do for things like
voltage setting in case we come up with ways of using the two bits
separately.

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

[PATCH 4/9] regulator: wm831x-ldo: move to set_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/wm831x-ldo.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/wm831x-ldo.c b/drivers/regulator/wm831x-ldo.c
index 7ae2dc8..40d391e 100644
--- a/drivers/regulator/wm831x-ldo.c
+++ b/drivers/regulator/wm831x-ldo.c
@@ -186,15 +186,20 @@ static int wm831x_gp_ldo_get_status(struct regulator_dev *rdev)
 		return regulator_mode_to_status(ret);
 }
 
-static unsigned int wm831x_gp_ldo_get_optimum_mode(struct regulator_dev *rdev,
-						   int input_uV,
-						   int output_uV, int load_uA)
+static int wm831x_gp_ldo_set_optimum_mode(struct regulator_dev *rdev,
+					  int input_uV,
+					  int output_uV, int load_uA)
 {
+	unsigned int mode;
+
 	if (load_uA < 20000)
-		return REGULATOR_MODE_STANDBY;
-	if (load_uA < 50000)
-		return REGULATOR_MODE_IDLE;
-	return REGULATOR_MODE_NORMAL;
+		mode = REGULATOR_MODE_STANDBY;
+	else if (load_uA < 50000)
+		mode = REGULATOR_MODE_IDLE;
+	else
+		mode = REGULATOR_MODE_NORMAL;
+
+	return wm831x_gp_ldo_set_mode(rdev, mode);
 }
 
 
@@ -207,7 +212,7 @@ static struct regulator_ops wm831x_gp_ldo_ops = {
 	.get_mode = wm831x_gp_ldo_get_mode,
 	.set_mode = wm831x_gp_ldo_set_mode,
 	.get_status = wm831x_gp_ldo_get_status,
-	.get_optimum_mode = wm831x_gp_ldo_get_optimum_mode,
+	.set_optimum_mode = wm831x_gp_ldo_set_optimum_mode,
 	.get_bypass = regulator_get_bypass_regmap,
 	.set_bypass = regulator_set_bypass_regmap,
 
-- 
1.9.1

[PATCH 5/9] regulator: wm8350: move to set_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/wm8350-regulator.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/wm8350-regulator.c b/drivers/regulator/wm8350-regulator.c
index 7ec7c39..f8bcaed 100644
--- a/drivers/regulator/wm8350-regulator.c
+++ b/drivers/regulator/wm8350-regulator.c
@@ -915,13 +915,13 @@ static unsigned int get_mode(int uA, const struct wm8350_dcdc_efficiency *eff)
 	return REGULATOR_MODE_NORMAL;
 }
 
-/* Query the regulator for it's most efficient mode @ uV,uA
+/* Configure the regulator for it's most efficient mode @ uV,uA
  * WM8350 regulator efficiency is pretty similar over
  * different input and output uV.
  */
-static unsigned int wm8350_dcdc_get_optimum_mode(struct regulator_dev *rdev,
-						 int input_uV, int output_uV,
-						 int output_uA)
+static int wm8350_dcdc_set_optimum_mode(struct regulator_dev *rdev,
+					int input_uV, int output_uV,
+					int output_uA)
 {
 	int dcdc = rdev_get_id(rdev), mode;
 
@@ -938,7 +938,7 @@ static unsigned int wm8350_dcdc_get_optimum_mode(struct regulator_dev *rdev,
 		mode = REGULATOR_MODE_NORMAL;
 		break;
 	}
-	return mode;
+	return wm8350_dcdc_set_mode(rdev, mode);
 }
 
 static struct regulator_ops wm8350_dcdc_ops = {
@@ -951,7 +951,7 @@ static struct regulator_ops wm8350_dcdc_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.get_mode = wm8350_dcdc_get_mode,
 	.set_mode = wm8350_dcdc_set_mode,
-	.get_optimum_mode = wm8350_dcdc_get_optimum_mode,
+	.set_optimum_mode = wm8350_dcdc_set_optimum_mode,
 	.set_suspend_voltage = wm8350_dcdc_set_suspend_voltage,
 	.set_suspend_enable = wm8350_dcdc_set_suspend_enable,
 	.set_suspend_disable = wm8350_dcdc_set_suspend_disable,
-- 
1.9.1

[PATCH 6/9] regulator: hi6421: move to set_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/hi6421-regulator.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/hi6421-regulator.c b/drivers/regulator/hi6421-regulator.c
index 42dc5fb..42b362d 100644
--- a/drivers/regulator/hi6421-regulator.c
+++ b/drivers/regulator/hi6421-regulator.c
@@ -477,15 +477,18 @@ static int hi6421_regulator_buck_set_mode(struct regulator_dev *rdev,
 	return 0;
 }
 
-unsigned int hi6421_regulator_ldo_get_optimum_mode(struct regulator_dev *rdev,
+static int hi6421_regulator_ldo_set_optimum_mode(struct regulator_dev *rdev,
 			int input_uV, int output_uV, int load_uA)
 {
 	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+	unsigned int mode;
 
 	if (load_uA > info->eco_microamp)
-		return REGULATOR_MODE_NORMAL;
+		mode = REGULATOR_MODE_NORMAL;
+	else
+		mode = REGULATOR_MODE_IDLE;
 
-	return REGULATOR_MODE_IDLE;
+	return hi6421_regulator_ldo_set_mode(rdev, mode);
 }
 
 static const struct regulator_ops hi6421_ldo_ops = {
@@ -498,7 +501,7 @@ static const struct regulator_ops hi6421_ldo_ops = {
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_mode = hi6421_regulator_ldo_get_mode,
 	.set_mode = hi6421_regulator_ldo_set_mode,
-	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+	.set_optimum_mode = hi6421_regulator_ldo_set_optimum_mode,
 };
 
 static const struct regulator_ops hi6421_ldo_linear_ops = {
@@ -511,7 +514,7 @@ static const struct regulator_ops hi6421_ldo_linear_ops = {
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_mode = hi6421_regulator_ldo_get_mode,
 	.set_mode = hi6421_regulator_ldo_set_mode,
-	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+	.set_optimum_mode = hi6421_regulator_ldo_set_optimum_mode,
 };
 
 static const struct regulator_ops hi6421_ldo_linear_range_ops = {
@@ -524,7 +527,7 @@ static const struct regulator_ops hi6421_ldo_linear_range_ops = {
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_mode = hi6421_regulator_ldo_get_mode,
 	.set_mode = hi6421_regulator_ldo_set_mode,
-	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+	.set_optimum_mode = hi6421_regulator_ldo_set_optimum_mode,
 };
 
 static const struct regulator_ops hi6421_buck012_ops = {
-- 
1.9.1

[PATCH 7/9] regulator: wm8400: move to set_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/wm8400-regulator.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/wm8400-regulator.c b/drivers/regulator/wm8400-regulator.c
index 82d8290..7f4c924 100644
--- a/drivers/regulator/wm8400-regulator.c
+++ b/drivers/regulator/wm8400-regulator.c
@@ -99,11 +99,11 @@ static int wm8400_dcdc_set_mode(struct regulator_dev *dev, unsigned int mode)
 	}
 }
 
-static unsigned int wm8400_dcdc_get_optimum_mode(struct regulator_dev *dev,
-						 int input_uV, int output_uV,
-						 int load_uA)
+static int wm8400_dcdc_set_optimum_mode(struct regulator_dev *dev,
+					int input_uV, int output_uV,
+					int load_uA)
 {
-	return REGULATOR_MODE_NORMAL;
+	return wm8400_dcdc_set_mode(rdev, REGULATOR_MODE_NORMAL);
 }
 
 static struct regulator_ops wm8400_dcdc_ops = {
@@ -116,7 +116,7 @@ static struct regulator_ops wm8400_dcdc_ops = {
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_mode = wm8400_dcdc_get_mode,
 	.set_mode = wm8400_dcdc_set_mode,
-	.get_optimum_mode = wm8400_dcdc_get_optimum_mode,
+	.set_optimum_mode = wm8400_dcdc_set_optimum_mode,
 };
 
 static struct regulator_desc regulators[] = {
-- 
1.9.1

[PATCH 8/9] regulator: Drop now unused get_optimum_mode

[Bjorn Andersson]

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/core.c         | 40 ++++++++--------------------------------
 include/linux/regulator/driver.h |  5 -----
 2 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2a53515..cfab6fc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -662,7 +662,6 @@ static int drms_uA_update(struct regulator_dev *rdev)
 {
 	struct regulator *sibling;
 	int current_uA = 0, output_uV, input_uV, err;
-	unsigned int mode;
 
 	/*
 	 * first check to see if we can set modes at all, otherwise just
@@ -672,14 +671,9 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	if (err < 0)
 		return 0;
 
-	if (!rdev->desc->ops->get_optimum_mode &&
-	    !rdev->desc->ops->set_optimum_mode)
+	if (!rdev->desc->ops->set_optimum_mode)
 		return 0;
 
-	if (!rdev->desc->ops->set_mode &&
-	    !rdev->desc->ops->set_optimum_mode)
-		return -EINVAL;
-
 	/* get output voltage */
 	output_uV = _regulator_get_voltage(rdev);
 	if (output_uV <= 0) {
@@ -702,31 +696,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	list_for_each_entry(sibling, &rdev->consumer_list, list)
 		current_uA += sibling->uA_load;
 
-	if (rdev->desc->ops->set_optimum_mode) {
-		/* set the optimum mode for our new total regulator load */
-		err = rdev->desc->ops->set_optimum_mode(rdev,
-							input_uV, output_uV,
-							current_uA);
-		if (err < 0)
-			rdev_err(rdev, "failed to set optimum mode @ %d uA %d -> %d uV\n",
-				 current_uA, input_uV, output_uV);
-	} else {
-		/* now get the optimum mode for our new total regulator load */
-		mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
-							 output_uV, current_uA);
-
-		/* check the new mode is allowed */
-		err = regulator_mode_constrain(rdev, &mode);
-		if (err < 0) {
-			rdev_err(rdev, "failed to get optimum mode @ %d uA %d -> %d uV\n",
-				 current_uA, input_uV, output_uV);
-			return err;
-		}
-
-		err = rdev->desc->ops->set_mode(rdev, mode);
-		if (err < 0)
-			rdev_err(rdev, "failed to set optimum mode %x\n", mode);
-	}
+	/* set the optimum mode for our new total regulator load */
+	err = rdev->desc->ops->set_optimum_mode(rdev,
+						input_uV, output_uV,
+						current_uA);
+	if (err < 0)
+		rdev_err(rdev, "failed to set optimum mode @ %d uA %d -> %d uV\n",
+			 current_uA, input_uV, output_uV);
 
 	return err;
 }
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 837addb..f472cb5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -95,8 +95,6 @@ struct regulator_linear_range {
  * @get_mode: Get the configured operating mode for the regulator.
  * @get_status: Return actual (not as-configured) status of regulator, as a
  *	REGULATOR_STATUS value (or negative errno)
- * @get_optimum_mode: Get the most efficient operating mode for the regulator
- *                    when running with the specified parameters.
  * @set_optimum_mode: Set the most efficient operating mode for the regulator
  *                    when running with the specified parameters.
  *
@@ -165,9 +163,6 @@ struct regulator_ops {
 	 */
 	int (*get_status)(struct regulator_dev *);
 
-	/* get most efficient regulator operating mode for load */
-	unsigned int (*get_optimum_mode) (struct regulator_dev *, int input_uV,
-					  int output_uV, int load_uA);
 	/* set most efficient regulator operating mode for load */
 	int (*set_optimum_mode)(struct regulator_dev *, int input_uV,
 				int output_uV, int load_uA);
-- 
1.9.1

[PATCH 9/9] regulator: qcom-rpm: Implement set_optimum_mode

[Bjorn Andersson]

Pass the requested load directly to the rpm.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/qcom_rpm-regulator.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 8364ff3..745dd75 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -388,6 +388,30 @@ static int rpm_reg_is_enabled(struct regulator_dev *rdev)
 	return vreg->is_enabled;
 }
 
+static int rpm_reg_set_optimum_mode(struct regulator_dev *rdev,
+				    int input_uV, int output_uV,
+				    int load_uA)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+	const struct rpm_reg_parts *parts = vreg->parts;
+	const struct request_member *req = &parts->ia;
+	int load_mA = load_uA / 1000;
+	int max_mA = req->mask >> req->shift;
+	int ret;
+
+	if (req->mask == 0)
+		return -EINVAL;
+
+	if (load_mA > max_mA)
+		load_mA = max_mA;
+
+	mutex_lock(&vreg->lock);
+	ret = rpm_reg_write(vreg, req, load_mA);
+	mutex_unlock(&vreg->lock);
+
+	return ret;
+}
+
 static struct regulator_ops uV_ops = {
 	.list_voltage = regulator_list_voltage_linear_range,
 
@@ -397,6 +421,8 @@ static struct regulator_ops uV_ops = {
 	.enable = rpm_reg_uV_enable,
 	.disable = rpm_reg_uV_disable,
 	.is_enabled = rpm_reg_is_enabled,
+
+	.set_optimum_mode = rpm_reg_set_optimum_mode,
 };
 
 static struct regulator_ops mV_ops = {
@@ -408,6 +434,8 @@ static struct regulator_ops mV_ops = {
 	.enable = rpm_reg_mV_enable,
 	.disable = rpm_reg_mV_disable,
 	.is_enabled = rpm_reg_is_enabled,
+
+	.set_optimum_mode = rpm_reg_set_optimum_mode,
 };
 
 static struct regulator_ops switch_ops = {
-- 
1.9.1