[PATCH v3 0/4] Add coupled regulators mechanism

[PATCH v3 0/4] 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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. 

Note on the locking model:
When balancing voltage of a group of coupled regulators, we lock all
of them for the whole operation. When voltage of an individual regulator
is about to change, its suppliers are additionally locked.


Best regards,

	Maciej Purski

---
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 (4):
  regulator: core: Move of_find_regulator_by_node() to of_regulator.c
  regulator: bindings: Add properties for coupled regulators
  regulator: core: Parse coupled regulators properties
  regulator: core: Balance coupled regulators voltages

 .../devicetree/bindings/regulator/regulator.txt    |   5 +
 drivers/regulator/core.c                           | 424 +++++++++++++++++++--
 drivers/regulator/internal.h                       |  16 +
 drivers/regulator/of_regulator.c                   |  74 ++++
 include/linux/regulator/driver.h                   |  16 +
 include/linux/regulator/machine.h                  |   4 +
 6 files changed, 501 insertions(+), 38 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/4] regulator: core: Move of_find_regulator_by_node() to of_regulator.c

[Maciej Purski]

As of_find_regulator_by_node() is an of function it should be moved from
core to of_regulator.c. It provides better separation of device tree
functions from the core and allows other of_functions in of_regulator
to resolve device_node to regulator_dev. This will be useful for
implementation of parsing coupled regulators properties.

Declare of_find_regulator_by_node() function in internal.h as well as
regulator_class and dev_to_rdev(), as they are needed by
of_find_regulator_by_node().

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c         | 23 +----------------------
 drivers/regulator/internal.h     |  9 +++++++++
 drivers/regulator/of_regulator.c | 14 ++++++++++++++
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b64b791..9662e54 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -58,8 +58,6 @@ static bool has_full_constraints;
 
 static struct dentry *debugfs_root;
 
-static struct class regulator_class;
-
 /*
  * struct regulator_map
  *
@@ -112,11 +110,6 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  const char *supply_name);
 static void _regulator_put(struct regulator *regulator);
 
-static struct regulator_dev *dev_to_rdev(struct device *dev)
-{
-	return container_of(dev, struct regulator_dev, dev);
-}
-
 static const char *rdev_get_name(struct regulator_dev *rdev)
 {
 	if (rdev->constraints && rdev->constraints->name)
@@ -1417,20 +1410,6 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 	}
 }
 
-static int of_node_match(struct device *dev, const void *data)
-{
-	return dev->of_node == data;
-}
-
-static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
-{
-	struct device *dev;
-
-	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
-
-	return dev ? dev_to_rdev(dev) : NULL;
-}
-
 static int regulator_match(struct device *dev, const void *data)
 {
 	struct regulator_dev *r = dev_to_rdev(dev);
@@ -3918,7 +3897,7 @@ static void regulator_dev_release(struct device *dev)
 	kfree(rdev);
 }
 
-static struct class regulator_class = {
+struct class regulator_class = {
 	.name = "regulator",
 	.dev_release = regulator_dev_release,
 	.dev_groups = regulator_dev_groups,
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 66a8ea0..2f3218b 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -35,6 +35,15 @@ struct regulator {
 	struct dentry *debugfs;
 };
 
+extern struct class regulator_class;
+
+static inline struct regulator_dev *dev_to_rdev(struct device *dev)
+{
+	return container_of(dev, struct regulator_dev, dev);
+}
+
+struct regulator_dev *of_find_regulator_by_node(struct device_node *np);
+
 #ifdef CONFIG_OF
 struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 14637a0..54e810a 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -376,3 +376,17 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 
 	return init_data;
 }
+
+static int of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
+
+	return dev ? dev_to_rdev(dev) : NULL;
+}
-- 
2.7.4

Applied "regulator: core: Move of_find_regulator_by_node() to of_regulator.c" to the regulator tree

[Mark Brown]

The patch

   regulator: core: Move of_find_regulator_by_node() to of_regulator.c

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 148096af0bf381c78afe253c07ef1c77778f0e68 Mon Sep 17 00:00:00 2001
From: Maciej Purski <m.purski@samsung.com>
Date: Mon, 22 Jan 2018 15:30:06 +0100
Subject: [PATCH] regulator: core: Move of_find_regulator_by_node() to
 of_regulator.c

As of_find_regulator_by_node() is an of function it should be moved from
core.c to of_regulator.c. It provides better separation of device tree
functions from the core and allows other of_functions in of_regulator.c
to resolve device_node to regulator_dev. This will be useful for
implementation of parsing coupled regulators properties.

Declare of_find_regulator_by_node() function in internal.h as well as
regulator_class and dev_to_rdev(), as they are needed by
of_find_regulator_by_node().

Signed-off-by: Maciej Purski <m.purski@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c         | 23 +----------------------
 drivers/regulator/internal.h     |  9 +++++++++
 drivers/regulator/of_regulator.c | 14 ++++++++++++++
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 365b32e3f505..5f7678292cef 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -58,8 +58,6 @@ static bool has_full_constraints;
 
 static struct dentry *debugfs_root;
 
-static struct class regulator_class;
-
 /*
  * struct regulator_map
  *
@@ -112,11 +110,6 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  const char *supply_name);
 static void _regulator_put(struct regulator *regulator);
 
-static struct regulator_dev *dev_to_rdev(struct device *dev)
-{
-	return container_of(dev, struct regulator_dev, dev);
-}
-
 static const char *rdev_get_name(struct regulator_dev *rdev)
 {
 	if (rdev->constraints && rdev->constraints->name)
@@ -1417,20 +1410,6 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 	}
 }
 
-static int of_node_match(struct device *dev, const void *data)
-{
-	return dev->of_node == data;
-}
-
-static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
-{
-	struct device *dev;
-
-	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
-
-	return dev ? dev_to_rdev(dev) : NULL;
-}
-
 static int regulator_match(struct device *dev, const void *data)
 {
 	struct regulator_dev *r = dev_to_rdev(dev);
@@ -3918,7 +3897,7 @@ static void regulator_dev_release(struct device *dev)
 	kfree(rdev);
 }
 
-static struct class regulator_class = {
+struct class regulator_class = {
 	.name = "regulator",
 	.dev_release = regulator_dev_release,
 	.dev_groups = regulator_dev_groups,
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 66a8ea0c8386..2f3218be5b8d 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -35,6 +35,15 @@ struct regulator {
 	struct dentry *debugfs;
 };
 
+extern struct class regulator_class;
+
+static inline struct regulator_dev *dev_to_rdev(struct device *dev)
+{
+	return container_of(dev, struct regulator_dev, dev);
+}
+
+struct regulator_dev *of_find_regulator_by_node(struct device_node *np);
+
 #ifdef CONFIG_OF
 struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 14637a01ba2d..54e810ae93d6 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -376,3 +376,17 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 
 	return init_data;
 }
+
+static int of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
+
+	return dev ? dev_to_rdev(dev) : NULL;
+}
-- 
2.15.1

[PATCH v3 3/4] 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.

Each coupled regulator, should have phandles to every other in their
DTS. A group of coupled regulators shares a common structure
coupling_desc, which contains information obtained from the device tree:
array of pointers to other coupled regulators and number of coupled
regulators, which can't be higher than defined MAX_COUPLED.

Obtain all the necessery data in regulator_resolve_coupling(),
which can succeed only if all the coupled regulators are already
initialized, so it should be done only once per coupled regulators group
by the last regulator initialized.

Check if coupled regulators phandles arrays match for all coupled
regulators and if their max_spread values are the same.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/regulator/core.c          | 132 ++++++++++++++++++++++++++++++++++++++
 drivers/regulator/internal.h      |   7 ++
 drivers/regulator/of_regulator.c  |  60 +++++++++++++++++
 include/linux/regulator/driver.h  |  16 +++++
 include/linux/regulator/machine.h |   4 ++
 5 files changed, 219 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9662e54..c3e6b35 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3940,6 +3940,133 @@ static int regulator_register_resolve_supply(struct device *dev, void *data)
 	return 0;
 }
 
+static int check_coupled_regulators_array(struct coupling_desc *c_desc,
+					  int index)
+{
+	struct regulator_dev **rdevs = c_desc->coupled_rdevs;
+	struct regulator_dev *rdev = rdevs[index];
+	struct device_node *node = rdev->dev.of_node;
+	int n_coupled = c_desc->n_coupled;
+	int i, j;
+
+	/* Different number of regulators coupled */
+	if (of_count_phandle_with_args(node, "regulator-coupled-with", 0) !=
+				      (n_coupled - 1))
+		return -EINVAL;
+
+	for (i = 0; i < n_coupled; i++) {
+		/* Don't look for rdev in its own array */
+		if (rdevs[i] == rdev)
+			continue;
+
+		for (j = 0; j < n_coupled - 1; j++) {
+			struct regulator_dev *tmp;
+
+			tmp = of_parse_coupled_regulator(rdev, j);
+			if (!tmp)
+				return -EINVAL;
+
+			/* Regulator found */
+			if (tmp == rdevs[i])
+				break;
+		}
+	}
+
+	return 0;
+}
+
+static int check_coupled_regulator_ops(struct coupling_desc *c_desc,
+				       int index)
+{
+	struct regulator_dev *rdev = c_desc->coupled_rdevs[index];
+
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
+		return -EINVAL;
+
+	if (!rdev->desc->ops->set_voltage &&
+	    !rdev->desc->ops->set_voltage_sel)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void regulator_resolve_coupling(struct regulator_dev *rdev)
+{
+	struct coupling_desc *c_desc;
+	int i;
+
+	c_desc = kzalloc(sizeof(*c_desc), GFP_KERNEL);
+	if (!c_desc)
+		return;
+
+	if (of_fill_coupled_regulators_array(rdev, c_desc))
+		goto err;
+
+	if (rdev->constraints->max_spread <= 0) {
+		rdev_err(rdev, "wrong max_spread value\n");
+		goto err;
+	}
+
+	/*
+	 * Each coupled regulator must have phandles to all regulators
+	 * they are coupled with. This should be checked by comparing
+	 * rdevs array with phandles array of each regulator.
+	 * There's no need for checking rdevs[0] as its device_node
+	 * was a source to fill rdevs array.
+	 */
+	for (i = 1; i < c_desc->n_coupled; i++) {
+		if (check_coupled_regulators_array(c_desc, i)) {
+			rdev_err(rdev,
+				 "coupled regulators arrays mismatch\n");
+			goto err;
+		}
+	}
+
+	for (i = 0; i < c_desc->n_coupled; i++) {
+		/*
+		 * All coupled regulators max_spread
+		 * must have the same value.
+		 */
+		if (c_desc->coupled_rdevs[i]->constraints->max_spread !=
+		    rdev->constraints->max_spread) {
+			rdev_err(rdev, "coupled regulators max_spread mismatch\n");
+			goto err;
+		}
+
+		/*
+		 * Regulators which can't change their voltage can't be
+		 * coupled.
+		 */
+		if (check_coupled_regulator_ops(c_desc, i)) {
+			rdev_err(rdev, "coupled regulators ops not valid\n");
+			goto err;
+		}
+	}
+
+	for (i = 0; i < c_desc->n_coupled; i++)
+		c_desc->coupled_rdevs[i]->coupling_desc = c_desc;
+
+	return;
+err:
+	kfree(c_desc);
+}
+
+static void regulator_clean_coupling(struct regulator_dev *rdev)
+{
+	struct regulator_dev *c_rdevs[MAX_COUPLED];
+	struct coupling_desc *c_desc;
+	int i;
+
+	if (!rdev->coupling_desc)
+		return;
+
+	c_desc = rdev->coupling_desc;
+	for (i = 0; i < c_desc->n_coupled; i++)
+		c_rdevs[0]->coupling_desc = NULL;
+
+	kfree(c_desc);
+}
+
 /**
  * regulator_register - register regulator
  * @regulator_desc: regulator to register
@@ -4103,6 +4230,10 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	dev_set_drvdata(&rdev->dev, rdev);
 	rdev_init_debugfs(rdev);
 
+	mutex_lock(&regulator_list_mutex);
+	regulator_resolve_coupling(rdev);
+	mutex_unlock(&regulator_list_mutex);
+
 	/* try to resolve regulators supply since a new one was registered */
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_register_resolve_supply);
@@ -4116,6 +4247,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 wash:
 	kfree(rdev->constraints);
 	mutex_lock(&regulator_list_mutex);
+	regulator_clean_coupling(rdev);
 	regulator_ena_gpio_free(rdev);
 	mutex_unlock(&regulator_list_mutex);
 clean:
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 2f3218b..6290384 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -16,6 +16,8 @@
 #ifndef __REGULATOR_INTERNAL_H
 #define __REGULATOR_INTERNAL_H
 
+#include <linux/regulator/driver.h>
+
 /*
  * struct regulator
  *
@@ -70,4 +72,9 @@ enum regulator_get_type {
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
 
+struct regulator_dev *of_parse_coupled_regulator(struct regulator_dev *rdev,
+						 int index);
+
+int of_fill_coupled_regulators_array(struct regulator_dev *rdev,
+				     struct coupling_desc *c_desc);
 #endif
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 54e810a..f6a70e6 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");
 
@@ -390,3 +394,59 @@ struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 
 	return dev ? dev_to_rdev(dev) : 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);
+	if (!c_rdev)
+		return NULL;
+
+	return c_rdev;
+}
+
+int of_fill_coupled_regulators_array(struct regulator_dev *rdev,
+				     struct coupling_desc *c_desc)
+{
+	struct regulator_dev *c_rdev;
+	struct device_node *node = rdev->dev.of_node;
+	int n_phandles, i;
+
+	n_phandles = of_count_phandle_with_args(node,
+						"regulator-coupled-with", 0);
+	if (n_phandles <= 0) {
+		dev_dbg(&rdev->dev, "no coupled regulators phandles provided\n");
+		return -EINVAL;
+	}
+
+	if (n_phandles >= MAX_COUPLED) {
+		dev_err(&rdev->dev, "too many coupled regulators phandles\n");
+		return -EINVAL;
+	}
+
+	c_desc->n_coupled = n_phandles + 1;
+
+	/*
+	 * Fill rdevs array with pointers to regulators parsed from
+	 * device tree
+	 */
+	c_desc->coupled_rdevs[0] = rdev;
+	for (i = 0; i < n_phandles; i++) {
+		c_rdev = of_parse_coupled_regulator(rdev, i);
+		if (!c_rdev) {
+			dev_dbg(&rdev->dev, "can't parse coupled regulators array\n");
+			return -EINVAL;
+		}
+		c_desc->coupled_rdevs[i + 1] = c_rdev;
+	}
+
+	return 0;
+}
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 94417b4..1a1613b 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>
@@ -402,6 +404,18 @@ struct regulator_config {
 };
 
 /*
+ * struct coupling_desc
+ *
+ * Describes coupling of regulators. Each coupled regulator
+ * contains a pointer to that structure. If the regulator is not
+ * coupled with any other, it should remain NULL.
+ */
+struct coupling_desc {
+	struct regulator_dev *coupled_rdevs[MAX_COUPLED];
+	int n_coupled;
+};
+
+/*
  * struct regulator_dev
  *
  * Voltage / Current regulator class device. One for each
@@ -424,6 +438,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 module *owner;
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 9cd4fef..f871fd1 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -85,6 +85,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.
  *
@@ -136,6 +137,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

Re: [PATCH v3 3/4] regulator: core: Parse coupled regulators properties

[Mark Brown]

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

On Thu, Dec 07, 2017 at 10:46:14AM +0100, Maciej Purski wrote:

> +static int check_coupled_regulators_array(struct coupling_desc *c_desc,
> +					  int index)
> +{

> +	/* Different number of regulators coupled */
> +	if (of_count_phandle_with_args(node, "regulator-coupled-with", 0) !=
> +				      (n_coupled - 1))
> +		return -EINVAL;

This is still DT only code in the core file, we really need the core to
not know anything about DT so that we know that we can support this
feature with other firmwares if we need to.  Just move all the parsing
code into the of_regulator.c then have a second step that goes through
and validates extra stuff like the presence of set voltage operations in
the generic code.

> +		if (c_desc->coupled_rdevs[i]->constraints->max_spread !=
> +		    rdev->constraints->max_spread) {
> +			rdev_err(rdev, "coupled regulators max_spread mismatch\n");
> +			goto err;
> +		}

It's better to print out failing values when things go wrong - it really
helps people debug their DTs if the error messages are specific about
what went wrong.  This applies to a bunch of the errors here.

> +	mutex_lock(&regulator_list_mutex);
> +	regulator_resolve_coupling(rdev);
> +	mutex_unlock(&regulator_list_mutex);
> +

I'd really expect us to be failing to probe if we run into an error.
Otherwise we could end up doing bad things to the hardware at runtime
later on, or confusing ourselves and crashing with partially set up
datastructures.  It's also not 100% clear to me how we handle the case
where only some of the coupled regulators have probed.

> diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
> index 2f3218b..6290384 100644
> --- a/drivers/regulator/internal.h
> +++ b/drivers/regulator/internal.h
> @@ -16,6 +16,8 @@
>  #ifndef __REGULATOR_INTERNAL_H
>  #define __REGULATOR_INTERNAL_H
>  
> +#include <linux/regulator/driver.h>
> +

Why do we need this?

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

Re: [PATCH v3 3/4] regulator: core: Parse coupled regulators properties

[Maciej Purski]

On 12/12/2017 12:35 PM, Mark Brown wrote:
> On Thu, Dec 07, 2017 at 10:46:14AM +0100, Maciej Purski wrote:
>> (...)
>> +	mutex_lock(&regulator_list_mutex);
>> +	regulator_resolve_coupling(rdev);
>> +	mutex_unlock(&regulator_list_mutex);
>> +
> 
> I'd really expect us to be failing to probe if we run into an error.
> Otherwise we could end up doing bad things to the hardware at runtime
> later on, or confusing ourselves and crashing with partially set up
> datastructures.  


We cannot fail to probe if some of the regulators are not registered 
successfully, as depending on the probing sequence, there will be some 
regulators, which won't be available when registering others. We can fail when 
resolving coupling if we encounter other errors such as max_spread not provided 
or disability to set voltage. Maybe that is what you meant.

> It's also not 100% clear to me how we handle the case
> where only some of the coupled regulators have probed.
> 

I have two ideas how to handle it. We can prohibit setting the voltage and 
always fail on attempt to set voltage of a coupled regulator. Or we can just 
ignore it and set voltage of the regulator without checking the unregistered ones.

Re: [PATCH v3 3/4] regulator: core: Parse coupled regulators properties

[Mark Brown]

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

On Thu, Dec 21, 2017 at 11:08:19AM +0100, Maciej Purski wrote:
> On 12/12/2017 12:35 PM, Mark Brown wrote:
> > On Thu, Dec 07, 2017 at 10:46:14AM +0100, Maciej Purski wrote:

> > > +	mutex_lock(&regulator_list_mutex);
> > > +	regulator_resolve_coupling(rdev);
> > > +	mutex_unlock(&regulator_list_mutex);

> > I'd really expect us to be failing to probe if we run into an error.
> > Otherwise we could end up doing bad things to the hardware at runtime
> > later on, or confusing ourselves and crashing with partially set up
> > datastructures.

> We cannot fail to probe if some of the regulators are not registered
> successfully, as depending on the probing sequence, there will be some
> regulators, which won't be available when registering others. We can fail
> when resolving coupling if we encounter other errors such as max_spread not
> provided or disability to set voltage. Maybe that is what you meant.

Yes, exactly - not having all the regulators yet isn't the only possible
error and this will just ignore all errors.

> > It's also not 100% clear to me how we handle the case
> > where only some of the coupled regulators have probed.

> I have two ideas how to handle it. We can prohibit setting the voltage and
> always fail on attempt to set voltage of a coupled regulator. Or we can just
> ignore it and set voltage of the regulator without checking the unregistered
> ones.

At least one of those needs to be implemented (not setting the voltage
seems safer).

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

[PATCH v3 2/4] 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.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 378f6dc..841b1bc 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -60,6 +60,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.
+- 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] regulator: bindings: Add properties for coupled regulators

[Rob Herring]

On Thu, Dec 07, 2017 at 10:46:13AM +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 b277c9976cd814d33fd3fa2122cd67ab31486f5b Mon Sep 17 00:00:00 2001
From: Maciej Purski <m.purski@samsung.com>
Date: Fri, 2 Mar 2018 09:42:45 +0100
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.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maciej Purski <m.purski@samsung.com>
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 2babe15b618d..62510d67cc3a 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.
+- regulator-coupled-max-spread: Max spread between voltages of coupled regulators
+  in microvolts.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
2.16.2

[PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[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_coupled(), which
keeps max_spread constraint fulfilled between a group of coupled
regulators. It should be called if a coupled 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_coupled_get_optimal_voltage()
should find the best possible change for the regulator, which doesn't break
max_spread constraint. In function regulator_balance_coupled_voltage()
optimize number of steps by finding highest voltage difference on each
iteration.

Signed-off-by: Maciej Purski <m.purski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/regulator/core.c | 269 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 253 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c3e6b35..dde77ee 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,9 @@ 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_set_voltage_rdev(struct regulator_dev *rdev,
+				      int min_uV, int max_uV);
+static int regulator_balance_coupled(struct coupling_desc *c_desc);
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
@@ -178,6 +181,32 @@ static void regulator_unlock_supply(struct regulator_dev *rdev)
 }
 
 /**
+ * regulator_lock_coupled - lock a group of coupled regulators
+ * @c_desc:      coupled regulators description source
+ */
+static void regulator_lock_coupled(struct coupling_desc *c_desc)
+{
+	int n_coupled = c_desc->n_coupled;
+	int i;
+
+	for (i = 0; i < n_coupled; i++)
+		mutex_lock_nested(&c_desc->coupled_rdevs[i]->mutex, i);
+}
+
+/**
+ * regulator_unlock_coupled - unlock a group of coupled regulators
+ * @c_desc:      coupled regulators description source
+ */
+static void regulator_unlock_coupled(struct coupling_desc *c_desc)
+{
+	int n_coupled = c_desc->n_coupled;
+	int i;
+
+	for (i = 0; i < n_coupled; i++)
+		mutex_unlock(&c_desc->coupled_rdevs[i]->mutex);
+}
+
+/**
  * 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
@@ -2197,6 +2226,8 @@ int regulator_enable(struct regulator *regulator)
 	if (ret != 0 && rdev->supply)
 		regulator_disable(rdev->supply);
 
+	regulator_balance_coupled(rdev->coupling_desc);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2302,6 +2333,8 @@ int regulator_disable(struct regulator *regulator)
 	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
 
+	regulator_balance_coupled(rdev->coupling_desc);
+
 	if (ret == 0 && rdev->supply)
 		regulator_disable(rdev->supply);
 
@@ -2358,6 +2391,8 @@ int regulator_force_disable(struct regulator *regulator)
 		while (rdev->open_count--)
 			regulator_disable(rdev->supply);
 
+	regulator_balance_coupled(rdev->coupling_desc);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_force_disable);
@@ -2447,10 +2482,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
 	return rdev->desc->ops->is_enabled(rdev);
 }
 
-static int _regulator_list_voltage(struct regulator *regulator,
-				    unsigned selector, int lock)
+static int _regulator_list_voltage(struct regulator_dev *rdev,
+				   unsigned selector, int lock)
 {
-	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
 	int ret;
 
@@ -2466,7 +2500,8 @@ static int _regulator_list_voltage(struct regulator *regulator,
 		if (lock)
 			mutex_unlock(&rdev->mutex);
 	} else if (rdev->is_switch && rdev->supply) {
-		ret = _regulator_list_voltage(rdev->supply, selector, lock);
+		ret = _regulator_list_voltage(rdev->supply->rdev,
+					      selector, lock);
 	} else {
 		return -EINVAL;
 	}
@@ -2542,7 +2577,7 @@ EXPORT_SYMBOL_GPL(regulator_count_voltages);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	return _regulator_list_voltage(regulator, selector, 1);
+	return _regulator_list_voltage(regulator->rdev, selector, 1);
 }
 EXPORT_SYMBOL_GPL(regulator_list_voltage);
 
@@ -2883,8 +2918,6 @@ 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 we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
@@ -2928,6 +2961,35 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	if (ret < 0)
 		goto out2;
 
+	/*
+	 * If the regulator is not coupled just set voltage normally, else
+	 * return after changing consumer demands without changing voltage.
+	 * This will be handled outside the function
+	 * by regulator_balance_coupled()
+	 */
+	if (!rdev->coupling_desc) {
+		ret = regulator_set_voltage_rdev(regulator->rdev,
+						 min_uV, max_uV);
+		if (ret < 0)
+			goto out2;
+	}
+
+out:
+	return 0;
+out2:
+	regulator->min_uV = old_min_uV;
+	regulator->max_uV = old_max_uV;
+
+	return ret;
+}
+
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
+				      int max_uV)
+{
+	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) &&
@@ -2939,13 +3001,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(regulator, selector, 0);
+		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;
@@ -2953,7 +3015,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;
@@ -2965,13 +3027,13 @@ 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;
 		}
 	}
 
 	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
 	if (ret < 0)
-		goto out2;
+		goto out;
 
 	if (supply_change_uV < 0) {
 		ret = regulator_set_voltage_unlocked(rdev->supply,
@@ -2985,9 +3047,181 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 out:
 	return ret;
-out2:
-	regulator->min_uV = old_min_uV;
-	regulator->max_uV = old_max_uV;
+}
+
+static int regulator_coupled_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_actual_uV = INT_MAX;
+	int max_actual_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);
+	if (ret < 0)
+		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);
+		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_actual_uV)
+			min_actual_uV = tmp_act;
+
+		if (tmp_act > max_actual_uV)
+			max_actual_uV = tmp_act;
+	}
+
+	/* There aren't any other regulators enabled */
+	if (max_actual_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_actual_uV - max_spread);
+		possible_uV = min(possible_uV, min_actual_uV + max_spread);
+	}
+
+	if (possible_uV > desired_max_uV) {
+		ret = -EINVAL;
+		goto out;
+	}
+	ret = possible_uV;
+
+out:
+	return ret;
+}
+
+static int regulator_balance_coupled(struct coupling_desc *c_desc)
+{
+	struct regulator_dev **c_rdevs;
+	struct regulator_dev *best_rdev;
+	int n_coupled;
+	int i, best_delta, best_uV, ret = 1;
+
+	if (!c_desc)
+		return -EINVAL;
+
+	c_rdevs = c_desc->coupled_rdevs;
+	n_coupled = c_desc->n_coupled;
+	/*
+	 * 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;
+
+		regulator_lock_coupled(c_desc);
+
+		/*
+		 * Find highest difference between optimal voltage
+		 * and actual 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, actual_uV;
+
+			optimal_uV = regulator_coupled_get_optimal_voltage(c_rdevs[i]);
+			if (optimal_uV < 0) {
+				ret = optimal_uV;
+				goto unlock;
+			}
+
+			actual_uV = _regulator_get_voltage(c_rdevs[i]);
+			if (actual_uV < 0) {
+				ret = optimal_uV;
+				goto unlock;
+			}
+
+			if (abs(best_delta) < abs(optimal_uV - actual_uV)) {
+				best_delta = optimal_uV - actual_uV;
+				best_rdev = c_rdevs[i];
+				best_uV = optimal_uV;
+			}
+		}
+
+		/* Nothing to change, return successfully */
+		if (!best_rdev) {
+			ret = 0;
+			goto unlock;
+		}
+		/*
+		 * 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);
+		if (best_rdev->supply)
+			regulator_unlock_supply(best_rdev->supply->rdev);
+
+unlock:
+		regulator_unlock_coupled(c_desc);
+
+		if (ret < 0 || !best_rdev)
+			break;
+	}
 
 	return ret;
 }
@@ -3020,6 +3254,9 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 
 	regulator_unlock_supply(regulator->rdev);
 
+	if (regulator->rdev->coupling_desc)
+		ret = regulator_balance_coupled(regulator->rdev->coupling_desc);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Mark Brown]

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

On Thu, Dec 07, 2017 at 10:46:15AM +0100, Maciej Purski wrote:

> @@ -2447,10 +2482,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
>  	return rdev->desc->ops->is_enabled(rdev);
>  }
>  
> -static int _regulator_list_voltage(struct regulator *regulator,
> -				    unsigned selector, int lock)
> +static int _regulator_list_voltage(struct regulator_dev *rdev,
> +				   unsigned selector, int lock)
>  {

Please split this refactoring of _list_voltage() into a separate patch
for ease of review.  It can go in separately which will make this change
smaller and easier to review.

> @@ -2928,6 +2961,35 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
>  	if (ret < 0)
>  		goto out2;
>  
> +	/*
> +	 * If the regulator is not coupled just set voltage normally, else
> +	 * return after changing consumer demands without changing voltage.
> +	 * This will be handled outside the function
> +	 * by regulator_balance_coupled()
> +	 */
> +	if (!rdev->coupling_desc) {
> +		ret = regulator_set_voltage_rdev(regulator->rdev,
> +						 min_uV, max_uV);
> +		if (ret < 0)
> +			goto out2;
> +	}

As I think I said on the previous version I'm not enthusiastic about
having two separate code paths for setting the voltage, it makes it much
more likely that things will break especially given how rare coupled
regulators are.  It would be cleaner to make uncoupled regulators just
be a special case of coupled regulators, that way more of the code is
shared.  To that end I'd adjust the code so that we always have a
coupling descriptor and then handle the case where there's only one
regulator described in there.

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

Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Maciej Purski]

On 12/12/2017 12:54 PM, Mark Brown wrote:
> On Thu, Dec 07, 2017 at 10:46:15AM +0100, Maciej Purski wrote:
> 
>> @@ -2447,10 +2482,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
>>   	return rdev->desc->ops->is_enabled(rdev);
>>   }
>>   
>> -static int _regulator_list_voltage(struct regulator *regulator,
>> -				    unsigned selector, int lock)
>> +static int _regulator_list_voltage(struct regulator_dev *rdev,
>> +				   unsigned selector, int lock)
>>   {
> 
> Please split this refactoring of _list_voltage() into a separate patch
> for ease of review.  It can go in separately which will make this change
> smaller and easier to review.
> 
>> @@ -2928,6 +2961,35 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
>>   	if (ret < 0)
>>   		goto out2;
>>   
>> +	/*
>> +	 * If the regulator is not coupled just set voltage normally, else
>> +	 * return after changing consumer demands without changing voltage.
>> +	 * This will be handled outside the function
>> +	 * by regulator_balance_coupled()
>> +	 */
>> +	if (!rdev->coupling_desc) {
>> +		ret = regulator_set_voltage_rdev(regulator->rdev,
>> +						 min_uV, max_uV);
>> +		if (ret < 0)
>> +			goto out2;
>> +	}
> 
> As I think I said on the previous version I'm not enthusiastic about
> having two separate code paths for setting the voltage, it makes it much
> more likely that things will break especially given how rare coupled
> regulators are.  It would be cleaner to make uncoupled regulators just
> be a special case of coupled regulators, that way more of the code is
> shared.  To that end I'd adjust the code so that we always have a
> coupling descriptor and then handle the case where there's only one
> regulator described in there.
> 

Do you have any suggestion, how should I implement that path? The thing which
makes it more complicated is locking, because set_voltage_unlocked is done under 
one regulator's mutex and its suppliers, while balance procedure locks every 
coupled regulator without its suppliers. The suppliers for a single regulator 
are locked when setting a single regulator's voltage takes place.

Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Mark Brown]

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

On Wed, Dec 13, 2017 at 10:25:00AM +0100, Maciej Purski wrote:

> > shared.  To that end I'd adjust the code so that we always have a
> > coupling descriptor and then handle the case where there's only one
> > regulator described in there.

> Do you have any suggestion, how should I implement that path? The thing which
> makes it more complicated is locking, because set_voltage_unlocked is done
> under one regulator's mutex and its suppliers, while balance procedure locks
> every coupled regulator without its suppliers. The suppliers for a single
> regulator are locked when setting a single regulator's voltage takes place.

We only really need to lock the supplies when doing the actual mechanics
of voltage changes so I'm not sure I see a big issue here - if we always
go through balancing first then voltage setting it should be fine.  If
everything is always balancing (even uncoupled regulators) then part of
the transition should be moving some if not all of the data updates to
balancing.

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

Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Maciej Purski]

On 12/15/2017 04:19 PM, Mark Brown wrote:
> On Wed, Dec 13, 2017 at 10:25:00AM +0100, Maciej Purski wrote:
> 
>>> shared.  To that end I'd adjust the code so that we always have a
>>> coupling descriptor and then handle the case where there's only one
>>> regulator described in there.
> 
>> Do you have any suggestion, how should I implement that path? The thing which
>> makes it more complicated is locking, because set_voltage_unlocked is done
>> under one regulator's mutex and its suppliers, while balance procedure locks
>> every coupled regulator without its suppliers. The suppliers for a single
>> regulator are locked when setting a single regulator's voltage takes place.
> 
> We only really need to lock the supplies when doing the actual mechanics
> of voltage changes so I'm not sure I see a big issue here - if we always
> go through balancing first then voltage setting it should be fine.  If
> everything is always balancing (even uncoupled regulators) then part of
> the transition should be moving some if not all of the data updates to
> balancing.
> 

Now I can understand your point, but I still have doubts what is the advantage 
of that solution. For non-coupled regulators we end up with useless data 
structure - coupling_desc. That also might cause some confusion. We expect 
coupled regulators to be a very rare case, so in most of the cases we will have 
a pointless structure in reg_dev with a pointer to itself. Maybe you suggest 
that coupling_desc should contain something different?

Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Mark Brown]

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

On Thu, Dec 21, 2017 at 02:29:14PM +0100, Maciej Purski wrote:

> Now I can understand your point, but I still have doubts what is the
> advantage of that solution. For non-coupled regulators we end up with
> useless data structure - coupling_desc. That also might cause some
> confusion. We expect coupled regulators to be a very rare case, so in most
> of the cases we will have a pointless structure in reg_dev with a pointer to
> itself. Maybe you suggest that coupling_desc should contain something
> different?

It's precisely because it's such an unusual case that I'm looking to see
it as part of the common path - if it's not then it seems very liklely
that it'll get broken down the line and nobody will notice because the
code is never run.  Yes, it's unusual but having the unusual thing be a
bit more visible than it is used seems better than having support there
that gets broken.

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