All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] regulator: core: remove dead code in _regulator_get()
@ 2017-02-03 21:56 Dmitry Torokhov
  2017-02-03 21:56 ` [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Dmitry Torokhov
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-03 21:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

There is no point in assigning value to 'ret' before calling
regulator_dev_lookup() as it will clobber 'ret' anyway.

Also, let's explicitly return -PROBE_DEFER when try_module_get() fails,
instead of relying that earlier initialization of "regulator" carries
correct value.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9a165b..b0ee068310c5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1584,7 +1584,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 					bool exclusive, bool allow_dummy)
 {
 	struct regulator_dev *rdev;
-	struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
+	struct regulator *regulator;
 	const char *devname = NULL;
 	int ret;
 
@@ -1596,11 +1596,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	if (dev)
 		devname = dev_name(dev);
 
-	if (have_full_constraints())
-		ret = -ENODEV;
-	else
-		ret = -EPROBE_DEFER;
-
 	rdev = regulator_dev_lookup(dev, id, &ret);
 	if (rdev)
 		goto found;
@@ -1656,6 +1651,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	}
 
 	if (!try_module_get(rdev->owner)) {
+		regulator = ERR_PTR(-EPROBE_DEFER);
 		put_device(&rdev->dev);
 		return regulator;
 	}
-- 
2.11.0.483.g087da7b7c-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors
  2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
@ 2017-02-03 21:56 ` Dmitry Torokhov
  2017-02-04 10:11   ` Mark Brown
  2017-02-03 21:56 ` [PATCH 3/5] regulator: core: have _regulator_get() accept get_type argument Dmitry Torokhov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-03 21:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

Instead of returning both regulator_dev structure as return value and
auxiliary error code in 'ret' argument, let's switch to using ERR_PTR
encoded values. This makes it more obvious what is going on at call sites.

Also, let's not unlock the mutex in the middle of a loop, but rather break
out and have single unlock path.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b0ee068310c5..25aca2096ac9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1455,12 +1455,14 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name)
  * lookup could succeed in the future.
  *
  * If successful, returns a struct regulator_dev that corresponds to the name
- * @supply and with the embedded struct device refcount incremented by one,
- * or NULL on failure. The refcount must be dropped by calling put_device().
+ * @supply and with the embedded struct device refcount incremented by one.
+ * The refcount must be dropped by calling put_device().
+ * On failure one of the following ERR-PTR-encoded values is returned:
+ * -ENODEV if lookup fails permanently, -EPROBE_DEFER if lookup could succeed
+ * in the future.
  */
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
-						  const char *supply,
-						  int *ret)
+						  const char *supply)
 {
 	struct regulator_dev *r;
 	struct device_node *node;
@@ -1476,16 +1478,12 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 			r = of_find_regulator_by_node(node);
 			if (r)
 				return r;
-			*ret = -EPROBE_DEFER;
-			return NULL;
-		} else {
+
 			/*
-			 * If we couldn't even get the node then it's
-			 * not just that the device didn't register
-			 * yet, there's no node and we'll never
-			 * succeed.
+			 * We have a node, but there is no device.
+			 * assume it has not registered yet.
 			 */
-			*ret = -ENODEV;
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 	}
 
@@ -1506,13 +1504,13 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 		if (strcmp(map->supply, supply) == 0 &&
 		    get_device(&map->regulator->dev)) {
-			mutex_unlock(&regulator_list_mutex);
-			return map->regulator;
+			r = map->regulator;
+			break;
 		}
 	}
 	mutex_unlock(&regulator_list_mutex);
 
-	return NULL;
+	return r ? r : ERR_PTR(-ENODEV);
 }
 
 static int regulator_resolve_supply(struct regulator_dev *rdev)
@@ -1529,8 +1527,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (rdev->supply)
 		return 0;
 
-	r = regulator_dev_lookup(dev, rdev->supply_name, &ret);
-	if (!r) {
+	r = regulator_dev_lookup(dev, rdev->supply_name);
+	if (IS_ERR(r)) {
+		ret = PTR_ERR(r);
+
 		if (ret == -ENODEV) {
 			/*
 			 * No supply was specified for this regulator and
@@ -1596,10 +1596,11 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	if (dev)
 		devname = dev_name(dev);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
-	if (rdev)
+	rdev = regulator_dev_lookup(dev, id);
+	if (!IS_ERR(rdev))
 		goto found;
 
+	ret = PTR_ERR(rdev);
 	regulator = ERR_PTR(ret);
 
 	/*
-- 
2.11.0.483.g087da7b7c-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/5] regulator: core: have _regulator_get() accept get_type argument
  2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
  2017-02-03 21:56 ` [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Dmitry Torokhov
@ 2017-02-03 21:56 ` Dmitry Torokhov
  2017-02-04 10:48   ` Applied "regulator: core: have _regulator_get() accept get_type argument" to the regulator tree Mark Brown
  2017-02-03 21:56 ` [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-03 21:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

Instead of separate "exclusive" and "allow_dummy" arguments, that formed 3
valid combinations (normal, exclusive and optional) and an invalid one,
let's accept explicit "get_type", like we did in devm-managed code.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c     | 23 ++++++++++++++---------
 drivers/regulator/devres.c   | 21 +--------------------
 drivers/regulator/internal.h | 10 ++++++++++
 3 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 25aca2096ac9..e495767fee85 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1580,14 +1580,19 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 }
 
 /* Internal regulator request function */
-static struct regulator *_regulator_get(struct device *dev, const char *id,
-					bool exclusive, bool allow_dummy)
+struct regulator *_regulator_get(struct device *dev, const char *id,
+				 enum regulator_get_type get_type)
 {
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
 	const char *devname = NULL;
 	int ret;
 
+	if (get_type >= MAX_GET_TYPE) {
+		dev_err(dev, "invalid type %d in %s\n", get_type, __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (id == NULL) {
 		pr_err("get() with no identifier\n");
 		return ERR_PTR(-EINVAL);
@@ -1617,7 +1622,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	 * Assume that a regulator is physically present and enabled
 	 * even if it isn't hooked up and just provide a dummy.
 	 */
-	if (have_full_constraints() && allow_dummy) {
+	if (have_full_constraints() && get_type == NORMAL_GET) {
 		pr_warn("%s supply %s not found, using dummy regulator\n",
 			devname, id);
 
@@ -1625,7 +1630,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 		get_device(&rdev->dev);
 		goto found;
 	/* Don't log an error when called from regulator_get_optional() */
-	} else if (!have_full_constraints() || exclusive) {
+	} else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) {
 		dev_warn(dev, "dummy supplies not allowed\n");
 	}
 
@@ -1638,7 +1643,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	if (exclusive && rdev->open_count) {
+	if (get_type == EXCLUSIVE_GET && rdev->open_count) {
 		regulator = ERR_PTR(-EBUSY);
 		put_device(&rdev->dev);
 		return regulator;
@@ -1666,7 +1671,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	}
 
 	rdev->open_count++;
-	if (exclusive) {
+	if (get_type == EXCLUSIVE_GET) {
 		rdev->exclusive = 1;
 
 		ret = _regulator_is_enabled(rdev);
@@ -1694,7 +1699,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
  */
 struct regulator *regulator_get(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, true);
+	return _regulator_get(dev, id, NORMAL_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get);
 
@@ -1721,7 +1726,7 @@ EXPORT_SYMBOL_GPL(regulator_get);
  */
 struct regulator *regulator_get_exclusive(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, true, false);
+	return _regulator_get(dev, id, EXCLUSIVE_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get_exclusive);
 
@@ -1747,7 +1752,7 @@ EXPORT_SYMBOL_GPL(regulator_get_exclusive);
  */
 struct regulator *regulator_get_optional(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, false);
+	return _regulator_get(dev, id, OPTIONAL_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get_optional);
 
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 6ec1d400adae..965d1d31ec8c 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -19,12 +19,6 @@
 
 #include "internal.h"
 
-enum {
-	NORMAL_GET,
-	EXCLUSIVE_GET,
-	OPTIONAL_GET,
-};
-
 static void devm_regulator_release(struct device *dev, void *res)
 {
 	regulator_put(*(struct regulator **)res);
@@ -39,20 +33,7 @@ static struct regulator *_devm_regulator_get(struct device *dev, const char *id,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	switch (get_type) {
-	case NORMAL_GET:
-		regulator = regulator_get(dev, id);
-		break;
-	case EXCLUSIVE_GET:
-		regulator = regulator_get_exclusive(dev, id);
-		break;
-	case OPTIONAL_GET:
-		regulator = regulator_get_optional(dev, id);
-		break;
-	default:
-		regulator = ERR_PTR(-EINVAL);
-	}
-
+	regulator = _regulator_get(dev, id, get_type);
 	if (!IS_ERR(regulator)) {
 		*ptr = regulator;
 		devres_add(dev, ptr);
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index c74ac8734023..1dd575b28564 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -51,4 +51,14 @@ regulator_of_get_init_data(struct device *dev,
 }
 #endif
 
+enum regulator_get_type {
+	NORMAL_GET,
+	EXCLUSIVE_GET,
+	OPTIONAL_GET,
+	MAX_GET_TYPE
+};
+
+struct regulator *_regulator_get(struct device *dev, const char *id,
+				 enum regulator_get_type get_type);
+
 #endif
-- 
2.11.0.483.g087da7b7c-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/5] regulator: core: simplify _regulator_get()
  2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
  2017-02-03 21:56 ` [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Dmitry Torokhov
  2017-02-03 21:56 ` [PATCH 3/5] regulator: core: have _regulator_get() accept get_type argument Dmitry Torokhov
@ 2017-02-03 21:56 ` Dmitry Torokhov
  2017-02-04 10:34   ` Mark Brown
  2017-02-08 18:34   ` Applied "regulator: core: simplify _regulator_get()" to the regulator tree Mark Brown
  2017-02-03 21:56 ` [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies Dmitry Torokhov
  2017-02-04 10:48 ` Applied "regulator: core: remove dead code in _regulator_get()" to the regulator tree Mark Brown
  4 siblings, 2 replies; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-03 21:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

The code in _regulator_get() got a bit confusing over time, with control
flow jumping to a label from couple of places. Let's untangle it a bit.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c | 66 +++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e495767fee85..e39bb2d41038 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1585,7 +1585,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 {
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
-	const char *devname = NULL;
+	const char *devname = dev ? dev_name(dev) : "deviceless";
 	int ret;
 
 	if (get_type >= MAX_GET_TYPE) {
@@ -1598,45 +1598,47 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (dev)
-		devname = dev_name(dev);
-
 	rdev = regulator_dev_lookup(dev, id);
-	if (!IS_ERR(rdev))
-		goto found;
+	if (IS_ERR(rdev)) {
+		ret = PTR_ERR(rdev);
 
-	ret = PTR_ERR(rdev);
-	regulator = ERR_PTR(ret);
+		/*
+		 * If regulator_dev_lookup() fails with error other
+		 * than -ENODEV our job here is done, we simply return it.
+		 */
+		if (ret != -ENODEV)
+			return ERR_PTR(ret);
 
-	/*
-	 * If we have return value from dev_lookup fail, we do not expect to
-	 * succeed, so, quit with appropriate error value
-	 */
-	if (ret && ret != -ENODEV)
-		return regulator;
+		if (!have_full_constraints()) {
+			dev_warn(dev,
+				 "incomplete constraints, dummy supplies not allowed\n");
+			return ERR_PTR(-ENODEV);
+		}
 
-	if (!devname)
-		devname = "deviceless";
+		switch (get_type) {
+		case NORMAL_GET:
+			/*
+			 * Assume that a regulator is physically present and
+			 * enabled, even if it isn't hooked up, and just
+			 * provide a dummy.
+			 */
+			dev_warn(dev,
+				 "%s supply %s not found, using dummy regulator\n",
+				 devname, id);
+			rdev = dummy_regulator_rdev;
+			get_device(&rdev->dev);
+			break;
 
-	/*
-	 * Assume that a regulator is physically present and enabled
-	 * even if it isn't hooked up and just provide a dummy.
-	 */
-	if (have_full_constraints() && get_type == NORMAL_GET) {
-		pr_warn("%s supply %s not found, using dummy regulator\n",
-			devname, id);
+		case EXCLUSIVE_GET:
+			dev_warn(dev,
+				 "dummy supplies not allowed for exclusive requests\n");
+			/* fall through */
 
-		rdev = dummy_regulator_rdev;
-		get_device(&rdev->dev);
-		goto found;
-	/* Don't log an error when called from regulator_get_optional() */
-	} else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) {
-		dev_warn(dev, "dummy supplies not allowed\n");
+		default:
+			return ERR_PTR(-ENODEV);
+		}
 	}
 
-	return regulator;
-
-found:
 	if (rdev->exclusive) {
 		regulator = ERR_PTR(-EPERM);
 		put_device(&rdev->dev);
-- 
2.11.0.483.g087da7b7c-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
                   ` (2 preceding siblings ...)
  2017-02-03 21:56 ` [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov
@ 2017-02-03 21:56 ` Dmitry Torokhov
  2017-02-04 11:48   ` Mark Brown
  2017-02-04 10:48 ` Applied "regulator: core: remove dead code in _regulator_get()" to the regulator tree Mark Brown
  4 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-03 21:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

Use of dummy supplies is normal and common occurrence in the kernel, let's
lower severity from warning to info.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e39bb2d41038..1d3aff7e2bb1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1622,7 +1622,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 			 * enabled, even if it isn't hooked up, and just
 			 * provide a dummy.
 			 */
-			dev_warn(dev,
+			dev_info(dev,
 				 "%s supply %s not found, using dummy regulator\n",
 				 devname, id);
 			rdev = dummy_regulator_rdev;
-- 
2.11.0.483.g087da7b7c-goog

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors
  2017-02-03 21:56 ` [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Dmitry Torokhov
@ 2017-02-04 10:11   ` Mark Brown
  2017-02-04 18:19     ` [PATCH v2 " Dmitry Torokhov
  0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2017-02-04 10:11 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Fri, Feb 03, 2017 at 01:56:01PM -0800, Dmitry Torokhov wrote:

> -	return NULL;
> +	return r ? r : ERR_PTR(-ENODEV);
>  }

Please write normal conditional statements to keep the code legible.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/5] regulator: core: simplify _regulator_get()
  2017-02-03 21:56 ` [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov
@ 2017-02-04 10:34   ` Mark Brown
  2017-02-04 18:27     ` Dmitry Torokhov
  2017-02-08 18:34   ` Applied "regulator: core: simplify _regulator_get()" to the regulator tree Mark Brown
  1 sibling, 1 reply; 21+ messages in thread
From: Mark Brown @ 2017-02-04 10:34 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Fri, Feb 03, 2017 at 01:56:03PM -0800, Dmitry Torokhov wrote:
> The code in _regulator_get() got a bit confusing over time, with control
> flow jumping to a label from couple of places. Let's untangle it a bit.

This is quite hard to review without a concrete description of what the
changes actually are...

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/regulator/core.c | 66 +++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 32 deletions(-)

...it's a fairly large change in core code which is as you say a little
complicated.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Applied "regulator: core: have _regulator_get() accept get_type argument" to the regulator tree
  2017-02-03 21:56 ` [PATCH 3/5] regulator: core: have _regulator_get() accept get_type argument Dmitry Torokhov
@ 2017-02-04 10:48   ` Mark Brown
  0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-04 10:48 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Brown, Mark Brown, Liam Girdwood, linux-kernel, linux-kernel

The patch

   regulator: core: have _regulator_get() accept get_type argument

has been applied to the regulator tree at

   git://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 a8bd42a97741aefa5942605fa87418fc8a6c4169 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Fri, 3 Feb 2017 13:56:02 -0800
Subject: [PATCH] regulator: core: have _regulator_get() accept get_type
 argument

Instead of separate "exclusive" and "allow_dummy" arguments, that formed 3
valid combinations (normal, exclusive and optional) and an invalid one,
let's accept explicit "get_type", like we did in devm-managed code.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c     | 23 ++++++++++++++---------
 drivers/regulator/devres.c   | 21 +--------------------
 drivers/regulator/internal.h | 10 ++++++++++
 3 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b0ee068310c5..206c274c0003 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1580,14 +1580,19 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 }
 
 /* Internal regulator request function */
-static struct regulator *_regulator_get(struct device *dev, const char *id,
-					bool exclusive, bool allow_dummy)
+struct regulator *_regulator_get(struct device *dev, const char *id,
+				 enum regulator_get_type get_type)
 {
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
 	const char *devname = NULL;
 	int ret;
 
+	if (get_type >= MAX_GET_TYPE) {
+		dev_err(dev, "invalid type %d in %s\n", get_type, __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (id == NULL) {
 		pr_err("get() with no identifier\n");
 		return ERR_PTR(-EINVAL);
@@ -1616,7 +1621,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	 * Assume that a regulator is physically present and enabled
 	 * even if it isn't hooked up and just provide a dummy.
 	 */
-	if (have_full_constraints() && allow_dummy) {
+	if (have_full_constraints() && get_type == NORMAL_GET) {
 		pr_warn("%s supply %s not found, using dummy regulator\n",
 			devname, id);
 
@@ -1624,7 +1629,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 		get_device(&rdev->dev);
 		goto found;
 	/* Don't log an error when called from regulator_get_optional() */
-	} else if (!have_full_constraints() || exclusive) {
+	} else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) {
 		dev_warn(dev, "dummy supplies not allowed\n");
 	}
 
@@ -1637,7 +1642,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	if (exclusive && rdev->open_count) {
+	if (get_type == EXCLUSIVE_GET && rdev->open_count) {
 		regulator = ERR_PTR(-EBUSY);
 		put_device(&rdev->dev);
 		return regulator;
@@ -1665,7 +1670,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	}
 
 	rdev->open_count++;
-	if (exclusive) {
+	if (get_type == EXCLUSIVE_GET) {
 		rdev->exclusive = 1;
 
 		ret = _regulator_is_enabled(rdev);
@@ -1693,7 +1698,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
  */
 struct regulator *regulator_get(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, true);
+	return _regulator_get(dev, id, NORMAL_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get);
 
@@ -1720,7 +1725,7 @@ EXPORT_SYMBOL_GPL(regulator_get);
  */
 struct regulator *regulator_get_exclusive(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, true, false);
+	return _regulator_get(dev, id, EXCLUSIVE_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get_exclusive);
 
@@ -1746,7 +1751,7 @@ EXPORT_SYMBOL_GPL(regulator_get_exclusive);
  */
 struct regulator *regulator_get_optional(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, false);
+	return _regulator_get(dev, id, OPTIONAL_GET);
 }
 EXPORT_SYMBOL_GPL(regulator_get_optional);
 
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 6ec1d400adae..965d1d31ec8c 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -19,12 +19,6 @@
 
 #include "internal.h"
 
-enum {
-	NORMAL_GET,
-	EXCLUSIVE_GET,
-	OPTIONAL_GET,
-};
-
 static void devm_regulator_release(struct device *dev, void *res)
 {
 	regulator_put(*(struct regulator **)res);
@@ -39,20 +33,7 @@ static struct regulator *_devm_regulator_get(struct device *dev, const char *id,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	switch (get_type) {
-	case NORMAL_GET:
-		regulator = regulator_get(dev, id);
-		break;
-	case EXCLUSIVE_GET:
-		regulator = regulator_get_exclusive(dev, id);
-		break;
-	case OPTIONAL_GET:
-		regulator = regulator_get_optional(dev, id);
-		break;
-	default:
-		regulator = ERR_PTR(-EINVAL);
-	}
-
+	regulator = _regulator_get(dev, id, get_type);
 	if (!IS_ERR(regulator)) {
 		*ptr = regulator;
 		devres_add(dev, ptr);
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index c74ac8734023..1dd575b28564 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -51,4 +51,14 @@ regulator_of_get_init_data(struct device *dev,
 }
 #endif
 
+enum regulator_get_type {
+	NORMAL_GET,
+	EXCLUSIVE_GET,
+	OPTIONAL_GET,
+	MAX_GET_TYPE
+};
+
+struct regulator *_regulator_get(struct device *dev, const char *id,
+				 enum regulator_get_type get_type);
+
 #endif
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Applied "regulator: core: remove dead code in _regulator_get()" to the regulator tree
  2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
                   ` (3 preceding siblings ...)
  2017-02-03 21:56 ` [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies Dmitry Torokhov
@ 2017-02-04 10:48 ` Mark Brown
  4 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-04 10:48 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Brown, Mark Brown, Liam Girdwood, linux-kernel, linux-kernel

The patch

   regulator: core: remove dead code in _regulator_get()

has been applied to the regulator tree at

   git://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 7d245afa24b3ed911f6fd90079d70932ac5e5923 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Fri, 3 Feb 2017 13:56:00 -0800
Subject: [PATCH] regulator: core: remove dead code in _regulator_get()

There is no point in assigning value to 'ret' before calling
regulator_dev_lookup() as it will clobber 'ret' anyway.

Also, let's explicitly return -PROBE_DEFER when try_module_get() fails,
instead of relying that earlier initialization of "regulator" carries
correct value.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9a165b..b0ee068310c5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1584,7 +1584,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 					bool exclusive, bool allow_dummy)
 {
 	struct regulator_dev *rdev;
-	struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
+	struct regulator *regulator;
 	const char *devname = NULL;
 	int ret;
 
@@ -1596,11 +1596,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	if (dev)
 		devname = dev_name(dev);
 
-	if (have_full_constraints())
-		ret = -ENODEV;
-	else
-		ret = -EPROBE_DEFER;
-
 	rdev = regulator_dev_lookup(dev, id, &ret);
 	if (rdev)
 		goto found;
@@ -1656,6 +1651,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	}
 
 	if (!try_module_get(rdev->owner)) {
+		regulator = ERR_PTR(-EPROBE_DEFER);
 		put_device(&rdev->dev);
 		return regulator;
 	}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-03 21:56 ` [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies Dmitry Torokhov
@ 2017-02-04 11:48   ` Mark Brown
  2017-02-04 17:45     ` Dmitry Torokhov
  0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2017-02-04 11:48 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Fri, Feb 03, 2017 at 01:56:04PM -0800, Dmitry Torokhov wrote:
> Use of dummy supplies is normal and common occurrence in the kernel, let's
> lower severity from warning to info.

It really shouldn't be either of these things, at least on DT systems.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-04 11:48   ` Mark Brown
@ 2017-02-04 17:45     ` Dmitry Torokhov
  2017-02-05 16:12       ` Mark Brown
  0 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-04 17:45 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

On Sat, Feb 04, 2017 at 12:48:19PM +0100, Mark Brown wrote:
> On Fri, Feb 03, 2017 at 01:56:04PM -0800, Dmitry Torokhov wrote:
> > Use of dummy supplies is normal and common occurrence in the kernel, let's
> > lower severity from warning to info.
> 
> It really shouldn't be either of these things, at least on DT systems.

But it definitely happens on ACPI systems for drivers for peripherals
that we share between both x86 and ARM.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors
  2017-02-04 10:11   ` Mark Brown
@ 2017-02-04 18:19     ` Dmitry Torokhov
  2017-02-05 14:49       ` Mark Brown
                         ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-04 18:19 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

Instead of returning both regulator_dev structure as return value and
auxiliary error code in 'ret' argument, let's switch to using ERR_PTR
encoded values. This makes it more obvious what is going on at call sites.

Also, let's not unlock the mutex in the middle of a loop, but rather break
out and have single unlock path.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

V2: replaced "return r ? r : ERR_PTR(-ENODEV);" with expanded "if"
statement.

 drivers/regulator/core.c |   42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b0ee068310c5..a39268bcab56 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1455,12 +1455,14 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name)
  * lookup could succeed in the future.
  *
  * If successful, returns a struct regulator_dev that corresponds to the name
- * @supply and with the embedded struct device refcount incremented by one,
- * or NULL on failure. The refcount must be dropped by calling put_device().
+ * @supply and with the embedded struct device refcount incremented by one.
+ * The refcount must be dropped by calling put_device().
+ * On failure one of the following ERR-PTR-encoded values is returned:
+ * -ENODEV if lookup fails permanently, -EPROBE_DEFER if lookup could succeed
+ * in the future.
  */
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
-						  const char *supply,
-						  int *ret)
+						  const char *supply)
 {
 	struct regulator_dev *r;
 	struct device_node *node;
@@ -1476,16 +1478,12 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 			r = of_find_regulator_by_node(node);
 			if (r)
 				return r;
-			*ret = -EPROBE_DEFER;
-			return NULL;
-		} else {
+
 			/*
-			 * If we couldn't even get the node then it's
-			 * not just that the device didn't register
-			 * yet, there's no node and we'll never
-			 * succeed.
+			 * We have a node, but there is no device.
+			 * assume it has not registered yet.
 			 */
-			*ret = -ENODEV;
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 	}
 
@@ -1506,13 +1504,16 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 		if (strcmp(map->supply, supply) == 0 &&
 		    get_device(&map->regulator->dev)) {
-			mutex_unlock(&regulator_list_mutex);
-			return map->regulator;
+			r = map->regulator;
+			break;
 		}
 	}
 	mutex_unlock(&regulator_list_mutex);
 
-	return NULL;
+	if (r)
+		return r;
+
+	return ERR_PTR(-ENODEV);
 }
 
 static int regulator_resolve_supply(struct regulator_dev *rdev)
@@ -1529,8 +1530,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (rdev->supply)
 		return 0;
 
-	r = regulator_dev_lookup(dev, rdev->supply_name, &ret);
-	if (!r) {
+	r = regulator_dev_lookup(dev, rdev->supply_name);
+	if (IS_ERR(r)) {
+		ret = PTR_ERR(r);
+
 		if (ret == -ENODEV) {
 			/*
 			 * No supply was specified for this regulator and
@@ -1596,10 +1599,11 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	if (dev)
 		devname = dev_name(dev);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
-	if (rdev)
+	rdev = regulator_dev_lookup(dev, id);
+	if (!IS_ERR(rdev))
 		goto found;
 
+	ret = PTR_ERR(rdev);
 	regulator = ERR_PTR(ret);
 
 	/*

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/5] regulator: core: simplify _regulator_get()
  2017-02-04 10:34   ` Mark Brown
@ 2017-02-04 18:27     ` Dmitry Torokhov
  2017-02-05 16:08       ` Mark Brown
  0 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-04 18:27 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

On Sat, Feb 04, 2017 at 11:34:58AM +0100, Mark Brown wrote:
> On Fri, Feb 03, 2017 at 01:56:03PM -0800, Dmitry Torokhov wrote:
> > The code in _regulator_get() got a bit confusing over time, with control
> > flow jumping to a label from couple of places. Let's untangle it a bit.
> 
> This is quite hard to review without a concrete description of what the
> changes actually are...
> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/regulator/core.c | 66 +++++++++++++++++++++++++-----------------------
> >  1 file changed, 34 insertions(+), 32 deletions(-)
> 
> ...it's a fairly large change in core code which is as you say a little
> complicated.

OK, so the change is twofold:

1. We make handling of missing supplies and substituting them with dummy
regulators more explicit:

- check if we not have full constraints and refuse considering dummy
  regulators with appropriate message

- use "switch (get_type)" to handle different types of request explicit
  as well. "Normal" requests will get dummies, exclusive will not and
  will notify user about that; optional will fail silently.

2. We will not be jumping to a label normal in the middle of the
function but instead have proper conditional flow. I believe jumps
should be reserved for error handling, breaking from inner loop, or
restarting a loop, but not for implementing normal conditional flow.

Do you need me to put this into commit description or this is
sufficient?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors
  2017-02-04 18:19     ` [PATCH v2 " Dmitry Torokhov
@ 2017-02-05 14:49       ` Mark Brown
  2017-02-05 23:49       ` Mark Brown
  2017-02-06 13:37       ` Applied "regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors" to the regulator tree Mark Brown
  2 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-05 14:49 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Sat, Feb 04, 2017 at 10:19:21AM -0800, Dmitry Torokhov wrote:

> V2: replaced "return r ? r : ERR_PTR(-ENODEV);" with expanded "if"
> statement.

Please don't send new versions of patches as replies into the middle of
existing patch serieses, especially not by replying to individual
patches and even especially when bits of the earlier series have been
applied.  This just makes everything much more difficult to follow -
looking at an archive of the list it's hard to tell what the current
version of the series is and if old bits of the series aren't there the
numbering within the series you posted gets broken as there are missing
patches.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/5] regulator: core: simplify _regulator_get()
  2017-02-04 18:27     ` Dmitry Torokhov
@ 2017-02-05 16:08       ` Mark Brown
  0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-05 16:08 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Sat, Feb 04, 2017 at 10:27:33AM -0800, Dmitry Torokhov wrote:

> Do you need me to put this into commit description or this is
> sufficient?

Yes, please make a commit log version - I suspect someone is going to
need it in future when trying to follow the code.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-04 17:45     ` Dmitry Torokhov
@ 2017-02-05 16:12       ` Mark Brown
  2017-02-07  0:56         ` Dmitry Torokhov
  0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2017-02-05 16:12 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Sat, Feb 04, 2017 at 09:45:02AM -0800, Dmitry Torokhov wrote:
> On Sat, Feb 04, 2017 at 12:48:19PM +0100, Mark Brown wrote:

> > It really shouldn't be either of these things, at least on DT systems.

> But it definitely happens on ACPI systems for drivers for peripherals
> that we share between both x86 and ARM.

Something that makes the warning informational only on ACPI systems
would be fine.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors
  2017-02-04 18:19     ` [PATCH v2 " Dmitry Torokhov
  2017-02-05 14:49       ` Mark Brown
@ 2017-02-05 23:49       ` Mark Brown
  2017-02-06 13:37       ` Applied "regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors" to the regulator tree Mark Brown
  2 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-05 23:49 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Sat, Feb 04, 2017 at 10:19:21AM -0800, Dmitry Torokhov wrote:
> Instead of returning both regulator_dev structure as return value and
> auxiliary error code in 'ret' argument, let's switch to using ERR_PTR
> encoded values. This makes it more obvious what is going on at call sites.

I did apply this but GNOME has decided to try to force gnome-keyring's
SSH agent on us with renewed force so until I fix that I can't use the
key on my smartcard to 2FA or push to kernel.org :/  It'll appear some
time tomorrow hopefully.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Applied "regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors" to the regulator tree
  2017-02-04 18:19     ` [PATCH v2 " Dmitry Torokhov
  2017-02-05 14:49       ` Mark Brown
  2017-02-05 23:49       ` Mark Brown
@ 2017-02-06 13:37       ` Mark Brown
  2 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-06 13:37 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Brown, Mark Brown, Liam Girdwood, linux-kernel, linux-kernel

The patch

   regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors

has been applied to the regulator tree at

   git://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 163478dae0b6ce2437488e54012705b53ef43f3d Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Sat, 4 Feb 2017 10:19:21 -0800
Subject: [PATCH] regulator: core: have regulator_dev_lookup() return
 ERR_PTR-encoded errors

Instead of returning both regulator_dev structure as return value and
auxiliary error code in 'ret' argument, let's switch to using ERR_PTR
encoded values. This makes it more obvious what is going on at call sites.

Also, let's not unlock the mutex in the middle of a loop, but rather break
out and have single unlock path.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 867756651544..3e246c82939d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1455,12 +1455,14 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name)
  * lookup could succeed in the future.
  *
  * If successful, returns a struct regulator_dev that corresponds to the name
- * @supply and with the embedded struct device refcount incremented by one,
- * or NULL on failure. The refcount must be dropped by calling put_device().
+ * @supply and with the embedded struct device refcount incremented by one.
+ * The refcount must be dropped by calling put_device().
+ * On failure one of the following ERR-PTR-encoded values is returned:
+ * -ENODEV if lookup fails permanently, -EPROBE_DEFER if lookup could succeed
+ * in the future.
  */
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
-						  const char *supply,
-						  int *ret)
+						  const char *supply)
 {
 	struct regulator_dev *r;
 	struct device_node *node;
@@ -1476,16 +1478,12 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 			r = of_find_regulator_by_node(node);
 			if (r)
 				return r;
-			*ret = -EPROBE_DEFER;
-			return NULL;
-		} else {
+
 			/*
-			 * If we couldn't even get the node then it's
-			 * not just that the device didn't register
-			 * yet, there's no node and we'll never
-			 * succeed.
+			 * We have a node, but there is no device.
+			 * assume it has not registered yet.
 			 */
-			*ret = -ENODEV;
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 	}
 
@@ -1506,13 +1504,16 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 		if (strcmp(map->supply, supply) == 0 &&
 		    get_device(&map->regulator->dev)) {
-			mutex_unlock(&regulator_list_mutex);
-			return map->regulator;
+			r = map->regulator;
+			break;
 		}
 	}
 	mutex_unlock(&regulator_list_mutex);
 
-	return NULL;
+	if (r)
+		return r;
+
+	return ERR_PTR(-ENODEV);
 }
 
 static int regulator_resolve_supply(struct regulator_dev *rdev)
@@ -1529,8 +1530,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (rdev->supply)
 		return 0;
 
-	r = regulator_dev_lookup(dev, rdev->supply_name, &ret);
-	if (!r) {
+	r = regulator_dev_lookup(dev, rdev->supply_name);
+	if (IS_ERR(r)) {
+		ret = PTR_ERR(r);
+
 		if (ret == -ENODEV) {
 			/*
 			 * No supply was specified for this regulator and
@@ -1601,10 +1604,11 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	if (dev)
 		devname = dev_name(dev);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
-	if (rdev)
+	rdev = regulator_dev_lookup(dev, id);
+	if (!IS_ERR(rdev))
 		goto found;
 
+	ret = PTR_ERR(rdev);
 	regulator = ERR_PTR(ret);
 
 	/*
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-05 16:12       ` Mark Brown
@ 2017-02-07  0:56         ` Dmitry Torokhov
  2017-02-08 18:19           ` Mark Brown
  0 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2017-02-07  0:56 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel

On Sun, Feb 05, 2017 at 05:12:46PM +0100, Mark Brown wrote:
> On Sat, Feb 04, 2017 at 09:45:02AM -0800, Dmitry Torokhov wrote:
> > On Sat, Feb 04, 2017 at 12:48:19PM +0100, Mark Brown wrote:
> 
> > > It really shouldn't be either of these things, at least on DT systems.
> 
> > But it definitely happens on ACPI systems for drivers for peripherals
> > that we share between both x86 and ARM.
> 
> Something that makes the warning informational only on ACPI systems
> would be fine.

I can definitely select the severity based on acpi_disabled flag, but I
think you should reconsider: from what I have seen in the kernel quite a
few driversi, even OF-specific ones, select regulator_get_optional() for
no good reason other than to avoid this "scary" warning. If its severity
were reduced quite a few drivers could be switched to normal
regulator_get().

Candidates for conversion (not complete, just examples):

drivers/input/touchscreen/zforce_ts.c
drivers/input/touchscreen/tsc200x-core.c
drivers/power/avs/rockchip-io-domain.c
drivers/mfd/stmpe.c

... and more...

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies
  2017-02-07  0:56         ` Dmitry Torokhov
@ 2017-02-08 18:19           ` Mark Brown
  0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-08 18:19 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Liam Girdwood, linux-kernel

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

On Mon, Feb 06, 2017 at 04:56:55PM -0800, Dmitry Torokhov wrote:

> I can definitely select the severity based on acpi_disabled flag, but I
> think you should reconsider: from what I have seen in the kernel quite a
> few driversi, even OF-specific ones, select regulator_get_optional() for
> no good reason other than to avoid this "scary" warning. If its severity
> were reduced quite a few drivers could be switched to normal
> regulator_get().

We had problems with people writing obviously problematic code long
before we had the warning (or optional regulators), I'm really
unconvinced that the warning has anything much to do with it.  There
will also be some code that predates dummy regulators, or where that
support was new enough that people weren't aware of it.  We get exactly
the same sort of problem with people writing constraints that are clear
nonsense (things like a supply named with a specific voltage but a large
voltage range).

> Candidates for conversion (not complete, just examples):

> drivers/input/touchscreen/zforce_ts.c
> drivers/input/touchscreen/tsc200x-core.c

These are broken, yes.  The zforce code at least is old enough that it
might predate dummy supplies.

> drivers/power/avs/rockchip-io-domain.c

This clearly cares about the voltages and could never work with a dummy
regulator, it is potentially a good use of optional supplies as far as I
can see (though I'm unsure about what an I/O domain without a reference
supply would mean).

> drivers/mfd/stmpe.c

This is again just obviously not good, it just logs but doesn't act on a
failure to enable the power supplies.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Applied "regulator: core: simplify _regulator_get()" to the regulator tree
  2017-02-03 21:56 ` [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov
  2017-02-04 10:34   ` Mark Brown
@ 2017-02-08 18:34   ` Mark Brown
  1 sibling, 0 replies; 21+ messages in thread
From: Mark Brown @ 2017-02-08 18:34 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Brown, Mark Brown, Liam Girdwood, linux-kernel, linux-kernel

The patch

   regulator: core: simplify _regulator_get()

has been applied to the regulator tree at

   git://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 a4d7641fa797b523c0789d2fa55b0a3d53abc2fb Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Mon, 6 Feb 2017 19:56:14 -0800
Subject: [PATCH] regulator: core: simplify _regulator_get()

The code in _regulator_get() got a bit confusing over time, with control
flow jumping to a label from couple of places. Let's untangle it a bit by
doing the following:

1. Make handling of missing supplies and substituting them with dummy
regulators more explicit:

- check if we not have full constraints and refuse considering dummy
  regulators with appropriate message;

- use "switch (get_type)" to handle different types of request explicitly
  as well. "Normal" requests will get dummies, exclusive will not and
  will notify user about that; optional will fail silently.

2. Stop jumping to a label in the middle of the function but instead have
proper conditional flow. I believe jumps should be reserved for error
handling, breaking from inner loop, or restarting a loop, but not for
implementing normal conditional flow.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 66 +++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3e246c82939d..a62f5b725061 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1588,7 +1588,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 {
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
-	const char *devname = NULL;
+	const char *devname = dev ? dev_name(dev) : "deviceless";
 	int ret;
 
 	if (get_type >= MAX_GET_TYPE) {
@@ -1601,45 +1601,47 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (dev)
-		devname = dev_name(dev);
-
 	rdev = regulator_dev_lookup(dev, id);
-	if (!IS_ERR(rdev))
-		goto found;
+	if (IS_ERR(rdev)) {
+		ret = PTR_ERR(rdev);
 
-	ret = PTR_ERR(rdev);
-	regulator = ERR_PTR(ret);
+		/*
+		 * If regulator_dev_lookup() fails with error other
+		 * than -ENODEV our job here is done, we simply return it.
+		 */
+		if (ret != -ENODEV)
+			return ERR_PTR(ret);
 
-	/*
-	 * If we have return value from dev_lookup fail, we do not expect to
-	 * succeed, so, quit with appropriate error value
-	 */
-	if (ret && ret != -ENODEV)
-		return regulator;
+		if (!have_full_constraints()) {
+			dev_warn(dev,
+				 "incomplete constraints, dummy supplies not allowed\n");
+			return ERR_PTR(-ENODEV);
+		}
 
-	if (!devname)
-		devname = "deviceless";
+		switch (get_type) {
+		case NORMAL_GET:
+			/*
+			 * Assume that a regulator is physically present and
+			 * enabled, even if it isn't hooked up, and just
+			 * provide a dummy.
+			 */
+			dev_warn(dev,
+				 "%s supply %s not found, using dummy regulator\n",
+				 devname, id);
+			rdev = dummy_regulator_rdev;
+			get_device(&rdev->dev);
+			break;
 
-	/*
-	 * Assume that a regulator is physically present and enabled
-	 * even if it isn't hooked up and just provide a dummy.
-	 */
-	if (have_full_constraints() && get_type == NORMAL_GET) {
-		pr_warn("%s supply %s not found, using dummy regulator\n",
-			devname, id);
+		case EXCLUSIVE_GET:
+			dev_warn(dev,
+				 "dummy supplies not allowed for exclusive requests\n");
+			/* fall through */
 
-		rdev = dummy_regulator_rdev;
-		get_device(&rdev->dev);
-		goto found;
-	/* Don't log an error when called from regulator_get_optional() */
-	} else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) {
-		dev_warn(dev, "dummy supplies not allowed\n");
+		default:
+			return ERR_PTR(-ENODEV);
+		}
 	}
 
-	return regulator;
-
-found:
 	if (rdev->exclusive) {
 		regulator = ERR_PTR(-EPERM);
 		put_device(&rdev->dev);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2017-02-08 18:36 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-03 21:56 [PATCH 1/5] regulator: core: remove dead code in _regulator_get() Dmitry Torokhov
2017-02-03 21:56 ` [PATCH 2/5] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Dmitry Torokhov
2017-02-04 10:11   ` Mark Brown
2017-02-04 18:19     ` [PATCH v2 " Dmitry Torokhov
2017-02-05 14:49       ` Mark Brown
2017-02-05 23:49       ` Mark Brown
2017-02-06 13:37       ` Applied "regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors" to the regulator tree Mark Brown
2017-02-03 21:56 ` [PATCH 3/5] regulator: core: have _regulator_get() accept get_type argument Dmitry Torokhov
2017-02-04 10:48   ` Applied "regulator: core: have _regulator_get() accept get_type argument" to the regulator tree Mark Brown
2017-02-03 21:56 ` [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov
2017-02-04 10:34   ` Mark Brown
2017-02-04 18:27     ` Dmitry Torokhov
2017-02-05 16:08       ` Mark Brown
2017-02-08 18:34   ` Applied "regulator: core: simplify _regulator_get()" to the regulator tree Mark Brown
2017-02-03 21:56 ` [PATCH 5/5] regulator: core: lower severity level of message about using dummy supplies Dmitry Torokhov
2017-02-04 11:48   ` Mark Brown
2017-02-04 17:45     ` Dmitry Torokhov
2017-02-05 16:12       ` Mark Brown
2017-02-07  0:56         ` Dmitry Torokhov
2017-02-08 18:19           ` Mark Brown
2017-02-04 10:48 ` Applied "regulator: core: remove dead code in _regulator_get()" to the regulator tree Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.