[PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[Richard Fitzgerald]

The driver was hardcoding REGULATOR_CHANGE_STATUS on the regulator
which made the regulator core assume that it can be powered off.

The power state of the regulator is controlled by the LDOENA pin so
this patch changes to only setting the REGULATOR_CHANGE_STATUS flag
if we have a valid gpio for LDOENA.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/regulator/arizona-ldo1.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index f7c88ff..edeb36b 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -161,16 +161,12 @@ static const struct regulator_init_data arizona_ldo1_dvfs = {
 	.constraints = {
 		.min_uV = 1200000,
 		.max_uV = 1800000,
-		.valid_ops_mask = REGULATOR_CHANGE_STATUS |
-				  REGULATOR_CHANGE_VOLTAGE,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE,
 	},
 	.num_consumer_supplies = 1,
 };
 
 static const struct regulator_init_data arizona_ldo1_default = {
-	.constraints = {
-		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
-	},
 	.num_consumer_supplies = 1,
 };
 
@@ -178,8 +174,7 @@ static const struct regulator_init_data arizona_ldo1_wm5110 = {
 	.constraints = {
 		.min_uV = 1175000,
 		.max_uV = 1200000,
-		.valid_ops_mask = REGULATOR_CHANGE_STATUS |
-				  REGULATOR_CHANGE_VOLTAGE,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE,
 	},
 	.num_consumer_supplies = 1,
 };
@@ -290,9 +285,9 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 	config.ena_gpio = arizona->pdata.ldoena;
 
 	if (arizona->pdata.ldo1)
-		config.init_data = arizona->pdata.ldo1;
-	else
-		config.init_data = &ldo1->init_data;
+		ldo1->init_data = *arizona->pdata.ldo1;
+
+	config.init_data = &ldo1->init_data;
 
 	/*
 	 * LDO1 can only be used to supply DCVDD so if it has no
@@ -301,6 +296,15 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 	if (config.init_data->num_consumer_supplies == 0)
 		arizona->external_dcvdd = true;
 
+	if (!arizona->external_dcvdd &&
+	    (config.ena_gpio || config.ena_gpio_initialized) &&
+	    gpio_is_valid(config.ena_gpio))
+		ldo1->init_data.constraints.valid_ops_mask |=
+			REGULATOR_CHANGE_STATUS;
+	else
+		dev_warn(arizona->dev,
+			 "No LDOENA: regulator will be always-on\n");
+
 	ldo1->regulator = devm_regulator_register(&pdev->dev, desc, &config);
 
 	of_node_put(config.of_node);
-- 
1.9.1

Re: [PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[Mark Brown]

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

On Fri, Apr 22, 2016 at 02:43:28PM +0100, Richard Fitzgerald wrote:
> The driver was hardcoding REGULATOR_CHANGE_STATUS on the regulator
> which made the regulator core assume that it can be powered off.
> 
> The power state of the regulator is controlled by the LDOENA pin so
> this patch changes to only setting the REGULATOR_CHANGE_STATUS flag
> if we have a valid gpio for LDOENA.

What's the difference between this and the previous version of the patch
and what problem is this aiming to solve?  If we want to disable the
regulator why would we not be happy to do that by removing the supply?

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

Re: [PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[Richard Fitzgerald]

On 22/04/16 16:04, Mark Brown wrote:
> On Fri, Apr 22, 2016 at 02:43:28PM +0100, Richard Fitzgerald wrote:
>> The driver was hardcoding REGULATOR_CHANGE_STATUS on the regulator
>> which made the regulator core assume that it can be powered off.
>>
>> The power state of the regulator is controlled by the LDOENA pin so
>> this patch changes to only setting the REGULATOR_CHANGE_STATUS flag
>> if we have a valid gpio for LDOENA.
> What's the difference between this and the previous version of the patch
> and what problem is this aiming to solve?  If we want to disable the
> regulator why would we not be happy to do that by removing the supply?
The background to all this is that runtime suspend and resume needs to 
know whether the DCVDD turned off. If it definitely turned off a regmap 
cache sync is safe - if not or I can't be sure then I need the overhead 
of a forced reset to restore register defaults before the sync.

What I'm trying to achieve here is to stop the regulator core sending 
false notifications that LDO1 has been turned off. The way that the 
regulator core code handles the disable notifier has no dependency on 
what happens to the parent supply. The REGULATOR_CHANGE_STATUS flag is 
used to indicate whether the status of _this_ regulator can be changed 
(it doesn't affect whether the parent is disabled).

So if LDO1 is disabled without an LDOENA and without this patch, it 
looks like the current core behaviour of the functions 
regulator_disable(), _regulator_disable() and _regulator_do_disable() is:

1) Are we the last user? - Yes
2) _regulator_can_change_status()? - Yes because REGULATOR_CHANGE_STATUS 
is set
3) Send PRE_DISABLE notification
4) call _regulator_do_disable(), no GPIO pin and no disable callback so 
return 0 (claims success even though there was no way to disable it)
5) Send REGULATOR_EVENT_DISABLE
6) Return to regulator_disable() and then disable the parent supply

The result will be that we got a disable notification though LDO1 wasn't 
disabled.

I think it's a bug that LDO1 claimed to be able to turn off when it 
couldn't, and fixing that prevents bogus disable notifications.

Re: [PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[Mark Brown]

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

On Fri, Apr 22, 2016 at 05:38:02PM +0100, Richard Fitzgerald wrote:

Please fix your mailer to leave blank lines between paragraphs (and not
delete them in quotes), it makes your mails harder to read.

> On 22/04/16 16:04, Mark Brown wrote:
> >On Fri, Apr 22, 2016 at 02:43:28PM +0100, Richard Fitzgerald wrote:

> >What's the difference between this and the previous version of the patch
> >and what problem is this aiming to solve?  If we want to disable the
> >regulator why would we not be happy to do that by removing the supply?

> The background to all this is that runtime suspend and resume needs to know
> whether the DCVDD turned off. If it definitely turned off a regmap cache
> sync is safe - if not or I can't be sure then I need the overhead of a
> forced reset to restore register defaults before the sync.

This is what regulator status change notifiers are for, register and
then you'll get a callback when the power is actually pulled (there's a
few other CODEC drivers that use them).

> What I'm trying to achieve here is to stop the regulator core sending false
> notifications that LDO1 has been turned off. The way that the regulator core

If you've found a problem with spurious notifications then fix the
spurious notifications, don't pile bodges into consumer drivers!  Every
single other driver that relies on these notifications is going to want
the same hack for the same reason though most of them aren't their own
supply so won't be able to do it.  The advantage of being able to change
the source code for the entire kernel is that we don't need to have
workarounds for the core in drivers, we can make the core do the right
thing.

> code handles the disable notifier has no dependency on what happens to the
> parent supply. The REGULATOR_CHANGE_STATUS flag is used to indicate whether
> the status of _this_ regulator can be changed (it doesn't affect whether the
> parent is disabled).

If the child can't change status then the disable can't propagate up the
tree and the child regulator needs to hold the parent enabled.

> I think it's a bug that LDO1 claimed to be able to turn off when it couldn't,
> and fixing that prevents bogus disable notifications.

How does the driver know it couldn't turn off the parent, it knows
nothing about the supply for LDO1?  If that's switchable you've just
removed the ability to switch it off since the rail will now never power
down.

Regulators with no enable control of their own need to not do their own
notifications but instead get notifications based on parent status
changes.

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

Re: [PATCH] regulator: arizona-ldo1: Only enable status change if we have LDOENA

[Richard Fitzgerald]

On 25/04/16 00:23, Mark Brown wrote:
> On Fri, Apr 22, 2016 at 05:38:02PM +0100, Richard Fitzgerald wrote:
>
> Please fix your mailer to leave blank lines between paragraphs (and not
> delete them in quotes), it makes your mails harder to read.
>
>> On 22/04/16 16:04, Mark Brown wrote:
>>> On Fri, Apr 22, 2016 at 02:43:28PM +0100, Richard Fitzgerald wrote:
>>> What's the difference between this and the previous version of the patch
>>> and what problem is this aiming to solve?  If we want to disable the
>>> regulator why would we not be happy to do that by removing the supply?
>> The background to all this is that runtime suspend and resume needs to know
>> whether the DCVDD turned off. If it definitely turned off a regmap cache
>> sync is safe - if not or I can't be sure then I need the overhead of a
>> forced reset to restore register defaults before the sync.
> This is what regulator status change notifiers are for, register and
> then you'll get a callback when the power is actually pulled (there's a
> few other CODEC drivers that use them).
>
Maybe what they were supposed to be for but the _DISABLE notifier 
doesn't appear to do this. It seems to get sent if regulator_disable() 
was called, regardless of whether that regulator was disabled, and not 
sent if a parent supply was turned off.
>> What I'm trying to achieve here is to stop the regulator core sending false
>> notifications that LDO1 has been turned off. The way that the regulator core
> If you've found a problem with spurious notifications then fix the
> spurious notifications, don't pile bodges into consumer drivers!  Every
> single other driver that relies on these notifications is going to want
> the same hack for the same reason though most of them aren't their own
> supply so won't be able to do it.  The advantage of being able to change
> the source code for the entire kernel is that we don't need to have
> workarounds for the core in drivers, we can make the core do the right
> thing.
It didn't look to me like a problem with spurious notifications, it 
looked like the LDO1 regulator not reporting its capabilities correctly 
(leading to the core thinking it can turn it off when it can't). But now 
I've looked several times at the regulator core I see that 
REGULATOR_CHANGE_STATUS means the same as always_on ("must not turn 
off"), rather than as ambiguously documented "can turn off". Given my 
misunderstanding my patch was ok no a bodge, but as the flag doesn't do 
what I thought the patch is definitely wrong.

>> code handles the disable notifier has no dependency on what happens to the
>> parent supply. The REGULATOR_CHANGE_STATUS flag is used to indicate whether
>> the status of _this_ regulator can be changed (it doesn't affect whether the
>> parent is disabled).
> If the child can't change status then the disable can't propagate up the
> tree and the child regulator needs to hold the parent enabled.
>
>> I think it's a bug that LDO1 claimed to be able to turn off when it couldn't,
>> and fixing that prevents bogus disable notifications.
> How does the driver know it couldn't turn off the parent, it knows
> nothing about the supply for LDO1?  If that's switchable you've just
> removed the ability to switch it off since the rail will now never power
> down.
>
> Regulators with no enable control of their own need to not do their own
> notifications but instead get notifications based on parent status
> changes.
It seems the real problem is an assumption that REGULATOR_EVENT_DISABLE 
means the supply was actually turned off. It's unclear from the 
description in the header ("Regulator was disabled.") what the intention 
of this notification was. The existing users of this call all seem to 
think it means actually turned off, so they all have a problem in the 
case where the supply was actually pulled by disabling a parent supply 
(which doesn't currently send a notifier).