Skip to contentSkip to conversation

regulator: vexpress: Add missing n_voltages setting

PATCH In review Axel Lin 2 participants 3 messages Dec 11, 2012 – Dec 12, 2012 1 message not in the archive

Subsystems (2)
Voltage and current regulator framework Arm/versatile express platform
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 regulator: vexpress: Add missing n_voltages setting

Otherwise regulator_can_change_voltage() return 0 for this driver.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/regulator/vexpress.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/regulator/vexpress.c +6 -2
index 4668c7f..9bb3aa0 100644
@@ -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;
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 3 lines · Axel Lin

Otherwise regulator_can_change_voltage() return 0 for this driver.

Signed-off-by Axel Lin <[email protected]>

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<--------------------

Quoted 1 line

From 1cafb644747c276a6c601096b8dc0972d10daac7 Mon Sep 17 00:00:00 2001

From: Pawel Moll <[email protected]>
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 <[email protected]>
---
 drivers/regulator/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/regulator/core.c +2 -1
index cd1b201..92768c3 100644
@@ -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;
Signature · 1 line
1.7.10.4

A message that is not in the archive (sent off-list or not yet received): CAFRkauBScqEMh3y1FcBuYO0dSM1yQV6OOG4Z5HQ8Ueq_Mmm9UA@mail.gmail.com

#3 lore raw
Quoted 23 lines · Axel Lin

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

drivers/regulator/core.c +8 -3
@@ -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;
Quoted 1 line

}

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:

drivers/regulator/core.c +1 -2
index cd1b201..38fe3a2 100644
@@ -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().

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 6 lines

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.

Quoted 2 lines

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ł