DVS regulator drivers

DVS regulator drivers

[Guennadi Liakhovetski]

As I mentioned in an earlier mail today [1] I have a difficulty seeing how 
the current regulator API can efficiently be used for DVS-type regulators. 
Trying to look at existing mainline drivers it seems to me they try to 
guess, what the user really wants, do that differently and don't manage to 
do that in a bug-free way. Specifically, I looked at 2 drivers:

wm831x-dcdc.c handles 2 voltages: "DVS" and "ON." If the new voltage in 
.set_voltage_sel() is equal to one of them, it is just used. If it is a 
new voltage, there's a comment in the driver in 
wm831x_buckv_set_voltage_sel():

	/* Always set the ON status to the minimum voltage */

but I actually don't see, where the minimum is selected. It seems instead 
in this case the "ON" value is just set:

	ret = wm831x_set_bits(wm831x, on_reg, WM831X_DC1_ON_VSEL_MASK, vsel);
	if (ret < 0)
		return ret;
	dcdc->on_vsel = vsel;

Then it goes on to actually switch the output voltage to this new "ON" 
value and then:

	/*
	 * If this VSEL is higher than the last one we've seen then
	 * remember it as the DVS VSEL.  This is optimised for CPUfreq
	 * usage where we want to get to the highest voltage very
	 * quickly.
	 */
	if (vsel > dcdc->dvs_vsel) {
		ret = wm831x_set_bits(wm831x, dvs_reg,
				      WM831X_DC1_DVS_VSEL_MASK,
				      dcdc->dvs_vsel);

Above the _old_ DVS value is set again?

		if (ret == 0)
			dcdc->dvs_vsel = vsel;

Now .dvs_vsel is set - after the voltage has been set to the old value. 
And now both - ON and DVS values are equal to the same value - vsel?... 
Confused.

lp872x.c seems to be selecting between ON and DVS (or V1 and V2 in lp872x' 
terminology) in lp872x_set_dvs(), which is called every time a voltage is 
set. But this function always gets the same arguments - supplied from the 
platform data and therefore only one voltage is ever used... The two 
"dynamic" DVS selection fields in struct lp872x: .dvs_pin and .dvs_gpio 
seem just to always stay constant, the latter is also just initialised 
once and is never used again.

Again, I don't have those devices, so I cannot test and would be a bit 
hesitant to provide patches, but that's my impression from just looking at 
them.

Thanks
Guennadi

[1] https://lkml.org/lkml/2012/11/19/169
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: DVS regulator drivers

[Mark Brown]

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

On Mon, Nov 19, 2012 at 12:52:09PM +0100, Guennadi Liakhovetski wrote:
> As I mentioned in an earlier mail today [1] I have a difficulty seeing how 
> the current regulator API can efficiently be used for DVS-type regulators. 

Please don't invent terminology or repurpose existing terminology like
this, it's just confusing - obvious essentially all regulators covered
by the regulator API support voltage scaling which is the usual meaning
of DVS.

> wm831x-dcdc.c handles 2 voltages: "DVS" and "ON." If the new voltage in 
> .set_voltage_sel() is equal to one of them, it is just used. If it is a 
> new voltage, there's a comment in the driver in 
> wm831x_buckv_set_voltage_sel():

> 	/* Always set the ON status to the minimum voltage */

> but I actually don't see, where the minimum is selected. It seems instead 
> in this case the "ON" value is just set:

> 	ret = wm831x_set_bits(wm831x, on_reg, WM831X_DC1_ON_VSEL_MASK, vsel);
> 	if (ret < 0)
> 		return ret;
> 	dcdc->on_vsel = vsel;

Can you be more specific about your concern here?  The above code does
exactly what the comment says, it will set the selector it just picked.

You did spot one bug (I think due to bitrot) which I just fixed but in
general I've just TLDRed this as it's a bit unclear what you're trying
to say here, can you be a bit more concise here?  I'm not sure if
there's a general point or if it's specific code issues?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: DVS regulator drivers

[Guennadi Liakhovetski]

Hi Mark

Thanks for your reply.

On Tue, 20 Nov 2012, Mark Brown wrote:

> On Mon, Nov 19, 2012 at 12:52:09PM +0100, Guennadi Liakhovetski wrote:
> > As I mentioned in an earlier mail today [1] I have a difficulty seeing how 
> > the current regulator API can efficiently be used for DVS-type regulators. 
> 
> Please don't invent terminology or repurpose existing terminology like
> this, it's just confusing - obvious essentially all regulators covered
> by the regulator API support voltage scaling which is the usual meaning
> of DVS.

Right, sorry, a more precise term would be a "pin-selectable DVS," right?

> > wm831x-dcdc.c handles 2 voltages: "DVS" and "ON." If the new voltage in 
> > .set_voltage_sel() is equal to one of them, it is just used. If it is a 
> > new voltage, there's a comment in the driver in 
> > wm831x_buckv_set_voltage_sel():
> 
> > 	/* Always set the ON status to the minimum voltage */
> 
> > but I actually don't see, where the minimum is selected. It seems instead 
> > in this case the "ON" value is just set:
> 
> > 	ret = wm831x_set_bits(wm831x, on_reg, WM831X_DC1_ON_VSEL_MASK, vsel);
> > 	if (ret < 0)
> > 		return ret;
> > 	dcdc->on_vsel = vsel;
> 
> Can you be more specific about your concern here?  The above code does
> exactly what the comment says, it will set the selector it just picked.

Your patch fixes exactly the problem, that I was pointing at, thanks.

> You did spot one bug (I think due to bitrot) which I just fixed but in
> general I've just TLDRed this as it's a bit unclear what you're trying
> to say here, can you be a bit more concise here?  I'm not sure if
> there's a general point or if it's specific code issues?

Sorry, let's try again. Just to bring back the "too long and a bit unclear 
part:"

> > lp872x.c seems to be selecting between ON and DVS (or V1 and V2 in lp872x' 
> > terminology) in lp872x_set_dvs(), which is called every time a voltage is 
> > set. But this function always gets the same arguments - supplied from the 
> > platform data and therefore only one voltage is ever used... The two 
> > "dynamic" DVS selection fields in struct lp872x: .dvs_pin and .dvs_gpio 
> > seem just to always stay constant, the latter is also just initialised 
> > once and is never used again.

Ok, what I meant is the following:

1. some fields of struct lp872x are superfluous / unused, e.g. .dvs_gpio 
is only set once and never used again, .dvs_pin seems to be used, but I 
think it's never changed either, because:

2. .dvs_pin is initialised in lp872x_init_dvs() from platform data:

	pinstate = dvs->init_state;
	...
	lp->dvs_pin = pinstate;

There's one more location in the code, where .dvs_pin gets assigned: in 
lp872x_set_dvs():

	lp->dvs_pin = state;

but, I think, it will always be set to one and the same value: "state" is 
calculated as

	state = dvs_sel == SEL_V1 ? DVS_HIGH : DVS_LOW;

where dvs_sel is a parameter of this function. The function is called only 
from one location in lp872x_buck_set_voltage_sel() as

		lp872x_set_dvs(lp, dvs->vsel, dvs->gpio);

where dvs is platform data, i.e., all parameters are constant. So, 
lp872x_set_dvs() never switches anything, and in fact it could just have 
been called just once from initialisation to set the GPIO. This also 
means, that only one context - either SEL_V1 or SEL_V2 is used.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: DVS regulator drivers

[Mark Brown]

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

On Tue, Nov 20, 2012 at 09:17:46AM +0100, Guennadi Liakhovetski wrote:
> On Tue, 20 Nov 2012, Mark Brown wrote:

> > Please don't invent terminology or repurpose existing terminology like
> > this, it's just confusing - obvious essentially all regulators covered
> > by the regulator API support voltage scaling which is the usual meaning
> > of DVS.

> Right, sorry, a more precise term would be a "pin-selectable DVS," right?

Yup.

> > Can you be more specific about your concern here?  The above code does
> > exactly what the comment says, it will set the selector it just picked.

> Your patch fixes exactly the problem, that I was pointing at, thanks.

Oh, right.  It sounded like you also had some concern with on_vsel which
I couldn't figure out.

> > You did spot one bug (I think due to bitrot) which I just fixed but in
> > general I've just TLDRed this as it's a bit unclear what you're trying
> > to say here, can you be a bit more concise here?  I'm not sure if
> > there's a general point or if it's specific code issues?

> Sorry, let's try again. Just to bring back the "too long and a bit unclear 
> part:"

It was the mail as a whole - I spent less time on lp872x since I don't
have any access to the hardware either.

> where dvs is platform data, i.e., all parameters are constant. So, 
> lp872x_set_dvs() never switches anything, and in fact it could just have 
> been called just once from initialisation to set the GPIO. This also 
> means, that only one context - either SEL_V1 or SEL_V2 is used.

I think you're right there.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: DVS regulator drivers

[Guennadi Liakhovetski]

Sorry, one more point, that I raised in the original mail yesterday and 
that I forgot about, when replying today:

On Tue, 20 Nov 2012, Mark Brown wrote:

> On Mon, Nov 19, 2012 at 12:52:09PM +0100, Guennadi Liakhovetski wrote:

[snip]

> > 	/* Always set the ON status to the minimum voltage */
> > 
> > but I actually don't see, where the minimum is selected. It seems instead 
> > in this case the "ON" value is just set:

In other words, I don't see where voltages are compared to select the 
minimum to be used for .on_vsel. Instead, it seems, .on_vsel is always set 
to the new value, and, if it is also higher then the old .dvs_vsel value, 
.dvs_vsel is _also_ set to the new voltage, in which case they become 
equal. Is this the intended behaviour?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: DVS regulator drivers

[Mark Brown]

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

On Tue, Nov 20, 2012 at 11:02:57AM +0100, Guennadi Liakhovetski wrote:
> On Tue, 20 Nov 2012, Mark Brown wrote:

> > On Mon, Nov 19, 2012 at 12:52:09PM +0100, Guennadi Liakhovetski wrote:

> > > 	/* Always set the ON status to the minimum voltage */

> > > but I actually don't see, where the minimum is selected. It seems instead 
> > > in this case the "ON" value is just set:

This is where I was asking you to clarify what you were trying to say :/

> In other words, I don't see where voltages are compared to select the 
> minimum to be used for .on_vsel. Instead, it seems, .on_vsel is always set 
> to the new value, and, if it is also higher then the old .dvs_vsel value, 
> .dvs_vsel is _also_ set to the new voltage, in which case they become 
> equal. Is this the intended behaviour?

There's no need to do any comparison because we only ever get one
selector value, there's nothing to compare it against.  We're setting it
to the dvs_vsel since normal practice is to keep the upper end of the
voltage range constant.  This isn't going to do much good if they but
anyone who actually has that use can worry about it - it's rare.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]