Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573470 > unrolled thread

[PATCH 4/5] regulator: core: simplify _regulator_get()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 23:00 +0100
    Re: [PATCH 4/5] regulator: core: simplify _regulator_get() Mark Brown <broonie@kernel.org> - 2017-02-04 11:40 +0100
      Re: [PATCH 4/5] regulator: core: simplify _regulator_get() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-04 19:30 +0100
        Re: [PATCH 4/5] regulator: core: simplify _regulator_get() Mark Brown <broonie@kernel.org> - 2017-02-05 17:10 +0100
    Applied "regulator: core: simplify _regulator_get()" to the regulator tree Mark Brown <broonie@kernel.org> - 2017-02-08 19:40 +0100

#1573470 — [PATCH 4/5] 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.

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

[toc] | [next] | [standalone]

#1573643

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1573728

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

[toc] | [prev] | [next] | [standalone]

#1573907

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1576819 — Applied "regulator: core: simplify _regulator_get()" to the regulator tree

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web