| Message ID | 1397845810-17002-1-git-send-email-tim.kryger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 18, 2014 at 11:30:10AM -0700, Tim Kryger wrote: > If a regulator consumer requests a voltage range that can be satisfied, > the return value should indicate success even if that regulator has a > fixed voltage. Since there is already logic to check if the requested > voltage range overlaps the allowed range, set REGULATOR_CHANGE_VOLTAGE > for regulators with constraints that include a positive voltage. This seems like the wrong place to fix this, it's nothing to do with DT and we shouldn't require that nonsensical permissions are set. Instead we should fix this at the point where we're implementing the permission check, have the failure case check the current voltage before returning an error.
On Fri, Apr 18, 2014 at 11:52 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 18, 2014 at 11:30:10AM -0700, Tim Kryger wrote: >> If a regulator consumer requests a voltage range that can be satisfied, >> the return value should indicate success even if that regulator has a >> fixed voltage. Since there is already logic to check if the requested >> voltage range overlaps the allowed range, set REGULATOR_CHANGE_VOLTAGE >> for regulators with constraints that include a positive voltage. > > This seems like the wrong place to fix this, it's nothing to do with DT > and we shouldn't require that nonsensical permissions are set. Instead > we should fix this at the point where we're implementing the permission > check, have the failure case check the current voltage before returning > an error. Are you saying that REGULATOR_CHANGE_VOLTAGE and REGULATOR_CHANGE_CURRENT are nonsense? It does seem like, even in the non-DT case, that the decision of whether to call the underlying set_voltage and set_current functions could be made solely based on the numerical voltage and current constraints. Thanks, Tim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, Apr 18, 2014 at 02:07:58PM -0700, Tim Kryger wrote: > On Fri, Apr 18, 2014 at 11:52 AM, Mark Brown <broonie@kernel.org> wrote: > > This seems like the wrong place to fix this, it's nothing to do with DT > > and we shouldn't require that nonsensical permissions are set. Instead > > we should fix this at the point where we're implementing the permission > > check, have the failure case check the current voltage before returning > > an error. > Are you saying that REGULATOR_CHANGE_VOLTAGE and > REGULATOR_CHANGE_CURRENT are nonsense? Flagging that it's possible to change the voltage of a fixed voltage regulator is nonsense. > It does seem like, even in the non-DT case, that the decision of > whether to call the underlying set_voltage and set_current functions > could be made solely based on the numerical voltage and current > constraints. The reason they're split is to encourage people to put the information about what's supposed to work in there - you might know what the valid range is but also know that the drivers are buggy and will break if they try to actually vary the voltage. But yes, in general you should never have a range without the ability to use it once everything is working properly so having one without the other at least indicates that things aren't complete in the non-DT case. It does also make the contract a bit clearer, one of the concerns initially was that we wanted to be absolutely clear that the machine integration was responsible for enabling the ability to change things in case people broke boards.
On Fri, Apr 18, 2014 at 11:52 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 18, 2014 at 11:30:10AM -0700, Tim Kryger wrote: >> If a regulator consumer requests a voltage range that can be satisfied, >> the return value should indicate success even if that regulator has a >> fixed voltage. Since there is already logic to check if the requested >> voltage range overlaps the allowed range, set REGULATOR_CHANGE_VOLTAGE >> for regulators with constraints that include a positive voltage. > > This seems like the wrong place to fix this, it's nothing to do with DT > and we shouldn't require that nonsensical permissions are set. Instead > we should fix this at the point where we're implementing the permission > check, have the failure case check the current voltage before returning > an error. I now see that Bjorn submitted a patch which did exactly that. https://lkml.org/lkml/2014/2/5/494 His change made it into v3.15-rc1, so mine is no longer necessary. Thanks, Tim Kryger -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index ea4f36f..a205d62 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -36,7 +36,7 @@ static void of_get_regulation_constraints(struct device_node *np, constraints->max_uV = be32_to_cpu(*max_uV); /* Voltage change possible? */ - if (constraints->min_uV != constraints->max_uV) + if (constraints->max_uV > 0) constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; /* Only one voltage? Then make sure it's set. */ if (min_uV && max_uV && constraints->min_uV == constraints->max_uV)
If a regulator consumer requests a voltage range that can be satisfied, the return value should indicate success even if that regulator has a fixed voltage. Since there is already logic to check if the requested voltage range overlaps the allowed range, set REGULATOR_CHANGE_VOLTAGE for regulators with constraints that include a positive voltage. Signed-off-by: Tim Kryger <tim.kryger@linaro.org> --- drivers/regulator/of_regulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)