Re: [PATCH V2] regulator: 88pm800: add regulator driver for 88pm800

[Mark Brown <broonie@kernel.org>]

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

On Mon, Jun 24, 2013 at 10:01:39AM +0800, Chao Xie wrote:
> On Fri, Jun 21, 2013 at 11:24 PM, Mark Brown <broonie@kernel.org> wrote:

> > Just provide get_voltage_sel(), the core will do the mapping to voltages
> > using list_voltage().

> I am a little confused.
> The BUCK voltage is not linear, and it contains a lot of voltages.
> If we use map_voltage_ascend, it will looking a a suitable voltage
> from begin to end one by one.

What does this have to do with get_voltage()?  And note that you can
write your own mapping function for a reason...

> If we directly make use of set_voltage and get_voltage, we can
> directly calculates the voltage which is
> suitable, and do not go through all the voltags.
> for example, now BUCK voltage table is
> range 1 from 600000 to 1587500, each step is 12500
> range 2 from 1600000 to 1800000, each step is 50000

No, this is nothing at all to do with using the selector versions of the
API.  Think about what the API is doing and take a look at the code.

> >> +     } else if (pdata->num_regulators) {
> >> +             /* Check whether num_regulator is valid. */
> >> +             unsigned int count = 0;
> >> +             for (i = 0; pdata->regulators[i]; i++)
> >> +                     count++;
> >> +             if (count != pdata->num_regulators)
> >> +                     return -EINVAL;

> > This looks...  odd.

> It is just make sure that pdata has correct number of regulators.
> It you think that it is redundant, i can remove it.

If you really need to have platform data for all the regulators then
just embed the array inside the platform data so there's no possibility
of any confusion.

> > With deferred probing you should just be able to use
> > module_platform_driver().

> The regulator controlles some BUCK regulators.
> These regulators may be used by application CPU or CP(communication
> CPU) for telephony.
> The CP may need different voltages if it goes deep initialization. if
> we defer the setting later, it is too late for
> CP initialization, and will impact the performance.

If your kernel startup is taking long enough for this to be an issue it
seems like there's much bigger problems here and things are going to be
very fragile anyway, it's going to be better to figure out what the root
issue is.

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