Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Wed, Feb 05, 2014 at 10:00:15AM -0800, Bjorn Andersson wrote: > On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown 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. signature.asc Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown 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 = <295>; >> regulator-max-microvolt = <295>; > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown broo...@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 = 295; regulator-max-microvolt = 295; 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Wed, Feb 05, 2014 at 10:00:15AM -0800, Bjorn Andersson wrote: On Tue, Feb 4, 2014 at 12:00 PM, Mark Brown broo...@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. signature.asc Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
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 = <295>; > regulator-max-microvolt = <295>; > 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. signature.asc Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 10:18 AM, Mark Brown 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 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 = <295>; regulator-max-microvolt = <295>; 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 200 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 04, 2014 at 10:02:14AM -0800, Bjorn Andersson wrote: > On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown 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? signature.asc Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
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.
signature.asc
Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
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.
signature.asc
Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown broo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 04, 2014 at 10:02:14AM -0800, Bjorn Andersson wrote: On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown broo...@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? signature.asc Description: Digital signature
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
On Tue, Feb 4, 2014 at 10:18 AM, Mark Brown broo...@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 broo...@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 = 295; regulator-max-microvolt = 295; 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 200 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Make regulator object reflect configured voltage
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 = 295; regulator-max-microvolt = 295; 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. signature.asc Description: Digital signature
[PATCH] regulator: core: Make regulator object reflect configured voltage
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
---
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(>mutex);
return regulator;
overflow_err:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: core: Make regulator object reflect configured voltage
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.anders...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
