[PATCH] regulator: lochnagar: Fix n_voltages setting for VDDCORE

[PATCH] regulator: lochnagar: Fix n_voltages setting for VDDCORE

[Axel Lin]

With current n_voltages setting, regulator_list_voltage will return
-EINVAL when selector >=57. The highest selector is 0x41, so the
n_voltages should be 0x41+1, i.e. 66.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/lochnagar-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/lochnagar-regulator.c b/drivers/regulator/lochnagar-regulator.c
index 4962d13659aa..ff97cc50f2eb 100644
--- a/drivers/regulator/lochnagar-regulator.c
+++ b/drivers/regulator/lochnagar-regulator.c
@@ -194,7 +194,7 @@ static const struct regulator_desc lochnagar_regulators[] = {
 		.name = "VDDCORE",
 		.supply_name = "SYSVDD",
 		.type = REGULATOR_VOLTAGE,
-		.n_voltages = 57,
+		.n_voltages = 66,
 		.ops = &lochnagar_vddcore_ops,
 
 		.id = LOCHNAGAR_VDDCORE,
-- 
2.17.1

Re: [PATCH] regulator: lochnagar: Fix n_voltages setting for VDDCORE

[Charles Keepax]

On Tue, Jan 29, 2019 at 11:01:56AM +0800, Axel Lin wrote:
> With current n_voltages setting, regulator_list_voltage will return
> -EINVAL when selector >=57. The highest selector is 0x41, so the
> n_voltages should be 0x41+1, i.e. 66.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/regulator/lochnagar-regulator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/lochnagar-regulator.c b/drivers/regulator/lochnagar-regulator.c
> index 4962d13659aa..ff97cc50f2eb 100644
> --- a/drivers/regulator/lochnagar-regulator.c
> +++ b/drivers/regulator/lochnagar-regulator.c
> @@ -194,7 +194,7 @@ static const struct regulator_desc lochnagar_regulators[] = {
>  		.name = "VDDCORE",
>  		.supply_name = "SYSVDD",
>  		.type = REGULATOR_VOLTAGE,
> -		.n_voltages = 57,
> +		.n_voltages = 66,
>  		.ops = &lochnagar_vddcore_ops,
>  
>  		.id = LOCHNAGAR_VDDCORE,
> -- 
> 2.17.1

Hmm... I think this might need a little more investigation, it
does indeed fail at present (amazing that it works upto exactly 1.2V
which is common the usecase) but this feels more like a core
problem. There are indeed only 57 valid voltage settings as the
range is 0x8 - 0x41, but it seems that presently the core treats
this less as n_voltages and more like max_selector. Should we
perhaps be teaching the core to understand the min_selector
instead?

Thanks,
Charles

Re: [PATCH] regulator: lochnagar: Fix n_voltages setting for VDDCORE

[Mark Brown]

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

On Tue, Jan 29, 2019 at 09:54:38AM +0000, Charles Keepax wrote:

> Hmm... I think this might need a little more investigation, it
> does indeed fail at present (amazing that it works upto exactly 1.2V
> which is common the usecase) but this feels more like a core
> problem. There are indeed only 57 valid voltage settings as the
> range is 0x8 - 0x41, but it seems that presently the core treats
> this less as n_voltages and more like max_selector. Should we
> perhaps be teaching the core to understand the min_selector
> instead?

We've not had a need for this before but that seems like a reasonable
addition.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: lochnagar: Fix n_voltages setting for VDDCORE

[Charles Keepax]

On Tue, Jan 29, 2019 at 06:05:47PM +0000, Mark Brown wrote:
> On Tue, Jan 29, 2019 at 09:54:38AM +0000, Charles Keepax wrote:
> 
> > Hmm... I think this might need a little more investigation, it
> > does indeed fail at present (amazing that it works upto exactly 1.2V
> > which is common the usecase) but this feels more like a core
> > problem. There are indeed only 57 valid voltage settings as the
> > range is 0x8 - 0x41, but it seems that presently the core treats
> > this less as n_voltages and more like max_selector. Should we
> > perhaps be teaching the core to understand the min_selector
> > instead?
> 
> We've not had a need for this before but that seems like a reasonable
> addition.

Ok been looking at this last couple of days and I am not so sure
it was a good idea after all.

There are basically two options:

1) Update the usages of selector so it is consistently an index
into the available voltages.
2) Update the handling of n_voltages so that it is converted to
max_selector when used.

Both of these really seem to end up with code that is more
complex than the current system. You end up needing to always
specify or calculate and store the minimum selector and maybe
the maximum one. This needs handling across the various different
schemes for specifying the voltage ranges for the regulators.

So I am adding my ack to the patch, and think we should really
just apply Axel's fix.

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

One thought that did cross my mind, would we be tempted to rename
n_voltages to max_selector? It would be more consistent with the
current usage, although its a horrible big change that doesn't
really make any functional difference.

Thanks,
Charles