Re: [PATCH] regulator: vexpress: Add missing n_voltages setting
On Tue, 2012-12-11 at 23:39 +, 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= 5
> init_data->constraints.max_uV=6
> What happen if vexpress_regulator_set_voltage is called with
> min_uV=8, max_uV=10?
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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: vexpress: Add missing n_voltages setting
On Tue, 2012-12-11 at 23:39 +, 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= 5
init_data-constraints.max_uV=6
What happen if vexpress_regulator_set_voltage is called with
min_uV=8, max_uV=10?
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ł
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: vexpress: Add missing n_voltages setting
On Tue, 2012-12-11 at 08:04 +, Axel Lin wrote: > Otherwise regulator_can_change_voltage() return 0 for this driver. > > Signed-off-by: Axel Lin 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 Date: Tue, 11 Dec 2012 13:44:07 + 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: vexpress: Add missing n_voltages setting
Otherwise regulator_can_change_voltage() return 0 for this driver.
Signed-off-by: Axel Lin
---
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 = _regulator_ops;
- else
+ reg->desc.n_voltages = init_data->constraints.max_uV -
+ init_data->constraints.min_uV + 1;
+ } else {
reg->desc.ops = _regulator_ops_ro;
+ reg->desc.n_voltages = 1;
+ }
config.dev = >dev;
config.init_data = init_data;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: vexpress: Add missing n_voltages setting
Otherwise regulator_can_change_voltage() return 0 for this driver.
Signed-off-by: Axel Lin axel@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: vexpress: Add missing n_voltages setting
On Tue, 2012-12-11 at 08:04 +, Axel Lin wrote: Otherwise regulator_can_change_voltage() return 0 for this driver. Signed-off-by: Axel Lin axel@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.m...@arm.com Date: Tue, 11 Dec 2012 13:44:07 + 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.m...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
