| Message ID | 1391493268-3242-1-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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:
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(+)