| Message ID | 1459347792-13515-1-git-send-email-georgi.djakov@linaro.org |
|---|---|
| State | New |
| Headers | show |
Quoting Mark Brown (2016-03-30 10:36:58) > On Wed, Mar 30, 2016 at 05:23:12PM +0300, Georgi Djakov wrote: > > > + } else if (_regulator_is_enabled(rdev) && > > + rdev->desc->ops->set_voltage_time_sel && > > + rdev->desc->ops->get_voltage) { > > + int uV = rdev->desc->ops->get_voltage(rdev); > > + > > + if (uV > 0) { > > + old_selector = regulator_map_voltage(rdev, uV, uV); > > + if (old_selector < 0) > > + return old_selector; > > + } > > If a driver is using selectors it should use selectors uninformly, it > should not mix and match selector and raw voltage interfaces. If we > the set and get operations are not symmetric I'd expect we're going to > run into problems sooner rather than later. This is for the qcom spmi regulator driver. I seem to have put in the set_voltage_time_sel op but missed the fact that the regulator core wasn't calling that op to find out how much time to delay. So we have raw voltage set and get ops and this selector based delay op. Do we need to change the ops to be selector based if we want the regulator core to delay after changing voltages? Or do we need to put the delay directly into the set_voltage() op in the driver? -- 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, Mar 30, 2016 at 11:17:09AM -0700, Stephen Boyd wrote: > This is for the qcom spmi regulator driver. I seem to have put in the > set_voltage_time_sel op but missed the fact that the regulator core > wasn't calling that op to find out how much time to delay. So we have > raw voltage set and get ops and this selector based delay op. > Do we need to change the ops to be selector based if we want the > regulator core to delay after changing voltages? Or do we need to put > the delay directly into the set_voltage() op in the driver? You need a consistent set of operations. If you want to use raw voltages you need to add a raw voltage interface for getting the delay, not mix selector and non-selector interfaces otherwise we'll run into problems. It is not sensible to expect a driver that does not use selectors to implement selectors for some operations, if a driver *does* use selectors then it should do so consistently. The latter is probably the more sensible option for this driver since it does have a list operation so does understand selectors.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 74e8a7a3b3e8..39806b4d580a 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2772,6 +2772,17 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev, old_selector = rdev->desc->ops->get_voltage_sel(rdev); if (old_selector < 0) return old_selector; + + } else if (_regulator_is_enabled(rdev) && + rdev->desc->ops->set_voltage_time_sel && + rdev->desc->ops->get_voltage) { + int uV = rdev->desc->ops->get_voltage(rdev); + + if (uV > 0) { + old_selector = regulator_map_voltage(rdev, uV, uV); + if (old_selector < 0) + return old_selector; + } } if (rdev->desc->ops->set_voltage) {
Currently a ramp-up delay is supported only for drivers which have an implementation of both set_voltage_time_sel() and get_voltage_sel(). But some drivers use get_voltage() instead of get_voltage_sel(). Allow the regulator core to support ramp-up delays for drivers which use get_voltage(). Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> --- drivers/regulator/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 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