[PATCH resend 0/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[PATCH resend 0/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

Hi Mark,

And here is a resend of a non bug-fix core patch + a follow-up patch for
the axp20x regulator driver.

See the commit message of the first patch for the why and how of this
patch-set.

Regards,

Hans

[PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

The axp20x and axp22x pmics have ldo regulators which are muxed to the
outside via gpio pins. Unfortunately regulator enable / disable is
implemented in the hardware via selecting a specific pin-mux option.

So if we want to use these pins as gpio pins we must not register
a regulator for these pins at all, otherwise any gpio use (switching
to input, or writing a value) gets undone when the regulator subsys
disables unused regulators at the end of kernel-init.

This commits allows the use of  "status = disabled" in regulator dts
nodes and makes regulator_register return ENODEV when this is set.

Note that this commit changes the loop to find the of-node in
regulator_of_get_init_data from using for_each_available_child_of_node
to using for_each_child_of_node. regulator_register is the only user
of regulator_of_get_init_data and the use of for_each_available_child...
makes little sense there since this will only cause the constraints
from regulator dts nodes marked as disabled to not be used, the
actual registration of the regulator would still continue.

So in a way this patch could be seen as a bugfix as it actually makes
regulators with an of_node which is marked as not available not register,
but this behavior change may cause some issues in some places.

Note that individual regulator drivers / callers of regulator_register
which may encounter disabled regulator (child) nodes need to be patched to
handle ENODEV (to not make it fail their probe method).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/regulator/core.c         | 6 ++++++
 drivers/regulator/of_regulator.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0b7642..8062c83 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3911,6 +3911,12 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		rdev->dev.of_node = of_node_get(config->of_node);
 	}
 
+	if (rdev->dev.of_node && !of_device_is_available(rdev->dev.of_node)) {
+		kfree(config);
+		kfree(rdev);
+		return ERR_PTR(-ENODEV);
+	}
+
 	mutex_lock(&regulator_list_mutex);
 
 	mutex_init(&rdev->mutex);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 6b0aa80..7af6e17 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -315,7 +315,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		return NULL;
 	}
 
-	for_each_available_child_of_node(search, child) {
+	for_each_child_of_node(search, child) {
 		name = of_get_property(child, "regulator-compatible", NULL);
 		if (!name)
 			name = child->name;
-- 
2.7.4

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Mark Brown]

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

On Wed, Apr 27, 2016 at 04:03:44PM +0200, Hans de Goede wrote:

> So if we want to use these pins as gpio pins we must not register
> a regulator for these pins at all, otherwise any gpio use (switching
> to input, or writing a value) gets undone when the regulator subsys
> disables unused regulators at the end of kernel-init.

The regulator API should not touch any regulators that it doesn't have
permission to change the state for.  All other regulators are strictly
read only.  

> This commits allows the use of  "status = disabled" in regulator dts
> nodes and makes regulator_register return ENODEV when this is set.

If the regulator can't be changed why is it in the DT in the first
place?

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

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

Hi,

On 27-04-16 16:24, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 04:03:44PM +0200, Hans de Goede wrote:
>
>> So if we want to use these pins as gpio pins we must not register
>> a regulator for these pins at all, otherwise any gpio use (switching
>> to input, or writing a value) gets undone when the regulator subsys
>> disables unused regulators at the end of kernel-init.
>
> The regulator API should not touch any regulators that it doesn't have
> permission to change the state for.  All other regulators are strictly
> read only.

How do we give permission to change state ? Is omitting the dts node,
and thus not returning a node / constrains from regulator_of_get_init_data
enough for the regulator API to not have permission ?

Is there any way to see this in sysfs ?

>> This commits allows the use of  "status = disabled" in regulator dts
>> nodes and makes regulator_register return ENODEV when this is set.
>
> If the regulator can't be changed why is it in the DT in the first
> place?

The regulator is part of the pmic and the axp20x regulator driver
registers all regulators on the pmic when the pmic-s mfd instantiated
regulators-platform-device gets probed.

We do use a whole bunch of the other regulators. This patch-set
is an attempt to make the control more fine-grained then register
all / no regulators by support status=disabled in the regulator
nodes. But maybe I'm missing something and this is not necessary,
see the earlier part of this reply.

Regards,

Hans

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Mark Brown]

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

On Wed, Apr 27, 2016 at 04:31:02PM +0200, Hans de Goede wrote:
> On 27-04-16 16:24, Mark Brown wrote:

> >The regulator API should not touch any regulators that it doesn't have
> >permission to change the state for.  All other regulators are strictly
> >read only.

> How do we give permission to change state ? Is omitting the dts node,
> and thus not returning a node / constrains from regulator_of_get_init_data
> enough for the regulator API to not have permission ?

Yes, omit the DT node or mark it always on at the minute.

> Is there any way to see this in sysfs ?

Not off the top of my head, but it's in debugfs.

> >If the regulator can't be changed why is it in the DT in the first
> >place?

> The regulator is part of the pmic and the axp20x regulator driver
> registers all regulators on the pmic when the pmic-s mfd instantiated
> regulators-platform-device gets probed.

That's the driver, that's not the DT.  Drivers should always register
all the regulators in the device they're for.

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

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

Hi,

On 27-04-16 16:37, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 04:31:02PM +0200, Hans de Goede wrote:
>> On 27-04-16 16:24, Mark Brown wrote:
>
>>> The regulator API should not touch any regulators that it doesn't have
>>> permission to change the state for.  All other regulators are strictly
>>> read only.
>
>> How do we give permission to change state ? Is omitting the dts node,
>> and thus not returning a node / constrains from regulator_of_get_init_data
>> enough for the regulator API to not have permission ?
>
> Yes, omit the DT node or mark it always on at the minute.

Or, since regulator_of_get_init_data uses for_each_available_child_of_node
which checks the "status" value we can actually already use
"status=disabled" cool. So this simply already works :)

Regards,

Hans

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Mark Brown]

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

On Wed, Apr 27, 2016 at 04:40:05PM +0200, Hans de Goede wrote:

> Or, since regulator_of_get_init_data uses for_each_available_child_of_node
> which checks the "status" value we can actually already use
> "status=disabled" cool. So this simply already works :)

To repeat you really shouldn't have *any* DT nodes for regulators that
aren't in use, there should be nothing to put in their nodes.  If
there's anything there that's a sign that your DT has problems.

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

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Maxime Ripard]

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

Hi Mark,

On Wed, Apr 27, 2016 at 04:01:08PM +0100, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 04:40:05PM +0200, Hans de Goede wrote:
> 
> > Or, since regulator_of_get_init_data uses for_each_available_child_of_node
> > which checks the "status" value we can actually already use
> > "status=disabled" cool. So this simply already works :)
> 
> To repeat you really shouldn't have *any* DT nodes for regulators that
> aren't in use, there should be nothing to put in their nodes.  If
> there's anything there that's a sign that your DT has problems.

How should we deal with regulators that are on by default but are not
used in the system then?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

Hi,

On 27-04-16 17:50, Maxime Ripard wrote:
> Hi Mark,
>
> On Wed, Apr 27, 2016 at 04:01:08PM +0100, Mark Brown wrote:
>> On Wed, Apr 27, 2016 at 04:40:05PM +0200, Hans de Goede wrote:
>>
>>> Or, since regulator_of_get_init_data uses for_each_available_child_of_node
>>> which checks the "status" value we can actually already use
>>> "status=disabled" cool. So this simply already works :)
>>
>> To repeat you really shouldn't have *any* DT nodes for regulators that
>> aren't in use, there should be nothing to put in their nodes.  If
>> there's anything there that's a sign that your DT has problems.
>
> How should we deal with regulators that are on by default but are not
> used in the system then?

I think we've already solved that one, we do list them, thereby giving the
regulator core permission to touch them and then let the regulator core
turn them off for us.

Regards,

Hans

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Hans de Goede]

Hi,

On 27-04-16 17:52, Hans de Goede wrote:
> Hi,
>
> On 27-04-16 17:50, Maxime Ripard wrote:
>> Hi Mark,
>>
>> On Wed, Apr 27, 2016 at 04:01:08PM +0100, Mark Brown wrote:
>>> On Wed, Apr 27, 2016 at 04:40:05PM +0200, Hans de Goede wrote:
>>>
>>>> Or, since regulator_of_get_init_data uses for_each_available_child_of_node
>>>> which checks the "status" value we can actually already use
>>>> "status=disabled" cool. So this simply already works :)
>>>
>>> To repeat you really shouldn't have *any* DT nodes for regulators that
>>> aren't in use, there should be nothing to put in their nodes.  If
>>> there's anything there that's a sign that your DT has problems.
>>
>> How should we deal with regulators that are on by default but are not
>> used in the system then?
>
> I think we've already solved that one, we do list them, thereby giving the
> regulator core permission to touch them and then let the regulator core
> turn them off for us.

To clarify, I do not believe that this is not about not having nodes for
unused regulators, but about not having nodes for regulators which should not
be touched.

Regards,

Hans

Re: [PATCH 1/2] regulator: core: Allow use of "status = disabled" in regulator dts nodes

[Mark Brown]

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

On Wed, Apr 27, 2016 at 05:54:48PM +0200, Hans de Goede wrote:
> On 27-04-16 17:52, Hans de Goede wrote:
> >On 27-04-16 17:50, Maxime Ripard wrote:
> >>On Wed, Apr 27, 2016 at 04:01:08PM +0100, Mark Brown wrote:

> >>>To repeat you really shouldn't have *any* DT nodes for regulators that
> >>>aren't in use, there should be nothing to put in their nodes.  If
> >>>there's anything there that's a sign that your DT has problems.

> >>How should we deal with regulators that are on by default but are not
> >>used in the system then?

> >I think we've already solved that one, we do list them, thereby giving the
> >regulator core permission to touch them and then let the regulator core
> >turn them off for us.

Yes.

> To clarify, I do not believe that this is not about not having nodes for
> unused regulators, but about not having nodes for regulators which should not
> be touched.

Correct.  If we have constraints for a regulator then they should be
accurate.

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

[PATCH 2/2] regulator: axp20x: Handle regulator_register returning ENODEV

[Hans de Goede]

Handle regulator_register returning ENODEV, this may happen when
the dts node for the regulator contains "status = disabled" which
is useful for the ldo_io regulators, to avoid the regulator code
getting in the way of gpio use of these pins.

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

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 5ddaa82..500d162 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -423,6 +423,9 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 
 		rdev = devm_regulator_register(&pdev->dev, desc, &config);
 		if (IS_ERR(rdev)) {
+			if (PTR_ERR(rdev) == -ENODEV)
+				continue;
+
 			dev_err(&pdev->dev, "Failed to register %s\n",
 				regulators[i].name);
 
-- 
2.7.4

Re: [PATCH 2/2] regulator: axp20x: Handle regulator_register returning ENODEV

[Mark Brown]

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

On Wed, Apr 27, 2016 at 04:03:45PM +0200, Hans de Goede wrote:

>  		rdev = devm_regulator_register(&pdev->dev, desc, &config);
>  		if (IS_ERR(rdev)) {
> +			if (PTR_ERR(rdev) == -ENODEV)
> +				continue;
> +

We would need to do this in every single regulator driver which is a bit
of a warning sign too...

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