[PATCH v4 1/4] regulator: Add set_voltage_time op

[PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

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

The new op is analogous to set_voltage_time_sel. It can be used by
regulators that don't have a table of discrete voltages. The function
returns the time for the regulator voltage output voltage to stabilize
after being set to a new value, in microseconds. The actual calculation
of the stabilization time is done in the same place for both types of
regulators.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- This patch is new for v4.

 drivers/regulator/core.c         | 140 +++++++++++++++++++++++++--------------
 include/linux/regulator/driver.h |   8 +++
 2 files changed, 97 insertions(+), 51 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index db320e8..b1cef47 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2751,6 +2751,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	int best_val = 0;
 	unsigned int selector;
 	int old_selector = -1;
+	int old_uV = _regulator_get_voltage(rdev);
 
 	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
 
@@ -2800,27 +2801,38 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 		ret = -EINVAL;
 	}
 
-	/* Call set_voltage_time_sel if successfully obtained old_selector */
-	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
-		&& old_selector != selector) {
+	if (ret != 0 || rdev->constraints->ramp_disable)
+		goto no_delay;
 
-		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
-						old_selector, selector);
-		if (delay < 0) {
-			rdev_warn(rdev, "set_voltage_time_sel() failed: %d\n",
-				  delay);
-			delay = 0;
-		}
+	if (rdev->desc->ops->set_voltage_time) {
+		int new_uV = _regulator_get_voltage(rdev);
 
-		/* Insert any necessary delays */
-		if (delay >= 1000) {
-			mdelay(delay / 1000);
-			udelay(delay % 1000);
-		} else if (delay) {
-			udelay(delay);
-		}
+		if (old_uV == new_uV)
+			goto no_delay;
+
+		delay =	rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (rdev->desc->ops->set_voltage_time_sel) {
+		if (old_selector < 0 || old_selector == selector)
+			goto no_delay;
+
+		delay = rdev->desc->ops->set_voltage_time_sel(
+			rdev, old_selector, selector);
+	}
+
+	if (delay < 0) {
+		rdev_warn(rdev, "failed to get delay: %d\n", delay);
+		delay = 0;
 	}
 
+	/* Insert any necessary delays */
+	if (delay >= 1000) {
+		mdelay(delay / 1000);
+		udelay(delay % 1000);
+	} else if (delay) {
+		udelay(delay);
+	}
+
+no_delay:
 	if (ret == 0 && best_val >= 0) {
 		unsigned long data = best_val;
 
@@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
-	int old_sel = -1;
-	int new_sel = -1;
-	int voltage;
-	int i;
 
-	/* Currently requires operations to do this */
-	if (!ops->list_voltage || !ops->set_voltage_time_sel
-	    || !rdev->desc->n_voltages)
-		return -EINVAL;
+	if (ops->set_voltage_time) {
+		return ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (ops->set_voltage_time_sel) {
+		int old_sel = -1;
+		int new_sel = -1;
+		int voltage;
+		int i;
 
-	for (i = 0; i < rdev->desc->n_voltages; i++) {
-		/* We only look for exact voltage matches here */
-		voltage = regulator_list_voltage(regulator, i);
-		if (voltage < 0)
+		/* Currently requires operations to do this */
+		if (!ops->list_voltage || !rdev->desc->n_voltages)
 			return -EINVAL;
-		if (voltage == 0)
-			continue;
-		if (voltage == old_uV)
-			old_sel = i;
-		if (voltage == new_uV)
-			new_sel = i;
-	}
 
-	if (old_sel < 0 || new_sel < 0)
-		return -EINVAL;
+		for (i = 0; i < rdev->desc->n_voltages; i++) {
+			/* We only look for exact voltage matches here */
+			voltage = regulator_list_voltage(regulator, i);
+			if (voltage < 0)
+				return -EINVAL;
+			if (voltage == 0)
+				continue;
+			if (voltage == old_uV)
+				old_sel = i;
+			if (voltage == new_uV)
+				new_sel = i;
+		}
+
+		if (old_sel < 0 || new_sel < 0)
+			return -EINVAL;
+
+		return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	}
 
-	return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time);
 
 /**
- * regulator_set_voltage_time_sel - get raise/fall time
- * @rdev: regulator source device
- * @old_selector: selector for starting voltage
- * @new_selector: selector for target voltage
+ * regulator_set_voltage_time_op - get raise/fall time
+ * @regulator: regulator source
+ * @old_uV: starting voltage in microvolts
+ * @new_uV: target voltage in microvolts
  *
- * Provided with the starting and target voltage selectors, this function
- * returns time in microseconds required to rise or fall to this new voltage
+ * Provided with the starting and ending voltage, this function calculates
+ * the time in microseconds required to rise or fall to this new voltage.
  *
  * Drivers providing ramp_delay in regulation_constraints can use this as their
- * set_voltage_time_sel() operation.
+ * set_voltage_time() operation.
  */
-int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
-				   unsigned int old_selector,
-				   unsigned int new_selector)
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV)
 {
 	unsigned int ramp_delay = 0;
-	int old_volt, new_volt;
 
 	if (rdev->constraints->ramp_delay)
 		ramp_delay = rdev->constraints->ramp_delay;
@@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 		return 0;
 	}
 
+	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
+}
+EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);
+
+/**
+ * regulator_set_voltage_time_sel - get raise/fall time
+ * @rdev: regulator source device
+ * @old_selector: selector for starting voltage
+ * @new_selector: selector for target voltage
+ *
+ * Provided with the starting and target voltage selectors, this function
+ * returns time in microseconds required to rise or fall to this new voltage
+ *
+ * Drivers providing ramp_delay in regulation_constraints can use this as their
+ * set_voltage_time_sel() operation.
+ */
+int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
+				   unsigned int old_selector,
+				   unsigned int new_selector)
+{
+	int old_volt, new_volt;
+
 	/* sanity check */
 	if (!rdev->desc->ops->list_voltage)
 		return -EINVAL;
@@ -3059,7 +3097,7 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 	old_volt = rdev->desc->ops->list_voltage(rdev, old_selector);
 	new_volt = rdev->desc->ops->list_voltage(rdev, new_selector);
 
-	return DIV_ROUND_UP(abs(new_volt - old_volt), ramp_delay);
+	return regulator_set_voltage_time_op(rdev, old_volt, new_volt);
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fcfa40a..537ca7f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -113,6 +113,10 @@ struct regulator_linear_range {
  *               stabilise after being enabled, in microseconds.
  * @set_ramp_delay: Set the ramp delay for the regulator. The driver should
  *		select ramp delay equal to or less than(closest) ramp_delay.
+ * @set_voltage_time: Time taken for the regulator voltage output voltage
+ *               to stabilise after being set to a new value, in microseconds.
+ *               The function provides the from and to voltage, the function
+ *               should return the worst case.
  * @set_voltage_time_sel: Time taken for the regulator voltage output voltage
  *               to stabilise after being set to a new value, in microseconds.
  *               The function provides the from and to voltage selector, the
@@ -168,6 +172,8 @@ struct regulator_ops {
 	/* Time taken to enable or set voltage on the regulator */
 	int (*enable_time) (struct regulator_dev *);
 	int (*set_ramp_delay) (struct regulator_dev *, int ramp_delay);
+	int (*set_voltage_time) (struct regulator_dev *,
+				int old_uV, int new_uV);
 	int (*set_voltage_time_sel) (struct regulator_dev *,
 				     unsigned int old_selector,
 				     unsigned int new_selector);
@@ -461,6 +467,8 @@ int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel);
 int regulator_is_enabled_regmap(struct regulator_dev *rdev);
 int regulator_enable_regmap(struct regulator_dev *rdev);
 int regulator_disable_regmap(struct regulator_dev *rdev);
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV);
 int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 				   unsigned int old_selector,
 				   unsigned int new_selector);
-- 
2.8.0.rc3.226.g39d4020



[-- Attachment #2: Type: text/html, Size: 1 bytes --]

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:

Whatever you're using to send these is not threading things as expected
and is adding a random HTML segment to the end of the e-mails - you
probably want to look at this.  If you're trying to use gmail via the
web interface you want to avoid that, it's got a tendency to mangle
things.

> -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> -		&& old_selector != selector) {
> +	if (ret != 0 || rdev->constraints->ramp_disable)
> +		goto no_delay;

You probably want to do the refactoring for splitting out decisions
about old_selector separately, it'll make the diff clearer.

> -		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
> -						old_selector, selector);

> +		delay = rdev->desc->ops->set_voltage_time_sel(
> +			rdev, old_selector, selector);

Coding style - there's no need to put the rdev on the new line and the
arguments should be more indented.  Look at how the original was
indented...

> +	/* Insert any necessary delays */
> +	if (delay >= 1000) {
> +		mdelay(delay / 1000);
> +		udelay(delay % 1000);
> +	} else if (delay) {
> +		udelay(delay);
> +	}
> +
> +no_delay:

Why were the gotos there?

> @@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	const struct regulator_ops *ops = rdev->desc->ops;
> -	int old_sel = -1;
> -	int new_sel = -1;
> -	int voltage;
> -	int i;
>  
> -	/* Currently requires operations to do this */
> -	if (!ops->list_voltage || !ops->set_voltage_time_sel
> -	    || !rdev->desc->n_voltages)
> -		return -EINVAL;
> +	if (ops->set_voltage_time) {
> +		return ops->set_voltage_time(rdev, old_uV, new_uV);
> +	} else if (ops->set_voltage_time_sel) {
> +		int old_sel = -1;
> +		int new_sel = -1;
> +		int voltage;
> +		int i;
>  
> -	for (i = 0; i < rdev->desc->n_voltages; i++) {
> -		/* We only look for exact voltage matches here */
> -		voltage = regulator_list_voltage(regulator, i);

The diff and I expect the resulting code would be a lot clearer if we
just left most of the function indented as it is and simply directly
returned set_voltage_time().  Which is what we do anyway so no need to
reindent the rest of the code.

> +int regulator_set_voltage_time_op(struct regulator_dev *rdev,
> +				int old_uV, int new_uV)
>  {
>  	unsigned int ramp_delay = 0;
> -	int old_volt, new_volt;
>  
>  	if (rdev->constraints->ramp_delay)
>  		ramp_delay = rdev->constraints->ramp_delay;
> @@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
>  		return 0;
>  	}
>  
> +	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
> +}
> +EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);

This is getting very messy.  Having the handling of the ramp delay DT
properties in the operation that the driver in theory implements is
making things harder than they should be (and we seem to have some
broken drivers that don't do one or both of the ramp delays).  I'm not
sure this series is the place to fully fix that but it'd be good to
start that refactoring by not requiring the op be set and instead just
doing this implementation by default.  That way there's less stuff to
clean up later on.

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:

Whatever you're using to send these is not threading things as expected
and is adding a random HTML segment to the end of the e-mails - you
probably want to look at this.  If you're trying to use gmail via the
web interface you want to avoid that, it's got a tendency to mangle
things.

> -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> -		&& old_selector != selector) {
> +	if (ret != 0 || rdev->constraints->ramp_disable)
> +		goto no_delay;

You probably want to do the refactoring for splitting out decisions
about old_selector separately, it'll make the diff clearer.

> -		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
> -						old_selector, selector);

> +		delay = rdev->desc->ops->set_voltage_time_sel(
> +			rdev, old_selector, selector);

Coding style - there's no need to put the rdev on the new line and the
arguments should be more indented.  Look at how the original was
indented...

> +	/* Insert any necessary delays */
> +	if (delay >= 1000) {
> +		mdelay(delay / 1000);
> +		udelay(delay % 1000);
> +	} else if (delay) {
> +		udelay(delay);
> +	}
> +
> +no_delay:

Why were the gotos there?

> @@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	const struct regulator_ops *ops = rdev->desc->ops;
> -	int old_sel = -1;
> -	int new_sel = -1;
> -	int voltage;
> -	int i;
>  
> -	/* Currently requires operations to do this */
> -	if (!ops->list_voltage || !ops->set_voltage_time_sel
> -	    || !rdev->desc->n_voltages)
> -		return -EINVAL;
> +	if (ops->set_voltage_time) {
> +		return ops->set_voltage_time(rdev, old_uV, new_uV);
> +	} else if (ops->set_voltage_time_sel) {
> +		int old_sel = -1;
> +		int new_sel = -1;
> +		int voltage;
> +		int i;
>  
> -	for (i = 0; i < rdev->desc->n_voltages; i++) {
> -		/* We only look for exact voltage matches here */
> -		voltage = regulator_list_voltage(regulator, i);

The diff and I expect the resulting code would be a lot clearer if we
just left most of the function indented as it is and simply directly
returned set_voltage_time().  Which is what we do anyway so no need to
reindent the rest of the code.

> +int regulator_set_voltage_time_op(struct regulator_dev *rdev,
> +				int old_uV, int new_uV)
>  {
>  	unsigned int ramp_delay = 0;
> -	int old_volt, new_volt;
>  
>  	if (rdev->constraints->ramp_delay)
>  		ramp_delay = rdev->constraints->ramp_delay;
> @@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
>  		return 0;
>  	}
>  
> +	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
> +}
> +EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);

This is getting very messy.  Having the handling of the ramp delay DT
properties in the operation that the driver in theory implements is
making things harder than they should be (and we seem to have some
broken drivers that don't do one or both of the ramp delays).  I'm not
sure this series is the place to fully fix that but it'd be good to
start that refactoring by not requiring the op be set and instead just
doing this implementation by default.  That way there's less stuff to
clean up later on.

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

Hi Mark,

thanks for your review, please find some comments below.

El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:

> On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
> 
> Whatever you're using to send these is not threading things as expected
> and is adding a random HTML segment to the end of the e-mails - you
> probably want to look at this.  If you're trying to use gmail via the
> web interface you want to avoid that, it's got a tendency to mangle
> things.

Sorry about that, I'll try to use git send-email in the future to avoid
this kind of things.

> > -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> > -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> > -		&& old_selector != selector) {
> > +	if (ret != 0 || rdev->constraints->ramp_disable)
> > +		goto no_delay;
> 
> You probably want to do the refactoring for splitting out decisions
> about old_selector separately, it'll make the diff clearer.

The old_selector conditions could be moved into the "else if
(rdev->desc->ops->set_voltage_sel)" branch above, is that you mean?

> > -		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
> > -						old_selector, selector);
> 
> > +		delay = rdev->desc->ops->set_voltage_time_sel(
> > +			rdev, old_selector, selector);
> 
> Coding style - there's no need to put the rdev on the new line and the
> arguments should be more indented.  Look at how the original was
> indented...

ok

> > +	/* Insert any necessary delays */
> > +	if (delay >= 1000) {
> > +		mdelay(delay / 1000);
> > +		udelay(delay % 1000);
> > +	} else if (delay) {
> > +		udelay(delay);
> > +	}
> > +
> > +no_delay:
> 
> Why were the gotos there?

Not sure how to interpret your question. Would you prefer no to use
gotos, should the notification be skipped in case the voltage is not
changed, do you expect a comment, ...?

> > @@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
> >  {
> >  	struct regulator_dev *rdev = regulator->rdev;
> >  	const struct regulator_ops *ops = rdev->desc->ops;
> > -	int old_sel = -1;
> > -	int new_sel = -1;
> > -	int voltage;
> > -	int i;
> >  
> > -	/* Currently requires operations to do this */
> > -	if (!ops->list_voltage || !ops->set_voltage_time_sel
> > -	    || !rdev->desc->n_voltages)
> > -		return -EINVAL;
> > +	if (ops->set_voltage_time) {
> > +		return ops->set_voltage_time(rdev, old_uV, new_uV);
> > +	} else if (ops->set_voltage_time_sel) {
> > +		int old_sel = -1;
> > +		int new_sel = -1;
> > +		int voltage;
> > +		int i;
> >  
> > -	for (i = 0; i < rdev->desc->n_voltages; i++) {
> > -		/* We only look for exact voltage matches here */
> > -		voltage = regulator_list_voltage(regulator, i);
> 
> The diff and I expect the resulting code would be a lot clearer if we
> just left most of the function indented as it is and simply directly
> returned set_voltage_time().  Which is what we do anyway so no need to
> reindent the rest of the code.

Ok, with your comment below on a default implementation this would
become something like:

if (ops->set_voltage_time) {
	return ops->set_voltage_time(...);
} else if (!ops->set_voltage_time_sel) {
	return _regulator_set_voltage_time(..);
}

// previous code

It will certainly make the diff clearer, for the resulting code my
preference would be to avoid the negative condition on
ops->set_voltage_time_sel to enter the default case, but that's a
minor question.

> > +int regulator_set_voltage_time_op(struct regulator_dev *rdev,
> > +				int old_uV, int new_uV)
> >  {
> >  	unsigned int ramp_delay = 0;
> > -	int old_volt, new_volt;
> >  
> >  	if (rdev->constraints->ramp_delay)
> >  		ramp_delay = rdev->constraints->ramp_delay;
> > @@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
> >  		return 0;
> >  	}
> >  
> > +	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
> > +}
> > +EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);
> 
> This is getting very messy.  Having the handling of the ramp delay DT
> properties in the operation that the driver in theory implements is
> making things harder than they should be (and we seem to have some
> broken drivers that don't do one or both of the ramp delays).  I'm not
> sure this series is the place to fully fix that but it'd be good to
> start that refactoring by not requiring the op be set and instead just
> doing this implementation by default.  That way there's less stuff to
> clean up later on.

Having this as default implementation sounds good. Thanks for the suggestion!

Matthias

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

Hi Mark,

thanks for your review, please find some comments below.

El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:

> On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
> 
> Whatever you're using to send these is not threading things as expected
> and is adding a random HTML segment to the end of the e-mails - you
> probably want to look at this.  If you're trying to use gmail via the
> web interface you want to avoid that, it's got a tendency to mangle
> things.

Sorry about that, I'll try to use git send-email in the future to avoid
this kind of things.

> > -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> > -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> > -		&& old_selector != selector) {
> > +	if (ret != 0 || rdev->constraints->ramp_disable)
> > +		goto no_delay;
> 
> You probably want to do the refactoring for splitting out decisions
> about old_selector separately, it'll make the diff clearer.

The old_selector conditions could be moved into the "else if
(rdev->desc->ops->set_voltage_sel)" branch above, is that you mean?

> > -		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
> > -						old_selector, selector);
> 
> > +		delay = rdev->desc->ops->set_voltage_time_sel(
> > +			rdev, old_selector, selector);
> 
> Coding style - there's no need to put the rdev on the new line and the
> arguments should be more indented.  Look at how the original was
> indented...

ok

> > +	/* Insert any necessary delays */
> > +	if (delay >= 1000) {
> > +		mdelay(delay / 1000);
> > +		udelay(delay % 1000);
> > +	} else if (delay) {
> > +		udelay(delay);
> > +	}
> > +
> > +no_delay:
> 
> Why were the gotos there?

Not sure how to interpret your question. Would you prefer no to use
gotos, should the notification be skipped in case the voltage is not
changed, do you expect a comment, ...?

> > @@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
> >  {
> >  	struct regulator_dev *rdev = regulator->rdev;
> >  	const struct regulator_ops *ops = rdev->desc->ops;
> > -	int old_sel = -1;
> > -	int new_sel = -1;
> > -	int voltage;
> > -	int i;
> >  
> > -	/* Currently requires operations to do this */
> > -	if (!ops->list_voltage || !ops->set_voltage_time_sel
> > -	    || !rdev->desc->n_voltages)
> > -		return -EINVAL;
> > +	if (ops->set_voltage_time) {
> > +		return ops->set_voltage_time(rdev, old_uV, new_uV);
> > +	} else if (ops->set_voltage_time_sel) {
> > +		int old_sel = -1;
> > +		int new_sel = -1;
> > +		int voltage;
> > +		int i;
> >  
> > -	for (i = 0; i < rdev->desc->n_voltages; i++) {
> > -		/* We only look for exact voltage matches here */
> > -		voltage = regulator_list_voltage(regulator, i);
> 
> The diff and I expect the resulting code would be a lot clearer if we
> just left most of the function indented as it is and simply directly
> returned set_voltage_time().  Which is what we do anyway so no need to
> reindent the rest of the code.

Ok, with your comment below on a default implementation this would
become something like:

if (ops->set_voltage_time) {
	return ops->set_voltage_time(...);
} else if (!ops->set_voltage_time_sel) {
	return _regulator_set_voltage_time(..);
}

// previous code

It will certainly make the diff clearer, for the resulting code my
preference would be to avoid the negative condition on
ops->set_voltage_time_sel to enter the default case, but that's a
minor question.

> > +int regulator_set_voltage_time_op(struct regulator_dev *rdev,
> > +				int old_uV, int new_uV)
> >  {
> >  	unsigned int ramp_delay = 0;
> > -	int old_volt, new_volt;
> >  
> >  	if (rdev->constraints->ramp_delay)
> >  		ramp_delay = rdev->constraints->ramp_delay;
> > @@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
> >  		return 0;
> >  	}
> >  
> > +	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
> > +}
> > +EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);
> 
> This is getting very messy.  Having the handling of the ramp delay DT
> properties in the operation that the driver in theory implements is
> making things harder than they should be (and we seem to have some
> broken drivers that don't do one or both of the ramp delays).  I'm not
> sure this series is the place to fully fix that but it'd be good to
> start that refactoring by not requiring the op be set and instead just
> doing this implementation by default.  That way there's less stuff to
> clean up later on.

Having this as default implementation sounds good. Thanks for the suggestion!

Matthias
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Mon, Sep 12, 2016 at 04:18:51PM -0700, Matthias Kaehlcke wrote:
> El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:
> > On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:

> > > -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> > > -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> > > -		&& old_selector != selector) {
> > > +	if (ret != 0 || rdev->constraints->ramp_disable)
> > > +		goto no_delay;

> > You probably want to do the refactoring for splitting out decisions
> > about old_selector separately, it'll make the diff clearer.

> The old_selector conditions could be moved into the "else if
> (rdev->desc->ops->set_voltage_sel)" branch above, is that you mean?

No, what I mean is this change is doing a bunch of moving code around as
well as adding new things which makes it hard to spot where the new
things are.  Moving the code around separately (that is, in a separate
patch) would make the review easier.

> > > +	/* Insert any necessary delays */
> > > +	if (delay >= 1000) {
> > > +		mdelay(delay / 1000);
> > > +		udelay(delay % 1000);
> > > +	} else if (delay) {
> > > +		udelay(delay);
> > > +	}

> > > +no_delay:

> > Why were the gotos there?

> Not sure how to interpret your question. Would you prefer no to use
> gotos, should the notification be skipped in case the voltage is not
> changed, do you expect a comment, ...?

I mean I couldn't tell why a goto was a good idea for what seemed like
perfectly normal conditional logic.  Either I couldn't tell because it's
not a good idea or it is a good idea but should be clearer in some way
but since I didn't really understand what the purpose of doing the gotos
was I can't say for sure either way.

> > The diff and I expect the resulting code would be a lot clearer if we
> > just left most of the function indented as it is and simply directly
> > returned set_voltage_time().  Which is what we do anyway so no need to
> > reindent the rest of the code.

> Ok, with your comment below on a default implementation this would
> become something like:

> if (ops->set_voltage_time) {
> 	return ops->set_voltage_time(...);
> } else if (!ops->set_voltage_time_sel) {
> 	return _regulator_set_voltage_time(..);
> }

I suspect you'll end up with more refactoring than that around
_set_voltage_time() and this'll be inside that function but I've lost
context here so ICBW.

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

El Tue, Sep 13, 2016 at 12:57:58AM +0100 Mark Brown ha dit:

> On Mon, Sep 12, 2016 at 04:18:51PM -0700, Matthias Kaehlcke wrote:
> > El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:
> > > On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
> 
> > > > -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> > > > -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> > > > -		&& old_selector != selector) {
> > > > +	if (ret != 0 || rdev->constraints->ramp_disable)
> > > > +		goto no_delay;
> 
> > > You probably want to do the refactoring for splitting out decisions
> > > about old_selector separately, it'll make the diff clearer.
> 
> > The old_selector conditions could be moved into the "else if
> > (rdev->desc->ops->set_voltage_sel)" branch above, is that you mean?
> 
> No, what I mean is this change is doing a bunch of moving code around as
> well as adding new things which makes it hard to spot where the new
> things are.  Moving the code around separately (that is, in a separate
> patch) would make the review easier.

Moving the code around is related with the gotos, which are related
with the new set_voltage_sel. If we can agree that using goto is the
right thing to do (please see my rationale below) I could create a
separate patch introducing it. However this will only somewhat
mitigate the code moving around, since we still need separate paths
for set_voltage_time and set_voltage_time_sel.

> > > > +	/* Insert any necessary delays */
> > > > +	if (delay >= 1000) {
> > > > +		mdelay(delay / 1000);
> > > > +		udelay(delay % 1000);
> > > > +	} else if (delay) {
> > > > +		udelay(delay);
> > > > +	}
> 
> > > > +no_delay:
> 
> > > Why were the gotos there?
> 
> > Not sure how to interpret your question. Would you prefer no to use
> > gotos, should the notification be skipped in case the voltage is not
> > changed, do you expect a comment, ...?
> 
> I mean I couldn't tell why a goto was a good idea for what seemed like
> perfectly normal conditional logic.  Either I couldn't tell because it's
> not a good idea or it is a good idea but should be clearer in some way
> but since I didn't really understand what the purpose of doing the gotos
> was I can't say for sure either way.

The main purpose is to avoid deeply nested code branches.

Without gotos I think we'd end up with something like this:

static int _regulator_do_set_voltage(struct regulator_dev *rdev,
                                     int min_uV, int max_uV)
{
	...
	if (ret == 0 && !rdev->constraints->ramp_disable) {
		if (rdev->desc->ops->set_voltage_time_sel) {
			if (old_selector >= 0 && old_selector != selector)
				  rdev->desc->ops->set_voltage_time_sel(rdev, old_selector, selector);
		} else {
		       if (old_uV != new_uV) {
				if (rdev->desc->ops->set_voltage_time)
					delay = rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
				else
					delay = _regulator_set_voltage_time(rdev, old_uV, new_uV);
		       }
		}

		// delay
	}
}

I can change the patch accordingly if this is preferred.

> > > The diff and I expect the resulting code would be a lot clearer if we
> > > just left most of the function indented as it is and simply directly
> > > returned set_voltage_time().  Which is what we do anyway so no need to
> > > reindent the rest of the code.
> 
> > Ok, with your comment below on a default implementation this would
> > become something like:
> 
> > if (ops->set_voltage_time) {
> > 	return ops->set_voltage_time(...);
> > } else if (!ops->set_voltage_time_sel) {
> > 	return _regulator_set_voltage_time(..);
> > }
> 
> I suspect you'll end up with more refactoring than that around
> _set_voltage_time() and this'll be inside that function but I've lost
> context here so ICBW.

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

El Tue, Sep 13, 2016 at 12:57:58AM +0100 Mark Brown ha dit:

> On Mon, Sep 12, 2016 at 04:18:51PM -0700, Matthias Kaehlcke wrote:
> > El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:
> > > On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
> 
> > > > -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> > > > -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> > > > -		&& old_selector != selector) {
> > > > +	if (ret != 0 || rdev->constraints->ramp_disable)
> > > > +		goto no_delay;
> 
> > > You probably want to do the refactoring for splitting out decisions
> > > about old_selector separately, it'll make the diff clearer.
> 
> > The old_selector conditions could be moved into the "else if
> > (rdev->desc->ops->set_voltage_sel)" branch above, is that you mean?
> 
> No, what I mean is this change is doing a bunch of moving code around as
> well as adding new things which makes it hard to spot where the new
> things are.  Moving the code around separately (that is, in a separate
> patch) would make the review easier.

Moving the code around is related with the gotos, which are related
with the new set_voltage_sel. If we can agree that using goto is the
right thing to do (please see my rationale below) I could create a
separate patch introducing it. However this will only somewhat
mitigate the code moving around, since we still need separate paths
for set_voltage_time and set_voltage_time_sel.

> > > > +	/* Insert any necessary delays */
> > > > +	if (delay >= 1000) {
> > > > +		mdelay(delay / 1000);
> > > > +		udelay(delay % 1000);
> > > > +	} else if (delay) {
> > > > +		udelay(delay);
> > > > +	}
> 
> > > > +no_delay:
> 
> > > Why were the gotos there?
> 
> > Not sure how to interpret your question. Would you prefer no to use
> > gotos, should the notification be skipped in case the voltage is not
> > changed, do you expect a comment, ...?
> 
> I mean I couldn't tell why a goto was a good idea for what seemed like
> perfectly normal conditional logic.  Either I couldn't tell because it's
> not a good idea or it is a good idea but should be clearer in some way
> but since I didn't really understand what the purpose of doing the gotos
> was I can't say for sure either way.

The main purpose is to avoid deeply nested code branches.

Without gotos I think we'd end up with something like this:

static int _regulator_do_set_voltage(struct regulator_dev *rdev,
                                     int min_uV, int max_uV)
{
	...
	if (ret == 0 && !rdev->constraints->ramp_disable) {
		if (rdev->desc->ops->set_voltage_time_sel) {
			if (old_selector >= 0 && old_selector != selector)
				  rdev->desc->ops->set_voltage_time_sel(rdev, old_selector, selector);
		} else {
		       if (old_uV != new_uV) {
				if (rdev->desc->ops->set_voltage_time)
					delay = rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
				else
					delay = _regulator_set_voltage_time(rdev, old_uV, new_uV);
		       }
		}

		// delay
	}
}

I can change the patch accordingly if this is preferred.

> > > The diff and I expect the resulting code would be a lot clearer if we
> > > just left most of the function indented as it is and simply directly
> > > returned set_voltage_time().  Which is what we do anyway so no need to
> > > reindent the rest of the code.
> 
> > Ok, with your comment below on a default implementation this would
> > become something like:
> 
> > if (ops->set_voltage_time) {
> > 	return ops->set_voltage_time(...);
> > } else if (!ops->set_voltage_time_sel) {
> > 	return _regulator_set_voltage_time(..);
> > }
> 
> I suspect you'll end up with more refactoring than that around
> _set_voltage_time() and this'll be inside that function but I've lost
> context here so ICBW.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

El Mon, Sep 12, 2016 at 06:18:31PM -0700 Matthias Kaehlcke ha dit:

> El Tue, Sep 13, 2016 at 12:57:58AM +0100 Mark Brown ha dit:
> 
> > On Mon, Sep 12, 2016 at 04:18:51PM -0700, Matthias Kaehlcke wrote:
> > > El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:
> > > > On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
...
> > > > Why were the gotos there?
> > 
> > > Not sure how to interpret your question. Would you prefer no to use
> > > gotos, should the notification be skipped in case the voltage is not
> > > changed, do you expect a comment, ...?
> > 
> > I mean I couldn't tell why a goto was a good idea for what seemed like
> > perfectly normal conditional logic.  Either I couldn't tell because it's
> > not a good idea or it is a good idea but should be clearer in some way
> > but since I didn't really understand what the purpose of doing the gotos
> > was I can't say for sure either way.
> 
> The main purpose is to avoid deeply nested code branches.
> 
> Without gotos I think we'd end up with something like this:
> 
> static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>                                      int min_uV, int max_uV)
> {
> 	...
> 	if (ret == 0 && !rdev->constraints->ramp_disable) {
> 		if (rdev->desc->ops->set_voltage_time_sel) {
> 			if (old_selector >= 0 && old_selector != selector)
> 				  rdev->desc->ops->set_voltage_time_sel(rdev, old_selector, selector);
> 		} else {
> 		       if (old_uV != new_uV) {
> 				if (rdev->desc->ops->set_voltage_time)
> 					delay = rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
> 				else
> 					delay = _regulator_set_voltage_time(rdev, old_uV, new_uV);
> 		       }
> 		}
> 
> 		// delay
> 	}
> }
> 
> I can change the patch accordingly if this is preferred.

The above improves a bit when a local ops variable is used instead of
rdev->desc->ops. With that it looks bearable and probably better than
the goto version.

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

El Mon, Sep 12, 2016 at 06:18:31PM -0700 Matthias Kaehlcke ha dit:

> El Tue, Sep 13, 2016 at 12:57:58AM +0100 Mark Brown ha dit:
> 
> > On Mon, Sep 12, 2016 at 04:18:51PM -0700, Matthias Kaehlcke wrote:
> > > El Mon, Sep 12, 2016 at 07:32:30PM +0100 Mark Brown ha dit:
> > > > On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:
...
> > > > Why were the gotos there?
> > 
> > > Not sure how to interpret your question. Would you prefer no to use
> > > gotos, should the notification be skipped in case the voltage is not
> > > changed, do you expect a comment, ...?
> > 
> > I mean I couldn't tell why a goto was a good idea for what seemed like
> > perfectly normal conditional logic.  Either I couldn't tell because it's
> > not a good idea or it is a good idea but should be clearer in some way
> > but since I didn't really understand what the purpose of doing the gotos
> > was I can't say for sure either way.
> 
> The main purpose is to avoid deeply nested code branches.
> 
> Without gotos I think we'd end up with something like this:
> 
> static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>                                      int min_uV, int max_uV)
> {
> 	...
> 	if (ret == 0 && !rdev->constraints->ramp_disable) {
> 		if (rdev->desc->ops->set_voltage_time_sel) {
> 			if (old_selector >= 0 && old_selector != selector)
> 				  rdev->desc->ops->set_voltage_time_sel(rdev, old_selector, selector);
> 		} else {
> 		       if (old_uV != new_uV) {
> 				if (rdev->desc->ops->set_voltage_time)
> 					delay = rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
> 				else
> 					delay = _regulator_set_voltage_time(rdev, old_uV, new_uV);
> 		       }
> 		}
> 
> 		// delay
> 	}
> }
> 
> I can change the patch accordingly if this is preferred.

The above improves a bit when a local ops variable is used instead of
rdev->desc->ops. With that it looks bearable and probably better than
the goto version.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

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

On Tue, Sep 6, 2016 at 4:38 PM, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Sep 06, 2016 at 11:40:54PM +0100, Mark Brown wrote:
> > On Tue, Sep 06, 2016 at 02:01:25PM -0700, Matthias Kaehlcke wrote:
> > > The new op is analogous to set_voltage_time_sel. It can be used by
> > > regulators that don't have a table of discrete voltages. The function
>
> > I've only got patch 1 in this series, what's going on here?
>
> Actually it turns out I have at least three copies of this, one copy of
> what's hopefully the reset of the series and none of them are threaded
> together :(  You probably want to take a look at your patch sending
> setup...
>

On my side it looked as if the first time the patch wasn't sent at all and
apparently the second one contained an unintended attachment.

Sorry for the spam :(

[-- Attachment #2: Type: text/html, Size: 1352 bytes --]

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 11:40:54PM +0100, Mark Brown wrote:
> On Tue, Sep 06, 2016 at 02:01:25PM -0700, Matthias Kaehlcke wrote:
> > The new op is analogous to set_voltage_time_sel. It can be used by
> > regulators that don't have a table of discrete voltages. The function

> I've only got patch 1 in this series, what's going on here?

Actually it turns out I have at least three copies of this, one copy of
what's hopefully the reset of the series and none of them are threaded
together :(  You probably want to take a look at your patch sending
setup...

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 11:40:54PM +0100, Mark Brown wrote:
> On Tue, Sep 06, 2016 at 02:01:25PM -0700, Matthias Kaehlcke wrote:
> > The new op is analogous to set_voltage_time_sel. It can be used by
> > regulators that don't have a table of discrete voltages. The function

> I've only got patch 1 in this series, what's going on here?

Actually it turns out I have at least three copies of this, one copy of
what's hopefully the reset of the series and none of them are threaded
together :(  You probably want to take a look at your patch sending
setup...

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 02:01:25PM -0700, Matthias Kaehlcke wrote:
> The new op is analogous to set_voltage_time_sel. It can be used by
> regulators that don't have a table of discrete voltages. The function

I've only got patch 1 in this series, what's going on here?

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

Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

[Mark Brown]

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

On Tue, Sep 06, 2016 at 02:01:25PM -0700, Matthias Kaehlcke wrote:
> The new op is analogous to set_voltage_time_sel. It can be used by
> regulators that don't have a table of discrete voltages. The function

I've only got patch 1 in this series, what's going on here?

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

[PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

The new op is analogous to set_voltage_time_sel. It can be used by
regulators that don't have a table of discrete voltages. The function
returns the time for the regulator voltage output voltage to stabilize
after being set to a new value, in microseconds. The actual calculation
of the stabilization time is done in the same place for both types of
regulators.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- This patch is new for v4.

 drivers/regulator/core.c         | 140 +++++++++++++++++++++++++--------------
 include/linux/regulator/driver.h |   8 +++
 2 files changed, 97 insertions(+), 51 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index db320e8..b1cef47 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2751,6 +2751,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	int best_val = 0;
 	unsigned int selector;
 	int old_selector = -1;
+	int old_uV = _regulator_get_voltage(rdev);
 
 	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
 
@@ -2800,27 +2801,38 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 		ret = -EINVAL;
 	}
 
-	/* Call set_voltage_time_sel if successfully obtained old_selector */
-	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
-		&& old_selector != selector) {
+	if (ret != 0 || rdev->constraints->ramp_disable)
+		goto no_delay;
 
-		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
-						old_selector, selector);
-		if (delay < 0) {
-			rdev_warn(rdev, "set_voltage_time_sel() failed: %d\n",
-				  delay);
-			delay = 0;
-		}
+	if (rdev->desc->ops->set_voltage_time) {
+		int new_uV = _regulator_get_voltage(rdev);
 
-		/* Insert any necessary delays */
-		if (delay >= 1000) {
-			mdelay(delay / 1000);
-			udelay(delay % 1000);
-		} else if (delay) {
-			udelay(delay);
-		}
+		if (old_uV == new_uV)
+			goto no_delay;
+
+		delay =	rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (rdev->desc->ops->set_voltage_time_sel) {
+		if (old_selector < 0 || old_selector == selector)
+			goto no_delay;
+
+		delay = rdev->desc->ops->set_voltage_time_sel(
+			rdev, old_selector, selector);
+	}
+
+	if (delay < 0) {
+		rdev_warn(rdev, "failed to get delay: %d\n", delay);
+		delay = 0;
 	}
 
+	/* Insert any necessary delays */
+	if (delay >= 1000) {
+		mdelay(delay / 1000);
+		udelay(delay % 1000);
+	} else if (delay) {
+		udelay(delay);
+	}
+
+no_delay:
 	if (ret == 0 && best_val >= 0) {
 		unsigned long data = best_val;
 
@@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
-	int old_sel = -1;
-	int new_sel = -1;
-	int voltage;
-	int i;
 
-	/* Currently requires operations to do this */
-	if (!ops->list_voltage || !ops->set_voltage_time_sel
-	    || !rdev->desc->n_voltages)
-		return -EINVAL;
+	if (ops->set_voltage_time) {
+		return ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (ops->set_voltage_time_sel) {
+		int old_sel = -1;
+		int new_sel = -1;
+		int voltage;
+		int i;
 
-	for (i = 0; i < rdev->desc->n_voltages; i++) {
-		/* We only look for exact voltage matches here */
-		voltage = regulator_list_voltage(regulator, i);
-		if (voltage < 0)
+		/* Currently requires operations to do this */
+		if (!ops->list_voltage || !rdev->desc->n_voltages)
 			return -EINVAL;
-		if (voltage == 0)
-			continue;
-		if (voltage == old_uV)
-			old_sel = i;
-		if (voltage == new_uV)
-			new_sel = i;
-	}
 
-	if (old_sel < 0 || new_sel < 0)
-		return -EINVAL;
+		for (i = 0; i < rdev->desc->n_voltages; i++) {
+			/* We only look for exact voltage matches here */
+			voltage = regulator_list_voltage(regulator, i);
+			if (voltage < 0)
+				return -EINVAL;
+			if (voltage == 0)
+				continue;
+			if (voltage == old_uV)
+				old_sel = i;
+			if (voltage == new_uV)
+				new_sel = i;
+		}
+
+		if (old_sel < 0 || new_sel < 0)
+			return -EINVAL;
+
+		return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	}
 
-	return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time);
 
 /**
- * regulator_set_voltage_time_sel - get raise/fall time
- * @rdev: regulator source device
- * @old_selector: selector for starting voltage
- * @new_selector: selector for target voltage
+ * regulator_set_voltage_time_op - get raise/fall time
+ * @regulator: regulator source
+ * @old_uV: starting voltage in microvolts
+ * @new_uV: target voltage in microvolts
  *
- * Provided with the starting and target voltage selectors, this function
- * returns time in microseconds required to rise or fall to this new voltage
+ * Provided with the starting and ending voltage, this function calculates
+ * the time in microseconds required to rise or fall to this new voltage.
  *
  * Drivers providing ramp_delay in regulation_constraints can use this as their
- * set_voltage_time_sel() operation.
+ * set_voltage_time() operation.
  */
-int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
-				   unsigned int old_selector,
-				   unsigned int new_selector)
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV)
 {
 	unsigned int ramp_delay = 0;
-	int old_volt, new_volt;
 
 	if (rdev->constraints->ramp_delay)
 		ramp_delay = rdev->constraints->ramp_delay;
@@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 		return 0;
 	}
 
+	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
+}
+EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);
+
+/**
+ * regulator_set_voltage_time_sel - get raise/fall time
+ * @rdev: regulator source device
+ * @old_selector: selector for starting voltage
+ * @new_selector: selector for target voltage
+ *
+ * Provided with the starting and target voltage selectors, this function
+ * returns time in microseconds required to rise or fall to this new voltage
+ *
+ * Drivers providing ramp_delay in regulation_constraints can use this as their
+ * set_voltage_time_sel() operation.
+ */
+int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
+				   unsigned int old_selector,
+				   unsigned int new_selector)
+{
+	int old_volt, new_volt;
+
 	/* sanity check */
 	if (!rdev->desc->ops->list_voltage)
 		return -EINVAL;
@@ -3059,7 +3097,7 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 	old_volt = rdev->desc->ops->list_voltage(rdev, old_selector);
 	new_volt = rdev->desc->ops->list_voltage(rdev, new_selector);
 
-	return DIV_ROUND_UP(abs(new_volt - old_volt), ramp_delay);
+	return regulator_set_voltage_time_op(rdev, old_volt, new_volt);
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fcfa40a..537ca7f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -113,6 +113,10 @@ struct regulator_linear_range {
  *               stabilise after being enabled, in microseconds.
  * @set_ramp_delay: Set the ramp delay for the regulator. The driver should
  *		select ramp delay equal to or less than(closest) ramp_delay.
+ * @set_voltage_time: Time taken for the regulator voltage output voltage
+ *               to stabilise after being set to a new value, in microseconds.
+ *               The function provides the from and to voltage, the function
+ *               should return the worst case.
  * @set_voltage_time_sel: Time taken for the regulator voltage output voltage
  *               to stabilise after being set to a new value, in microseconds.
  *               The function provides the from and to voltage selector, the
@@ -168,6 +172,8 @@ struct regulator_ops {
 	/* Time taken to enable or set voltage on the regulator */
 	int (*enable_time) (struct regulator_dev *);
 	int (*set_ramp_delay) (struct regulator_dev *, int ramp_delay);
+	int (*set_voltage_time) (struct regulator_dev *,
+				int old_uV, int new_uV);
 	int (*set_voltage_time_sel) (struct regulator_dev *,
 				     unsigned int old_selector,
 				     unsigned int new_selector);
@@ -461,6 +467,8 @@ int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel);
 int regulator_is_enabled_regmap(struct regulator_dev *rdev);
 int regulator_enable_regmap(struct regulator_dev *rdev);
 int regulator_disable_regmap(struct regulator_dev *rdev);
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV);
 int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 				   unsigned int old_selector,
 				   unsigned int new_selector);
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 1/4] regulator: Add set_voltage_time op

[Matthias Kaehlcke]

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

The new op is analogous to set_voltage_time_sel. It can be used by
regulators that don't have a table of discrete voltages. The function
returns the time for the regulator voltage output voltage to stabilize
after being set to a new value, in microseconds. The actual calculation
of the stabilization time is done in the same place for both types of
regulators.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- This patch is new for v4.

 drivers/regulator/core.c         | 140 +++++++++++++++++++++++++--------------
 include/linux/regulator/driver.h |   8 +++
 2 files changed, 97 insertions(+), 51 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index db320e8..b1cef47 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2751,6 +2751,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	int best_val = 0;
 	unsigned int selector;
 	int old_selector = -1;
+	int old_uV = _regulator_get_voltage(rdev);
 
 	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
 
@@ -2800,27 +2801,38 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 		ret = -EINVAL;
 	}
 
-	/* Call set_voltage_time_sel if successfully obtained old_selector */
-	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
-		&& old_selector != selector) {
+	if (ret != 0 || rdev->constraints->ramp_disable)
+		goto no_delay;
 
-		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
-						old_selector, selector);
-		if (delay < 0) {
-			rdev_warn(rdev, "set_voltage_time_sel() failed: %d\n",
-				  delay);
-			delay = 0;
-		}
+	if (rdev->desc->ops->set_voltage_time) {
+		int new_uV = _regulator_get_voltage(rdev);
 
-		/* Insert any necessary delays */
-		if (delay >= 1000) {
-			mdelay(delay / 1000);
-			udelay(delay % 1000);
-		} else if (delay) {
-			udelay(delay);
-		}
+		if (old_uV == new_uV)
+			goto no_delay;
+
+		delay =	rdev->desc->ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (rdev->desc->ops->set_voltage_time_sel) {
+		if (old_selector < 0 || old_selector == selector)
+			goto no_delay;
+
+		delay = rdev->desc->ops->set_voltage_time_sel(
+			rdev, old_selector, selector);
+	}
+
+	if (delay < 0) {
+		rdev_warn(rdev, "failed to get delay: %d\n", delay);
+		delay = 0;
 	}
 
+	/* Insert any necessary delays */
+	if (delay >= 1000) {
+		mdelay(delay / 1000);
+		udelay(delay % 1000);
+	} else if (delay) {
+		udelay(delay);
+	}
+
+no_delay:
 	if (ret == 0 && best_val >= 0) {
 		unsigned long data = best_val;
 
@@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
-	int old_sel = -1;
-	int new_sel = -1;
-	int voltage;
-	int i;
 
-	/* Currently requires operations to do this */
-	if (!ops->list_voltage || !ops->set_voltage_time_sel
-	    || !rdev->desc->n_voltages)
-		return -EINVAL;
+	if (ops->set_voltage_time) {
+		return ops->set_voltage_time(rdev, old_uV, new_uV);
+	} else if (ops->set_voltage_time_sel) {
+		int old_sel = -1;
+		int new_sel = -1;
+		int voltage;
+		int i;
 
-	for (i = 0; i < rdev->desc->n_voltages; i++) {
-		/* We only look for exact voltage matches here */
-		voltage = regulator_list_voltage(regulator, i);
-		if (voltage < 0)
+		/* Currently requires operations to do this */
+		if (!ops->list_voltage || !rdev->desc->n_voltages)
 			return -EINVAL;
-		if (voltage == 0)
-			continue;
-		if (voltage == old_uV)
-			old_sel = i;
-		if (voltage == new_uV)
-			new_sel = i;
-	}
 
-	if (old_sel < 0 || new_sel < 0)
-		return -EINVAL;
+		for (i = 0; i < rdev->desc->n_voltages; i++) {
+			/* We only look for exact voltage matches here */
+			voltage = regulator_list_voltage(regulator, i);
+			if (voltage < 0)
+				return -EINVAL;
+			if (voltage == 0)
+				continue;
+			if (voltage == old_uV)
+				old_sel = i;
+			if (voltage == new_uV)
+				new_sel = i;
+		}
+
+		if (old_sel < 0 || new_sel < 0)
+			return -EINVAL;
+
+		return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	}
 
-	return ops->set_voltage_time_sel(rdev, old_sel, new_sel);
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time);
 
 /**
- * regulator_set_voltage_time_sel - get raise/fall time
- * @rdev: regulator source device
- * @old_selector: selector for starting voltage
- * @new_selector: selector for target voltage
+ * regulator_set_voltage_time_op - get raise/fall time
+ * @regulator: regulator source
+ * @old_uV: starting voltage in microvolts
+ * @new_uV: target voltage in microvolts
  *
- * Provided with the starting and target voltage selectors, this function
- * returns time in microseconds required to rise or fall to this new voltage
+ * Provided with the starting and ending voltage, this function calculates
+ * the time in microseconds required to rise or fall to this new voltage.
  *
  * Drivers providing ramp_delay in regulation_constraints can use this as their
- * set_voltage_time_sel() operation.
+ * set_voltage_time() operation.
  */
-int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
-				   unsigned int old_selector,
-				   unsigned int new_selector)
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV)
 {
 	unsigned int ramp_delay = 0;
-	int old_volt, new_volt;
 
 	if (rdev->constraints->ramp_delay)
 		ramp_delay = rdev->constraints->ramp_delay;
@@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 		return 0;
 	}
 
+	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
+}
+EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);
+
+/**
+ * regulator_set_voltage_time_sel - get raise/fall time
+ * @rdev: regulator source device
+ * @old_selector: selector for starting voltage
+ * @new_selector: selector for target voltage
+ *
+ * Provided with the starting and target voltage selectors, this function
+ * returns time in microseconds required to rise or fall to this new voltage
+ *
+ * Drivers providing ramp_delay in regulation_constraints can use this as their
+ * set_voltage_time_sel() operation.
+ */
+int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
+				   unsigned int old_selector,
+				   unsigned int new_selector)
+{
+	int old_volt, new_volt;
+
 	/* sanity check */
 	if (!rdev->desc->ops->list_voltage)
 		return -EINVAL;
@@ -3059,7 +3097,7 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 	old_volt = rdev->desc->ops->list_voltage(rdev, old_selector);
 	new_volt = rdev->desc->ops->list_voltage(rdev, new_selector);
 
-	return DIV_ROUND_UP(abs(new_volt - old_volt), ramp_delay);
+	return regulator_set_voltage_time_op(rdev, old_volt, new_volt);
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fcfa40a..537ca7f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -113,6 +113,10 @@ struct regulator_linear_range {
  *               stabilise after being enabled, in microseconds.
  * @set_ramp_delay: Set the ramp delay for the regulator. The driver should
  *		select ramp delay equal to or less than(closest) ramp_delay.
+ * @set_voltage_time: Time taken for the regulator voltage output voltage
+ *               to stabilise after being set to a new value, in microseconds.
+ *               The function provides the from and to voltage, the function
+ *               should return the worst case.
  * @set_voltage_time_sel: Time taken for the regulator voltage output voltage
  *               to stabilise after being set to a new value, in microseconds.
  *               The function provides the from and to voltage selector, the
@@ -168,6 +172,8 @@ struct regulator_ops {
 	/* Time taken to enable or set voltage on the regulator */
 	int (*enable_time) (struct regulator_dev *);
 	int (*set_ramp_delay) (struct regulator_dev *, int ramp_delay);
+	int (*set_voltage_time) (struct regulator_dev *,
+				int old_uV, int new_uV);
 	int (*set_voltage_time_sel) (struct regulator_dev *,
 				     unsigned int old_selector,
 				     unsigned int new_selector);
@@ -461,6 +467,8 @@ int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel);
 int regulator_is_enabled_regmap(struct regulator_dev *rdev);
 int regulator_enable_regmap(struct regulator_dev *rdev);
 int regulator_disable_regmap(struct regulator_dev *rdev);
+int regulator_set_voltage_time_op(struct regulator_dev *rdev,
+				int old_uV, int new_uV);
 int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 				   unsigned int old_selector,
 				   unsigned int new_selector);
-- 
2.8.0.rc3.226.g39d4020


[-- Attachment #2: Type: text/html, Size: 730 bytes --]