[PATCH] regulator: vexpress: Add missing n_voltages setting

[PATCH] regulator: vexpress: Add missing n_voltages setting

[Axel Lin]

Otherwise regulator_can_change_voltage() return 0 for this driver.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/vexpress.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/vexpress.c b/drivers/regulator/vexpress.c
index 4668c7f..9bb3aa0 100644
--- a/drivers/regulator/vexpress.c
+++ b/drivers/regulator/vexpress.c
@@ -86,10 +86,14 @@ static int vexpress_regulator_probe(struct platform_device *pdev)
 	}
 
 	init_data->constraints.apply_uV = 0;
-	if (init_data->constraints.min_uV && init_data->constraints.max_uV)
+	if (init_data->constraints.min_uV && init_data->constraints.max_uV) {
 		reg->desc.ops = &vexpress_regulator_ops;
-	else
+		reg->desc.n_voltages = init_data->constraints.max_uV -
+				       init_data->constraints.min_uV + 1;
+	} else {
 		reg->desc.ops = &vexpress_regulator_ops_ro;
+		reg->desc.n_voltages = 1;
+	}
 
 	config.dev = &pdev->dev;
 	config.init_data = init_data;
-- 
1.7.9.5

Re: [PATCH] regulator: vexpress: Add missing n_voltages setting

[Pawel Moll]

On Tue, 2012-12-11 at 08:04 +0000, Axel Lin wrote:
> Otherwise regulator_can_change_voltage() return 0 for this driver.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

We've been here before, haven't we? ;-) So I'll just repeat myself -
this regulator does _not_ have operating points. What I believe should
be fixed is the mentioned function itself, something like the patch
below (untested)...

Pawel

8<--------------------
>From 1cafb644747c276a6c601096b8dc0972d10daac7 Mon Sep 17 00:00:00 2001
From: Pawel Moll <pawel.moll@arm.com>
Date: Tue, 11 Dec 2012 13:44:07 +0000
Subject: [PATCH] regulator: core: Fix regulator_can_change_voltage() for
 continuous case

Function regulator_can_change_voltage() didn't take regulators with
continuous voltage range under consideration. Fixed now.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/regulator/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd1b201..92768c3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1886,7 +1886,8 @@ int regulator_can_change_voltage(struct regulator *regulator)
 
 	if (rdev->constraints &&
 	    rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE &&
-	    rdev->desc->n_voltages > 1)
+	    (rdev->desc->n_voltages > 1 ||
+	     rdev->desc->continuous_voltage_range))
 		return 1;
 
 	return 0;
-- 
1.7.10.4

Re: [PATCH] regulator: vexpress: Add missing n_voltages setting

[Pawel Moll]

On Tue, 2012-12-11 at 23:39 +0000, Axel Lin wrote:
> I was thinking below patch to fix the issue:
>  diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index cd1b201..891bc96 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1885,9 +1885,14 @@ int regulator_can_change_voltage(struct
> regulator *regulator)
>         struct regulator_dev    *rdev = regulator->rdev;
>  
>         if (rdev->constraints &&
> -           rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE &&
> -           rdev->desc->n_voltages > 1)
> -               return 1;
> +           rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) {
> +               if (rdev->desc->n_voltages > 1)
> +                       return 1;
> +               if (rdev->desc->continuous_voltage_range &&
> +                   rdev->constraints->min_uV && rdev->constraints->max_uV)
> +                       return 1;
> +
> +       }
>  
>         return 0;
>  }

I wouldn't say so, although of course it's not my call. To my ming the
(valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) should really be the only
condition here. I'd even risk saying that checking n_voltages > 1 or
continuous_voltage_range is a bit over the top. So maybe the correct
thing to do would be:

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd1b201..38fe3a2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1885,8 +1885,7 @@ int regulator_can_change_voltage(struct regulator *regulator)
 	struct regulator_dev	*rdev = regulator->rdev;
 
 	if (rdev->constraints &&
-	    rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE &&
-	    rdev->desc->n_voltages > 1)
+	    rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)
 		return 1;
 
 	return 0;

And before you ask - I initialize regulator data from the device tree,
so I get all constraints and valid_ops_mask set by
of_get_regulator_init_data() and of_get_regulation_constraints().

> But then I think if the core relies on n_voltages settings, why not
> set n_voltages in the driver
> even if a driver has continuous_voltage_range set.

I'm not sure that you can say that "the core relies on n_voltages". This
is probably a question for Mark, but to my mind it's one of the possible
cases.
> 
> Maybe I'm still not full understand about continuous_voltage_range,
> A driver with continuous_voltage_range looks special to me:
> 1. regulator_count_voltages() always return 0
> 2. regulator_list_voltage() returns -EINVAL. ( Does it make sense to
> not implement list_voltage ? )

Because it doesn't have "discreet" operating points. The count/list
voltage interface is supposed to represent a regulator that can be set
to (for example) 1V, 2V, 3V, 4V or 5V. "My" regulator (example again)
can be set to any value between 1V and 5V, for example 2.3456. Why would
you like to enumerate all thousands of possible values between 1 and 5?

> 3. vexpress_regulator_set_voltage() looks does not have voltage range
> checking:
>     Given init_data->constraints.min_uV= 50000
> init_data->constraints.max_uV=60000
>     What happen if vexpress_regulator_set_voltage is called with
> min_uV=80000, max_uV=100000?

The core takes care of that - have a look at regulator_set_voltage()
(hint: regulator_check_voltage ;-). The driver can assume that it will
get values within the constraints.

> 4. It's unclear to me if only one of
> init_data->constraints->min_uV/init_data->constraints->max_uV is set.

Again, from my point of view it's the core's problem. I don't think it's
a legal case though.

Paweł