[PATCH 0/3] Regulator core fixes

[PATCH 0/3] Regulator core fixes

[Marco Felsch]

Hi,

this series address several improvements I found during devel a
battery-powered device.

The first one address the usage count. IMHO it should be possible to
disable a boot-on marked regulator because the regulator can be left on
by the fw but it isn't forbidden to disable it.

The 2nd address a probe failure. I found that the only dts using
regulator-suspend-min/max-microvolt is the at91-sama5d2_xplained.dts and
this dt use the binding as described in the documentation.
Unfortunately the doc and the implementation are different.

The 3rd adds the support for EPROBE_DEFER.

Regards,
  Marco

Marco Felsch (3):
  regulator: core: fix boot-on regulators use_count usage
  regulator: of: fix suspend-min/max-voltage parsing
  regulator: core: make regulator_register() EPROBE_DEFER aware

 drivers/regulator/core.c         | 18 ++++++++++++++++++
 drivers/regulator/of_regulator.c | 27 ++++++++++++++++++---------
 2 files changed, 36 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

Since commit 1fc12b05895e ("regulator: core: Avoid propagating to
supplies when possible") regulators marked with boot-on can't be
disabled anymore because the commit handles always-on and boot-on
regulators the same way.

Now commit 05f224ca6693 ("regulator: core: Clean enabling always-on
regulators + their supplies") changed the regulator_resolve_supply()
logic a bit by using 'use_count'. So we can't just skip the
'use_count++' during set_machine_constraints(). The easiest way I found
is to correct the 'use_count' just before returning the rdev device
during regulator_register().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/regulator/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0c0cf462004..f9444f509440 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5170,6 +5170,11 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	/* try to resolve regulators supply since a new one was registered */
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_register_resolve_supply);
+
+	/* cleanup use_count -> boot-on marked regulators can be disabled */
+	if (rdev->constraints->boot_on && !rdev->constraints->always_on)
+		rdev->use_count--;
+
 	kfree(config);
 	return rdev;
 
-- 
2.20.1

[PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing

[Marco Felsch]

Currently the regulator-suspend-min/max-microvolt must be within the
root regulator node but the dt-bindings specifies it as subnode
properties for the regulator-state-[mem/disk/standby] node. The only DT
using this bindings currently is the at91-sama5d2_xplained.dts and this
DT uses it correctly. I don't know if it isn't tested but it can't work
without this fix.

Fixes: f7efad10b5c4 ("regulator: add PM suspend and resume hooks")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/regulator/of_regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 9112faa6a9a0..38dd06fbab38 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -231,12 +231,12 @@ static int of_get_regulation_constraints(struct device *dev,
 					"regulator-off-in-suspend"))
 			suspend_state->enabled = DISABLE_IN_SUSPEND;
 
-		if (!of_property_read_u32(np, "regulator-suspend-min-microvolt",
-					  &pval))
+		if (!of_property_read_u32(suspend_np,
+				"regulator-suspend-min-microvolt", &pval))
 			suspend_state->min_uV = pval;
 
-		if (!of_property_read_u32(np, "regulator-suspend-max-microvolt",
-					  &pval))
+		if (!of_property_read_u32(suspend_np,
+				"regulator-suspend-max-microvolt", &pval))
 			suspend_state->max_uV = pval;
 
 		if (!of_property_read_u32(suspend_np,
-- 
2.20.1

[PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Marco Felsch]

Sometimes it can happen that the regulator_of_get_init_data() can't
retrieve the config due to a not probed device the regulator depends on.
Fix that by checking the return value of of_parse_cb() and return
EPROBE_DEFER in such cases.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/regulator/core.c         | 13 +++++++++++++
 drivers/regulator/of_regulator.c | 19 ++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f9444f509440..e8e145c2a5df 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5047,6 +5047,19 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
 					       &rdev->dev.of_node);
+
+	/*
+	 * Sometimes not all resources are probed already so we need to take
+	 * that into account. This happens most the time if the ena_gpiod comes
+	 * from a gpio extender or something else.
+	 */
+	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
+		kfree(config);
+		kfree(rdev);
+		ret = -EPROBE_DEFER;
+		goto rinse;
+	}
+
 	/*
 	 * We need to keep track of any GPIO descriptor coming from the
 	 * device tree until we have handled it over to the core. If the
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 38dd06fbab38..ef7198b76e50 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -445,11 +445,20 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		goto error;
 	}
 
-	if (desc->of_parse_cb && desc->of_parse_cb(child, desc, config)) {
-		dev_err(dev,
-			"driver callback failed to parse DT for regulator %pOFn\n",
-			child);
-		goto error;
+	if (desc->of_parse_cb) {
+		int ret;
+
+		ret = desc->of_parse_cb(child, desc, config);
+		if (ret) {
+			if (ret == -EPROBE_DEFER) {
+				of_node_put(child);
+				return ERR_PTR(-EPROBE_DEFER);
+			}
+			dev_err(dev,
+				"driver callback failed to parse DT for regulator %pOFn\n",
+				child);
+			goto error;
+		}
 	}
 
 	*node = child;
-- 
2.20.1

Applied "regulator: core: make regulator_register() EPROBE_DEFER aware" to the regulator tree

[Mark Brown]

The patch

   regulator: core: make regulator_register() EPROBE_DEFER aware

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4

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 f8970d341eec73c976a3462b9ecdb02b60b84dd6 Mon Sep 17 00:00:00 2001
From: Marco Felsch <m.felsch@pengutronix.de>
Date: Tue, 17 Sep 2019 17:40:21 +0200
Subject: [PATCH] regulator: core: make regulator_register() EPROBE_DEFER aware

Sometimes it can happen that the regulator_of_get_init_data() can't
retrieve the config due to a not probed device the regulator depends on.
Fix that by checking the return value of of_parse_cb() and return
EPROBE_DEFER in such cases.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Link: https://lore.kernel.org/r/20190917154021.14693-4-m.felsch@pengutronix.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c         | 13 +++++++++++++
 drivers/regulator/of_regulator.c | 19 ++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index afe94470b67f..a46be221dbdc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5053,6 +5053,19 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
 					       &rdev->dev.of_node);
+
+	/*
+	 * Sometimes not all resources are probed already so we need to take
+	 * that into account. This happens most the time if the ena_gpiod comes
+	 * from a gpio extender or something else.
+	 */
+	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
+		kfree(config);
+		kfree(rdev);
+		ret = -EPROBE_DEFER;
+		goto rinse;
+	}
+
 	/*
 	 * We need to keep track of any GPIO descriptor coming from the
 	 * device tree until we have handled it over to the core. If the
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 38dd06fbab38..ef7198b76e50 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -445,11 +445,20 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		goto error;
 	}
 
-	if (desc->of_parse_cb && desc->of_parse_cb(child, desc, config)) {
-		dev_err(dev,
-			"driver callback failed to parse DT for regulator %pOFn\n",
-			child);
-		goto error;
+	if (desc->of_parse_cb) {
+		int ret;
+
+		ret = desc->of_parse_cb(child, desc, config);
+		if (ret) {
+			if (ret == -EPROBE_DEFER) {
+				of_node_put(child);
+				return ERR_PTR(-EPROBE_DEFER);
+			}
+			dev_err(dev,
+				"driver callback failed to parse DT for regulator %pOFn\n",
+				child);
+			goto error;
+		}
 	}
 
 	*node = child;
-- 
2.20.1

Applied "regulator: of: fix suspend-min/max-voltage parsing" to the regulator tree

[Mark Brown]

The patch

   regulator: of: fix suspend-min/max-voltage parsing

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4

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 131cb1210d4b58acb0695707dad2eb90dcb50a2a Mon Sep 17 00:00:00 2001
From: Marco Felsch <m.felsch@pengutronix.de>
Date: Tue, 17 Sep 2019 17:40:20 +0200
Subject: [PATCH] regulator: of: fix suspend-min/max-voltage parsing

Currently the regulator-suspend-min/max-microvolt must be within the
root regulator node but the dt-bindings specifies it as subnode
properties for the regulator-state-[mem/disk/standby] node. The only DT
using this bindings currently is the at91-sama5d2_xplained.dts and this
DT uses it correctly. I don't know if it isn't tested but it can't work
without this fix.

Fixes: f7efad10b5c4 ("regulator: add PM suspend and resume hooks")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Link: https://lore.kernel.org/r/20190917154021.14693-3-m.felsch@pengutronix.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/of_regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 9112faa6a9a0..38dd06fbab38 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -231,12 +231,12 @@ static int of_get_regulation_constraints(struct device *dev,
 					"regulator-off-in-suspend"))
 			suspend_state->enabled = DISABLE_IN_SUSPEND;
 
-		if (!of_property_read_u32(np, "regulator-suspend-min-microvolt",
-					  &pval))
+		if (!of_property_read_u32(suspend_np,
+				"regulator-suspend-min-microvolt", &pval))
 			suspend_state->min_uV = pval;
 
-		if (!of_property_read_u32(np, "regulator-suspend-max-microvolt",
-					  &pval))
+		if (!of_property_read_u32(suspend_np,
+				"regulator-suspend-max-microvolt", &pval))
 			suspend_state->max_uV = pval;
 
 		if (!of_property_read_u32(suspend_np,
-- 
2.20.1

Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Dmitry Torokhov]

On Tue, Sep 17, 2019 at 4:42 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Sometimes it can happen that the regulator_of_get_init_data() can't
> retrieve the config due to a not probed device the regulator depends on.
> Fix that by checking the return value of of_parse_cb() and return
> EPROBE_DEFER in such cases.

Treating EPROBE_DEFER in a special way is usually wrong.
regulator_of_get_init_data() may fail for multiple reasons (no memory,
invalid DT, etc, etc). All of them should abort instantiating
regulator.

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Marco Felsch]

On 19-09-17 17:57, Dmitry Torokhov wrote:
> On Tue, Sep 17, 2019 at 4:42 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > Sometimes it can happen that the regulator_of_get_init_data() can't
> > retrieve the config due to a not probed device the regulator depends on.
> > Fix that by checking the return value of of_parse_cb() and return
> > EPROBE_DEFER in such cases.
> 
> Treating EPROBE_DEFER in a special way is usually wrong.
> regulator_of_get_init_data() may fail for multiple reasons (no memory,
> invalid DT, etc, etc). All of them should abort instantiating
> regulator.

Those errors are handled but the behaviour of this funciton is to return
NULL in such errors which is fine for the caller of this function. I
only want to handle EPROBE_DEFER special..

Regards,
  Marco


> Thanks.
> 
> -- 
> Dmitry
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Dmitry Torokhov]

On Wed, Sep 18, 2019 at 1:18 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 19-09-17 17:57, Dmitry Torokhov wrote:
> > On Tue, Sep 17, 2019 at 4:42 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >
> > > Sometimes it can happen that the regulator_of_get_init_data() can't
> > > retrieve the config due to a not probed device the regulator depends on.
> > > Fix that by checking the return value of of_parse_cb() and return
> > > EPROBE_DEFER in such cases.
> >
> > Treating EPROBE_DEFER in a special way is usually wrong.
> > regulator_of_get_init_data() may fail for multiple reasons (no memory,
> > invalid DT, etc, etc). All of them should abort instantiating
> > regulator.
>
> Those errors are handled but the behaviour of this funciton is to return
> NULL in such errors which is fine for the caller of this function. I
> only want to handle EPROBE_DEFER special..

And I am saying it is wrong to handle only EPROBE_DEFER.
regulator_of_get_init_data() should always return ERR_PTR()-encoded
error code when parsing callback returns error, so that regulator core
does not mistakenly believe that there is no configuration/init data
when in fact there is, but we failed to handle it properly.

IOW I'm advocating for extending you patch so that it reads:

+               ret = desc->of_parse_cb(child, desc, config);
+               if (ret) {
+                       of_node_put(child);
+                       return ERR_PTR(ret);
+               }

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Marco Felsch]

On 19-09-18 08:53, Dmitry Torokhov wrote:
> On Wed, Sep 18, 2019 at 1:18 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > On 19-09-17 17:57, Dmitry Torokhov wrote:
> > > On Tue, Sep 17, 2019 at 4:42 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > >
> > > > Sometimes it can happen that the regulator_of_get_init_data() can't
> > > > retrieve the config due to a not probed device the regulator depends on.
> > > > Fix that by checking the return value of of_parse_cb() and return
> > > > EPROBE_DEFER in such cases.
> > >
> > > Treating EPROBE_DEFER in a special way is usually wrong.
> > > regulator_of_get_init_data() may fail for multiple reasons (no memory,
> > > invalid DT, etc, etc). All of them should abort instantiating
> > > regulator.
> >
> > Those errors are handled but the behaviour of this funciton is to return
> > NULL in such errors which is fine for the caller of this function. I
> > only want to handle EPROBE_DEFER special..
> 
> And I am saying it is wrong to handle only EPROBE_DEFER.
> regulator_of_get_init_data() should always return ERR_PTR()-encoded
> error code when parsing callback returns error, so that regulator core
> does not mistakenly believe that there is no configuration/init data
> when in fact there is, but we failed to handle it properly.
> 
> IOW I'm advocating for extending you patch so that it reads:
> 
> +               ret = desc->of_parse_cb(child, desc, config);
> +               if (ret) {
> +                       of_node_put(child);
> +                       return ERR_PTR(ret);
> +               }

I know what you mean but I wanted to keep the core changes minimal and I
tought that it was intentional by the core.

Regards,
  Marco

> Thanks.
> 
> -- 
> Dmitry
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware

[Mark Brown]

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

On Wed, Sep 18, 2019 at 08:53:40AM -0700, Dmitry Torokhov wrote:
> On Wed, Sep 18, 2019 at 1:18 AM Marco Felsch <m.felsch@pengutronix.de> wrote:

> > Those errors are handled but the behaviour of this funciton is to return
> > NULL in such errors which is fine for the caller of this function. I
> > only want to handle EPROBE_DEFER special..

> And I am saying it is wrong to handle only EPROBE_DEFER.
> regulator_of_get_init_data() should always return ERR_PTR()-encoded
> error code when parsing callback returns error, so that regulator core
> does not mistakenly believe that there is no configuration/init data
> when in fact there is, but we failed to handle it properly.

> IOW I'm advocating for extending you patch so that it reads:

> +               ret = desc->of_parse_cb(child, desc, config);
> +               if (ret) {
> +                       of_node_put(child);
> +                       return ERR_PTR(ret);
> +               }

> Thanks.

That's a more invasive change and we do have a fallback after DT for
configuration passed in when the regulator is registered, we want to
distinguish between the valid case where there just wasn't anything
there and the error case where we found something but it wasn't parsable
which is always a bit annoying to do.  The logic around this could
really do with a bigger refactoring TBH.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Doug Anderson]

Hi,


On Tue, Sep 17, 2019 at 8:40 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Since commit 1fc12b05895e ("regulator: core: Avoid propagating to
> supplies when possible") regulators marked with boot-on can't be
> disabled anymore because the commit handles always-on and boot-on
> regulators the same way.
>
> Now commit 05f224ca6693 ("regulator: core: Clean enabling always-on
> regulators + their supplies") changed the regulator_resolve_supply()
> logic a bit by using 'use_count'. So we can't just skip the
> 'use_count++' during set_machine_constraints(). The easiest way I found
> is to correct the 'use_count' just before returning the rdev device
> during regulator_register().
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/regulator/core.c | 5 +++++
>  1 file changed, 5 insertions(+)

I will freely admit my ignorance here, but I've always been slightly
confused by the "always-on" vs. "boot-on" distinction...

The bindings say:

  regulator-always-on:
    description: boolean, regulator should never be disabled

  regulator-boot-on:
    description: bootloader/firmware enabled regulator

For 'boot-on' that's a bit ambiguous about what it means.  The
constraints have a bit more details:

 * @always_on: Set if the regulator should never be disabled.
 * @boot_on: Set if the regulator is enabled when the system is initially
 *           started.  If the regulator is not enabled by the hardware or
 *           bootloader then it will be enabled when the constraints are
 *           applied.


What I would take from that is that "boot_on" means that we expect
that the firmware probably turned this regulator on but if it didn't
then the kernel should turn it on (and presumably leave it on?).  That
would mean that your patch is incorrect, I think?


...but then that begs the question of why we have two attributes?
Maybe this has already been discussed before and someone can point me
to a previous discussion?  We should probably make it more clear in
the bindings and/or the constraints.

===

NOTE: I'm curious why you even have the "regulator-boot-on" attribute
in your case to begin with.  Why not leave it off?


-Doug

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Mon, Sep 23, 2019 at 11:02:26AM -0700, Doug Anderson wrote:

> I will freely admit my ignorance here, but I've always been slightly
> confused by the "always-on" vs. "boot-on" distinction...

> The bindings say:

>   regulator-always-on:
>     description: boolean, regulator should never be disabled

>   regulator-boot-on:
>     description: bootloader/firmware enabled regulator

> For 'boot-on' that's a bit ambiguous about what it means.  The
> constraints have a bit more details:

Boot on means that it's powered on when the kernel starts, it's
for regulators that we can't read back the status of.

> ...but then that begs the question of why we have two attributes?
> Maybe this has already been discussed before and someone can point me
> to a previous discussion?  We should probably make it more clear in
> the bindings and/or the constraints.

boot-on just refers to the status at boot, we can still turn
those regulators off later on if we want to.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Doug Anderson]

Hi,

On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Sep 23, 2019 at 11:02:26AM -0700, Doug Anderson wrote:
>
> > I will freely admit my ignorance here, but I've always been slightly
> > confused by the "always-on" vs. "boot-on" distinction...
>
> > The bindings say:
>
> >   regulator-always-on:
> >     description: boolean, regulator should never be disabled
>
> >   regulator-boot-on:
> >     description: bootloader/firmware enabled regulator
>
> > For 'boot-on' that's a bit ambiguous about what it means.  The
> > constraints have a bit more details:
>
> Boot on means that it's powered on when the kernel starts, it's
> for regulators that we can't read back the status of.

1. Would it be valid to say that it's always incorrect to set this
property if there is a way to read the status back from the regulator?

2. Would this be a valid description of how the property is expected to behave
a) At early boot this regulator will be turned on if it wasn't already on.
b) If no clients are found for this regulator after everything has
loaded, this regulator will be automatically disabled.

If so then I don't _think_ #2b is happening, but I haven't confirmed.


> > ...but then that begs the question of why we have two attributes?
> > Maybe this has already been discussed before and someone can point me
> > to a previous discussion?  We should probably make it more clear in
> > the bindings and/or the constraints.
>
> boot-on just refers to the status at boot, we can still turn
> those regulators off later on if we want to.

How, exactly?  As of my commit 5451781dadf8 ("regulator: core: Only
count load for enabled consumers") if you do:

  r = regulator_get(...)
  regulator_disable(r)

...then you'll get "Underflow of regulator enable count".  In other
words, if a given regulator client disables more times than it enables
then you will get an error.  Since there is no client that did the
initial "boot" enable then there's no way to do the disable unless it
happens automatically (as per 2b above).

...or do you mean that people could call regulator_force_disable()?
Couldn't they also do that with an always-on regulator?



-Doug

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <broonie@kernel.org> wrote:

> > Boot on means that it's powered on when the kernel starts, it's
> > for regulators that we can't read back the status of.

> 1. Would it be valid to say that it's always incorrect to set this
> property if there is a way to read the status back from the regulator?

As originally intended, yes.  I'm now not 100% sure that it won't
break any existing systems though :/

> 2. Would this be a valid description of how the property is expected to behave
> a) At early boot this regulator will be turned on if it wasn't already on.
> b) If no clients are found for this regulator after everything has
> loaded, this regulator will be automatically disabled.

> If so then I don't _think_ #2b is happening, but I haven't confirmed.

> > boot-on just refers to the status at boot, we can still turn
> > those regulators off later on if we want to.

> How, exactly?  As of my commit 5451781dadf8 ("regulator: core: Only
> count load for enabled consumers") if you do:

>   r = regulator_get(...)
>   regulator_disable(r)

> ...then you'll get "Underflow of regulator enable count".  In other
> words, if a given regulator client disables more times than it enables
> then you will get an error.  Since there is no client that did the
> initial "boot" enable then there's no way to do the disable unless it
> happens automatically (as per 2b above).

It should be possible to do a regulator_disable() though I'm not
sure anyone actually uses that.  The pattern for a regular
consumer should be the normal enable/disable pair to handle
shared usage, only an exclusive consumer should be able to use
just a straight disable.

> ...or do you mean that people could call regulator_force_disable()?
> Couldn't they also do that with an always-on regulator?

No, nothing should use that in a non-emergency situation.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Doug Anderson]

Hi,

On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> > On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > Boot on means that it's powered on when the kernel starts, it's
> > > for regulators that we can't read back the status of.
>
> > 1. Would it be valid to say that it's always incorrect to set this
> > property if there is a way to read the status back from the regulator?
>
> As originally intended, yes.  I'm now not 100% sure that it won't
> break any existing systems though :/

Should I change the bindings doc to say that?


> > 2. Would this be a valid description of how the property is expected to behave
> > a) At early boot this regulator will be turned on if it wasn't already on.
> > b) If no clients are found for this regulator after everything has
> > loaded, this regulator will be automatically disabled.
>
> > If so then I don't _think_ #2b is happening, but I haven't confirmed.
>
> > > boot-on just refers to the status at boot, we can still turn
> > > those regulators off later on if we want to.
>
> > How, exactly?  As of my commit 5451781dadf8 ("regulator: core: Only
> > count load for enabled consumers") if you do:
>
> >   r = regulator_get(...)
> >   regulator_disable(r)
>
> > ...then you'll get "Underflow of regulator enable count".  In other
> > words, if a given regulator client disables more times than it enables
> > then you will get an error.  Since there is no client that did the
> > initial "boot" enable then there's no way to do the disable unless it
> > happens automatically (as per 2b above).
>
> It should be possible to do a regulator_disable() though I'm not
> sure anyone actually uses that.  The pattern for a regular
> consumer should be the normal enable/disable pair to handle
> shared usage, only an exclusive consumer should be able to use
> just a straight disable.

Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
this working for you?  I wonder if we need to match
"regulator->enable_count" to "rdev->use_count" at the end of
_regulator_get() in the exclusive case...

-Doug

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:

> > > 1. Would it be valid to say that it's always incorrect to set this
> > > property if there is a way to read the status back from the regulator?

> > As originally intended, yes.  I'm now not 100% sure that it won't
> > break any existing systems though :/

> Should I change the bindings doc to say that?

Sure.

> > It should be possible to do a regulator_disable() though I'm not
> > sure anyone actually uses that.  The pattern for a regular
> > consumer should be the normal enable/disable pair to handle
> > shared usage, only an exclusive consumer should be able to use
> > just a straight disable.

> Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> this working for you?  I wonder if we need to match
> "regulator->enable_count" to "rdev->use_count" at the end of
> _regulator_get() in the exclusive case...

Yes, I think that case has been missed when adding the enable
counts - I've never actually had a system myself that made any
use of this stuff.  It probably needs an audit of the users to
make sure nobody's relying on the current behaviour though I
can't think how they would.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Doug Anderson]

Hi,

On Tue, Sep 24, 2019 at 11:28 AM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> > On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
>
> > > > 1. Would it be valid to say that it's always incorrect to set this
> > > > property if there is a way to read the status back from the regulator?
>
> > > As originally intended, yes.  I'm now not 100% sure that it won't
> > > break any existing systems though :/
>
> > Should I change the bindings doc to say that?
>
> Sure.

Sent ("regulator: Document "regulator-boot-on" binding more thoroughly").

https://lore.kernel.org/r/20190926124115.1.Ice34ad5970a375c3c03cb15c3859b3ee501561bf@changeid


> > > It should be possible to do a regulator_disable() though I'm not
> > > sure anyone actually uses that.  The pattern for a regular
> > > consumer should be the normal enable/disable pair to handle
> > > shared usage, only an exclusive consumer should be able to use
> > > just a straight disable.
>
> > Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> > this working for you?  I wonder if we need to match
> > "regulator->enable_count" to "rdev->use_count" at the end of
> > _regulator_get() in the exclusive case...
>
> Yes, I think that case has been missed when adding the enable
> counts - I've never actually had a system myself that made any
> use of this stuff.  It probably needs an audit of the users to
> make sure nobody's relying on the current behaviour though I
> can't think how they would.

Marco: I'm going to assume you'll tackle this since I don't actually
have any use cases that need this.

-Doug

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

Hi Doug, Mark,

sorry for the delay..

On 19-09-26 12:44, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 24, 2019 at 11:28 AM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> > > On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <broonie@kernel.org> wrote:
> > > > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> >
> > > > > 1. Would it be valid to say that it's always incorrect to set this
> > > > > property if there is a way to read the status back from the regulator?
> >
> > > > As originally intended, yes.  I'm now not 100% sure that it won't
> > > > break any existing systems though :/
> >
> > > Should I change the bindings doc to say that?
> >
> > Sure.
> 
> Sent ("regulator: Document "regulator-boot-on" binding more thoroughly").
> 
> https://lore.kernel.org/r/20190926124115.1.Ice34ad5970a375c3c03cb15c3859b3ee501561bf@changeid

Yes, I saw it and thanks for it =)

> > > > It should be possible to do a regulator_disable() though I'm not
> > > > sure anyone actually uses that.  The pattern for a regular
> > > > consumer should be the normal enable/disable pair to handle
> > > > shared usage, only an exclusive consumer should be able to use
> > > > just a straight disable.

In my case it is a regulator-fixed which uses the enable/disable pair.
But as my descriptions says this will not work currently because boot-on
marked regulators can't be disabled right now (using the same logic as
always-on regulators).

> > > Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> > > this working for you?  I wonder if we need to match
> > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > _regulator_get() in the exclusive case...

So my fix isn't correct to fix this in general?

> > Yes, I think that case has been missed when adding the enable
> > counts - I've never actually had a system myself that made any
> > use of this stuff.  It probably needs an audit of the users to
> > make sure nobody's relying on the current behaviour though I
> > can't think how they would.
> 
> Marco: I'm going to assume you'll tackle this since I don't actually
> have any use cases that need this.

My use case is a simple regulator-fixed which is turned on by the
bootloader or to be more precise by the pmic-rom. To map that correctly
I marked this regulator as boot-on. Unfortunately as I pointed out above
this is handeld the same way as always-on.

Regards,
  Marco

> -Doug
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Doug Anderson]

Hi,

On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > sure anyone actually uses that.  The pattern for a regular
> > > > > consumer should be the normal enable/disable pair to handle
> > > > > shared usage, only an exclusive consumer should be able to use
> > > > > just a straight disable.
>
> In my case it is a regulator-fixed which uses the enable/disable pair.
> But as my descriptions says this will not work currently because boot-on
> marked regulators can't be disabled right now (using the same logic as
> always-on regulators).
>
> > > > Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> > > > this working for you?  I wonder if we need to match
> > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > _regulator_get() in the exclusive case...
>
> So my fix isn't correct to fix this in general?

I don't think your fix is correct.  It sounds as if the intention of
"regulator-boot-on" is to have the OS turn the regulator on at bootup
and it keep an implicit reference until someone explicitly tells the
OS to drop the reference.


> > > Yes, I think that case has been missed when adding the enable
> > > counts - I've never actually had a system myself that made any
> > > use of this stuff.  It probably needs an audit of the users to
> > > make sure nobody's relying on the current behaviour though I
> > > can't think how they would.
> >
> > Marco: I'm going to assume you'll tackle this since I don't actually
> > have any use cases that need this.
>
> My use case is a simple regulator-fixed which is turned on by the
> bootloader or to be more precise by the pmic-rom. To map that correctly
> I marked this regulator as boot-on. Unfortunately as I pointed out above
> this is handeld the same way as always-on.

It's a fixed regulator controlled by a GPIO?  Presumably the GPIO can
be read.  That would mean it ideally shouldn't be using
"regulator-boot-on" since this is _not_ a regulator whose software
state can't be read.  Just remove the property.


-Doug

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Matti Vaittinen]

Hi dee Ho Peeps,

Long time no hear =)

On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > > sure anyone actually uses that.  The pattern for a regular
> > > > > > consumer should be the normal enable/disable pair to handle
> > > > > > shared usage, only an exclusive consumer should be able to use
> > > > > > just a straight disable.
> >
> > In my case it is a regulator-fixed which uses the enable/disable pair.
> > But as my descriptions says this will not work currently because boot-on
> > marked regulators can't be disabled right now (using the same logic as
> > always-on regulators).
> >

I was developing driver for yet-another ROHM PMIC when I hit the
phenomena you have been discussing here (I think) :) I used regulator-boot-on
flag from DT in my test setup and then did a test consumer who does

regulator_get()
regulator_enable()
regulator_disable() pair.

As this 'test consumer' was only user for the regulator I expected the
regulator to be disabled after call to regulator_disable. But it was
not.

It seems to me that the use_count is incremented for boot-on regulators
before first call to regulator_enable(). So when the consumer does first
regulator_enable() the use_count will actually go to 2. Hence the
corresponding regulator_disable() won't actually disable the regulator
even though the consumer is actually only known user.

I did unbalanced regulator_disable() - which does disable the regulator
but it also spills the warning.

I did instrument the regmap helpers and regulator_enable/disable to
dump out the actual i2c accesses and use_counts. Regulator enable prints
use_count _before_ incrementing it.


Check enable state after regulator_get (calls regulator_is_enabled)
root@arm:/sys/kernel/mva_test/regulators# cat buck3_en 
[  123.251499] dbg_regulator_is_enabled_regmap: called for 'buck3'
[  123.257524] regulator_is_enabled_regmap_dbg: Reading reg 0x1c
[  123.267386] regulator_is_enabled_regmap_dbg: read succeeded, val 0xe

Enable regulator by test consumer (no i2c access as regulator is on)
1root@arm:/sys/kernel/mva_test/regulators# echo 1 > buck3_en 
[  171.438524] Calling regulator_enable
[  171.446324] Enable requested, use-count 1

/* disable regulator by consumer */
root@arm:/sys/kernel/mva_test/regulators# echo 0 > buck3_en                                                                                     
[  187.799956] Calling regulator_disable
[  187.805935] regulator disable requested, use-count 2, always-on 0

/* Unbalanced disble */
root@arm:/sys/kernel/mva_test/regulators# echo 0 > buck3_en 
[  207.832682] Calling regulator_disable
[  207.842949] regulator disable requested, use-count 1, always-on 0
[  207.849237] regulator do disable
[  207.852502] dbg_regulator_disable_regmap: called for 'buck3'
[  207.858272] regulator_disable_regmap_dbg: reg 0x1c mask 0x8 val 0x0, masked_val 0x0
[  207.909942] buck3: Underflow of regulator enable count
[  207.915189] Failed to toggle regulator state. error(-22)
bash: echo: write error: Invalid argument
root@arm:/sys/kernel/mva_test/regulators# 

> > > > > Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> > > > > this working for you?  I wonder if we need to match
> > > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > > _regulator_get() in the exclusive case...
> >
> > So my fix isn't correct to fix this in general?
> 
> I don't think your fix is correct.  It sounds as if the intention of
> "regulator-boot-on" is to have the OS turn the regulator on at bootup
> and it keep an implicit reference until someone explicitly tells the
> OS to drop the reference.

Hmm.. What is the intended way to explicitly tell the OS to drop the
reference? I would assume we should still use same logic as with other
regulators - if last user calls regulator_disable() we should disable
the regulator? (I may not understand all this well enough though)
 
> > > > Yes, I think that case has been missed when adding the enable
> > > > counts - I've never actually had a system myself that made any
> > > > use of this stuff.  It probably needs an audit of the users to
> > > > make sure nobody's relying on the current behaviour though I
> > > > can't think how they would.
> > >
> > > Marco: I'm going to assume you'll tackle this since I don't actually
> > > have any use cases that need this.
> >
> > My use case is a simple regulator-fixed which is turned on by the
> > bootloader or to be more precise by the pmic-rom. To map that correctly
> > I marked this regulator as boot-on. Unfortunately as I pointed out above
> > this is handeld the same way as always-on.

Here I am again just a man in the middle as I am "only a component vendor"
and lack of complete system information. But I _think_ some of the users
of BD71827 and BD71847 PMICs do use setup where regulator-boot-on is
used to enable certain BUCKs to power some graphics chip at start-up. At
later stage it should be possible to cut the power in order to do power
saving or decrease heating when graphichs are not needed. So I think it
would be nice to fix this somehow.

> It's a fixed regulator controlled by a GPIO?  Presumably the GPIO can
> be read.  That would mean it ideally shouldn't be using
> "regulator-boot-on" since this is _not_ a regulator whose software
> state can't be read.  Just remove the property.

How should we handle cases where we want OS to enable regulator at
boot-up - possibly before consumer drivers can be load?


Br,
	Matti Vaittinen
-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Fri, Oct 04, 2019 at 09:34:43AM +0300, Matti Vaittinen wrote:
> On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:

> > I don't think your fix is correct.  It sounds as if the intention of
> > "regulator-boot-on" is to have the OS turn the regulator on at bootup
> > and it keep an implicit reference until someone explicitly tells the
> > OS to drop the reference.

> Hmm.. What is the intended way to explicitly tell the OS to drop the
> reference? I would assume we should still use same logic as with other
> regulators - if last user calls regulator_disable() we should disable
> the regulator? (I may not understand all this well enough though)

Yes.

> > It's a fixed regulator controlled by a GPIO?  Presumably the GPIO can
> > be read.  That would mean it ideally shouldn't be using
> > "regulator-boot-on" since this is _not_ a regulator whose software
> > state can't be read.  Just remove the property.

> How should we handle cases where we want OS to enable regulator at
> boot-up - possibly before consumer drivers can be load?

If you want the regulator to be on without any driver present then mark
it always-on.  If you want the regulator to be enabled prior to the
driver being loaded then the expectation is that the bootloader will do
that, it's difficult to see what the benefit there is from having the
kernel enable things when it starts prior to having a driver unless the
intent is to keep the regulator always on.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Vaittinen, Matti]

On Fri, 2019-10-04 at 12:32 +0100, Mark Brown wrote:
> On Fri, Oct 04, 2019 at 09:34:43AM +0300, Matti Vaittinen wrote:
> > On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:
> > > I don't think your fix is correct.  It sounds as if the intention
> > > of
> > > "regulator-boot-on" is to have the OS turn the regulator on at
> > > bootup
> > > and it keep an implicit reference until someone explicitly tells
> > > the
> > > OS to drop the reference.
> > Hmm.. What is the intended way to explicitly tell the OS to drop
> > the
> > reference? I would assume we should still use same logic as with
> > other
> > regulators - if last user calls regulator_disable() we should
> > disable
> > the regulator? (I may not understand all this well enough though)
> 
> Yes.
> 
> > > It's a fixed regulator controlled by a GPIO?  Presumably the GPIO
> > > can
> > > be read.  That would mean it ideally shouldn't be using
> > > "regulator-boot-on" since this is _not_ a regulator whose
> > > software
> > > state can't be read.  Just remove the property.
> > How should we handle cases where we want OS to enable regulator at
> > boot-up - possibly before consumer drivers can be load?
> 
> If you want the regulator to be on without any driver present then
> mark
> it always-on.  If you want the regulator to be enabled prior to the
> driver being loaded then the expectation is that the bootloader will
> do
> that, it's difficult to see what the benefit there is from having the
> kernel enable things when it starts prior to having a driver unless
> the
> intent is to keep the regulator always on.

I thought the regulator-boot-on could have been used for that. But as I
said - I don't really know all this so well =) And no, I am not opposed
to offloading this from kernel to boot, I was just trying to learn what
is the correct thing to do (tm). Thanks for educating me on this :) I
will suggest adding the enabling to boot code if (when) I get questions
concerning this. (always-on won't do for regulators which need to be
controlled for power saving or heating issues).

Br,
	Matti Vaittinen

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Fri, Oct 04, 2019 at 12:03:12PM +0000, Vaittinen, Matti wrote:
> On Fri, 2019-10-04 at 12:32 +0100, Mark Brown wrote:

> > If you want the regulator to be on without any driver present then
> > mark
> > it always-on.  If you want the regulator to be enabled prior to the
> > driver being loaded then the expectation is that the bootloader will
> > do
> > that, it's difficult to see what the benefit there is from having the
> > kernel enable things when it starts prior to having a driver unless
> > the
> > intent is to keep the regulator always on.

> I thought the regulator-boot-on could have been used for that. But as I
> said - I don't really know all this so well =) And no, I am not opposed
> to offloading this from kernel to boot, I was just trying to learn what
> is the correct thing to do (tm). Thanks for educating me on this :) I
> will suggest adding the enabling to boot code if (when) I get questions
> concerning this. (always-on won't do for regulators which need to be
> controlled for power saving or heating issues).

If you want the kernel to do it early on without the bootloader then I
think we really need to understand the use case.  My guess would be that
the underlying request would be to get the driver up earlier which is
something we should be better at but often easier said than done.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

Hi Doug, Mark,

On 19-10-01 12:57, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > > sure anyone actually uses that.  The pattern for a regular
> > > > > > consumer should be the normal enable/disable pair to handle
> > > > > > shared usage, only an exclusive consumer should be able to use
> > > > > > just a straight disable.
> >
> > In my case it is a regulator-fixed which uses the enable/disable pair.
> > But as my descriptions says this will not work currently because boot-on
> > marked regulators can't be disabled right now (using the same logic as
> > always-on regulators).
> >
> > > > > Ah, I see, I wasn't aware of the "exclusive" special case!  Marco: is
> > > > > this working for you?  I wonder if we need to match
> > > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > > _regulator_get() in the exclusive case...
> >
> > So my fix isn't correct to fix this in general?
> 
> I don't think your fix is correct.  It sounds as if the intention of
> "regulator-boot-on" is to have the OS turn the regulator on at bootup
> and it keep an implicit reference until someone explicitly tells the
> OS to drop the reference.
> 
> 
> > > > Yes, I think that case has been missed when adding the enable
> > > > counts - I've never actually had a system myself that made any
> > > > use of this stuff.  It probably needs an audit of the users to
> > > > make sure nobody's relying on the current behaviour though I
> > > > can't think how they would.
> > >
> > > Marco: I'm going to assume you'll tackle this since I don't actually
> > > have any use cases that need this.
> >
> > My use case is a simple regulator-fixed which is turned on by the
> > bootloader or to be more precise by the pmic-rom. To map that correctly
> > I marked this regulator as boot-on. Unfortunately as I pointed out above
> > this is handeld the same way as always-on.
> 
> It's a fixed regulator controlled by a GPIO?  Presumably the GPIO can
> be read.  That would mean it ideally shouldn't be using
> "regulator-boot-on" since this is _not_ a regulator whose software
> state can't be read.  Just remove the property.

Sorry that won't fix my problem. If I drop the regulator-boot-on state
the fixed-regulator will disable this regulator but disable/enable this
regulator is only valid during suspend/resume. I don't say that my fix
is correct but we should fix this.

Regards,
  Marco

> -Doug
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:

> Sorry that won't fix my problem. If I drop the regulator-boot-on state
> the fixed-regulator will disable this regulator but disable/enable this
> regulator is only valid during suspend/resume. I don't say that my fix
> is correct but we should fix this.

I'm having a bit of trouble parsing this but it sounds like you want the
regulator to be always on in which case you should use the property
specifically for that.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

On 19-10-07 19:29, Mark Brown wrote:
> On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
> 
> > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > the fixed-regulator will disable this regulator but disable/enable this
> > regulator is only valid during suspend/resume. I don't say that my fix
> > is correct but we should fix this.
> 
> I'm having a bit of trouble parsing this but it sounds like you want the
> regulator to be always on in which case you should use the property
> specifically for that.

Sorry my english wasn't the best.. Imagine this case: The bootloader
turned the display on to show an early bootlogo. Now if I miss the
regulator-boot-on property the display is turned off and on. The turn
off comes from the regulator probe, the turn on comes from the cosumer.
Is that assumption correct?

Regards,
  Marco

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Tue, Oct 08, 2019 at 08:03:11AM +0200, Marco Felsch wrote:
> On 19-10-07 19:29, Mark Brown wrote:
> > On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:

> > > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > > the fixed-regulator will disable this regulator but disable/enable this
> > > regulator is only valid during suspend/resume. I don't say that my fix
> > > is correct but we should fix this.

> > I'm having a bit of trouble parsing this but it sounds like you want the
> > regulator to be always on in which case you should use the property
> > specifically for that.

> Sorry my english wasn't the best.. Imagine this case: The bootloader
> turned the display on to show an early bootlogo. Now if I miss the
> regulator-boot-on property the display is turned off and on. The turn
> off comes from the regulator probe, the turn on comes from the cosumer.
> Is that assumption correct?

No, we shouldn't do anything when the regulator probes - we'll only
disable unused regulators when we get to the end of boot (currently we
delay this by 30s to give userspace a chance to run, that's a hack but
we're fresh out of better ideas).  During boot the regulator state will
only be changed if some consumer appears and changes the state.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

On 19-10-08 13:51, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 08:03:11AM +0200, Marco Felsch wrote:
> > On 19-10-07 19:29, Mark Brown wrote:
> > > On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
> 
> > > > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > > > the fixed-regulator will disable this regulator but disable/enable this
> > > > regulator is only valid during suspend/resume. I don't say that my fix
> > > > is correct but we should fix this.
> 
> > > I'm having a bit of trouble parsing this but it sounds like you want the
> > > regulator to be always on in which case you should use the property
> > > specifically for that.
> 
> > Sorry my english wasn't the best.. Imagine this case: The bootloader
> > turned the display on to show an early bootlogo. Now if I miss the
> > regulator-boot-on property the display is turned off and on. The turn
> > off comes from the regulator probe, the turn on comes from the cosumer.
> > Is that assumption correct?
> 
> No, we shouldn't do anything when the regulator probes - we'll only
> disable unused regulators when we get to the end of boot (currently we
> delay this by 30s to give userspace a chance to run, that's a hack but
> we're fresh out of better ideas).  During boot the regulator state will
> only be changed if some consumer appears and changes the state.

Okay, so this won't disable the regualtor?

8<----------------------------------------------------------------
static int reg_fixed_voltage_probe(struct platform_device *pdev)
{
	...

	if (config->enabled_at_boot)
		gflags = GPIOD_OUT_HIGH;
	else
		gflags = GPIOD_OUT_LOW;

	...
}
8<----------------------------------------------------------------

Regards,
  Marco

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Tue, Oct 08, 2019 at 04:56:05PM +0200, Marco Felsch wrote:
> On 19-10-08 13:51, Mark Brown wrote:

> > No, we shouldn't do anything when the regulator probes - we'll only
> > disable unused regulators when we get to the end of boot (currently we
> > delay this by 30s to give userspace a chance to run, that's a hack but
> > we're fresh out of better ideas).  During boot the regulator state will
> > only be changed if some consumer appears and changes the state.

> Okay, so this won't disable the regualtor?

> 8<----------------------------------------------------------------
> static int reg_fixed_voltage_probe(struct platform_device *pdev)
> {
> 	...
> 
> 	if (config->enabled_at_boot)
> 		gflags = GPIOD_OUT_HIGH;
> 	else
> 		gflags = GPIOD_OUT_LOW;
> 
> 	...
> }
> 8<----------------------------------------------------------------

If this is a GPIO regulator then the Linux APIs mean you can't read the
status back so it's one of the regulators for which this property was
invented.  This is a real limitation of the Linux APIs, with most
hardware you can actually read the status back so we shouldn't need
this.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

On 19-10-08 16:42, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 04:56:05PM +0200, Marco Felsch wrote:
> > On 19-10-08 13:51, Mark Brown wrote:
> 
> > > No, we shouldn't do anything when the regulator probes - we'll only
> > > disable unused regulators when we get to the end of boot (currently we
> > > delay this by 30s to give userspace a chance to run, that's a hack but
> > > we're fresh out of better ideas).  During boot the regulator state will
> > > only be changed if some consumer appears and changes the state.
> 
> > Okay, so this won't disable the regualtor?
> 
> > 8<----------------------------------------------------------------
> > static int reg_fixed_voltage_probe(struct platform_device *pdev)
> > {
> > 	...
> > 
> > 	if (config->enabled_at_boot)
> > 		gflags = GPIOD_OUT_HIGH;
> > 	else
> > 		gflags = GPIOD_OUT_LOW;
> > 
> > 	...
> > }
> > 8<----------------------------------------------------------------
> 
> If this is a GPIO regulator then the Linux APIs mean you can't read the
> status back so it's one of the regulators for which this property was
> invented.  This is a real limitation of the Linux APIs, with most
> hardware you can actually read the status back so we shouldn't need
> this.

I know and I followed the discussion between you and Doug. But it
is a valid use-case to have a external gpio-enabled regualtor connected
to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
the fixed.c driver (IMHO this is correct), the regulator gets disabled
during probe. So I will have a panel off/ panel on sequence during boot.
To avoid this I set the 'regualtor-boot-on' property but then I can't
disable the panel during suspend..

Can you give me an advice how I can handle that otherwise?

Regards,
  Marco

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Tue, Oct 08, 2019 at 06:16:40PM +0200, Marco Felsch wrote:
> On 19-10-08 16:42, Mark Brown wrote:

> > If this is a GPIO regulator then the Linux APIs mean you can't read the
> > status back so it's one of the regulators for which this property was
> > invented.  This is a real limitation of the Linux APIs, with most
> > hardware you can actually read the status back so we shouldn't need
> > this.

> I know and I followed the discussion between you and Doug. But it
> is a valid use-case to have a external gpio-enabled regualtor connected
> to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
> the fixed.c driver (IMHO this is correct), the regulator gets disabled
> during probe. So I will have a panel off/ panel on sequence during boot.

Right, this is why I am saying that this is one of the regulators for
which this property was defined and where you should be using it.

> To avoid this I set the 'regualtor-boot-on' property but then I can't
> disable the panel during suspend..

As you'll have seen from the discussion that's a bug, nothing should be
taking a reference to the regulator outside of explicit enable calls.

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

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Marco Felsch]

On 19-10-08 17:23, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 06:16:40PM +0200, Marco Felsch wrote:
> > On 19-10-08 16:42, Mark Brown wrote:
> 
> > > If this is a GPIO regulator then the Linux APIs mean you can't read the
> > > status back so it's one of the regulators for which this property was
> > > invented.  This is a real limitation of the Linux APIs, with most
> > > hardware you can actually read the status back so we shouldn't need
> > > this.
> 
> > I know and I followed the discussion between you and Doug. But it
> > is a valid use-case to have a external gpio-enabled regualtor connected
> > to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
> > the fixed.c driver (IMHO this is correct), the regulator gets disabled
> > during probe. So I will have a panel off/ panel on sequence during boot.
> 
> Right, this is why I am saying that this is one of the regulators for
> which this property was defined and where you should be using it.
> 
> > To avoid this I set the 'regualtor-boot-on' property but then I can't
> > disable the panel during suspend..
> 
> As you'll have seen from the discussion that's a bug, nothing should be
> taking a reference to the regulator outside of explicit enable calls.

Okay now we are on the right way :) Is the solution proposed by Doug:
".. we need to match "regulator->enable_count" to "rdev->use_count" at
the end of _regulator_get() in the exclusive case..." the correct fix?

Another question. Please can you have a look on the "DA9062 PMIC fixes
and features" series as well?

Regards,
  Marco

Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

[Mark Brown]

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

On Tue, Oct 08, 2019 at 10:16:22PM +0200, Marco Felsch wrote:
> On 19-10-08 17:23, Mark Brown wrote:

> > As you'll have seen from the discussion that's a bug, nothing should be
> > taking a reference to the regulator outside of explicit enable calls.

> Okay now we are on the right way :) Is the solution proposed by Doug:
> ".. we need to match "regulator->enable_count" to "rdev->use_count" at
> the end of _regulator_get() in the exclusive case..." the correct fix?

Yes, I think so.

> Another question. Please can you have a look on the "DA9062 PMIC fixes
> and features" series as well?

I don't know what that is, sorry.

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