[PATCH v2 resend 0/3] regulator: axp20x: 3 bugfixes

[PATCH v2 resend 0/3] regulator: axp20x: 3 bugfixes

[Hans de Goede]

Hi Mark, et al,

Here are 3 bugfixes for the axp20x regulator driver, sorry for not
sending these to the right email address before.

Regards,

Hans

[PATCH v2 1/3] regulator: axp20x: Fix axp209 ldo4 ranges

[Hans de Goede]

The axp209 ldo4 regulator has a hole at (skips) 2600 mV and 2900 mV, fix
its range table to match.

Fixes: 13d57e64352a ("regulator: axp20x: Use linear voltage ranges for AXP20X LDO4")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v2:
-Add Fixes, Acked-by Chen-Yu tags
---
 drivers/regulator/axp20x-regulator.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 40cd894..29ab098 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -157,7 +157,9 @@ static struct regulator_ops axp20x_ops_sw = {
 static const struct regulator_linear_range axp20x_ldo4_ranges[] = {
 	REGULATOR_LINEAR_RANGE(1250000, 0x0, 0x0, 0),
 	REGULATOR_LINEAR_RANGE(1300000, 0x1, 0x8, 100000),
-	REGULATOR_LINEAR_RANGE(2500000, 0x9, 0xf, 100000),
+	REGULATOR_LINEAR_RANGE(2500000, 0x9, 0x9, 0),
+	REGULATOR_LINEAR_RANGE(2700000, 0xa, 0xb, 100000),
+	REGULATOR_LINEAR_RANGE(3000000, 0xc, 0xf, 100000),
 };
 
 static const struct regulator_desc axp20x_regulators[] = {
-- 
2.7.4

[PATCH v2 2/3] regulator: axp20x: Fix axp22x ldo_io voltage ranges

[Hans de Goede]

The minium voltage of 1800mV is a copy and paste error from the axp20x
regulator info. The correct minimum voltage for the ldo_io regulators
on the axp22x is 700mV.

Fixes: 1b82b4e4f954 ("regulator: axp20x: Add support for AXP22X regulators")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v2:
-Add Fixes, Acked-by Chen-Yu tags
---
 drivers/regulator/axp20x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 29ab098..89f6842 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -217,10 +217,10 @@ static const struct regulator_desc axp22x_regulators[] = {
 		 AXP22X_ELDO2_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(1)),
 	AXP_DESC(AXP22X, ELDO3, "eldo3", "eldoin", 700, 3300, 100,
 		 AXP22X_ELDO3_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(2)),
-	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO0_V_OUT, 0x1f, AXP20X_GPIO0_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
-	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO1_V_OUT, 0x1f, AXP20X_GPIO1_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
 	AXP_DESC_FIXED(AXP22X, RTC_LDO, "rtc_ldo", "ips", 3000),
-- 
2.7.4

[PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Hans de Goede]

The maximum supported voltage for ldo_io# is 3.3V, but on cold
boot the selector comes up at 0x1f, which maps to 3.8V.

This causes _regulator_get_voltage() to fail with -EINVAL which
causes regulator registration to fail when constrains are used:

[    1.467788] vcc-touchscreen: failed to get the current voltage(-22)
[    1.474209] axp20x-regulator axp20x-regulator: Failed to register ldo_io1
[    1.483363] axp20x-regulator: probe of axp20x-regulator failed with error -22

This commits makes axp20x_probe fix-up the selector values in
the chip before registering regulators avoiding the above errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/regulator/axp20x-regulator.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 89f6842..5ddaa82 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -347,6 +347,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 		.driver_data = axp20x,
 	};
 	int ret, i, nregulators;
+	unsigned int reg, sel;
 	u32 workmode;
 	const char *axp22x_dc1_name = axp22x_regulators[AXP22X_DCDC1].name;
 	const char *axp22x_dc5_name = axp22x_regulators[AXP22X_DCDC5].name;
@@ -361,6 +362,24 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	case AXP223_ID:
 		regulators = axp22x_regulators;
 		nregulators = AXP22X_REG_ID_MAX;
+		/*
+		 * On cold boot ldo_io# sel is 0x1f which is out of spec,
+		 * fix this up here to avoid _regulator_get_voltage returning
+		 * -EINVAL when applying constraints.
+		 */
+		for (reg = AXP22X_LDO_IO0_V_OUT;
+		     reg <= AXP22X_LDO_IO1_V_OUT; reg += 2) {
+			ret = regmap_read(axp20x->regmap, reg, &sel);
+			if (ret)
+				return ret;
+			sel &= 0x1f;
+			if (sel > 0x1a) {
+				ret = regmap_update_bits(axp20x->regmap, reg,
+							 0x1f, 0x1a);
+				if (ret)
+					return ret;
+			}
+		}
 		break;
 	default:
 		dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
-- 
2.7.4

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Mark Brown]

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

On Wed, Apr 27, 2016 at 03:59:28PM +0200, Hans de Goede wrote:
> The maximum supported voltage for ldo_io# is 3.3V, but on cold
> boot the selector comes up at 0x1f, which maps to 3.8V.

Why not just implement that?  We know what it does and it preserves the
expected behaviour where we don't touch the regualtor unless explicitly
told it's OK.  We'll only ever try to set that value if the machine
explicitly gives permission for it.

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

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Hans de Goede]

Hi,

On 27-04-16 17:12, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 03:59:28PM +0200, Hans de Goede wrote:
>> The maximum supported voltage for ldo_io# is 3.3V, but on cold
>> boot the selector comes up at 0x1f, which maps to 3.8V.
>
> Why not just implement that?

I guess I was not clear in my commit msg, when I wrote:
"which maps to 3.8V", what I mean is that:

Given the formula in the datasheet to calculate the ldo_io
regulator voltage 0x1f maps to 3.8V, but according to the
datasheet the maximum voltage supported is 3.3V, iow the
power-on-reset value of this register is out of spec
according to the datasheet.

Note the datasheet does not explicitly mention this being
out of spec. But a reset value of 0x1f has been observed,
and putting that in the formula for getting the voltage
leads to an out of spec value of 3.8V

> We know what it does and it preserves the
> expected behaviour where we don't touch the regualtor unless explicitly
> told it's OK.  We'll only ever try to set that value if the machine
> explicitly gives permission for it.

The problem is that if we do not fix the out of spec
register value then _regulator_get_voltage returns
-EINVAL because the register value exceeds n_voltages

This causes things to fail when we do actually want to use the
regulator and on registering it try to apply constraints when
registering:

     [    1.467788] vcc-touchscreen: failed to get the current voltage(-22)
     [    1.474209] axp20x-regulator axp20x-regulator: Failed to register ldo_io1
     [    1.483363] axp20x-regulator: probe of axp20x-regulator failed with error -22

Are you suggesting that we simply make n_voltages 0x1f / 31 and rely
on dts constraints to never use the out of spec 3.4 - 3.8 volt
settings ? That will fix things, but it feels wrong.

Thinking more about this, doing this will result in the exact same behavior
(program the ldo to 3.3V when it is still at its power-on-reset value when
  registering) but only when the regulator is used, which means not touching
it unless explicitly told it's ok.

So I guess that this is how you want us to fix this ?

Regards,

Hans

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Mark Brown]

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

On Wed, Apr 27, 2016 at 05:35:31PM +0200, Hans de Goede wrote:
> On 27-04-16 17:12, Mark Brown wrote:

> >Why not just implement that?

> Given the formula in the datasheet to calculate the ldo_io
> regulator voltage 0x1f maps to 3.8V, but according to the
> datasheet the maximum voltage supported is 3.3V, iow the
> power-on-reset value of this register is out of spec
> according to the datasheet.

Well, I guess someone can just measure what happens?

> >We know what it does and it preserves the
> >expected behaviour where we don't touch the regualtor unless explicitly
> >told it's OK.  We'll only ever try to set that value if the machine
> >explicitly gives permission for it.

> The problem is that if we do not fix the out of spec
> register value then _regulator_get_voltage returns
> -EINVAL because the register value exceeds n_voltages

This will no longer be the case when the driver understands what the
startup value means.

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

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Hans de Goede]

Hi,

On 27-04-16 17:48, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 05:35:31PM +0200, Hans de Goede wrote:
>> On 27-04-16 17:12, Mark Brown wrote:
>
>>> Why not just implement that?
>
>> Given the formula in the datasheet to calculate the ldo_io
>> regulator voltage 0x1f maps to 3.8V, but according to the
>> datasheet the maximum voltage supported is 3.3V, iow the
>> power-on-reset value of this register is out of spec
>> according to the datasheet.
>
> Well, I guess someone can just measure what happens?

What happens will likely depend on the pmic input voltage,
which can be either 5V from a charger / usb or can be approx
3.8V from a lion or lipo battery. All linear regulators in
the axp20x / axp22x pmic are listed as having a max output
voltage of 3.3V, this likely has to do with the minimum
voltage drop compared to the input value.

So in some conditions the output voltage at a 0x1f register
value may very well be different then at others. IMHO we
should just avoid any out of spec. values.

The 2 ldo-s we're talking about now are special in 2 ways:

1) They are the only ones to have an out of spec p-o-r value
2) They are the only ones which do not have a dedicated pin,
    they are muxed to the outside sharing pins with 2 gpio
    pins, with the muxing defaulting to gpio-input, which makes
    1) ok(-ish) I guess

I believe that we really need to write an in-spec value to the
register controlling the voltage, before enabling this regulator
(which is done by selecting the mux to connect it to the pin).

Since you do not like this patch, I believe that the best way
to do this instead is to make n_voltages span the whole
range, have get_voltage return 3.8 for 0x1f and limit things
using constraints so that if the register contains 0x1b - 0x1f
we will call set_voltage to a supported value when applying the
constraints.

>>> We know what it does and it preserves the
>>> expected behaviour where we don't touch the regualtor unless explicitly
>>> told it's OK.  We'll only ever try to set that value if the machine
>>> explicitly gives permission for it.
>
>> The problem is that if we do not fix the out of spec
>> register value then _regulator_get_voltage returns
>> -EINVAL because the register value exceeds n_voltages
>
> This will no longer be the case when the driver understands what the
> startup value means.

Ack, which is what I'm suggesting by suggesting to increase
n_voltages.

Regards,

Hans

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Mark Brown]

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

On Wed, Apr 27, 2016 at 06:04:53PM +0200, Hans de Goede wrote:
> On 27-04-16 17:48, Mark Brown wrote:

> >Well, I guess someone can just measure what happens?

> What happens will likely depend on the pmic input voltage,
> which can be either 5V from a charger / usb or can be approx
> 3.8V from a lion or lipo battery. All linear regulators in
> the axp20x / axp22x pmic are listed as having a max output
> voltage of 3.3V, this likely has to do with the minimum
> voltage drop compared to the input value.

That sounds like it's a non-regulating bypass mode which is something we
support; if the regulator is in bypass mode we'll look for the voltage
from the parent rather than in the child regulator.

> So in some conditions the output voltage at a 0x1f register
> value may very well be different then at others. IMHO we
> should just avoid any out of spec. values.

As I've said before it'll be read only unless the system integrator
explicitly says otherwise.

> I believe that we really need to write an in-spec value to the
> register controlling the voltage, before enabling this regulator
> (which is done by selecting the mux to connect it to the pin).

We have a strong policy that we don't touch the hardware unless we are
explicitly told it's OK to do so by the system integration, it's very
hard to tell if things are in general going to be safe and if we get
things wrong that could lead to physical damage.

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

Applied "regulator: axp20x: Fix axp22x ldo_io voltage ranges" to the regulator tree

[Mark Brown]

The patch

   regulator: axp20x: Fix axp22x ldo_io voltage ranges

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 05195ed3ec96926388d253608a40d7f2b4b07413 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 27 Apr 2016 15:59:27 +0200
Subject: [PATCH] regulator: axp20x: Fix axp22x ldo_io voltage ranges

The minium voltage of 1800mV is a copy and paste error from the axp20x
regulator info. The correct minimum voltage for the ldo_io regulators
on the axp22x is 700mV.

Fixes: 1b82b4e4f954 ("regulator: axp20x: Add support for AXP22X regulators")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/axp20x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 40cd894e4df5..0c0e7a31ba49 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -215,10 +215,10 @@ static const struct regulator_desc axp22x_regulators[] = {
 		 AXP22X_ELDO2_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(1)),
 	AXP_DESC(AXP22X, ELDO3, "eldo3", "eldoin", 700, 3300, 100,
 		 AXP22X_ELDO3_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(2)),
-	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO0_V_OUT, 0x1f, AXP20X_GPIO0_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
-	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO1_V_OUT, 0x1f, AXP20X_GPIO1_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
 	AXP_DESC_FIXED(AXP22X, RTC_LDO, "rtc_ldo", "ips", 3000),
-- 
2.8.0.rc3

Applied "regulator: axp20x: Fix axp22x ldo_io voltage ranges" to the regulator tree

[Mark Brown]

The patch

   regulator: axp20x: Fix axp22x ldo_io voltage ranges

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From a2262e5a12e05389ab4c7fc5cf60016b041dd8dc Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 27 Apr 2016 15:59:27 +0200
Subject: [PATCH] regulator: axp20x: Fix axp22x ldo_io voltage ranges

The minium voltage of 1800mV is a copy and paste error from the axp20x
regulator info. The correct minimum voltage for the ldo_io regulators
on the axp22x is 700mV.

Fixes: 1b82b4e4f954 ("regulator: axp20x: Add support for AXP22X regulators")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/axp20x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 29ab0985b46e..89f684295657 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -217,10 +217,10 @@ static const struct regulator_desc axp22x_regulators[] = {
 		 AXP22X_ELDO2_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(1)),
 	AXP_DESC(AXP22X, ELDO3, "eldo3", "eldoin", 700, 3300, 100,
 		 AXP22X_ELDO3_V_OUT, 0x1f, AXP22X_PWR_OUT_CTRL2, BIT(2)),
-	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO0, "ldo_io0", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO0_V_OUT, 0x1f, AXP20X_GPIO0_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
-	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 1800, 3300, 100,
+	AXP_DESC_IO(AXP22X, LDO_IO1, "ldo_io1", "ips", 700, 3300, 100,
 		    AXP22X_LDO_IO1_V_OUT, 0x1f, AXP20X_GPIO1_CTRL, 0x07,
 		    AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
 	AXP_DESC_FIXED(AXP22X, RTC_LDO, "rtc_ldo", "ips", 3000),
-- 
2.8.0.rc3

Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot

[Hans de Goede]

Hi,

On 27-04-16 18:30, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 06:04:53PM +0200, Hans de Goede wrote:
>> On 27-04-16 17:48, Mark Brown wrote:
>
>>> Well, I guess someone can just measure what happens?
>
>> What happens will likely depend on the pmic input voltage,
>> which can be either 5V from a charger / usb or can be approx
>> 3.8V from a lion or lipo battery. All linear regulators in
>> the axp20x / axp22x pmic are listed as having a max output
>> voltage of 3.3V, this likely has to do with the minimum
>> voltage drop compared to the input value.
>
> That sounds like it's a non-regulating bypass mode which is something we
> support; if the regulator is in bypass mode we'll look for the voltage
> from the parent rather than in the child regulator.

No these regulators do not have pass-by mode, what I'm trying
to say is that if the configured voltage gets to close to the supply
(e.g. 3.8V on a 3.8V battery) that the actual output then will not
be (even close to) what is configured due to the voltage drop
all linear regulators have.

My plan is to express this limitation using constraints in the dts,
while exposing the full 0.7 - 3.8V range in the driver, so that
the driver will no longer error out on the 0x1f register value
it encounters on the first get_voltage after a cold boot.

>> So in some conditions the output voltage at a 0x1f register
>> value may very well be different then at others. IMHO we
>> should just avoid any out of spec. values.
>
> As I've said before it'll be read only unless the system integrator
> explicitly says otherwise.

Ack.

>> I believe that we really need to write an in-spec value to the
>> register controlling the voltage, before enabling this regulator
>> (which is done by selecting the mux to connect it to the pin).
>
> We have a strong policy that we don't touch the hardware unless we are
> explicitly told it's OK to do so by the system integration, it's very
> hard to tell if things are in general going to be safe and if we get
> things wrong that could lead to physical damage.

Ack, I understand. The way I'm proposing to fix this does exactly
this. It will make us not touch the regulator unless there is a
dts node giving constraints for it.

Let me just send the new patch, that should make things clear.

Regards,

Hans