* [PATCH 0/3] Regulator core fixes
@ 2019-09-17 15:40 Marco Felsch
2019-09-17 15:40 ` [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage Marco Felsch
` (2 more replies)
0 siblings, 3 replies; 34+ messages in thread
From: Marco Felsch @ 2019-09-17 15:40 UTC (permalink / raw)
To: zhang.chunyan, dianders, lgirdwood, broonie, ckeepax; +Cc: linux-kernel, kernel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-17 15:40 [PATCH 0/3] Regulator core fixes Marco Felsch @ 2019-09-17 15:40 ` Marco Felsch 2019-09-23 18:02 ` Doug Anderson 2019-09-17 15:40 ` [PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing Marco Felsch 2019-09-17 15:40 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Marco Felsch 2 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-09-17 15:40 UTC (permalink / raw) To: zhang.chunyan, dianders, lgirdwood, broonie, ckeepax; +Cc: linux-kernel, kernel 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(®ulator_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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-17 15:40 ` [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage Marco Felsch @ 2019-09-23 18:02 ` Doug Anderson 2019-09-23 18:14 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Doug Anderson @ 2019-09-23 18:02 UTC (permalink / raw) To: Marco Felsch Cc: zhang.chunyan, Liam Girdwood, Mark Brown, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-23 18:02 ` Doug Anderson @ 2019-09-23 18:14 ` Mark Brown 2019-09-23 18:36 ` Doug Anderson 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-09-23 18:14 UTC (permalink / raw) To: Doug Anderson Cc: Marco Felsch, zhang.chunyan, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-23 18:14 ` Mark Brown @ 2019-09-23 18:36 ` Doug Anderson 2019-09-23 18:49 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Doug Anderson @ 2019-09-23 18:36 UTC (permalink / raw) To: Mark Brown Cc: Marco Felsch, zhang.chunyan, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-23 18:36 ` Doug Anderson @ 2019-09-23 18:49 ` Mark Brown 2019-09-23 22:40 ` Doug Anderson 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-09-23 18:49 UTC (permalink / raw) To: Doug Anderson Cc: Marco Felsch, zhang.chunyan, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-23 18:49 ` Mark Brown @ 2019-09-23 22:40 ` Doug Anderson 2019-09-24 18:27 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Doug Anderson @ 2019-09-23 22:40 UTC (permalink / raw) To: Mark Brown Cc: Marco Felsch, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-23 22:40 ` Doug Anderson @ 2019-09-24 18:27 ` Mark Brown 2019-09-26 19:44 ` Doug Anderson 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-09-24 18:27 UTC (permalink / raw) To: Doug Anderson Cc: Marco Felsch, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-24 18:27 ` Mark Brown @ 2019-09-26 19:44 ` Doug Anderson 2019-09-27 8:47 ` Marco Felsch 0 siblings, 1 reply; 34+ messages in thread From: Doug Anderson @ 2019-09-26 19:44 UTC (permalink / raw) To: Mark Brown, Marco Felsch Cc: Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-26 19:44 ` Doug Anderson @ 2019-09-27 8:47 ` Marco Felsch 2019-10-01 19:57 ` Doug Anderson 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-09-27 8:47 UTC (permalink / raw) To: Doug Anderson Cc: Mark Brown, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 | ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-09-27 8:47 ` Marco Felsch @ 2019-10-01 19:57 ` Doug Anderson 2019-10-04 6:34 ` Matti Vaittinen 2019-10-07 9:34 ` Marco Felsch 0 siblings, 2 replies; 34+ messages in thread From: Doug Anderson @ 2019-10-01 19:57 UTC (permalink / raw) To: Marco Felsch Cc: Mark Brown, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-01 19:57 ` Doug Anderson @ 2019-10-04 6:34 ` Matti Vaittinen 2019-10-04 11:32 ` Mark Brown 2019-10-07 9:34 ` Marco Felsch 1 sibling, 1 reply; 34+ messages in thread From: Matti Vaittinen @ 2019-10-04 6:34 UTC (permalink / raw) To: Doug Anderson Cc: Marco Felsch, Mark Brown, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 =] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-04 6:34 ` Matti Vaittinen @ 2019-10-04 11:32 ` Mark Brown 2019-10-04 12:03 ` Vaittinen, Matti 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-10-04 11:32 UTC (permalink / raw) To: Matti Vaittinen Cc: Doug Anderson, Marco Felsch, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-04 11:32 ` Mark Brown @ 2019-10-04 12:03 ` Vaittinen, Matti 2019-10-04 15:01 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Vaittinen, Matti @ 2019-10-04 12:03 UTC (permalink / raw) To: broonie Cc: lgirdwood, dianders, m.felsch, kernel, zhang.chunyan, linux-kernel, ckeepax 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-04 12:03 ` Vaittinen, Matti @ 2019-10-04 15:01 ` Mark Brown 0 siblings, 0 replies; 34+ messages in thread From: Mark Brown @ 2019-10-04 15:01 UTC (permalink / raw) To: Vaittinen, Matti Cc: lgirdwood, dianders, m.felsch, kernel, zhang.chunyan, linux-kernel, ckeepax [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-01 19:57 ` Doug Anderson 2019-10-04 6:34 ` Matti Vaittinen @ 2019-10-07 9:34 ` Marco Felsch 2019-10-07 18:29 ` Mark Brown 1 sibling, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-10-07 9:34 UTC (permalink / raw) To: Doug Anderson Cc: Mark Brown, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 | ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-07 9:34 ` Marco Felsch @ 2019-10-07 18:29 ` Mark Brown 2019-10-08 6:03 ` Marco Felsch 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-10-07 18:29 UTC (permalink / raw) To: Marco Felsch Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-07 18:29 ` Mark Brown @ 2019-10-08 6:03 ` Marco Felsch 2019-10-08 12:51 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-10-08 6:03 UTC (permalink / raw) To: Mark Brown Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 | ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 6:03 ` Marco Felsch @ 2019-10-08 12:51 ` Mark Brown 2019-10-08 14:56 ` Marco Felsch 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-10-08 12:51 UTC (permalink / raw) To: Marco Felsch Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 12:51 ` Mark Brown @ 2019-10-08 14:56 ` Marco Felsch 2019-10-08 15:42 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-10-08 14:56 UTC (permalink / raw) To: Mark Brown Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 14:56 ` Marco Felsch @ 2019-10-08 15:42 ` Mark Brown 2019-10-08 16:16 ` Marco Felsch 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-10-08 15:42 UTC (permalink / raw) To: Marco Felsch Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 15:42 ` Mark Brown @ 2019-10-08 16:16 ` Marco Felsch 2019-10-08 16:23 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-10-08 16:16 UTC (permalink / raw) To: Mark Brown Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 16:16 ` Marco Felsch @ 2019-10-08 16:23 ` Mark Brown 2019-10-08 20:16 ` Marco Felsch 0 siblings, 1 reply; 34+ messages in thread From: Mark Brown @ 2019-10-08 16:23 UTC (permalink / raw) To: Marco Felsch Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 16:23 ` Mark Brown @ 2019-10-08 20:16 ` Marco Felsch 2019-10-09 9:54 ` Mark Brown 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-10-08 20:16 UTC (permalink / raw) To: Mark Brown Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage 2019-10-08 20:16 ` Marco Felsch @ 2019-10-09 9:54 ` Mark Brown 0 siblings, 0 replies; 34+ messages in thread From: Mark Brown @ 2019-10-09 9:54 UTC (permalink / raw) To: Marco Felsch Cc: Doug Anderson, Chunyan Zhang, Liam Girdwood, ckeepax, LKML, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing 2019-09-17 15:40 [PATCH 0/3] Regulator core fixes Marco Felsch 2019-09-17 15:40 ` [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage Marco Felsch @ 2019-09-17 15:40 ` Marco Felsch 2019-09-17 16:02 ` Applied "regulator: of: fix suspend-min/max-voltage parsing" to the regulator tree Mark Brown 2019-09-17 15:40 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Marco Felsch 2 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-09-17 15:40 UTC (permalink / raw) To: zhang.chunyan, dianders, lgirdwood, broonie, ckeepax; +Cc: linux-kernel, kernel 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Applied "regulator: of: fix suspend-min/max-voltage parsing" to the regulator tree 2019-09-17 15:40 ` [PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing Marco Felsch @ 2019-09-17 16:02 ` Mark Brown 0 siblings, 0 replies; 34+ messages in thread From: Mark Brown @ 2019-09-17 16:02 UTC (permalink / raw) To: Marco Felsch Cc: broonie, ckeepax, dianders, kernel, lgirdwood, linux-kernel, Mark Brown, zhang.chunyan 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-17 15:40 [PATCH 0/3] Regulator core fixes Marco Felsch 2019-09-17 15:40 ` [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage Marco Felsch 2019-09-17 15:40 ` [PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing Marco Felsch @ 2019-09-17 15:40 ` Marco Felsch 2019-09-17 16:02 ` Applied "regulator: core: make regulator_register() EPROBE_DEFER aware" to the regulator tree Mark Brown 2019-09-18 0:57 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Dmitry Torokhov 2 siblings, 2 replies; 34+ messages in thread From: Marco Felsch @ 2019-09-17 15:40 UTC (permalink / raw) To: zhang.chunyan, dianders, lgirdwood, broonie, ckeepax; +Cc: linux-kernel, kernel 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Applied "regulator: core: make regulator_register() EPROBE_DEFER aware" to the regulator tree 2019-09-17 15:40 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Marco Felsch @ 2019-09-17 16:02 ` Mark Brown 2019-09-18 0:57 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Dmitry Torokhov 1 sibling, 0 replies; 34+ messages in thread From: Mark Brown @ 2019-09-17 16:02 UTC (permalink / raw) To: Marco Felsch Cc: broonie, ckeepax, dianders, kernel, lgirdwood, linux-kernel, Mark Brown, zhang.chunyan 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-17 15:40 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Marco Felsch 2019-09-17 16:02 ` Applied "regulator: core: make regulator_register() EPROBE_DEFER aware" to the regulator tree Mark Brown @ 2019-09-18 0:57 ` Dmitry Torokhov 2019-09-18 8:18 ` Marco Felsch 1 sibling, 1 reply; 34+ messages in thread From: Dmitry Torokhov @ 2019-09-18 0:57 UTC (permalink / raw) To: Marco Felsch Cc: zhang.chunyan, Doug Anderson, Liam Girdwood, Mark Brown, ckeepax, lkml, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-18 0:57 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Dmitry Torokhov @ 2019-09-18 8:18 ` Marco Felsch 2019-09-18 15:53 ` Dmitry Torokhov 0 siblings, 1 reply; 34+ messages in thread From: Marco Felsch @ 2019-09-18 8:18 UTC (permalink / raw) To: Dmitry Torokhov Cc: zhang.chunyan, Doug Anderson, Liam Girdwood, Mark Brown, ckeepax, lkml, Sascha Hauer 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 | ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-18 8:18 ` Marco Felsch @ 2019-09-18 15:53 ` Dmitry Torokhov 2019-09-18 16:06 ` Marco Felsch 2019-09-18 16:08 ` Mark Brown 0 siblings, 2 replies; 34+ messages in thread From: Dmitry Torokhov @ 2019-09-18 15:53 UTC (permalink / raw) To: Marco Felsch Cc: zhang.chunyan, Doug Anderson, Liam Girdwood, Mark Brown, ckeepax, lkml, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-18 15:53 ` Dmitry Torokhov @ 2019-09-18 16:06 ` Marco Felsch 2019-09-18 16:08 ` Mark Brown 1 sibling, 0 replies; 34+ messages in thread From: Marco Felsch @ 2019-09-18 16:06 UTC (permalink / raw) To: Dmitry Torokhov Cc: ckeepax, lkml, Liam Girdwood, Doug Anderson, Mark Brown, Sascha Hauer, zhang.chunyan 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 | ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware 2019-09-18 15:53 ` Dmitry Torokhov 2019-09-18 16:06 ` Marco Felsch @ 2019-09-18 16:08 ` Mark Brown 1 sibling, 0 replies; 34+ messages in thread From: Mark Brown @ 2019-09-18 16:08 UTC (permalink / raw) To: Dmitry Torokhov Cc: Marco Felsch, zhang.chunyan, Doug Anderson, Liam Girdwood, ckeepax, lkml, Sascha Hauer [-- 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 --] ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-10-09 9:54 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-17 15:40 [PATCH 0/3] Regulator core fixes Marco Felsch 2019-09-17 15:40 ` [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage Marco Felsch 2019-09-23 18:02 ` Doug Anderson 2019-09-23 18:14 ` Mark Brown 2019-09-23 18:36 ` Doug Anderson 2019-09-23 18:49 ` Mark Brown 2019-09-23 22:40 ` Doug Anderson 2019-09-24 18:27 ` Mark Brown 2019-09-26 19:44 ` Doug Anderson 2019-09-27 8:47 ` Marco Felsch 2019-10-01 19:57 ` Doug Anderson 2019-10-04 6:34 ` Matti Vaittinen 2019-10-04 11:32 ` Mark Brown 2019-10-04 12:03 ` Vaittinen, Matti 2019-10-04 15:01 ` Mark Brown 2019-10-07 9:34 ` Marco Felsch 2019-10-07 18:29 ` Mark Brown 2019-10-08 6:03 ` Marco Felsch 2019-10-08 12:51 ` Mark Brown 2019-10-08 14:56 ` Marco Felsch 2019-10-08 15:42 ` Mark Brown 2019-10-08 16:16 ` Marco Felsch 2019-10-08 16:23 ` Mark Brown 2019-10-08 20:16 ` Marco Felsch 2019-10-09 9:54 ` Mark Brown 2019-09-17 15:40 ` [PATCH 2/3] regulator: of: fix suspend-min/max-voltage parsing Marco Felsch 2019-09-17 16:02 ` Applied "regulator: of: fix suspend-min/max-voltage parsing" to the regulator tree Mark Brown 2019-09-17 15:40 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Marco Felsch 2019-09-17 16:02 ` Applied "regulator: core: make regulator_register() EPROBE_DEFER aware" to the regulator tree Mark Brown 2019-09-18 0:57 ` [PATCH 3/3] regulator: core: make regulator_register() EPROBE_DEFER aware Dmitry Torokhov 2019-09-18 8:18 ` Marco Felsch 2019-09-18 15:53 ` Dmitry Torokhov 2019-09-18 16:06 ` Marco Felsch 2019-09-18 16:08 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).