Skip to contentSkip to conversation

regulator: enable/disable refcounting

PATCH In review David Brownell 3 participants 8 messages Nov 16, 2008 – Nov 20, 2008 Voltage and current regulator framework

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 regulator: enable/disable refcounting

From: David Brownell <[email protected]>

Make the <linux/regulator.h> framework treat enable/disable call pairs like the <linux/clk.h> and <linux/interrupt.h> frameworks do:
they're refcounted, so that different parts of a driver don't need to put work into coordination that frameworks normally handle. It's a minor object code shrink.

It also makes the regulator_is_disabled() kerneldoc say what it's actually returning: return value is not a refcount, and may report an error (e.g. I/O error from I2C).

It also fixes some minor regulator_put() goofage: removing unlocked access to the enable state. (But still not making regulator put/get match the refcounting pattern they invoke.)

---
 drivers/regulator/core.c |   71 ++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 35 deletions(-)
drivers/regulator/core.c +36 -35
@@ -79,7 +79,7 @@ struct regulator {
 	int uA_load;
 	int min_uV;
 	int max_uV;
-	int enabled; /* client has called enabled */
+	int enabled; /* count of client enables */
 	char *supply_name;
 	struct device_attribute dev_attr;
 	struct regulator_dev *rdev;
@@ -933,16 +933,13 @@ void regulator_put(struct regulator *reg
 	if (regulator == NULL || IS_ERR(regulator))
 		return;
 
-	if (regulator->enabled) {
-		printk(KERN_WARNING "Releasing supply %s while enabled\n",
-		       regulator->supply_name);
-		WARN_ON(regulator->enabled);
-		regulator_disable(regulator);
-	}
-
 	mutex_lock(&regulator_list_mutex);
 	rdev = regulator->rdev;
 
+	if (WARN(regulator->enabled, "Releasing supply %s while enabled\n",
+			       regulator->supply_name))
+		_regulator_disable(rdev);
+
 	/* remove any sysfs entries */
 	if (regulator->dev) {
 		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
@@ -1012,21 +1009,17 @@ static int _regulator_enable(struct regu
  */
 int regulator_enable(struct regulator *regulator)
 {
-	int ret;
-
-	if (regulator->enabled) {
-		printk(KERN_CRIT "Regulator %s already enabled\n",
-		       regulator->supply_name);
-		WARN_ON(regulator->enabled);
-		return 0;
-	}
+	struct regulator_dev *rdev = regulator->rdev;
+	int ret = 0;
 
-	mutex_lock(&regulator->rdev->mutex);
-	regulator->enabled = 1;
-	ret = _regulator_enable(regulator->rdev);
-	if (ret != 0)
-		regulator->enabled = 0;
-	mutex_unlock(&regulator->rdev->mutex);
+	mutex_lock(&rdev->mutex);
+	if (regulator->enabled == 0)
+		ret = _regulator_enable(rdev);
+	else if (regulator->enabled < 0)
+		ret = -EIO;
+	if (ret == 0)
+		regulator->enabled++;
+	mutex_unlock(&rdev->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
@@ -1078,19 +1071,21 @@ static int _regulator_disable(struct reg
  */
 int regulator_disable(struct regulator *regulator)
 {
-	int ret;
-
-	if (!regulator->enabled) {
-		printk(KERN_ERR "%s: not in use by this consumer\n",
-			__func__);
-		return 0;
-	}
+	struct regulator_dev *rdev = regulator->rdev;
+	int ret = 0;
 
-	mutex_lock(&regulator->rdev->mutex);
-	regulator->enabled = 0;
-	regulator->uA_load = 0;
-	ret = _regulator_disable(regulator->rdev);
-	mutex_unlock(&regulator->rdev->mutex);
+	mutex_lock(&rdev->mutex);
+	if (regulator->enabled == 1) {
+		ret = _regulator_disable(rdev);
+		if (ret == 0)
+			regulator->uA_load = 0;
+	} else if (WARN(regulator->enabled <= 0,
+			"unbalanced disables for supply %s\n",
+			regulator->supply_name))
+		ret = -EIO;
+	if (ret == 0)
+		regulator->enabled--;
+	mutex_unlock(&rdev->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_disable);
@@ -1166,7 +1161,13 @@ out:
  * regulator_is_enabled - is the regulator output enabled
  * @regulator: regulator source
  *
- * Returns zero for disabled otherwise return number of enable requests.
+ * Returns positive if the regulator driver backing the source/client
+ * has requested that the device be enabled, zero if it hasn't, else a
+ * negative errno code.
+ *
+ * Note that the device backing this regulator handle can have multiple
+ * users, so it might be enabled even if regulator_enable() was never
+ * called for this particular source.
  */
 int regulator_is_enabled(struct regulator *regulator)
 {
#1 lore raw

drivers/regulator/core.c static int _regulator_disable(struct reg · line 1081

Quoted 5 lines · David Brownell
drivers/regulator/core.c +5 -0
+	mutex_lock(&rdev->mutex);
+	if (regulator->enabled == 1) {
+		ret = _regulator_disable(rdev);
+		if (ret == 0)
+			regulator->uA_load = 0;

Why clear the load here? I'd expect to be able to enable and disable the regulator while preserving the configuration.

#2 lore raw
Quoted 10 lines · Mark Brown

drivers/regulator/core.c static int _regulator_disable(struct reg · line 1081

Quoted 5 lines · David Brownell
drivers/regulator/core.c +5 -0
+	mutex_lock(&rdev->mutex);
+	if (regulator->enabled == 1) {
+		ret = _regulator_disable(rdev);
+		if (ret == 0)
+			regulator->uA_load = 0;

Why clear the load here? I'd expect to be able to enable and disable the regulator while preserving the configuration.

That clear was in the original code. Didn't make much sense to me either, but I was just caring it forward...

#3 lore raw
Quoted 12 lines · David Brownell
Quoted 10 lines · Mark Brown

drivers/regulator/core.c static int _regulator_disable(struct reg · line 1081

Quoted 5 lines · David Brownell
drivers/regulator/core.c +5 -0
+	mutex_lock(&rdev->mutex);
+	if (regulator->enabled == 1) {
+		ret = _regulator_disable(rdev);
+		if (ret == 0)
+			regulator->uA_load = 0;

Why clear the load here? I'd expect to be able to enable and disable the regulator while preserving the configuration.

Quoted 3 lines

That clear was in the original code. Didn't make much sense to me either, but I was just caring it forward...

Hmm, this looks like a bug - I'll fix in a subsequent patch.

Will have time to look over the rest later tonight.

Thanks

Liam

#7 lore raw
Quoted 4 lines · Liam Girdwood
Quoted 3 lines · David Brownell
Quoted 1 line · Liam Girdwood

Both -rc5 patches applied.

Quoted 8 lines
Quoted 6 lines

How about the other two which seemed non-controversial?

http://marc.info/?l=linux-kernel&m=122645403604873&w=2 (code shrink)
http://marc.info/?l=linux-kernel&m=122645416305013&w=2 (attribute shrink)

Those were sent against RC4.

Both applied.

Great! Now, I think I have some comments from Mark to respond to ... He seems to be saying a new regulator_ops method is needed.

- Dave