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

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

[Georgi Djakov]

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(+)

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

Re: [PATCH] regulator: Support ramp-up delay for drivers with get_voltage()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: Support ramp-up delay for drivers with get_voltage()

[Stephen Boyd]

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?

Re: [PATCH] regulator: Support ramp-up delay for drivers with get_voltage()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: Support ramp-up delay for drivers with get_voltage()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Wed, Mar 30, 2016 at 12:34:26PM -0700, Stephen Boyd wrote:

> Ok. Just curious, why is there a 'selector' argument to the
> set_voltage() op then? The qcom spmi driver is using that to allow the
> list operation to return the voltage that was actually set. Is that a
> 'selector' interface? We could just as easily have read the hardware to
> figure out the voltage, but I think we implement the list op to avoid
> reading hardware when we know what selector has been chosen during
> voltage changes.

Historical reasons - this predates having the split selector and map
operations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]