[PATCH] regulator: core: Make regulator object reflect configured voltage

[PATCH] regulator: core: Make regulator object reflect configured voltage

[Bjorn Andersson]

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Mark Brown]

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

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.

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Bjorn Andersson]

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Mark Brown]

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

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?

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Bjorn Andersson]

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Mark Brown]

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

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.

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Bjorn Andersson]

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

Re: [PATCH] regulator: core: Make regulator object reflect configured voltage

[Mark Brown]

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

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.

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

[PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators

[Bjorn Andersson]

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

Re: [PATCH] regulator: core: Allow regulator_set_voltage for fixed regulators

[Mark Brown]

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

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.

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