regulator: Add set_voltage_time op

drivers/regulator/core.c +1 -8

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

drivers/regulator/core.c +2 -1

+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;

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.

drivers/regulator/core.c static int _regulator_do_set_voltage(struct regulator_dev *rdev, · line 2805

Quoted 5 lines

drivers/regulator/core.c +2 -3

-	/* 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.

drivers/regulator/core.c static int _regulator_do_set_voltage(struct regulator_dev *rdev, · line 2819

Quoted 5 lines

drivers/regulator/core.c +2 -2

-		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...

drivers/regulator/core.c static int _regulator_do_set_voltage(struct regulator_dev *rdev, · line 2835

Quoted 9 lines

drivers/regulator/core.c +9 -0

+	/* 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?

Quoted 21 lines

drivers/regulator/core.c +7 -8

@@ -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;
 

drivers/regulator/core.c int regulator_set_voltage_time(struct regulator *regulator, · line 3008

Quoted 3 lines

drivers/regulator/core.c +0 -3

-	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.

Quoted 12 lines

drivers/regulator/core.c +2 -1

+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;
 	}
 

drivers/regulator/core.c int regulator_set_voltage_time_sel(struct regulator_dev *rdev, · line 3073

Quoted 3 lines

drivers/regulator/core.c +3 -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.

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

Quoted 10 lines · Matthias Kaehlcke

drivers/regulator/core.c static int _regulator_do_set_voltage(struct regulator_dev *rdev, · line 2805

Quoted 6 lines

drivers/regulator/core.c +2 -3

-	/* 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?

Quoted 7 lines

Quoted 7 lines

drivers/regulator/core.c +7 -0

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

Quoted 3 lines

Quoted 1 line

+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, ...?

Quoted 4 lines

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(..);
}

Quoted 11 lines

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

Quoted 10 lines · Matthias Kaehlcke

drivers/regulator/core.c static int _regulator_do_set_voltage(struct regulator_dev *rdev, · line 2805

Quoted 6 lines

drivers/regulator/core.c +2 -3

-	/* 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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

drivers/regulator/core.c +7 -0

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

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

+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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

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

Quoted 3 lines · Matthias Kaehlcke

Quoted 2 lines

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

Quoted 1 line · Matthias Kaehlcke

Quoted 11 lines

Quoted 5 lines

Quoted 1 line

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.