diff mbox

[1/9] OMAP: DSS2: Change DSI platform device name from "omapdss_dsi1" to "omapdss_dsi"

Message ID 20110510134730.GB25877@opensource.wolfsonmicro.com (mailing list archive)
State New, archived
Delegated to: Tomi Valkeinen
Headers show

Commit Message

Mark Brown May 10, 2011, 1:47 p.m. UTC 
On Tue, May 10, 2011 at 03:30:52PM +0300, Tomi Valkeinen wrote:

> This sounds just the thing we need here. Do you think there's a chance
> the code could get merged in the next window? And I'm also happy to test
> the code with omapdss, whenever you have something ready.

Patch is below, tested with my "hey look, this compiles!" test plan -
I'd be a bit surprised if it ran straight off.

From 819cc21f22b1f570f7d5d8fe981bbb03e28bdb9d Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Mon, 9 May 2011 13:07:41 +0200
Subject: [PATCH] regulator: Refactor supply implementation to work as regular consumers

Currently the regulator supply implementation is somewhat complex and
fragile as it doesn't look like standard consumers but is instead a
parallel implementation. This causes issues with locking and reference
counting.

Move the implementation over to using standard consumers to address this.
Rather than only notifying the supply on the first enable/disable we do so
every time the regulator is enabled or disabled, simplifying locking as we
don't need to hold a lock on the consumer we are about to enable.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c         |   99 +++++++++++++-------------------------
 include/linux/regulator/driver.h |    4 +-
 2 files changed, 35 insertions(+), 68 deletions(-)

Comments

Tomi Valkeinen May 11, 2011, 9:23 a.m. UTC | #1 
On Tue, 2011-05-10 at 15:47 +0200, Mark Brown wrote:
> On Tue, May 10, 2011 at 03:30:52PM +0300, Tomi Valkeinen wrote:
> 
> > This sounds just the thing we need here. Do you think there's a chance
> > the code could get merged in the next window? And I'm also happy to test
> > the code with omapdss, whenever you have something ready.
> 
> Patch is below, tested with my "hey look, this compiles!" test plan -
> I'd be a bit surprised if it ran straight off.

So how should the regulator be set up?

If we currently have in the board file:

static struct regulator_consumer_supply sdp4430_vcxio_supply[] = {
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dss"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dsi.0"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dsi.1"),
};

static struct regulator_init_data sdp4430_vcxio = {
	.constraints = {
		...
	},
	.num_consumer_supplies	= ARRAY_SIZE(sdp4430_vcxio_supply),
	.consumer_supplies	= sdp4430_vcxio_supply,
};

I should setup a fixed regulator, which is supplied from VCXIO, and
omapdss will get its powers from this fixed regulator?

So, in the common display file:

static struct regulator_consumer_supply fixed_supply[] = {
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dss"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dsi.0"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dsi.1"),
};

static struct regulator_init_data fixed_reg_init_data = {
	.constraints = {
		...
	},
	.num_consumer_supplies	= ARRAY_SIZE(fixed_supply),
	.consumer_supplies	= fixed_supply,
};

static struct fixed_voltage_config omap_dss_fixed_reg_data = {
	.supply_name = "what should this be?",
	.init_data = &fixed_reg_init_data,
};

static struct platform_device omap_dss_fixed_reg_device = {
	.name	= "reg-fixed-voltage",
	.id	= 0,
	.dev	= {
		.platform_data	= &omap_dss_fixed_reg_data,
	},
};

And in the board file:

static struct regulator_consumer_supply sdp4430_vcxio_supply[] = {
	REGULATOR_SUPPLY("what should this be?", "reg-fixed-voltage.0"),
};

static struct regulator_init_data sdp4430_vcxio = {
	.constraints = {
		...
	},
	.num_consumer_supplies	= ARRAY_SIZE(sdp4430_vcxio_supply),
	.consumer_supplies	= sdp4430_vcxio_supply,
};

If so, it's again slightly difficult. I want to do the common display
stuff in a one place, used by all the boards. If I setup the fixed
regulator in the common file, I need to give it an id which may conflict
with a possible fixed regulator set up in the board file.

Or am I totally on the wrong track here =)?

 Tomi


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 11, 2011, 12:12 p.m. UTC | #2 
On Wed, May 11, 2011 at 12:23:45PM +0300, Tomi Valkeinen wrote:

> So how should the regulator be set up?

You need to create a new regulator of some kind and then provide a way
for machines to set the supply_regulator in the init_data.

> I should setup a fixed regulator, which is supplied from VCXIO, and
> omapdss will get its powers from this fixed regulator?

Yes, and the example code I've deleted all looks about right.

> static struct fixed_voltage_config omap_dss_fixed_reg_data = {
> 	.supply_name = "what should this be?",
> 	.init_data = &fixed_reg_init_data,
> };

The supply name shold be supplied by the board - it's whatever the
supply that's shared by all the regulators in the OMAP is.

> static struct regulator_consumer_supply sdp4430_vcxio_supply[] = {
> 	REGULATOR_SUPPLY("what should this be?", "reg-fixed-voltage.0"),
> };

You don't need to specify a supply to hook the supplies up.  This is a
bit of a wart, but it's the current API - it's specified in the child
regulator rather than the parent which is the other way around to
consumers.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 58452ac..38bb499 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -81,8 +81,7 @@  struct regulator {
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
-static int _regulator_disable(struct regulator_dev *rdev,
-		struct regulator_dev **supply_rdev_ptr);
+static int _regulator_disable(struct regulator_dev *rdev);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -90,6 +89,9 @@  static void _notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data);
 static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				     int min_uV, int max_uV);
+static struct regulator *create_regulator(struct regulator_dev *rdev,
+					  struct device *dev,
+					  const char *supply_name);
 
 static const char *rdev_get_name(struct regulator_dev *rdev)
 {
@@ -922,21 +924,18 @@  out:
  * core if it's child is enabled.
  */
 static int set_supply(struct regulator_dev *rdev,
-	struct regulator_dev *supply_rdev)
+		      struct regulator_dev *supply_rdev)
 {
 	int err;
 
-	err = sysfs_create_link(&rdev->dev.kobj, &supply_rdev->dev.kobj,
-				"supply");
-	if (err) {
-		rdev_err(rdev, "could not add device link %s err %d\n",
-			 supply_rdev->dev.kobj.name, err);
-		       goto out;
+	rdev->supply = create_regulator(rdev, &rdev->dev, "SUPPLY");
+	if (IS_ERR(rdev->supply)) {
+		err = PTR_ERR(rdev->supply);
+		rdev->supply = NULL;
+		return err;
 	}
-	rdev->supply = supply_rdev;
-	list_add(&rdev->slist, &supply_rdev->supply_list);
-out:
-	return err;
+
+	return 0;
 }
 
 /**
@@ -1294,19 +1293,6 @@  static int _regulator_enable(struct regulator_dev *rdev)
 {
 	int ret, delay;
 
-	if (rdev->use_count == 0) {
-		/* do we need to enable the supply regulator first */
-		if (rdev->supply) {
-			mutex_lock(&rdev->supply->mutex);
-			ret = _regulator_enable(rdev->supply);
-			mutex_unlock(&rdev->supply->mutex);
-			if (ret < 0) {
-				rdev_err(rdev, "failed to enable: %d\n", ret);
-				return ret;
-			}
-		}
-	}
-
 	/* check voltage and requested load before enabling */
 	if (rdev->constraints &&
 	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
@@ -1381,19 +1367,27 @@  int regulator_enable(struct regulator *regulator)
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret = 0;
 
+	if (rdev->supply) {
+		ret = regulator_enable(rdev->supply);
+		if (ret != 0)
+			return ret;
+	}
+
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_enable(rdev);
 	mutex_unlock(&rdev->mutex);
+
+	if (ret != 0)
+		regulator_disable(rdev->supply);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
 
 /* locks held by regulator_disable() */
-static int _regulator_disable(struct regulator_dev *rdev,
-		struct regulator_dev **supply_rdev_ptr)
+static int _regulator_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
-	*supply_rdev_ptr = NULL;
 
 	if (WARN(rdev->use_count <= 0,
 		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
@@ -1420,9 +1414,6 @@  static int _regulator_disable(struct regulator_dev *rdev,
 					     NULL);
 		}
 
-		/* decrease our supplies ref count and disable if required */
-		*supply_rdev_ptr = rdev->supply;
-
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
 
@@ -1433,6 +1424,7 @@  static int _regulator_disable(struct regulator_dev *rdev,
 
 		rdev->use_count--;
 	}
+
 	return ret;
 }
 
@@ -1451,29 +1443,21 @@  static int _regulator_disable(struct regulator_dev *rdev,
 int regulator_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
-	struct regulator_dev *supply_rdev = NULL;
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	ret = _regulator_disable(rdev, &supply_rdev);
+	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
 
-	/* decrease our supplies ref count and disable if required */
-	while (supply_rdev != NULL) {
-		rdev = supply_rdev;
-
-		mutex_lock(&rdev->mutex);
-		_regulator_disable(rdev, &supply_rdev);
-		mutex_unlock(&rdev->mutex);
-	}
+	if (ret == 0 && rdev->supply)
+		regulator_disable(rdev->supply);
 
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_disable);
 
 /* locks held by regulator_force_disable() */
-static int _regulator_force_disable(struct regulator_dev *rdev,
-		struct regulator_dev **supply_rdev_ptr)
+static int _regulator_force_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
 
@@ -1490,10 +1474,6 @@  static int _regulator_force_disable(struct regulator_dev *rdev,
 			REGULATOR_EVENT_DISABLE, NULL);
 	}
 
-	/* decrease our supplies ref count and disable if required */
-	*supply_rdev_ptr = rdev->supply;
-
-	rdev->use_count = 0;
 	return ret;
 }
 
@@ -1509,16 +1489,16 @@  static int _regulator_force_disable(struct regulator_dev *rdev,
 int regulator_force_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
-	struct regulator_dev *supply_rdev = NULL;
 	int ret;
 
 	mutex_lock(&rdev->mutex);
 	regulator->uA_load = 0;
-	ret = _regulator_force_disable(rdev, &supply_rdev);
+	ret = _regulator_force_disable(regulator->rdev);
 	mutex_unlock(&rdev->mutex);
 
-	if (supply_rdev)
-		regulator_disable(get_device_regulator(rdev_get_dev(supply_rdev)));
+	if (rdev->supply)
+		while (rdev->open_count--)
+			regulator_disable(rdev->supply);
 
 	return ret;
 }
@@ -2117,7 +2097,7 @@  int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
 	/* get input voltage */
 	input_uV = 0;
 	if (rdev->supply)
-		input_uV = _regulator_get_voltage(rdev->supply);
+		input_uV = regulator_get_voltage(rdev->supply);
 	if (input_uV <= 0)
 		input_uV = rdev->constraints->input_uV;
 	if (input_uV <= 0) {
@@ -2187,17 +2167,8 @@  EXPORT_SYMBOL_GPL(regulator_unregister_notifier);
 static void _notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
-	struct regulator_dev *_rdev;
-
 	/* call rdev chain first */
 	blocking_notifier_call_chain(&rdev->notifier, event, NULL);
-
-	/* now notify regulator we supply */
-	list_for_each_entry(_rdev, &rdev->supply_list, slist) {
-		mutex_lock(&_rdev->mutex);
-		_notifier_call_chain(_rdev, event, data);
-		mutex_unlock(&_rdev->mutex);
-	}
 }
 
 /**
@@ -2570,9 +2541,7 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
 	rdev->owner = regulator_desc->owner;
 	rdev->desc = regulator_desc;
 	INIT_LIST_HEAD(&rdev->consumer_list);
-	INIT_LIST_HEAD(&rdev->supply_list);
 	INIT_LIST_HEAD(&rdev->list);
-	INIT_LIST_HEAD(&rdev->slist);
 	BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
 
 	/* preform any regulator specific init */
@@ -2684,7 +2653,7 @@  void regulator_unregister(struct regulator_dev *rdev)
 	unset_regulator_supplies(rdev);
 	list_del(&rdev->list);
 	if (rdev->supply)
-		sysfs_remove_link(&rdev->dev.kobj, "supply");
+		regulator_put(rdev->supply);
 	device_unregister(&rdev->dev);
 	kfree(rdev->constraints);
 	mutex_unlock(&regulator_list_mutex);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 6c433b8..1a80bc7 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -188,18 +188,16 @@  struct regulator_dev {
 
 	/* lists we belong to */
 	struct list_head list; /* list of all regulators */
-	struct list_head slist; /* list of supplied regulators */
 
 	/* lists we own */
 	struct list_head consumer_list; /* consumers we supply */
-	struct list_head supply_list; /* regulators we supply */
 
 	struct blocking_notifier_head notifier;
 	struct mutex mutex; /* consumer lock */
 	struct module *owner;
 	struct device dev;
 	struct regulation_constraints *constraints;
-	struct regulator_dev *supply;	/* for tree */
+	struct regulator *supply;	/* for tree */
 
 	void *reg_data;		/* regulator_dev data */