> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Thursday, February 18, 2010 4:11 PM
> To: Nayak, Rajendra
> Cc: [email protected]; Liam Girdwood; Samuel Ortiz
> Subject: Re: [PATCH 1/2] twl6030: regulator: Fix vsel
> calculations in set/get voltage apis
>
> On Thu, Feb 18, 2010 at 03:49:29PM +0530, Nayak, Rajendra wrote:
>
> > > The TWL6030 case should be doing something more like
> other regulator
> > > drivers with straightforward mappings between selector and
> > > voltage (most
> > > of them) and not using the TWL4030 value table.
>
> > The code is'nt looking into twl4030 tables for finding the valid
> > voltage for twl6030. There are seperate tables for twl4030
> and twl6030
> > regulators with all the valid supported voltages.
>
> > The difference is, in case of twl4030 once you find a valid voltage
> > from the right table you could just use the table index to program
> > the register on twl4030.
> > In case of twl6030, once you find a valid voltage, again
> from the right
> > table (meant for twl6030) you still need to derive
> > what value needs to be programmed in the register, so
> twl6030 understands
> > it. Using the table index does not help.
>
> In that case the tables for TWL6030 are not ideal as-is and should
> either be fixed somehow, have a different function accessing
> them which
> makes their meaning clear or removed and replaced with just
> the formula.
>
> I know what the code is trying to do, it just seems to be trying to do
> it at the wrong abstraction level and so looks buggy on code
> inspection.
> My point here is that the code looks wrong since it's iterating over a
> table giving voltage to selector mappings but then ignoring
> the selector
> value it comes up with and instead using a formula which at
> least gives
> way more options than most of the selector tables do.
Mark, I get your point. The table values were derived from what the spec
suggests, but I agree that its confusing that in theory the formula
suggests there could be more selectors than whats present in the table.
I will check if its indeed possible to support all possible selectors
between say a min/max range. And then have maybe seperate kind of tables
for twl6030 which only specify min/max range and then use the formula to
generate the right selector.
> It's
> readability
> that's my focus here, the code possibly does do exactly what
> it ought to
> but it looks wrong.
>
> > In case of TWL6030
> > static const u16 VAUX1_6030_VSEL_table[] = {
> > 1000, 1300, 1800, 2500,
> > 2800, 2900, 3000, 3000,
> > };
>
> > if the request is to set 2800mv, look up the table,
> > once found calulate the value to be programmed
> > value = (2800 - 1000)/100 + 1 = 0x13
>
> Are these really the only supported values (obviously you can
> set other
> selectors)? Note also that the name says "_VSEL_table" which is
> definitely no longer true, this isn't helping clarity at all.
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
