[v2,04/13] regulators: Versatile Express regulator driver

Commit Message

Pawel Moll Sept. 19, 2012, 4:58 p.m. UTC

On Wed, 2012-09-19 at 03:21 +0100, Mark Brown wrote:
> No, I mean discovering which regulators are present and what they can
> do.

Nope. This driver is supposed to work only on Device Tree "enabled"
platforms. Having said that, I should have added the bindings
documentation in the patch. Will do.

> > > So this is going to break interoperation with a bunch of consumer
> > > drivers that rely on being able to tell what voltages are supported.
> > > The key thing for them would be that regulator_is_supported_voltage()
> > > works, currently it relies on list_voltage() as that's the only way to
> > > do that right now.
> 
> > Ok, I guess I should use regulator_list_voltage_linear() and
> > regulator_map_voltage_linear() then? I'll just have to carefully think
> > what step to choose.
> 
> No, we should provide a way to describe this situation in the API - it's
> not unreasonable and having to pick step sizes is obviously suboptimal.

Actually before I finally got this mail (our mail system was playing
stupid today), I came up with idea of using the power supply specified
tolerance as a base to chose the step sizes. This comes down to such
code (with the regulator_*_voltage_linear in the ops):

8<---
                int range = init_data->constraints.max_uV -
                                init_data->constraints.min_uV;
                u32 tolerance;
                int avg_error;

                err = of_property_read_u32(pdev->dev.of_node,
                                "arm,vexpress-volt,tolerance", &tolerance);
                if (err)
                        goto error_property_read;

                reg->desc.min_uV = init_data->constraints.min_uV;
                avg_error = (range / 2 + reg->desc.min_uV) * tolerance / 100;
                reg->desc.n_voltages = range / avg_error + 1;
                reg->desc.uV_step = range / (reg->desc.n_voltages - 1);
8<---

so it doesn't look that bad to me. Now, if you are considering changing
the API, I would propose something like this:

8<---
8<---

I originally assumed that if I provide no operating points (in the form
of list_voltage function) any voltage within the constraints range will
be allowed.

Both options are perfectly fine with me.

Thanks!

Pawel

Comments

Mark Brown Sept. 20, 2012, 1:01 p.m. UTC | #1

On Wed, Sep 19, 2012 at 05:58:25PM +0100, Pawel Moll wrote:
> On Wed, 2012-09-19 at 03:21 +0100, Mark Brown wrote:

> > No, we should provide a way to describe this situation in the API - it's
> > not unreasonable and having to pick step sizes is obviously suboptimal.

> Actually before I finally got this mail (our mail system was playing
> stupid today), I came up with idea of using the power supply specified
> tolerance as a base to chose the step sizes. This comes down to such
> code (with the regulator_*_voltage_linear in the ops):

That's probably OK, though check that the numbers come out sensible
(people tend to work to nice round numbers).

> +       if (ret < 0) {
> +               /* No operating points defined, allow any value within range */
> +               struct regulation_constraints *constraints =
> +                               regulator->rdev->constraints;
> +
> +               return min_uV >= constraints->min_uV &&
> +                               max_uV <= constraints->max_uV;
> +       }

I'd rather have the driver explicitly say it supports continuous
voltages with a flag in the descriptor (to make sure we don't end up
being overly optimistic because the driver wasn't well implemented) but
other than that this looks good.

Pawel Moll Sept. 20, 2012, 5:34 p.m. UTC | #2

On Thu, 2012-09-20 at 14:01 +0100, Mark Brown wrote:
> > Actually before I finally got this mail (our mail system was playing
> > stupid today), I came up with idea of using the power supply specified
> > tolerance as a base to chose the step sizes. This comes down to such
> > code (with the regulator_*_voltage_linear in the ops):
> 
> That's probably OK, though check that the numbers come out sensible
> (people tend to work to nice round numbers).

These calculations try to maximize the range, but in most cases it's
impossible to be exactly in line with constraints. The delta should be
less then 1%, eg. in my test case I get:

A15 Vcore: override max_uV, 1050000 -> 1049990
A15 Vcore: 800 <--> 1049 mV at 906 mV 

I could try to add more math to interpolate the operating points to get
perfect match, but this sounds like a serious overkill...

> > +       if (ret < 0) {
> > +               /* No operating points defined, allow any value within range */
> > +               struct regulation_constraints *constraints =
> > +                               regulator->rdev->constraints;
> > +
> > +               return min_uV >= constraints->min_uV &&
> > +                               max_uV <= constraints->max_uV;
> > +       }
> 
> I'd rather have the driver explicitly say it supports continuous
> voltages with a flag in the descriptor (to make sure we don't end up
> being overly optimistic because the driver wasn't well implemented) 

Ok, so I see two options:

1. Something like bool "regulator_desc.linear"

2. A magic value for regulator_desc.n_voltages, something like
	#define N_VOLTAGES_LINEAR (~0)

Does any of them seem reasonable?

Pawel

Mark Brown Sept. 20, 2012, 6:15 p.m. UTC | #3

On Thu, Sep 20, 2012 at 06:34:32PM +0100, Pawel Moll wrote:

> These calculations try to maximize the range, but in most cases it's
> impossible to be exactly in line with constraints. The delta should be
> less then 1%, eg. in my test case I get:

No, what I mean is that we don't want steps of 343uV or something.

> 1. Something like bool "regulator_desc.linear"

> 2. A magic value for regulator_desc.n_voltages, something like
> 	#define N_VOLTAGES_LINEAR (~0)

> Does any of them seem reasonable?

The former of them seems much more tasteful, though I'd go with
continuous_volt or something, we already have linear maps.

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4838531..a091719 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1948,8 +1948,14 @@  int regulator_is_supported_voltage(struct regulator *regulator,
        }
 
        ret = regulator_count_voltages(regulator);
-       if (ret < 0)
-               return ret;
+       if (ret < 0) {
+               /* No operating points defined, allow any value within range */
+               struct regulation_constraints *constraints =
+                               regulator->rdev->constraints;
+
+               return min_uV >= constraints->min_uV &&
+                               max_uV <= constraints->max_uV;
+       }
        voltages = ret;
 
        for (i = 0; i < voltages; i++) {