* [PATCH] regulator: core: Make regulator object reflect configured voltage @ 2014-02-04 5:54 Bjorn Andersson 2014-02-04 11:05 ` Mark Brown 2014-02-05 20:30 ` [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators Bjorn Andersson 0 siblings, 2 replies; 10+ messages in thread From: Bjorn Andersson @ 2014-02-04 5:54 UTC (permalink / raw) To: linux-arm-kernel In the case when a regulator is initialized from DT with equal min and max voltages the voltage is applied on initialization and future calls to regulator_set_voltage fails. This behavious is different than if the regulator is configured to be a span and therefor requires logic to handle this difference in the consumer driver. Eliminate this difference by populating the min_uV and max_uV of the newly created regulator from the constraints so that calles to regulator_set_voltage is considered no-ops and not a failure. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- drivers/regulator/core.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index d85f313..9c82d37 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1209,6 +1209,16 @@ static struct regulator *create_regulator(struct regulator_dev *rdev, _regulator_is_enabled(rdev)) regulator->always_on = true; + /* + * Make the regulator reflect the configured voltage selected in + * machine_constraints_voltage() + */ + if (rdev->constraints->apply_uV && + rdev->constraints->min_uV == rdev->constraints->max_uV) { + regulator->min_uV = rdev->constraints->min_uV; + regulator->max_uV = rdev->constraints->min_uV; + } + mutex_unlock(&rdev->mutex); return regulator; overflow_err: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 5:54 [PATCH] regulator: core: Make regulator object reflect configured voltage Bjorn Andersson @ 2014-02-04 11:05 ` Mark Brown 2014-02-04 18:02 ` Bjorn Andersson 2014-02-05 20:30 ` [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators Bjorn Andersson 1 sibling, 1 reply; 10+ messages in thread From: Mark Brown @ 2014-02-04 11:05 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 03, 2014 at 09:54:28PM -0800, Bjorn Andersson wrote: > + /* > + * Make the regulator reflect the configured voltage selected in > + * machine_constraints_voltage() > + */ > + if (rdev->constraints->apply_uV && > + rdev->constraints->min_uV == rdev->constraints->max_uV) { > + regulator->min_uV = rdev->constraints->min_uV; > + regulator->max_uV = rdev->constraints->min_uV; > + } > + Why not do this at the time we apply the voltage? That would seem to be more robust, doing it in a separate place means that we might update one bit of code and not the other or might change the execution path so that one gets run and the other doesn't. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/539dcbfa/attachment.sig> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 11:05 ` Mark Brown @ 2014-02-04 18:02 ` Bjorn Andersson 2014-02-04 18:18 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2014-02-04 18:02 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Feb 03, 2014 at 09:54:28PM -0800, Bjorn Andersson wrote: > >> + /* >> + * Make the regulator reflect the configured voltage selected in >> + * machine_constraints_voltage() >> + */ >> + if (rdev->constraints->apply_uV && >> + rdev->constraints->min_uV == rdev->constraints->max_uV) { >> + regulator->min_uV = rdev->constraints->min_uV; >> + regulator->max_uV = rdev->constraints->min_uV; >> + } >> + > > Why not do this at the time we apply the voltage? That would seem to be > more robust, doing it in a separate place means that we might update one > bit of code and not the other or might change the execution path so that > one gets run and the other doesn't. I do share your concerns about having this logic mirrored here is risky, unfortunately the regulator object is created upon request from a consumer; so it is not available when regulator_register() calls set_machine_constraints(). An alternative is to drop the conditional setting of REGULATOR_CHANGE_VOLTAGE from of_regulator.c and force the regulator drivers to set this flag explicitly; to avoid the difference in behavior depending on configuration. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 18:02 ` Bjorn Andersson @ 2014-02-04 18:18 ` Mark Brown 2014-02-04 19:09 ` Bjorn Andersson 0 siblings, 1 reply; 10+ messages in thread From: Mark Brown @ 2014-02-04 18:18 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 04, 2014 at 10:02:14AM -0800, Bjorn Andersson wrote: > On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown <broonie@kernel.org> wrote: > > Why not do this at the time we apply the voltage? That would seem to be > > more robust, doing it in a separate place means that we might update one > > bit of code and not the other or might change the execution path so that > > one gets run and the other doesn't. > I do share your concerns about having this logic mirrored here is > risky, unfortunately the regulator object is created upon request from > a consumer; so it is not available when regulator_register() calls > set_machine_constraints(). Oh, hang on - that's what you mean by a regulator object... I don't think this fixes the problem you think it does. What is the actual problem you're trying to fix here? The min_uV and max_uV on a consumer struct are supposed to be the request from that consumer, they should only be set if the consumer actually made a request for a voltage range. > An alternative is to drop the conditional setting of > REGULATOR_CHANGE_VOLTAGE from of_regulator.c and force the regulator > drivers to set this flag explicitly; to avoid the difference in > behavior depending on configuration. Why would having each individual driver open code things help? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/9e18bf84/attachment.sig> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 18:18 ` Mark Brown @ 2014-02-04 19:09 ` Bjorn Andersson 2014-02-04 20:00 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2014-02-04 19:09 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 4, 2014 at 10:18 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 04, 2014 at 10:02:14AM -0800, Bjorn Andersson wrote: >> On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown <broonie@kernel.org> wrote: > >> > Why not do this at the time we apply the voltage? That would seem to be >> > more robust, doing it in a separate place means that we might update one >> > bit of code and not the other or might change the execution path so that >> > one gets run and the other doesn't. > >> I do share your concerns about having this logic mirrored here is >> risky, unfortunately the regulator object is created upon request from >> a consumer; so it is not available when regulator_register() calls >> set_machine_constraints(). > > Oh, hang on - that's what you mean by a regulator object... I don't > think this fixes the problem you think it does. What is the actual > problem you're trying to fix here? The min_uV and max_uV on a consumer > struct are supposed to be the request from that consumer, they should > only be set if the consumer actually made a request for a voltage range. I have a regulator that's being configured from DT as: regulator-min-microvolt = <2950000>; regulator-max-microvolt = <2950000>; In the consumer I do regulator_set_voltage(2.95V). As min == max the voltage is applied by the regulator framework on registration of the regulator; and the regulator_set_voltage() fails as REGULATOR_CHANGE_VOLTAGE is not set for this regulator. This makes sense, until I change regulator-min-microvolt to say 2000000 then the regulator_set_voltage() succeeds; and the call is required for the device to function properly. So in the consumer I get different behavior depending on how the regulator is configured in DT. The proposed fix solves this by making the consumer object aware of the initialized voltage (as it's fixed in this case), making it okay for calls to regulator_set_voltage() given that it's the same value as the configured value; failing for others. > >> An alternative is to drop the conditional setting of >> REGULATOR_CHANGE_VOLTAGE from of_regulator.c and force the regulator >> drivers to set this flag explicitly; to avoid the difference in >> behavior depending on configuration. > > Why would having each individual driver open code things help? Without this fix I explicitly need to add REGULATOR_CHANGE_VOLTAGE to the valid_ops_mask of my regulators, ignoring the fact that of_get_regulation_constraints() does apply some logic in this area. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 19:09 ` Bjorn Andersson @ 2014-02-04 20:00 ` Mark Brown 2014-02-05 18:00 ` Bjorn Andersson 0 siblings, 1 reply; 10+ messages in thread From: Mark Brown @ 2014-02-04 20:00 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 04, 2014 at 11:09:03AM -0800, Bjorn Andersson wrote: > I have a regulator that's being configured from DT as: > regulator-min-microvolt = <2950000>; > regulator-max-microvolt = <2950000>; > In the consumer I do regulator_set_voltage(2.95V). > As min == max the voltage is applied by the regulator framework on registration > of the regulator; and the regulator_set_voltage() fails as > REGULATOR_CHANGE_VOLTAGE is not set for this regulator. So we should be changing the code to allow a set_voltage() that sets the voltage to the existing voltage regardless of constraints allowing a change then - that's what the underlying issue is. Your change wouldn't cover the case where the hardware defualt is being used for example. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/0f13e826/attachment-0001.sig> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-04 20:00 ` Mark Brown @ 2014-02-05 18:00 ` Bjorn Andersson 2014-02-05 18:18 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2014-02-05 18:00 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 04, 2014 at 11:09:03AM -0800, Bjorn Andersson wrote: > >> I have a regulator that's being configured from DT as: >> regulator-min-microvolt = <2950000>; >> regulator-max-microvolt = <2950000>; > >> In the consumer I do regulator_set_voltage(2.95V). > >> As min == max the voltage is applied by the regulator framework on registration >> of the regulator; and the regulator_set_voltage() fails as >> REGULATOR_CHANGE_VOLTAGE is not set for this regulator. > > So we should be changing the code to allow a set_voltage() that sets the > voltage to the existing voltage regardless of constraints allowing a > change then - that's what the underlying issue is. Your change wouldn't > cover the case where the hardware defualt is being used for example. Makes sense, but the only thing we could check for would be if min_uV == max_uV == current-voltage. That would work out fine for this use case, but do you think it would be good enough? The best thing I've come up with then is to add the following check in regulator_set_voltage(). if (min_uV == max_uV && _regulator_get_voltage(rdev) == min_uV) goto out; Would this be acceptable? It's achieving the same thing as my patch, is more robust and covers the case of setting the voltage to the hw default value. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Make regulator object reflect configured voltage 2014-02-05 18:00 ` Bjorn Andersson @ 2014-02-05 18:18 ` Mark Brown 0 siblings, 0 replies; 10+ messages in thread From: Mark Brown @ 2014-02-05 18:18 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 05, 2014 at 10:00:15AM -0800, Bjorn Andersson wrote: > On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown <broonie@kernel.org> wrote: > > So we should be changing the code to allow a set_voltage() that sets the > > voltage to the existing voltage regardless of constraints allowing a > > change then - that's what the underlying issue is. Your change wouldn't > > cover the case where the hardware defualt is being used for example. > Makes sense, but the only thing we could check for would be if min_uV > == max_uV == current-voltage. That would work out fine for this use > case, but do you think it would be good enough? It should be fine to check for min_uV <= current-voltage <= max_uV instead if CHANGE_VOLTAGE isn't available, so long as the existing setting is in the range it's fine. > The best thing I've come up with then is to add the following check in > regulator_set_voltage(). > if (min_uV == max_uV && _regulator_get_voltage(rdev) == min_uV) > goto out; > Would this be acceptable? It's achieving the same thing as my patch, > is more robust and covers the case of setting the voltage to the hw > default value. That sort of thing yes, just short circuit out the main logic in this case. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140205/fda85355/attachment.sig> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators 2014-02-04 5:54 [PATCH] regulator: core: Make regulator object reflect configured voltage Bjorn Andersson 2014-02-04 11:05 ` Mark Brown @ 2014-02-05 20:30 ` Bjorn Andersson 2014-02-07 12:14 ` Mark Brown 1 sibling, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2014-02-05 20:30 UTC (permalink / raw) To: linux-arm-kernel Make it okay to call regulator_set_voltage on regulators with fixed voltage if the requested range overlaps the current/configured voltage. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- drivers/regulator/core.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index b38a6b6..0cd1a3b 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2395,6 +2395,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV) struct regulator_dev *rdev = regulator->rdev; int ret = 0; int old_min_uV, old_max_uV; + int current_uV; mutex_lock(&rdev->mutex); @@ -2405,6 +2406,19 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV) if (regulator->min_uV == min_uV && regulator->max_uV == max_uV) goto out; + /* If we're trying to set a range that overlaps the current voltage, + * return succesfully even though the regulator does not support + * changing the voltage. + */ + if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) { + current_uV = _regulator_get_voltage(rdev); + if (min_uV <= current_uV && current_uV <= max_uV) { + regulator->min_uV = min_uV; + regulator->max_uV = max_uV; + goto out; + } + } + /* sanity check */ if (!rdev->desc->ops->set_voltage && !rdev->desc->ops->set_voltage_sel) { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators 2014-02-05 20:30 ` [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators Bjorn Andersson @ 2014-02-07 12:14 ` Mark Brown 0 siblings, 0 replies; 10+ messages in thread From: Mark Brown @ 2014-02-07 12:14 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 05, 2014 at 12:30:26PM -0800, Bjorn Andersson wrote: > Make it okay to call regulator_set_voltage on regulators with fixed > voltage if the requested range overlaps the current/configured voltage. Applied, thanks. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140207/655ab5f2/attachment-0001.sig> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-02-07 12:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-04 5:54 [PATCH] regulator: core: Make regulator object reflect configured voltage Bjorn Andersson 2014-02-04 11:05 ` Mark Brown 2014-02-04 18:02 ` Bjorn Andersson 2014-02-04 18:18 ` Mark Brown 2014-02-04 19:09 ` Bjorn Andersson 2014-02-04 20:00 ` Mark Brown 2014-02-05 18:00 ` Bjorn Andersson 2014-02-05 18:18 ` Mark Brown 2014-02-05 20:30 ` [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators Bjorn Andersson 2014-02-07 12:14 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox