Problems while designing TPS65023 regulator driver

Commit Message

David Brownell March 8, 2009, 8:54 p.m. UTC

On Saturday 07 March 2009, Mark Brown wrote:
> What's happening here is that the kernel is making sure that the
> information it was given about the state of the regulator is actually
> true in case it was important ...

If that's a goal, then I suggest merging the appended patch,
which addresses a similar case:  both boot_on and always_on
are clear, so the regulator should not be enabled.

This *can* be important, as e.g. if those flags are clear
but the bootloader turned the regulator on, then drivers
can't disable the regulator (on penalty of a stackdump!)
unless they issue a spurious/pointless/undesirable enable()
beforehand ...

- Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Brown March 8, 2009, 10:41 p.m. UTC | #1

On Sun, Mar 08, 2009 at 12:54:35PM -0800, David Brownell wrote:

> but the bootloader turned the regulator on, then drivers
> can't disable the regulator (on penalty of a stackdump!)
> unless they issue a spurious/pointless/undesirable enable()
> beforehand ...

We can't easily have both reference counting and unbalanced disables,
sadly.

>  - Regulators not marked as "boot_on" or "always_on" won't
>    be active (and usecount will be 0) on return from setup.

This breaks the idea that we don't do anything unless explictly told to
do so.  I did actually still consider adding code to power off the
regulator but thought that there may also be situations where the state
really is unknown (eg, it depends on what the system booted from) and
it'd be useful to be able to punt to the consumers to figure it out.

I'm a bit ambivalent on this one, though - avoiding a sprawl of options
is certainly neater.  An enum for the initial power state has an appeal
here.

> -	/* are we enabled at boot time by firmware / bootloader */
> -	if (rdev->constraints->boot_on)
> -		rdev->use_count = 1;
> -

That's not there with the current regulator tree (this was the bug with
not being able to disable boot_on regulators, there's no way to drop
that use count later on).

Much of the rest of your patch will fail to apply due to similar
changes; the logic that's there now is roughly the same as what you have
here except we don't bother to check is_enabled() any more (no harm
adding that back, it'd be useful if enable() can't be called for an
already enabled regulator) and we don't disable the regulator.

> +		} else if (is_enabled < 0) {
> +			pr_warning("%s: hoping regulator '%s' is %sd...\n",
> +				       __func__, name, "enable");
> +		}

I'm really not loving this %s for the enabled - yes, it'll save a small
amount of memory but it hurts gepability.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

========== CUT HERE
From: David Brownell <[e-mail]>

Make the regulator setup code simpler and more consistent:

 - The only difference between "boot_on" and "always_on" is
   that an "always_on" regulator won't be disabled.  Both will
   be active (and usecount will be 1) on return from setup.

 - Regulators not marked as "boot_on" or "always_on" won't
   be active (and usecount will be 0) on return from setup.

The exception to that simple policy is when there's a non-Linux
interface to the regulator ... e.g. if either a DSP or the CPU
running Linux can enable the regulator, and the DSP needs it to
be on, then it will be on.

Signed-off-by: David Brownell <[e-mail]>
---
 drivers/regulator/core.c |   62 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 15 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -711,6 +711,8 @@  static int set_machine_constraints(struc
 	int ret = 0;
 	const char *name;
 	struct regulator_ops *ops = rdev->desc->ops;
+	int enable = 0;
+	int is_enabled = -ENOSYS;
 
 	if (constraints->name)
 		name = constraints->name;
@@ -799,10 +801,6 @@  static int set_machine_constraints(struc
 			}
 	}
 
-	/* are we enabled at boot time by firmware / bootloader */
-	if (rdev->constraints->boot_on)
-		rdev->use_count = 1;
-
 	/* do we need to setup our suspend state */
 	if (constraints->initial_state) {
 		ret = suspend_prepare(rdev, constraints->initial_state);
@@ -814,17 +812,51 @@  static int set_machine_constraints(struc
 		}
 	}
 
-	/* if always_on is set then turn the regulator on if it's not
-	 * already on. */
-	if (constraints->always_on && ops->enable &&
-	    ((ops->is_enabled && !ops->is_enabled(rdev)) ||
-	     (!ops->is_enabled && !constraints->boot_on))) {
-		ret = ops->enable(rdev);
-		if (ret < 0) {
-			printk(KERN_ERR "%s: failed to enable %s\n",
-			       __func__, name);
-			rdev->constraints = NULL;
-			goto out;
+	/* Should this be enabled when we return from here?  The difference
+	 * between "boot_on" and "always_on" is that "always_on" regulators
+	 * won't ever be disabled.
+	 */
+	if (constraints->boot_on || constraints->always_on)
+		enable = 1;
+
+	/* Make sure the regulator isn't wrongly enabled or disabled.
+	 * Bootloaders are often sloppy about leaving things on; and
+	 * sometimes Linux wants to use a different model.
+	 */
+	if (ops->is_enabled)
+		is_enabled = ops->is_enabled(rdev);
+	if (enable) {
+		if (ops->enable) {
+			/* forcibly enable if it's off or we can't tell */
+			if (is_enabled <= 0) {
+				ret = ops->enable(rdev);
+				pr_warning("%s: %s '%s' --> %d\n",
+					       __func__, "enable", name, ret);
+				if (ret < 0) {
+					rdev->constraints = NULL;
+					goto out;
+				}
+			}
+		} else if (is_enabled < 0) {
+			pr_warning("%s: hoping regulator '%s' is %sd...\n",
+				       __func__, name, "enable");
+		}
+		rdev->use_count = 1;
+	} else {
+		if (ops->disable) {
+			/* forcibly disable if it's on or we can't tell */
+			if (is_enabled != 0) {
+				ret = ops->disable(rdev);
+				pr_warning("%s: %s '%s' --> %d\n",
+					       __func__, "disable", name, ret);
+				if (ret < 0) {
+					rdev->constraints = NULL;
+					goto out;
+				}
+			}
+		} else if (is_enabled < 0) {
+			pr_warning("%s: hoping regulator '%s' is %sd...\n",
+				       __func__, name, "disable");
 		}
 	}