[PATCH v6 0/5] Add coupled regulators mechanism

[PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

Hi all,

this patchset adds a new mechanism to the framework - regulators' coupling.

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires 
higher voltage, there might occur a situation that the spread between 
devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators. 

Algorithmicaly the problem was solved by: 
Inderpal Singh <inderpal.s@samsung.com>
Doug Anderson <dianders@chromium.org>

The discussion on that subject can be found here:
https://lkml.org/lkml/2014/4/29/28

Therefore this patchset is an attempt to apply the idea to regulators core
as concluded in the discussion by Mark Brown and Doug Anderson.

This feature is required to enable support for generic CPUfreq 
and devfreq drivers for the mentioned boards. 

The current assumption is that an uncoupled regulator is a special case
of a coupled one, so they should share a common voltage setting path.

The previous version caused locking problems on some boards. The discussion
can be found here:
https://lkml.org/lkml/2018/3/5/965

Current idea, which should solve the locking issues is making it possible to try
to lock a mutex multiple times by a single task just like it is done in clk/clk.c.
This should handle all possible coupling-supply combinations except coupling with
a supply.

I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.

Best regards,

	Maciej Purski

---
Changes in v6:
- change locking model
- fix build errors on non-of architectures
- update bindings

Changes in v5:
- rebase against current Mark Brown's regulators next

Changes in v4:
- make paths for coupled and uncoupled regulators common
- coupling descriptors are now always present in regulator_dev
- fail to probe if data inconsistency is detected
- retry to resolve coupling regultors in late init call
- rebase on top of linux-next 20180119
- fix commit messages
- split patches to make the patchset easier to review

Changes in v3:
- move dts parsing code to of_regulator.c, in order to the so,
  add a new commit in which of_regulator_find_by_node() is moved
  to of_regulator.c as well
- improve error messages
- move max_spread variable to constraints
- perform resolving of coupled regulators under a list mutex
- remove useless locking functions
- some minor refactorization
- improve commit messages

Changes in RFC v2:
- allow coupling n regulators (in fact up to constant value, now
  set to 10)
- change algorithm to be more readable
- introduce better locking
- add more comments
- split first patch into two
- update commit messages
- change sequence of the patches

Maciej Purski (6):
  regulator: core: Make locks re-entrant
  regulator: bindings: Add properties for coupled regulators
  regulator: core: Parse coupled regulators properties
  regulator: core: Resolve coupled regulators
  regulator: core: Add voltage balancing mechanism
  regulator: core: Change voltage setting path

 .../devicetree/bindings/regulator/regulator.txt    |   5 +
 drivers/regulator/core.c                           | 549 ++++++++++++++++++---
 drivers/regulator/internal.h                       |  28 +-
 drivers/regulator/of_regulator.c                   | 151 ++++++
 include/linux/regulator/driver.h                   |  20 +
 include/linux/regulator/machine.h                  |   4 +
 6 files changed, 677 insertions(+), 80 deletions(-)

-- 
2.7.4

[PATCH v6 1/6] regulator: core: Make locks re-entrant

[Maciej Purski]

Setting voltage, enabling/disabling regulators requires operations on
all regulators related with the regulator being changed. Therefore,
all of them should be locked for the whole operation. With the current
locking implementation, adding additional dependency (regulators
coupling) causes deadlocks in some cases.

Introduce a possibility to attempt to lock a mutex multiple times
by the same task without waiting on a mutex. This should handle all
reasonable coupling-supplying combinations, especially when two coupled
regulators share common supplies. The only situation that should be
forbidden is simultaneous coupling and supplying between a pair of
regulators.

The idea is based on clk core.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c         | 132 +++++++++++++++++++++++++++------------
 include/linux/regulator/driver.h |   2 +
 2 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5494189..f6e784c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -147,6 +147,56 @@ static inline struct regulator_dev *rdev_get_supply(struct regulator_dev *rdev)
 }
 
 /**
+ * regulator_lock_nested - lock a single regulator
+ * @rdev:		regulator source
+ * @subclass:		mutex subclass used for lockdep
+ *
+ * This function can be called many times by one task on
+ * a single regulator and its mutex will be locked only
+ * once. If a task, which is calling this function is other
+ * than the one, which initially locked the mutex, it will
+ * wait on mutex.
+ */
+static void regulator_lock_nested(struct regulator_dev *rdev,
+				  unsigned int subclass)
+{
+	if (!mutex_trylock(&rdev->mutex)) {
+		if (rdev->mutex_owner == current) {
+			rdev->ref_cnt++;
+			return;
+		}
+		mutex_lock_nested(&rdev->mutex, subclass);
+	}
+
+	rdev->ref_cnt = 1;
+	rdev->mutex_owner = current;
+}
+
+static inline void regulator_lock(struct regulator_dev *rdev)
+{
+	regulator_lock_nested(rdev, 0);
+}
+
+/**
+ * regulator_unlock - unlock a single regulator
+ * @rdev:		regulator_source
+ *
+ * This function unlocks the mutex when the
+ * reference counter reaches 0.
+ */
+static void regulator_unlock(struct regulator_dev *rdev)
+{
+	if (rdev->ref_cnt != 0) {
+		rdev->ref_cnt--;
+
+		if (!rdev->ref_cnt) {
+			rdev->mutex_owner = NULL;
+			mutex_unlock(&rdev->mutex);
+		}
+	}
+}
+
+/**
  * regulator_lock_supply - lock a regulator and its supplies
  * @rdev:         regulator source
  */
@@ -155,7 +205,7 @@ static void regulator_lock_supply(struct regulator_dev *rdev)
 	int i;
 
 	for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
-		mutex_lock_nested(&rdev->mutex, i);
+		regulator_lock_nested(rdev, i);
 }
 
 /**
@@ -167,7 +217,7 @@ static void regulator_unlock_supply(struct regulator_dev *rdev)
 	struct regulator *supply;
 
 	while (1) {
-		mutex_unlock(&rdev->mutex);
+		regulator_unlock(rdev);
 		supply = rdev->supply;
 
 		if (!rdev->supply)
@@ -350,9 +400,9 @@ static ssize_t regulator_uV_show(struct device *dev,
 	struct regulator_dev *rdev = dev_get_drvdata(dev);
 	ssize_t ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	ret = sprintf(buf, "%d\n", _regulator_get_voltage(rdev));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -416,9 +466,9 @@ static ssize_t regulator_state_show(struct device *dev,
 	struct regulator_dev *rdev = dev_get_drvdata(dev);
 	ssize_t ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	ret = regulator_print_state(buf, _regulator_is_enabled(rdev));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -526,10 +576,10 @@ static ssize_t regulator_total_uA_show(struct device *dev,
 	struct regulator *regulator;
 	int uA = 0;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	list_for_each_entry(regulator, &rdev->consumer_list, list)
 		uA += regulator->uA_load;
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return sprintf(buf, "%d\n", uA);
 }
 static DEVICE_ATTR(requested_microamps, 0444, regulator_total_uA_show, NULL);
@@ -1321,7 +1371,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	if (regulator == NULL)
 		return NULL;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	regulator->rdev = rdev;
 	list_add(&regulator->list, &rdev->consumer_list);
 
@@ -1376,12 +1426,12 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return regulator;
 overflow_err:
 	list_del(&regulator->list);
 	kfree(regulator);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return NULL;
 }
 
@@ -1770,13 +1820,13 @@ static void _regulator_put(struct regulator *regulator)
 	/* remove any sysfs entries */
 	if (regulator->dev)
 		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	list_del(&regulator->list);
 
 	rdev->open_count--;
 	rdev->exclusive = 0;
 	put_device(&rdev->dev);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	kfree_const(regulator->supply_name);
 	kfree(regulator);
@@ -2384,7 +2434,7 @@ static void regulator_disable_work(struct work_struct *work)
 						  disable_work.work);
 	int count, i, ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	BUG_ON(!rdev->deferred_disables);
 
@@ -2405,7 +2455,7 @@ static void regulator_disable_work(struct work_struct *work)
 			rdev_err(rdev, "Deferred disable failed: %d\n", ret);
 	}
 
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	if (rdev->supply) {
 		for (i = 0; i < count; i++) {
@@ -2440,11 +2490,11 @@ int regulator_disable_deferred(struct regulator *regulator, int ms)
 	if (!ms)
 		return regulator_disable(regulator);
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	rdev->deferred_disables++;
 	mod_delayed_work(system_power_efficient_wq, &rdev->disable_work,
 			 msecs_to_jiffies(ms));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return 0;
 }
@@ -2476,10 +2526,10 @@ static int _regulator_list_voltage(struct regulator_dev *rdev,
 		if (selector >= rdev->desc->n_voltages)
 			return -EINVAL;
 		if (lock)
-			mutex_lock(&rdev->mutex);
+			regulator_lock(rdev);
 		ret = ops->list_voltage(rdev, selector);
 		if (lock)
-			mutex_unlock(&rdev->mutex);
+			regulator_unlock(rdev);
 	} else if (rdev->is_switch && rdev->supply) {
 		ret = _regulator_list_voltage(rdev->supply->rdev,
 					      selector, lock);
@@ -3252,7 +3302,7 @@ int regulator_sync_voltage(struct regulator *regulator)
 	struct regulator_voltage *voltage = &regulator->voltage[PM_SUSPEND_ON];
 	int ret, min_uV, max_uV;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	if (!rdev->desc->ops->set_voltage &&
 	    !rdev->desc->ops->set_voltage_sel) {
@@ -3281,7 +3331,7 @@ int regulator_sync_voltage(struct regulator *regulator)
 	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
 
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_sync_voltage);
@@ -3374,7 +3424,7 @@ int regulator_set_current_limit(struct regulator *regulator,
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	/* sanity check */
 	if (!rdev->desc->ops->set_current_limit) {
@@ -3389,7 +3439,7 @@ int regulator_set_current_limit(struct regulator *regulator,
 
 	ret = rdev->desc->ops->set_current_limit(rdev, min_uA, max_uA);
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_current_limit);
@@ -3398,7 +3448,7 @@ static int _regulator_get_current_limit(struct regulator_dev *rdev)
 {
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	/* sanity check */
 	if (!rdev->desc->ops->get_current_limit) {
@@ -3408,7 +3458,7 @@ static int _regulator_get_current_limit(struct regulator_dev *rdev)
 
 	ret = rdev->desc->ops->get_current_limit(rdev);
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 
@@ -3444,7 +3494,7 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 	int ret;
 	int regulator_curr_mode;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	/* sanity check */
 	if (!rdev->desc->ops->set_mode) {
@@ -3468,7 +3518,7 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 
 	ret = rdev->desc->ops->set_mode(rdev, mode);
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_mode);
@@ -3477,7 +3527,7 @@ static unsigned int _regulator_get_mode(struct regulator_dev *rdev)
 {
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	/* sanity check */
 	if (!rdev->desc->ops->get_mode) {
@@ -3487,7 +3537,7 @@ static unsigned int _regulator_get_mode(struct regulator_dev *rdev)
 
 	ret = rdev->desc->ops->get_mode(rdev);
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 
@@ -3508,7 +3558,7 @@ static int _regulator_get_error_flags(struct regulator_dev *rdev,
 {
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	/* sanity check */
 	if (!rdev->desc->ops->get_error_flags) {
@@ -3518,7 +3568,7 @@ static int _regulator_get_error_flags(struct regulator_dev *rdev,
 
 	ret = rdev->desc->ops->get_error_flags(rdev, flags);
 out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 	return ret;
 }
 
@@ -3567,10 +3617,10 @@ int regulator_set_load(struct regulator *regulator, int uA_load)
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	regulator->uA_load = uA_load;
 	ret = drms_uA_update(rdev);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -3598,7 +3648,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_BYPASS))
 		return 0;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	if (enable && !regulator->bypass) {
 		rdev->bypass_count++;
@@ -3622,7 +3672,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 	if (ret == 0)
 		regulator->bypass = enable;
 
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -4288,9 +4338,9 @@ static int _regulator_suspend_late(struct device *dev, void *data)
 	suspend_state_t *state = data;
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 	ret = suspend_set_state(rdev, *state);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -4319,14 +4369,14 @@ static int _regulator_resume_early(struct device *dev, void *data)
 	if (rstate == NULL)
 		return 0;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	if (rdev->desc->ops->resume_early &&
 	    (rstate->enabled == ENABLE_IN_SUSPEND ||
 	     rstate->enabled == DISABLE_IN_SUSPEND))
 		ret = rdev->desc->ops->resume_early(rdev);
 
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return ret;
 }
@@ -4628,7 +4678,7 @@ static int __init regulator_late_cleanup(struct device *dev, void *data)
 	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
 		return 0;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
 
 	if (rdev->use_count)
 		goto unlock;
@@ -4659,7 +4709,7 @@ static int __init regulator_late_cleanup(struct device *dev, void *data)
 	}
 
 unlock:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
 
 	return 0;
 }
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4fc96cb..659a031 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -431,6 +431,8 @@ struct regulator_dev {
 
 	struct blocking_notifier_head notifier;
 	struct mutex mutex; /* consumer lock */
+	struct task_struct *mutex_owner;
+	int ref_cnt;
 	struct module *owner;
 	struct device dev;
 	struct regulation_constraints *constraints;
-- 
2.7.4

[PATCH v6 2/6] regulator: bindings: Add properties for coupled regulators

[Maciej Purski]

Some regulators require keeping their voltage spread below defined
max_spread.

Add properties to provide information on regulators' coupling.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 2babe15b..a096fae 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -68,6 +68,11 @@ Optional properties:
 	0: Disable active discharge.
 	1: Enable active discharge.
 	Absence of this property will leave configuration to default.
+- regulator-coupled-with: Regulators with which the regulator
+  is coupled. The linkage is 2-way - all coupled regulators should be linked
+  with each other. A regulator should not be coupled with its supplier.
+- regulator-coupled-max-spread: Max spread between voltages of coupled regulators
+  in microvolts.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
2.7.4

Re: [PATCH v6 2/6] regulator: bindings: Add properties for coupled regulators

[Rob Herring]

On Fri, Mar 09, 2018 at 01:22:04PM +0100, Maciej Purski wrote:
> Some regulators require keeping their voltage spread below defined
> max_spread.
> 
> Add properties to provide information on regulators' coupling.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  Documentation/devicetree/bindings/regulator/regulator.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Applied "regulator: bindings: Add properties for coupled regulators" to the regulator tree

[Mark Brown]

The patch

   regulator: bindings: Add properties for coupled regulators

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From f98618b34542706eddc3b66abc271f1c8d8c4a05 Mon Sep 17 00:00:00 2001
From: Maciej Purski <m.purski@samsung.com>
Date: Mon, 23 Apr 2018 16:33:38 +0200
Subject: [PATCH] regulator: bindings: Add properties for coupled regulators

Some regulators require keeping their voltage spread below defined
max_spread.

Add properties to provide information on regulators' coupling.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index c627aa08f0da..a7cd36877bfe 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -73,6 +73,11 @@ Optional properties:
 	0: Disable active discharge.
 	1: Enable active discharge.
 	Absence of this property will leave configuration to default.
+- regulator-coupled-with: Regulators with which the regulator
+  is coupled. The linkage is 2-way - all coupled regulators should be linked
+  with each other. A regulator should not be coupled with its supplier.
+- regulator-coupled-max-spread: Max spread between voltages of coupled regulators
+  in microvolts.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
2.17.0

[PATCH v6 3/6] regulator: core: Parse coupled regulators properties

[Maciej Purski]

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.

Add new structure "coupling_desc" to regulator_dev, which contains
pointers to all coupled regulators including the owner of the structure,
number of coupled regulators and counter of currently resolved
regulators.

Add of_functions to parse all data needed in regulator coupling.
Provide method to check DTS data consistency. Check if each coupled
regulator's max_spread is equal and if their lists of regulators match.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/internal.h      |  28 ++++++-
 drivers/regulator/of_regulator.c  | 151 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h  |  18 +++++
 include/linux/regulator/machine.h |   4 +
 4 files changed, 199 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index abfd56e..485db15 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -63,6 +63,14 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
 				 struct regulator_config *config,
 				 struct device_node **node);
+
+struct regulator_dev *of_parse_coupled_regulator(struct regulator_dev *rdev,
+						 int index);
+
+int of_get_n_coupled(struct regulator_dev *rdev);
+
+bool of_check_coupling_data(struct regulator_dev *rdev);
+
 #else
 static inline struct regulator_init_data *
 regulator_of_get_init_data(struct device *dev,
@@ -72,8 +80,25 @@ regulator_of_get_init_data(struct device *dev,
 {
 	return NULL;
 }
-#endif
 
+static inline struct regulator_dev *
+of_parse_coupled_regulator(struct regulator_dev *rdev,
+			   int index)
+{
+	return NULL;
+}
+
+static inline int of_get_n_coupled(struct regulator_dev *rdev)
+{
+	return 0;
+}
+
+static inline bool of_check_coupling_data(struct regulator_dev *rdev)
+{
+	return false;
+}
+
+#endif
 enum regulator_get_type {
 	NORMAL_GET,
 	EXCLUSIVE_GET,
@@ -83,5 +108,4 @@ enum regulator_get_type {
 
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
-
 #endif
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index f47264f..c6e61c7 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -138,6 +138,10 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!of_property_read_u32(np, "regulator-system-load", &pval))
 		constraints->system_load = pval;
 
+	if (!of_property_read_u32(np, "regulator-coupled-max-spread",
+				  &pval))
+		constraints->max_spread = pval;
+
 	constraints->over_current_protection = of_property_read_bool(np,
 					"regulator-over-current-protection");
 
@@ -407,3 +411,150 @@ struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 
 	return dev ? dev_to_rdev(dev) : NULL;
 }
+
+/*
+ * Returns number of regulators coupled with rdev.
+ */
+int of_get_n_coupled(struct regulator_dev *rdev)
+{
+	struct device_node *node = rdev->dev.of_node;
+	int n_phandles;
+
+	n_phandles = of_count_phandle_with_args(node,
+						"regulator-coupled-with",
+						NULL);
+
+	return (n_phandles > 0) ? n_phandles : 0;
+}
+
+/* Looks for "to_find" device_node in src's "regulator-coupled-with" property */
+static bool of_coupling_find_node(struct device_node *src,
+				  struct device_node *to_find)
+{
+	int n_phandles, i;
+	bool found = false;
+
+	n_phandles = of_count_phandle_with_args(src,
+						"regulator-coupled-with",
+						NULL);
+
+	for (i = 0; i < n_phandles; i++) {
+		struct device_node *tmp = of_parse_phandle(src,
+					   "regulator-coupled-with", i);
+
+		if (!tmp)
+			break;
+
+		/* found */
+		if (tmp == to_find)
+			found = true;
+
+		of_node_put(tmp);
+
+		if (found)
+			break;
+	}
+
+	return found;
+}
+
+/**
+ * of_check_coupling_data - Parse rdev's coupling properties and check data
+ *			    consistency
+ * @rdev - pointer to regulator_dev whose data is checked
+ *
+ * Function checks if all the following conditions are met:
+ * - rdev's max_spread is greater than 0
+ * - all coupled regulators have the same max_spread
+ * - all coupled regulators have the same number of regulator_dev phandles
+ * - all regulators are linked to each other
+ *
+ * Returns true if all conditions are met.
+ */
+bool of_check_coupling_data(struct regulator_dev *rdev)
+{
+	int max_spread = rdev->constraints->max_spread;
+	struct device_node *node = rdev->dev.of_node;
+	int n_phandles = of_get_n_coupled(rdev);
+	struct device_node *c_node;
+	int i;
+	bool ret = true;
+
+	if (max_spread <= 0) {
+		dev_err(&rdev->dev, "max_spread value invalid\n");
+		return false;
+	}
+
+	/* iterate over rdev's phandles */
+	for (i = 0; i < n_phandles; i++) {
+		int c_max_spread, c_n_phandles;
+
+		c_node = of_parse_phandle(node,
+					  "regulator-coupled-with", i);
+
+		if (!c_node)
+			ret = false;
+
+		c_n_phandles = of_count_phandle_with_args(c_node,
+							  "regulator-coupled-with",
+							  NULL);
+
+		if (c_n_phandles != n_phandles) {
+			dev_err(&rdev->dev, "number of couped reg phandles mismatch\n");
+			ret = false;
+			goto clean;
+		}
+
+		if (of_property_read_u32(c_node, "regulator-coupled-max-spread",
+					 &c_max_spread)) {
+			ret = false;
+			goto clean;
+		}
+
+		if (c_max_spread != max_spread) {
+			dev_err(&rdev->dev,
+				"coupled regulators max_spread mismatch\n");
+			ret = false;
+			goto clean;
+		}
+
+		if (!of_coupling_find_node(c_node, node)) {
+			dev_err(&rdev->dev, "missing 2-way linking for coupled regulators\n");
+			ret = false;
+		}
+
+clean:
+		of_node_put(c_node);
+		if (!ret)
+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * of_parse_coupled regulator - Get regulator_dev pointer from rdev's property
+ * @rdev: Pointer to regulator_dev, whose DTS is used as a source to parse
+ *	  "regulator-coupled-with" property
+ * @index: Index in phandles array
+ *
+ * Returns the regulator_dev pointer parsed from DTS. If it has not been yet
+ * registered, returns NULL
+ */
+struct regulator_dev *of_parse_coupled_regulator(struct regulator_dev *rdev,
+						 int index)
+{
+	struct device_node *node = rdev->dev.of_node;
+	struct device_node *c_node;
+	struct regulator_dev *c_rdev;
+
+	c_node = of_parse_phandle(node, "regulator-coupled-with", index);
+	if (!c_node)
+		return NULL;
+
+	c_rdev = of_find_regulator_by_node(c_node);
+
+	of_node_put(c_node);
+
+	return c_rdev;
+}
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 659a031..8928fd6 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -15,6 +15,8 @@
 #ifndef __LINUX_REGULATOR_DRIVER_H_
 #define __LINUX_REGULATOR_DRIVER_H_
 
+#define MAX_COUPLED		10
+
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/regulator/consumer.h>
@@ -407,6 +409,20 @@ struct regulator_config {
 };
 
 /*
+ * struct coupling_desc
+ *
+ * Describes coupling of regulators. Each regulator should have
+ * at least a pointer to itself in coupled_rdevs array.
+ * When a new coupled regulator is resolved, n_resolved is
+ * incremented.
+ */
+struct coupling_desc {
+	struct regulator_dev *coupled_rdevs[MAX_COUPLED];
+	int n_resolved;
+	int n_coupled;
+};
+
+/*
  * struct regulator_dev
  *
  * Voltage / Current regulator class device. One for each
@@ -429,6 +445,8 @@ struct regulator_dev {
 	/* lists we own */
 	struct list_head consumer_list; /* consumers we supply */
 
+	struct coupling_desc coupling_desc;
+
 	struct blocking_notifier_head notifier;
 	struct mutex mutex; /* consumer lock */
 	struct task_struct *mutex_owner;
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 93a0489..3468703 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -103,6 +103,7 @@ struct regulator_state {
  * @ilim_uA: Maximum input current.
  * @system_load: Load that isn't captured by any consumer requests.
  *
+ * @max_spread: Max possible spread between coupled regulators
  * @valid_modes_mask: Mask of modes which may be configured by consumers.
  * @valid_ops_mask: Operations which may be performed by consumers.
  *
@@ -154,6 +155,9 @@ struct regulation_constraints {
 
 	int system_load;
 
+	/* used for coupled regulators */
+	int max_spread;
+
 	/* valid regulator operating modes for this machine */
 	unsigned int valid_modes_mask;
 
-- 
2.7.4

[PATCH v6 4/6] regulator: core: Resolve coupled regulators

[Maciej Purski]

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.

Fill coupling descriptor with data obtained from DTS using previously
defined of_functions. Fail to register a regulator, if some data
inconsistency occurs. If some coupled regulators are not yet registered,
don't fail to register, but try to resolve them in late init call.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f6e784c..60fb05d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4117,6 +4117,96 @@ static int regulator_register_resolve_supply(struct device *dev, void *data)
 	return 0;
 }
 
+static int regulator_fill_coupling_array(struct regulator_dev *rdev)
+{
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	int n_coupled = c_desc->n_coupled;
+	struct regulator_dev *c_rdev;
+	int i;
+
+	for (i = 1; i < n_coupled; i++) {
+		/* already resolved */
+		if (c_desc->coupled_rdevs[i])
+			continue;
+
+		c_rdev = of_parse_coupled_regulator(rdev, i - 1);
+
+		if (c_rdev) {
+			c_desc->coupled_rdevs[i] = c_rdev;
+			c_desc->n_resolved++;
+		}
+	}
+
+	if (rdev->coupling_desc.n_resolved < n_coupled)
+		return -1;
+	else
+		return 0;
+}
+
+static int regulator_register_fill_coupling_array(struct device *dev,
+						  void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return 0;
+
+	if (regulator_fill_coupling_array(rdev))
+		rdev_dbg(rdev, "unable to resolve coupling\n");
+
+	return 0;
+}
+
+static int regulator_resolve_coupling(struct regulator_dev *rdev)
+{
+	int n_phandles;
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return 0;
+
+	n_phandles = of_get_n_coupled(rdev);
+
+	if (n_phandles + 1 > MAX_COUPLED) {
+		rdev_err(rdev, "too many regulators coupled\n");
+		return -EPERM;
+	}
+
+	/*
+	 * Every regulator should always have coupling descriptor filled with
+	 * at least pointer to itself.
+	 */
+	rdev->coupling_desc.coupled_rdevs[0] = rdev;
+	rdev->coupling_desc.n_coupled = n_phandles + 1;
+	rdev->coupling_desc.n_resolved++;
+
+	/* regulator isn't coupled */
+	if (n_phandles == 0)
+		return 0;
+
+	/* regulator, which can't change its voltage, can't be coupled */
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
+		rdev_err(rdev, "voltage operation not allowed\n");
+		return -EPERM;
+	}
+
+	if (rdev->constraints->max_spread <= 0) {
+		rdev_err(rdev, "wrong max_spread value\n");
+		return -EPERM;
+	}
+
+	if (!of_check_coupling_data(rdev))
+		return -EPERM;
+
+	/*
+	 * After everything has been checked, try to fill rdevs array
+	 * with pointers to regulators parsed from device tree. If some
+	 * regulators are not registered yet, retry in late init call
+	 */
+	regulator_fill_coupling_array(rdev);
+
+	return 0;
+}
+
 /**
  * regulator_register - register regulator
  * @regulator_desc: regulator to register
@@ -4250,6 +4340,13 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (ret < 0)
 		goto wash;
 
+	mutex_lock(&regulator_list_mutex);
+	ret = regulator_resolve_coupling(rdev);
+	mutex_unlock(&regulator_list_mutex);
+
+	if (ret != 0)
+		goto wash;
+
 	/* add consumers devices */
 	if (init_data) {
 		mutex_lock(&regulator_list_mutex);
@@ -4743,6 +4840,9 @@ static int __init regulator_init_complete(void)
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_late_cleanup);
 
+	class_for_each_device(&regulator_class, NULL, NULL,
+			      regulator_register_fill_coupling_array);
+
 	return 0;
 }
 late_initcall_sync(regulator_init_complete);
-- 
2.7.4

[PATCH v6 5/6] regulator: core: Add voltage balancing mechanism

[Maciej Purski]

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.

Introduce new function regulator_balance_voltage(), which
keeps max_spread constraint fulfilled between a group of coupled
regulators. It should be called if a regulator changes its
voltage or after disabling or enabling. Disabled regulators should
follow changes of the enabled ones, but their consumers' demands
shouldn't be taken into account while calculating voltage of other
coupled regulators.

Find voltages, which are closest to suiting all the consumers' demands,
while fulfilling max_spread constraint, keeping the following rules:
- if one regulator is about to rise its voltage, rise others
  voltages in order to keep the max_spread
- if a regulator, which has caused rising other regulators, is
  lowered, lower other regulators if possible
- if one regulator is about to lower its voltage, but it hasn't caused
  rising other regulators, don't change its voltage if it breaks the
  max_spread

Change regulators' voltages step by step, keeping max_spread constraint
fulfilled all the time. Function regulator_get_optimal_voltage()
should find the best possible change for the regulator, which doesn't
break max_spread constraint. In function regulator_balance_voltage()
optimize number of steps by finding highest voltage difference on
each iteration.

If a regulator, which is about to change its voltage, is not coupled,
method regulator_get_optimal_voltage() should simply return the lowest
voltage fulfilling consumers' demands.

Coupling should be checked only if the system is in PM_SUSPEND_ON state.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 192 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 60fb05d..1b1e0375 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,8 @@ static int _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 int regulator_balance_voltage(struct regulator_dev *rdev,
+				     suspend_state_t state);
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
@@ -3090,6 +3092,196 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	return ret;
 }
 
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+{
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
+	int max_spread = rdev->constraints->max_spread;
+	int n_coupled = c_desc->n_coupled;
+	int desired_min_uV, desired_max_uV, min_current_uV = INT_MAX;
+	int max_current_uV = 0, highest_min_uV = 0, target_uV, possible_uV;
+	int i, ret;
+
+	/* If consumers don't provide any demands, set voltage to min_uV */
+	desired_min_uV = rdev->constraints->min_uV;
+	desired_max_uV = rdev->constraints->max_uV;
+	ret = regulator_check_consumers(rdev,
+					&desired_min_uV,
+					&desired_max_uV, PM_SUSPEND_ON);
+	if (ret < 0)
+		goto out;
+
+	/*
+	 * If there are no coupled regulators, simply set the voltage demanded
+	 * by consumers.
+	 */
+	if (n_coupled == 1) {
+		ret = desired_min_uV;
+		goto out;
+	}
+
+	/* Find highest min desired voltage */
+	for (i = 0; i < n_coupled; i++) {
+		int tmp_min = 0;
+		int tmp_max = INT_MAX;
+
+		if (!_regulator_is_enabled(c_rdevs[i]))
+			continue;
+
+		ret = regulator_check_consumers(c_rdevs[i],
+						&tmp_min,
+						&tmp_max, PM_SUSPEND_ON);
+		if (ret < 0)
+			goto out;
+
+		if (tmp_min > highest_min_uV)
+			highest_min_uV = tmp_min;
+	}
+
+	/*
+	 * Let target_uV be equal to the desired one if possible.
+	 * If not, set it to minimum voltage, allowed by other coupled
+	 * regulators.
+	 */
+	target_uV = max(desired_min_uV,  highest_min_uV - max_spread);
+
+	/*
+	 * Find min and max voltages, which currently aren't
+	 * violating max_spread
+	 */
+	for (i = 0; i < n_coupled; i++) {
+		int tmp_act;
+
+		/*
+		 * Don't check the regulator, which is about
+		 * to change voltage
+		 */
+		if (c_rdevs[i] == rdev)
+			continue;
+		if (!_regulator_is_enabled(c_rdevs[i]))
+			continue;
+
+		tmp_act = _regulator_get_voltage(c_rdevs[i]);
+		if (tmp_act < 0) {
+			ret = tmp_act;
+			goto out;
+		}
+
+		if (tmp_act < min_current_uV)
+			min_current_uV = tmp_act;
+
+		if (tmp_act > max_current_uV)
+			max_current_uV = tmp_act;
+	}
+
+	/* There aren't any other regulators enabled */
+	if (max_current_uV == 0) {
+		possible_uV = target_uV;
+	} else {
+		/*
+		 * Correct target voltage, so as it currently isn't
+		 * violating max_spread
+		 */
+		possible_uV = max(target_uV, max_current_uV - max_spread);
+		possible_uV = min(possible_uV, min_current_uV + max_spread);
+	}
+
+	if (possible_uV > desired_max_uV) {
+		ret = -EINVAL;
+		goto out;
+	}
+	ret = possible_uV;
+
+out:
+	return ret;
+}
+
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+				     suspend_state_t state)
+{
+	struct regulator_dev **c_rdevs;
+	struct regulator_dev *best_rdev;
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	int n_coupled;
+	int i, best_delta, best_uV, ret = 1;
+
+	c_rdevs = c_desc->coupled_rdevs;
+	n_coupled = c_desc->n_coupled;
+
+	/*
+	 * if system is in a state other than PM_SUSPEND_ON, don't check
+	 * other coupled regulators
+	 */
+	if (state != PM_SUSPEND_ON)
+		n_coupled = 1;
+
+	/*
+	 * Find the best possible voltage change on each loop. Leave the loop
+	 * if there isn't any possible change.
+	 */
+	while (1) {
+		best_delta = 0;
+		best_uV = 0;
+		best_rdev = NULL;
+
+		/*
+		 * Find highest difference between optimal voltage
+		 * and current voltage.
+		 */
+		for (i = 0; i < n_coupled; i++) {
+			/*
+			 * optimal_uV is the best voltage that can be set for
+			 * i-th regulator at the moment without violating
+			 * max_spread constraint in order to balance
+			 * the coupled voltages.
+			 */
+			int optimal_uV, current_uV;
+
+			optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+			if (optimal_uV < 0) {
+				ret = optimal_uV;
+				goto out;
+			}
+
+			current_uV = _regulator_get_voltage(c_rdevs[i]);
+			if (current_uV < 0) {
+				ret = optimal_uV;
+				goto out;
+			}
+
+			if (abs(best_delta) < abs(optimal_uV - current_uV)) {
+				best_delta = optimal_uV - current_uV;
+				best_rdev = c_rdevs[i];
+				best_uV = optimal_uV;
+			}
+		}
+
+		/* Nothing to change, return successfully */
+		if (!best_rdev) {
+			ret = 0;
+			goto out;
+		}
+
+		/*
+		 * Lock just the supply regulators, as the regulator itself
+		 * is already locked by regulator_lock_coupled().
+		 */
+		if (best_rdev->supply)
+			regulator_lock_supply(best_rdev->supply->rdev);
+
+		ret = regulator_set_voltage_rdev(best_rdev, best_uV,
+						 best_uV, state);
+		if (best_rdev->supply)
+			regulator_unlock_supply(best_rdev->supply->rdev);
+
+		if (ret < 0)
+			goto out;
+	}
+
+out:
+	return ret;
+}
+
 /**
  * regulator_set_voltage - set regulator output voltage
  * @regulator: regulator source
-- 
2.7.4

[PATCH v6 6/6] regulator: core: Change voltage setting path

[Maciej Purski]

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.

Uncoupled regulators should be a special case of coupled regulators, so
they should share a common voltage setting path. When enabling,
disabling or setting voltage of a coupled regulator, all coupled
regulators should be locked. Regulator's supplies should be locked, when
setting voltage of a single regulator. Enabling a coupled regulator or
setting its voltage should not be possible if some of its coupled
regulators, has not been registered.

Add function for locking coupled regulators and supplies. Extract
a new function regulator_set_voltage_rdev() from
regulator_set_voltage_unlocked(), which is called when setting
voltage of a single regulator.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c | 155 +++++++++++++++++++++++++++++++----------------
 1 file changed, 103 insertions(+), 52 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1b1e0375..0e80ba5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -107,6 +107,9 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				     int min_uV, int max_uV);
 static int regulator_balance_voltage(struct regulator_dev *rdev,
 				     suspend_state_t state);
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
+				      int min_uV, int max_uV,
+				      suspend_state_t state);
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
@@ -198,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
 	}
 }
 
-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev:         regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+				    unsigned int subclass)
 {
+	struct regulator_dev *c_rdev;
 	int i;
 
-	for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
-		regulator_lock_nested(rdev, i);
+	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+		if (!c_rdev)
+			continue;
+
+		regulator_lock_nested(c_rdev, subclass++);
+
+		if (c_rdev->supply)
+			subclass =
+				regulator_lock_recursive(c_rdev->supply->rdev,
+							 subclass);
+	}
+
+	return subclass;
 }
 
 /**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev:         regulator source
+ * regulator_unlock_dependent - unlock regulator's suppliers and coupled
+ *				regulators
+ * @rdev:			regulator source
+ *
+ * Unlock all regulators related with rdev by coupling or suppling.
  */
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
 {
-	struct regulator *supply;
+	struct regulator_dev *c_rdev;
+	int i;
 
-	while (1) {
-		regulator_unlock(rdev);
-		supply = rdev->supply;
+	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
 
-		if (!rdev->supply)
-			return;
+		if (!c_rdev)
+			continue;
+
+		regulator_unlock(c_rdev);
 
-		rdev = supply->rdev;
+		if (c_rdev->supply)
+			regulator_unlock_dependent(c_rdev->supply->rdev);
 	}
 }
 
 /**
+ * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
+ * @rdev:			regulator source
+ *
+ * This function as a wrapper on regulator_lock_recursive(), which locks
+ * all regulators related with rdev by coupling or suppling.
+ */
+static inline void regulator_lock_dependent(struct regulator_dev *rdev)
+{
+	regulator_lock_recursive(rdev, 0);
+}
+
+/**
  * of_get_regulator - get a regulator device node based on supply name
  * @dev: Device pointer for the consumer (of regulator) device
  * @supply: regulator supply name
@@ -2249,6 +2281,11 @@ int regulator_enable(struct regulator *regulator)
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret = 0;
 
+	if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+		rdev_err(rdev, "not all coupled regulators registered\n");
+		return -EPERM;
+	}
+
 	if (regulator->always_on)
 		return 0;
 
@@ -2258,9 +2295,10 @@ int regulator_enable(struct regulator *regulator)
 			return ret;
 	}
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock_dependent(rdev);
 	ret = _regulator_enable(rdev);
-	mutex_unlock(&rdev->mutex);
+	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+	regulator_unlock_dependent(rdev);
 
 	if (ret != 0 && rdev->supply)
 		regulator_disable(rdev->supply);
@@ -2366,9 +2404,10 @@ int regulator_disable(struct regulator *regulator)
 	if (regulator->always_on)
 		return 0;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock_dependent(rdev);
 	ret = _regulator_disable(rdev);
-	mutex_unlock(&rdev->mutex);
+	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+	regulator_unlock_dependent(rdev);
 
 	if (ret == 0 && rdev->supply)
 		regulator_disable(rdev->supply);
@@ -2417,10 +2456,11 @@ int regulator_force_disable(struct regulator *regulator)
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
 
-	mutex_lock(&rdev->mutex);
+	regulator_lock_dependent(rdev);
 	regulator->uA_load = 0;
 	ret = _regulator_force_disable(regulator->rdev);
-	mutex_unlock(&rdev->mutex);
+	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+	regulator_unlock_dependent(rdev);
 
 	if (rdev->supply)
 		while (rdev->open_count--)
@@ -2568,9 +2608,9 @@ int regulator_is_enabled(struct regulator *regulator)
 	if (regulator->always_on)
 		return 1;
 
-	mutex_lock(&regulator->rdev->mutex);
+	regulator_lock_dependent(regulator->rdev);
 	ret = _regulator_is_enabled(regulator->rdev);
-	mutex_unlock(&regulator->rdev->mutex);
+	regulator_unlock_dependent(regulator->rdev);
 
 	return ret;
 }
@@ -2979,8 +3019,12 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	int ret = 0;
 	int old_min_uV, old_max_uV;
 	int current_uV;
-	int best_supply_uV = 0;
-	int supply_change_uV = 0;
+
+	if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+		rdev_err(rdev, "not all coupled regulators registered\n");
+		ret = -EPERM;
+		goto out;
+	}
 
 	/* If we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
@@ -3024,6 +3068,27 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	if (ret < 0)
 		goto out2;
 
+	/* for not coupled regulators this will just set the voltage */
+	ret = regulator_balance_voltage(rdev, state);
+	if (ret < 0)
+		goto out2;
+
+out:
+	return 0;
+out2:
+	voltage->min_uV = old_min_uV;
+	voltage->max_uV = old_max_uV;
+
+	return ret;
+}
+
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
+				      int max_uV, suspend_state_t state)
+{
+	int best_supply_uV = 0;
+	int supply_change_uV = 0;
+	int ret;
+
 	if (rdev->supply &&
 	    regulator_ops_is_valid(rdev->supply->rdev,
 				   REGULATOR_CHANGE_VOLTAGE) &&
@@ -3035,13 +3100,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		selector = regulator_map_voltage(rdev, min_uV, max_uV);
 		if (selector < 0) {
 			ret = selector;
-			goto out2;
+			goto out;
 		}
 
 		best_supply_uV = _regulator_list_voltage(rdev, selector, 0);
 		if (best_supply_uV < 0) {
 			ret = best_supply_uV;
-			goto out2;
+			goto out;
 		}
 
 		best_supply_uV += rdev->desc->min_dropout_uV;
@@ -3049,7 +3114,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		current_supply_uV = _regulator_get_voltage(rdev->supply->rdev);
 		if (current_supply_uV < 0) {
 			ret = current_supply_uV;
-			goto out2;
+			goto out;
 		}
 
 		supply_change_uV = best_supply_uV - current_supply_uV;
@@ -3061,7 +3126,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		if (ret) {
 			dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
 					ret);
-			goto out2;
+			goto out;
 		}
 	}
 
@@ -3071,7 +3136,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		ret = _regulator_do_set_suspend_voltage(rdev, min_uV,
 							max_uV, state);
 	if (ret < 0)
-		goto out2;
+		goto out;
 
 	if (supply_change_uV < 0) {
 		ret = regulator_set_voltage_unlocked(rdev->supply,
@@ -3085,11 +3150,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 out:
 	return ret;
-out2:
-	voltage->min_uV = old_min_uV;
-	voltage->max_uV = old_max_uV;
-
-	return ret;
 }
 
 static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
@@ -3262,17 +3322,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
 			goto out;
 		}
 
-		/*
-		 * Lock just the supply regulators, as the regulator itself
-		 * is already locked by regulator_lock_coupled().
-		 */
-		if (best_rdev->supply)
-			regulator_lock_supply(best_rdev->supply->rdev);
-
 		ret = regulator_set_voltage_rdev(best_rdev, best_uV,
 						 best_uV, state);
-		if (best_rdev->supply)
-			regulator_unlock_supply(best_rdev->supply->rdev);
 
 		if (ret < 0)
 			goto out;
@@ -3304,12 +3355,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 {
 	int ret = 0;
 
-	regulator_lock_supply(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);
 
 	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
 					     PM_SUSPEND_ON);
 
-	regulator_unlock_supply(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);
 
 	return ret;
 }
@@ -3387,12 +3438,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
 	if (regulator_check_states(state) || state == PM_SUSPEND_ON)
 		return -EINVAL;
 
-	regulator_lock_supply(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);
 
 	ret = _regulator_set_suspend_voltage(regulator, min_uV,
 					     max_uV, state);
 
-	regulator_unlock_supply(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);
 
 	return ret;
 }
@@ -3584,11 +3635,11 @@ int regulator_get_voltage(struct regulator *regulator)
 {
 	int ret;
 
-	regulator_lock_supply(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);
 
 	ret = _regulator_get_voltage(regulator->rdev);
 
-	regulator_unlock_supply(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);
 
 	return ret;
 }
-- 
2.7.4

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

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

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:

> I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> series on their boards.

That'd be great, yeah - I can run some tests with kernelci as well,
though the latency is relatively high there.

Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.

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

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:42 PM, Mark Brown wrote:
> On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> 
>> I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
>> series on their boards.
> 
> That'd be great, yeah - I can run some tests with kernelci as well,
> though the latency is relatively high there.
> 
> Is it possible to respin this in terms of the test/coupled branch in my
> git tree?  There were a bunch of build fixes that got sent and I don't
> want to loose any of those.
> 

I have included those build fixes in my new patchset. I can extract the newest 
changes and rebase them against your test/coupled branch, if that's what you're 
asking for.

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

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

On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:
> On 03/09/2018 01:42 PM, Mark Brown wrote:

> > Is it possible to respin this in terms of the test/coupled branch in my
> > git tree?  There were a bunch of build fixes that got sent and I don't
> > want to loose any of those.

> I have included those build fixes in my new patchset. I can extract the
> newest changes and rebase them against your test/coupled branch, if that's
> what you're asking for.

That was what I was asking for, yeah (though it can wait till people
manage to test).  It would also be helpful from a review point of view
to see what the fix is.

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

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:56 PM, Mark Brown wrote:
> On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:
>> On 03/09/2018 01:42 PM, Mark Brown wrote:
> 
>>> Is it possible to respin this in terms of the test/coupled branch in my
>>> git tree?  There were a bunch of build fixes that got sent and I don't
>>> want to loose any of those.
> 
>> I have included those build fixes in my new patchset. I can extract the
>> newest changes and rebase them against your test/coupled branch, if that's
>> what you're asking for.
> 
> That was what I was asking for, yeah (though it can wait till people
> manage to test).  It would also be helpful from a review point of view
> to see what the fix is.
>

Hi,
this is the patch containing all the new changes applied to coupled regulators.


 From 3b073fa86bd55feda7d3855c0c776aad6b659f67 Mon Sep 17 00:00:00 2001
From: Maciej Purski <m.purski@samsung.com>
Date: Fri, 9 Mar 2018 14:02:03 +0100
Subject: [PATCH] regulator: core: Make locks re-entrant

Setting voltage, enabling/disabling regulators requires operations on
all regulators related with the regulator being changed. Therefore,
all of them should be locked for the whole operation. With the current
locking implementation, adding additional dependency (regulators
coupling) causes deadlocks in some cases.

Introduce a possibility to attempt to lock a mutex multiple times
by the same task without waiting on a mutex. This should handle all
reasonable coupling-supplying combinations, especially when two coupled
regulators share common supplies. The only situation that should be
forbidden is simultaneous coupling and supplying between a pair of
regulators.

The idea is based on clk core.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
  drivers/regulator/core.c         | 214 ++++++++++++++++++++-------------------
  include/linux/regulator/driver.h |   2 +
  2 files changed, 113 insertions(+), 103 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e685f8b..1999290 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -96,6 +96,7 @@ struct regulator_supply_alias {
  	const char *alias_supply;
  };

+
  static int _regulator_is_enabled(struct regulator_dev *rdev);
  static int _regulator_disable(struct regulator_dev *rdev);
  static int _regulator_get_voltage(struct regulator_dev *rdev);
@@ -151,65 +152,81 @@ static inline struct regulator_dev *rdev_get_supply(struct 
regulator_dev *rdev)
  	return NULL;
  }

-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev:         regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static void regulator_lock_nested(struct regulator_dev *rdev,
+				  unsigned int subclass)
  {
-	int i;
+	if (!mutex_trylock(&rdev->mutex)) {
+		if (rdev->mutex_owner == current) {
+			rdev->ref_cnt++;
+			return;
+		}
+		mutex_lock_nested(&rdev->mutex, subclass);
+	}

-	for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
-		mutex_lock_nested(&rdev->mutex, i);
+	rdev->ref_cnt = 1;
+	rdev->mutex_owner = current;
  }

-/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev:         regulator source
- */
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock(struct regulator_dev *rdev)
  {
-	struct regulator *supply;
+	if (rdev->ref_cnt != 0) {
+		rdev->ref_cnt--;

-	while (1) {
-		mutex_unlock(&rdev->mutex);
-		supply = rdev->supply;
+		if (!rdev->ref_cnt) {
+			rdev->mutex_owner = NULL;
+			mutex_unlock(&rdev->mutex);
+		}
+	}
+}

-		if (!rdev->supply)
-			return;
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+				    unsigned int subclass)
+{
+	struct regulator_dev *c_rdev;
+	int i;
+
+	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+		if (!c_rdev)
+			continue;
+
+		regulator_lock_nested(c_rdev, subclass++);

-		rdev = supply->rdev;
+		if (c_rdev->supply)
+			subclass = regulator_lock_recursive(c_rdev->supply->rdev,
+							    subclass);
  	}
+
+	return subclass;
  }

-/**
- * regulator_lock_coupled - lock a group of coupled regulators
- * @rdev:      regulator source
- */
-static void regulator_lock_coupled(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
  {
-	struct coupling_desc *c_desc = &rdev->coupling_desc;
-	int n_coupled = c_desc->n_coupled;
+	struct regulator_dev *c_rdev;
  	int i;

-	for (i = 0; i < n_coupled; i++)
-		if (c_desc->coupled_rdevs[i])
-			mutex_lock_nested(&c_desc->coupled_rdevs[i]->mutex, i);
+	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+		if (!c_rdev)
+			continue;
+
+		regulator_unlock(c_rdev);
+
+		if (c_rdev->supply)
+			regulator_unlock_dependent(c_rdev->supply->rdev);
+	}
  }

-/**
- * regulator_unlock_coupled - unlock a group of coupled regulators
- * @rdev:      regulator source
- */
-static void regulator_unlock_coupled(struct regulator_dev *rdev)
+static inline void regulator_lock(struct regulator_dev *rdev)
  {
-	struct coupling_desc *c_desc = &rdev->coupling_desc;
-	int n_coupled = c_desc->n_coupled;
-	int i;
+	regulator_lock_nested(rdev, 0);
+}

-	for (i = 0; i < n_coupled; i++)
-		if (c_desc->coupled_rdevs[i])
-			mutex_unlock(&c_desc->coupled_rdevs[i]->mutex);
+static inline void regulator_lock_dependent(struct regulator_dev *rdev)
+{
+	regulator_lock_recursive(rdev, 0);
  }

  /**
@@ -385,9 +402,9 @@ static ssize_t regulator_uV_show(struct device *dev,
  	struct regulator_dev *rdev = dev_get_drvdata(dev);
  	ssize_t ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	ret = sprintf(buf, "%d\n", _regulator_get_voltage(rdev));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -451,9 +468,9 @@ static ssize_t regulator_state_show(struct device *dev,
  	struct regulator_dev *rdev = dev_get_drvdata(dev);
  	ssize_t ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	ret = regulator_print_state(buf, _regulator_is_enabled(rdev));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -561,10 +578,10 @@ static ssize_t regulator_total_uA_show(struct device *dev,
  	struct regulator *regulator;
  	int uA = 0;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	list_for_each_entry(regulator, &rdev->consumer_list, list)
  		uA += regulator->uA_load;
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return sprintf(buf, "%d\n", uA);
  }
  static DEVICE_ATTR(requested_microamps, 0444, regulator_total_uA_show, NULL);
@@ -1356,7 +1373,7 @@ static struct regulator *create_regulator(struct 
regulator_dev *rdev,
  	if (regulator == NULL)
  		return NULL;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	regulator->rdev = rdev;
  	list_add(&regulator->list, &rdev->consumer_list);

@@ -1411,12 +1428,12 @@ static struct regulator *create_regulator(struct 
regulator_dev *rdev,
  	    _regulator_is_enabled(rdev))
  		regulator->always_on = true;

-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return regulator;
  overflow_err:
  	list_del(&regulator->list);
  	kfree(regulator);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return NULL;
  }

@@ -1805,13 +1822,13 @@ static void _regulator_put(struct regulator *regulator)
  	/* remove any sysfs entries */
  	if (regulator->dev)
  		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	list_del(&regulator->list);

  	rdev->open_count--;
  	rdev->exclusive = 0;
  	put_device(&rdev->dev);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	kfree_const(regulator->supply_name);
  	kfree(regulator);
@@ -2240,10 +2257,10 @@ int regulator_enable(struct regulator *regulator)
  			return ret;
  	}

-	regulator_lock_coupled(rdev);
+	regulator_lock_dependent(rdev);
  	ret = _regulator_enable(rdev);
  	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
-	regulator_unlock_coupled(rdev);
+	regulator_unlock_dependent(rdev);

  	if (ret != 0 && rdev->supply)
  		regulator_disable(rdev->supply);
@@ -2349,10 +2366,10 @@ int regulator_disable(struct regulator *regulator)
  	if (regulator->always_on)
  		return 0;

-	regulator_lock_coupled(rdev);
+	regulator_lock_dependent(rdev);
  	ret = _regulator_disable(rdev);
  	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
-	regulator_unlock_coupled(rdev);
+	regulator_unlock_dependent(rdev);

  	if (ret == 0 && rdev->supply)
  		regulator_disable(rdev->supply);
@@ -2401,11 +2418,11 @@ int regulator_force_disable(struct regulator *regulator)
  	struct regulator_dev *rdev = regulator->rdev;
  	int ret;

-	regulator_lock_coupled(rdev);
+	regulator_lock_dependent(rdev);
  	regulator->uA_load = 0;
  	ret = _regulator_force_disable(regulator->rdev);
  	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
-	regulator_unlock_coupled(rdev);
+	regulator_unlock_dependent(rdev);

  	if (rdev->supply)
  		while (rdev->open_count--)
@@ -2421,7 +2438,7 @@ static void regulator_disable_work(struct work_struct *work)
  						  disable_work.work);
  	int count, i, ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	BUG_ON(!rdev->deferred_disables);

@@ -2442,7 +2459,7 @@ static void regulator_disable_work(struct work_struct *work)
  			rdev_err(rdev, "Deferred disable failed: %d\n", ret);
  	}

-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	if (rdev->supply) {
  		for (i = 0; i < count; i++) {
@@ -2477,11 +2494,11 @@ int regulator_disable_deferred(struct regulator 
*regulator, int ms)
  	if (!ms)
  		return regulator_disable(regulator);

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	rdev->deferred_disables++;
  	mod_delayed_work(system_power_efficient_wq, &rdev->disable_work,
  			 msecs_to_jiffies(ms));
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return 0;
  }
@@ -2513,10 +2530,10 @@ static int _regulator_list_voltage(struct regulator_dev 
*rdev,
  		if (selector >= rdev->desc->n_voltages)
  			return -EINVAL;
  		if (lock)
-			mutex_lock(&rdev->mutex);
+			regulator_lock(rdev);
  		ret = ops->list_voltage(rdev, selector);
  		if (lock)
-			mutex_unlock(&rdev->mutex);
+			regulator_unlock(rdev);
  	} else if (rdev->is_switch && rdev->supply) {
  		ret = _regulator_list_voltage(rdev->supply->rdev,
  					      selector, lock);
@@ -2553,9 +2570,9 @@ int regulator_is_enabled(struct regulator *regulator)
  	if (regulator->always_on)
  		return 1;

-	mutex_lock(&regulator->rdev->mutex);
+	regulator_lock_dependent(regulator->rdev);
  	ret = _regulator_is_enabled(regulator->rdev);
-	mutex_unlock(&regulator->rdev->mutex);
+	regulator_unlock_dependent(regulator->rdev);

  	return ret;
  }
@@ -3267,17 +3284,8 @@ static int regulator_balance_voltage(struct regulator_dev 
*rdev,
  			goto out;
  		}

-		/*
-		 * Lock just the supply regulators, as the regulator itself
-		 * is already locked by regulator_lock_coupled().
-		 */
-		if (best_rdev->supply)
-			regulator_lock_supply(best_rdev->supply->rdev);
-
  		ret = regulator_set_voltage_rdev(best_rdev, best_uV,
  						 best_uV, state);
-		if (best_rdev->supply)
-			regulator_unlock_supply(best_rdev->supply->rdev);

  		if (ret < 0)
  			goto out;
@@ -3309,12 +3317,12 @@ int regulator_set_voltage(struct regulator *regulator, 
int min_uV, int max_uV)
  {
  	int ret = 0;

-	regulator_lock_coupled(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);

  	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
  					     PM_SUSPEND_ON);

-	regulator_unlock_coupled(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);

  	return ret;
  }
@@ -3392,12 +3400,12 @@ int regulator_set_suspend_voltage(struct regulator 
*regulator, int min_uV,
  	if (regulator_check_states(state) || state == PM_SUSPEND_ON)
  		return -EINVAL;

-	regulator_lock_supply(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);

  	ret = _regulator_set_suspend_voltage(regulator, min_uV,
  					     max_uV, state);

-	regulator_unlock_supply(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);

  	return ret;
  }
@@ -3499,7 +3507,7 @@ int regulator_sync_voltage(struct regulator *regulator)
  	struct regulator_voltage *voltage = &regulator->voltage[PM_SUSPEND_ON];
  	int ret, min_uV, max_uV;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	if (!rdev->desc->ops->set_voltage &&
  	    !rdev->desc->ops->set_voltage_sel) {
@@ -3528,7 +3536,7 @@ int regulator_sync_voltage(struct regulator *regulator)
  	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);

  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }
  EXPORT_SYMBOL_GPL(regulator_sync_voltage);
@@ -3589,11 +3597,11 @@ int regulator_get_voltage(struct regulator *regulator)
  {
  	int ret;

-	regulator_lock_supply(regulator->rdev);
+	regulator_lock_dependent(regulator->rdev);

  	ret = _regulator_get_voltage(regulator->rdev);

-	regulator_unlock_supply(regulator->rdev);
+	regulator_unlock_dependent(regulator->rdev);

  	return ret;
  }
@@ -3621,7 +3629,7 @@ int regulator_set_current_limit(struct regulator *regulator,
  	struct regulator_dev *rdev = regulator->rdev;
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	/* sanity check */
  	if (!rdev->desc->ops->set_current_limit) {
@@ -3636,7 +3644,7 @@ int regulator_set_current_limit(struct regulator *regulator,

  	ret = rdev->desc->ops->set_current_limit(rdev, min_uA, max_uA);
  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }
  EXPORT_SYMBOL_GPL(regulator_set_current_limit);
@@ -3645,7 +3653,7 @@ static int _regulator_get_current_limit(struct 
regulator_dev *rdev)
  {
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	/* sanity check */
  	if (!rdev->desc->ops->get_current_limit) {
@@ -3655,7 +3663,7 @@ static int _regulator_get_current_limit(struct 
regulator_dev *rdev)

  	ret = rdev->desc->ops->get_current_limit(rdev);
  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }

@@ -3691,7 +3699,7 @@ int regulator_set_mode(struct regulator *regulator, 
unsigned int mode)
  	int ret;
  	int regulator_curr_mode;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	/* sanity check */
  	if (!rdev->desc->ops->set_mode) {
@@ -3715,7 +3723,7 @@ int regulator_set_mode(struct regulator *regulator, 
unsigned int mode)

  	ret = rdev->desc->ops->set_mode(rdev, mode);
  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }
  EXPORT_SYMBOL_GPL(regulator_set_mode);
@@ -3724,7 +3732,7 @@ static unsigned int _regulator_get_mode(struct 
regulator_dev *rdev)
  {
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	/* sanity check */
  	if (!rdev->desc->ops->get_mode) {
@@ -3734,7 +3742,7 @@ static unsigned int _regulator_get_mode(struct 
regulator_dev *rdev)

  	ret = rdev->desc->ops->get_mode(rdev);
  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }

@@ -3755,7 +3763,7 @@ static int _regulator_get_error_flags(struct regulator_dev 
*rdev,
  {
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	/* sanity check */
  	if (!rdev->desc->ops->get_error_flags) {
@@ -3765,7 +3773,7 @@ static int _regulator_get_error_flags(struct regulator_dev 
*rdev,

  	ret = rdev->desc->ops->get_error_flags(rdev, flags);
  out:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);
  	return ret;
  }

@@ -3814,10 +3822,10 @@ int regulator_set_load(struct regulator *regulator, int 
uA_load)
  	struct regulator_dev *rdev = regulator->rdev;
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	regulator->uA_load = uA_load;
  	ret = drms_uA_update(rdev);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -3845,7 +3853,7 @@ int regulator_allow_bypass(struct regulator *regulator, 
bool enable)
  	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_BYPASS))
  		return 0;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	if (enable && !regulator->bypass) {
  		rdev->bypass_count++;
@@ -3869,7 +3877,7 @@ int regulator_allow_bypass(struct regulator *regulator, 
bool enable)
  	if (ret == 0)
  		regulator->bypass = enable;

-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -4631,9 +4639,9 @@ static int _regulator_suspend_late(struct device *dev, 
void *data)
  	suspend_state_t *state = data;
  	int ret;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);
  	ret = suspend_set_state(rdev, *state);
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -4662,14 +4670,14 @@ static int _regulator_resume_early(struct device *dev, 
void *data)
  	if (rstate == NULL)
  		return -EINVAL;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	if (rdev->desc->ops->resume_early &&
  	    (rstate->enabled == ENABLE_IN_SUSPEND ||
  	     rstate->enabled == DISABLE_IN_SUSPEND))
  		ret = rdev->desc->ops->resume_early(rdev);

-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return ret;
  }
@@ -4971,7 +4979,7 @@ static int __init regulator_late_cleanup(struct device 
*dev, void *data)
  	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
  		return 0;

-	mutex_lock(&rdev->mutex);
+	regulator_lock(rdev);

  	if (rdev->use_count)
  		goto unlock;
@@ -5002,7 +5010,7 @@ static int __init regulator_late_cleanup(struct device 
*dev, void *data)
  	}

  unlock:
-	mutex_unlock(&rdev->mutex);
+	regulator_unlock(rdev);

  	return 0;
  }
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 85452bb..505bc60 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -446,6 +446,8 @@ struct regulator_dev {

  	struct blocking_notifier_head notifier;
  	struct mutex mutex; /* consumer lock */
+	struct task_struct *mutex_owner;
+	int ref_cnt;
  	struct module *owner;
  	struct device dev;
  	struct regulation_constraints *constraints;
-- 
2.7.4

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [180309 12:43]:
> On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> 
> > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> > series on their boards.

I gave it a quick try and this set still causes at least mmc0
to fail for me.

Regards,

Tony

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 04:58 PM, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [180309 12:43]:
>> On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
>>
>>> I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
>>> series on their boards.
> 
> I gave it a quick try and this set still causes at least mmc0
> to fail for me.
> 
> Regards,
> 
> Tony
> 
> 

Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
where the problem is.

Best Regards,
Maciej Purski

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f1f11cf..0e80ba5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
  {
  	struct regulator_dev *rdev = regulator->rdev;
  	int ret = 0;
-	int ret2;

  	if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
  		rdev_err(rdev, "not all coupled regulators registered\n");
@@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)

  	regulator_lock_dependent(rdev);
  	ret = _regulator_enable(rdev);
-	ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
  	regulator_unlock_dependent(rdev);

-	if (ret2 != 0) {
-		rdev_err(rdev,
-			"balancing failed when trying to enable regulator: %d",
-			ret2);
-	}
-
  	if (ret != 0 && rdev->supply)
  		regulator_disable(rdev->supply);

@@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
  		ret = regulator_set_voltage_unlocked(rdev->supply,
  				best_supply_uV, INT_MAX, state);
  		if (ret)
-			dev_err(&rdev->dev, "Failed to decrease supply voltage: %d\n",
+			dev_warn(&rdev->dev, "Failed to decrease supply voltage: %d\n",
  					ret);
  		/* No need to fail here */
  		ret = 0;
@@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
  		ret = regulator_set_voltage_rdev(best_rdev, best_uV,
  						 best_uV, state);

-		if (ret < 0) {
-			rdev_err(rdev,
-				"Failed to set voltage with error: %d", ret);
+		if (ret < 0)
  			goto out;
-		}
  	}

  out:

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Maciej Purski <m.purski@samsung.com> [180312 12:24]:
> On 03/09/2018 04:58 PM, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [180309 12:43]:
> > > On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> > > 
> > > > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> > > > series on their boards.
> > 
> > I gave it a quick try and this set still causes at least mmc0
> > to fail for me.
>
> Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
> where the problem is.

Sorry for the delay, now back from ELC. I tried applying this on
top of Linux next + your six patches but it fails to apply. Do
I need something else too?

Regards,

Tony


> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index f1f11cf..0e80ba5 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	int ret = 0;
> -	int ret2;
> 
>  	if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
>  		rdev_err(rdev, "not all coupled regulators registered\n");
> @@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)
> 
>  	regulator_lock_dependent(rdev);
>  	ret = _regulator_enable(rdev);
> -	ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
> +	regulator_balance_voltage(rdev, PM_SUSPEND_ON);
>  	regulator_unlock_dependent(rdev);
> 
> -	if (ret2 != 0) {
> -		rdev_err(rdev,
> -			"balancing failed when trying to enable regulator: %d",
> -			ret2);
> -	}
> -
>  	if (ret != 0 && rdev->supply)
>  		regulator_disable(rdev->supply);
> 
> @@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
>  		ret = regulator_set_voltage_unlocked(rdev->supply,
>  				best_supply_uV, INT_MAX, state);
>  		if (ret)
> -			dev_err(&rdev->dev, "Failed to decrease supply voltage: %d\n",
> +			dev_warn(&rdev->dev, "Failed to decrease supply voltage: %d\n",
>  					ret);
>  		/* No need to fail here */
>  		ret = 0;
> @@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
>  		ret = regulator_set_voltage_rdev(best_rdev, best_uV,
>  						 best_uV, state);
> 
> -		if (ret < 0) {
> -			rdev_err(rdev,
> -				"Failed to set voltage with error: %d", ret);
> +		if (ret < 0)
>  			goto out;
> -		}
>  	}
> 
>  out:
> 
>