| Message ID | 1422413199-17273-3-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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
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 --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);
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(-)