regulator: Support ramp-up delay for drivers with get_voltage()

Message ID 1459347792-13515-1-git-send-email-georgi.djakov@linaro.org
State New
Headers show

Commit Message

Georgi Djakov March 30, 2016, 2:23 p.m.
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

Comments

Stephen Boyd March 30, 2016, 6:17 p.m. | #1
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
Mark Brown March 30, 2016, 6:32 p.m. | #2
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.

Patch

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) {