[PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Afzal Mohammed]

Count of selector voltage is required for regulator_set_voltage
to work via set_voltage_sel. VDD1/2 currently have it as zero,
so regulator_set_voltage won't work for VDD1/2

Update count (n_voltages) for VDD1/2

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/regulator/tps65910-regulator.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index a620e25..81f629f 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -930,6 +930,7 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 
 		if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
 			pmic->desc[i].ops = &tps65910_ops_dcdc;
+			pmic->desc[i].n_voltages = VDD1_2_NUM_VOLTS * 3;
 		} else if (i == TPS65910_REG_VDD3) {
 			if (tps65910_chip_id(tps65910) == TPS65910)
 				pmic->desc[i].ops = &tps65910_ops_vdd3;
-- 
1.6.2.4

Re: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mark Brown]

On Fri, Nov 04, 2011 at 06:18:48PM +0530, Afzal Mohammed wrote:

>  		if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
>  			pmic->desc[i].ops = &tps65910_ops_dcdc;
> +			pmic->desc[i].n_voltages = VDD1_2_NUM_VOLTS * 3;

This looks suspicous - what's the * 3 about?

RE: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mohammed, Afzal]

Hi Brown,

On Fri, Nov 04, 2011 at 19:34:22, Mark Brown wrote:
> On Fri, Nov 04, 2011 at 06:18:48PM +0530, Afzal Mohammed wrote:
> 
> >  		if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
> >  			pmic->desc[i].ops = &tps65910_ops_dcdc;
> > +			pmic->desc[i].n_voltages = VDD1_2_NUM_VOLTS * 3;
> 
> This looks suspicous - what's the * 3 about?
> 

Gain in voltage possible is x1, 0x2 & 0x3, I am shaky
about this change, but voltage could be changed with this,
cc'ing original author's

Specification @ http://www.ti.com/product/tps65910

Regards
Afzal

Re: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mark Brown]

On Fri, Nov 04, 2011 at 02:26:05PM +0000, Mohammed, Afzal wrote:
> Hi Brown,

Ahem.

> On Fri, Nov 04, 2011 at 19:34:22, Mark Brown wrote:
> > On Fri, Nov 04, 2011 at 06:18:48PM +0530, Afzal Mohammed wrote:

> > >  		if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
> > >  			pmic->desc[i].ops = &tps65910_ops_dcdc;
> > > +			pmic->desc[i].n_voltages = VDD1_2_NUM_VOLTS * 3;

> > This looks suspicous - what's the * 3 about?

> Gain in voltage possible is x1, 0x2 & 0x3, I am shaky
> about this change, but voltage could be changed with this,

That doesn't really clarify things - the question is why the number of
voltages we can set is three times a constant called _NUM_VOLTS?

RE: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mohammed, Afzal]

Hi Mark,

On Fri, Nov 04, 2011 at 20:55:59, Mark Brown wrote:
> On Fri, Nov 04, 2011 at 02:26:05PM +0000, Mohammed, Afzal wrote:
> > Hi Brown,
> 
> Ahem.

I am sorry for that if it offended you, not deliberate, this may
have to do with our different cultures, for me it is always a
confusion whether I should call a person with their first or
second name (unless he writes it at the end of mail). Normally my
name is written "Mohammed Afzal", but I am called "Afzal".


> 
> > On Fri, Nov 04, 2011 at 19:34:22, Mark Brown wrote:
> That doesn't really clarify things - the question is why the number of
> voltages we can set is three times a constant called _NUM_VOLTS?
> 

_NUM_VOLTS is the number of voltage steps.

I will try to come up with a better explanation or rather the
right solution, new to regulator world as of now.

Regards
Afzal

Re: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mark Brown]

On Fri, Nov 04, 2011 at 04:01:04PM +0000, Mohammed, Afzal wrote:
> On Fri, Nov 04, 2011 at 20:55:59, Mark Brown wrote:

> > That doesn't really clarify things - the question is why the number of
> > voltages we can set is three times a constant called _NUM_VOLTS?

> _NUM_VOLTS is the number of voltage steps.

> I will try to come up with a better explanation or rather the
> right solution, new to regulator world as of now.

So that definitely seems wrong then - n_voltages is supposed to be the
number of voltages that can be selected so if the regulator supports
_NUM_VOLTS steps then I'd expect to see that constant used directly.
Otherwise I'd suggest that the magic number needs a #define.

RE: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mohammed, Afzal]

Hi Mark,

On Fri, Nov 04, 2011 at 21:48:56, Mark Brown wrote:
> So that definitely seems wrong then - n_voltages is supposed to be the
> number of voltages that can be selected so if the regulator supports
> _NUM_VOLTS steps then I'd expect to see that constant used directly.
> Otherwise I'd suggest that the magic number needs a #define.
> 

A gain of 0x1, 0x2, 0x3 is possible for each of the voltage steps,
so we have a total of _NUM_VOLTS * 3 steps (although some of them
would be same values).

Let me know your opinion on using _NUM_VOLTS * NUM_GAIN instead,
with #define NUM_GAIN 3.

Regards
Afzal

Re: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mark Brown]

On Fri, Nov 04, 2011 at 04:32:43PM +0000, Mohammed, Afzal wrote:
> On Fri, Nov 04, 2011 at 21:48:56, Mark Brown wrote:
> > So that definitely seems wrong then - n_voltages is supposed to be the
> > number of voltages that can be selected so if the regulator supports
> > _NUM_VOLTS steps then I'd expect to see that constant used directly.
> > Otherwise I'd suggest that the magic number needs a #define.

> A gain of 0x1, 0x2, 0x3 is possible for each of the voltage steps,
> so we have a total of _NUM_VOLTS * 3 steps (although some of them
> would be same values).

What do you mean when you say "gain"?

RE: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mohammed, Afzal]

Hi Mark,

On Fri, Nov 04, 2011 at 22:10:09, Mark Brown wrote:
> 
> What do you mean when you say "gain"?
> 

Effective voltage expression is (value1 * 12.5mV + 562.5 mV) * value2.

In this value2 is being called as gain.

value1 can have values from 3 to 75, both inclusive (73 steps)
value2 can have from 1 to 3, both inclusive (3 numbers)

Regards
Afzal

Re: [PATCH 2/2] regulator:TPS65910: VDD1/2 voltage selector count

[Mark Brown]

On Fri, Nov 04, 2011 at 04:48:09PM +0000, Mohammed, Afzal wrote:

> Effective voltage expression is (value1 * 12.5mV + 562.5 mV) * value2.

> In this value2 is being called as gain.

> value1 can have values from 3 to 75, both inclusive (73 steps)
> value2 can have from 1 to 3, both inclusive (3 numbers)

Right, that makes sense.  It looks like you need a new constant for the
gain and the existing constant for value1 should be renamed to make it
more obvious that it's not the only part of the selector, the current
constant is confusingly named given the above.