[PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[Laxman Dewangan]

From: Laxman Dewangan <ldewangan@nvidia.com>

The given rail is said to be enabled only if this rail is eanbled
along with supply rail.
Adding check for the supply rail whether it is enabled or not when
query about rail enabled.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
When consumer of any rails query about whether rail is enabled
or not, the function regulator_is_enabled() should return enabled
only if this rail and supply rail (both) are enabled.
if any one of rail, whether the given rail or supply rail, is enabled
then function should return as not enabled.

 drivers/regulator/core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dbdebed..d914435 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1701,6 +1701,12 @@ int regulator_is_enabled(struct regulator *regulator)
 {
 	int ret;
 
+	if (regulator->rdev->supply) {
+		ret = regulator_is_enabled(regulator->rdev->supply);
+		if (ret <= 0)
+			return ret;
+	}
+
 	mutex_lock(&regulator->rdev->mutex);
 	ret = _regulator_is_enabled(regulator->rdev);
 	mutex_unlock(&regulator->rdev->mutex);
-- 
1.7.1.1

Re: [PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[Mark Brown]

On Tue, Jan 03, 2012 at 11:39:43AM +0530, Laxman Dewangan wrote:
> From: Laxman Dewangan <ldewangan@nvidia.com>
> 
> The given rail is said to be enabled only if this rail is eanbled
> along with supply rail.
> Adding check for the supply rail whether it is enabled or not when
> query about rail enabled.

This feels wrong - the code in general assumes that the parents will all
be enabled for an enabled child (and does the required stuff on enable
and disable).  Doing the check isn't unreasonable but if it fails we
really ought to be complaining loudly as we're probably confused and
things might be going wrong elsewhere.

We should also look at the bootstrapping code, we're not really making
much effort to verify that the hardware configuration on boot is sane
(though realistically it's unlikely that it won't be).

> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> When consumer of any rails query about whether rail is enabled
> or not, the function regulator_is_enabled() should return enabled
> only if this rail and supply rail (both) are enabled.
> if any one of rail, whether the given rail or supply rail, is enabled
> then function should return as not enabled.

Please just put things in the changelog if they're useful.

RE: [PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[Laxman Dewangan]

> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Wednesday, January 04, 2012 1:46 AM +0530
> >
> > The given rail is said to be enabled only if this rail is eanbled
> > along with supply rail.
> > Adding check for the supply rail whether it is enabled or not when
> > query about rail enabled.
> 
> This feels wrong - the code in general assumes that the parents will all
> be enabled for an enabled child (and does the required stuff on enable
> and disable).  Doing the check isn't unreasonable but if it fails we
> really ought to be complaining loudly as we're probably confused and
> things might be going wrong elsewhere.
>
Yes, in general checking of the parent rail is not really required but it is
good to have on safer side because it directly checks at the interface of
regulator level rather than the ref count and give the more correct results.

I came to this issue when I set the supply regulator for a rail and set the rail
is always on but the supply regulator was not always on. And so this function
returns true but actually the rail is not enabled because supply rail was not enabled.
Although it is fixed in other patch but such checks help more.

> We should also look at the bootstrapping code, we're not really making
> much effort to verify that the hardware configuration on boot is sane
> (though realistically it's unlikely that it won't be).
> 
> > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> > ---
> > When consumer of any rails query about whether rail is enabled
> > or not, the function regulator_is_enabled() should return enabled
> > only if this rail and supply rail (both) are enabled.
> > if any one of rail, whether the given rail or supply rail, is enabled
> > then function should return as not enabled.
> 
> Please just put things in the changelog if they're useful.

I already send the 2nd patch for putting this info on changelog.

Thanks,
Laxman

Re: [PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[Mark Brown]

On Wed, Jan 04, 2012 at 11:49:14AM +0530, Laxman Dewangan wrote:

> > This feels wrong - the code in general assumes that the parents will all
> > be enabled for an enabled child (and does the required stuff on enable
> > and disable).  Doing the check isn't unreasonable but if it fails we
> > really ought to be complaining loudly as we're probably confused and
> > things might be going wrong elsewhere.

> returns true but actually the rail is not enabled because supply rail was not enabled.
> Although it is fixed in other patch but such checks help more.

You're not quite getting my point here - we should be treating this as
an error and complaining about it when we notice it, your patch will
silently mask the condition which seems likely to just cause the bug to
manifest elsewhere.

RE: [PATCH] regulator: Rail is said to be enable only if this and supply rails are enabled.

[Laxman Dewangan]

> From: linux-tegra-owner@vger.kernel.org [mailto:linux-tegra-
> owner@vger.kernel.org] On Behalf Of Mark Brown
> Sent: Thursday, January 05, 2012 11:37 AM


> > returns true but actually the rail is not enabled because supply rail was
> not enabled.
> > Although it is fixed in other patch but such checks help more.
> 
> You're not quite getting my point here - we should be treating this as
> an error and complaining about it when we notice it, your patch will
> silently mask the condition which seems likely to just cause the bug to
> manifest elsewhere.

Agree with you. Thanks for such details.

Thanks,
Laxman