Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1389076 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration error on cold boot Hans de Goede <hdegoede@redhat.com> - 2016-04-27 16:00 +0200
    Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Mark Brown <broonie@kernel.org> - 2016-04-27 17:20 +0200
      Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Hans de Goede <hdegoede@redhat.com> - 2016-04-27 17:40 +0200
        Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Mark Brown <broonie@kernel.org> - 2016-04-27 17:50 +0200
          Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Hans de Goede <hdegoede@redhat.com> - 2016-04-27 18:10 +0200
            Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Mark Brown <broonie@kernel.org> - 2016-04-27 18:40 +0200
              Re: [PATCH v2 3/3] regulator: axp20x: Fix axp22x ldo_io registration  error on cold boot Hans de Goede <hdegoede@redhat.com> - 2016-04-27 20:40 +0200

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

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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web