diff mbox

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

Message ID 1422413199-17273-3-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bjorn Andersson Jan. 28, 2015, 2:46 a.m. UTC
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(-)

Comments

Mark Brown Jan. 28, 2015, 7:52 p.m. UTC | #1
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.
Bjorn Andersson Jan. 30, 2015, 12:07 a.m. UTC | #2
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
Mark Brown Jan. 30, 2015, 12:27 p.m. UTC | #3
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.
Bjorn Andersson Feb. 5, 2015, 10:16 p.m. UTC | #4
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
Mark Brown Feb. 6, 2015, 11:46 a.m. UTC | #5
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.
Bjorn Andersson Feb. 9, 2015, 10:08 p.m. UTC | #6
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
Mark Brown Feb. 10, 2015, 7:08 a.m. UTC | #7
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?
diff mbox

Patch

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);