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

Groups > linux.kernel > #1384411 > unrolled thread

[PATCH 0/5] regulator: A few fixes for supply resolution

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] regulator: A few fixes for supply resolution Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 18:20 +0200
    [PATCH 1/5] regulator: core: Don't terminate supply resolution early Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 18:20 +0200
      Applied "regulator: core: Don't terminate supply resolution early" to the regulator tree Mark Brown <broonie@kernel.org> - 2016-04-22 13:00 +0200
    [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 18:20 +0200
      Re: [PATCH 4/5] regulator: core: Add early supply resolution for a  bypassed regulator Mark Brown <broonie@kernel.org> - 2016-04-22 12:50 +0200
        Re: [PATCH 4/5] regulator: core: Add early supply resolution for a  bypassed regulator Jon Hunter <jonathanh@nvidia.com> - 2016-04-22 13:30 +0200
          Re: [PATCH 4/5] regulator: core: Add early supply resolution for a  bypassed regulator Mark Brown <broonie@kernel.org> - 2016-04-22 16:00 +0200
            Re: [PATCH 4/5] regulator: core: Add early supply resolution for a  bypassed regulator Jon Hunter <jonathanh@nvidia.com> - 2016-04-25 16:50 +0200
              Re: [PATCH 4/5] regulator: core: Add early supply resolution for a  bypassed regulator Mark Brown <broonie@kernel.org> - 2016-04-25 19:00 +0200

#1384411 — [PATCH 0/5] regulator: A few fixes for supply resolution

This series is to mainly address the boot failures reported on the
Tegra124 Jetson TK1 where the AS3772 PMIC is failing to probe on -next [0].
However, there are a couple other patches I am including here which
came from looking at this problem. Here is a summary of the patches ...

Patch 1: Allows additional supplies to be resolved even if one fails
Patch 2: Fixes potential crash if turning on a supply fails
Patch 3-5: Address issue with AS3722 probe failing

[0] http://marc.info/?l=linux-tegra&m=145927821311669&w=2

Jon Hunter (5):
  regulator: core: Don't terminate supply resolution early
  regulator: core: Clear the supply pointer if enabling fails
  regulator: core: Move registration of regulator device
  regulator: core: Add early supply resolution for a bypassed regulator
  regulator: helpers: Ensure bypass register field matches ON value

 drivers/regulator/core.c    | 59 ++++++++++++++++++++++++++-------------------
 drivers/regulator/helpers.c |  2 +-
 2 files changed, 35 insertions(+), 26 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1384412 — [PATCH 1/5] regulator: core: Don't terminate supply resolution early

The function regulator_register_resolve_supply() is called from the
context of class_for_each_dev() (during the regulator registration) to
resolve any supplies added. regulator_register_resolve_supply() will
return an error if a regulator's supply cannot be resolved and this will
terminate the loop in class_for_each_dev(). This means that we will not
attempt to resolve any other supplies after one has failed. Hence, this
may delay the resolution of other regulator supplies until the failing
one itself can be resolved.

Rather than terminating the loop early, don't return an error code and
keep attempting to resolve any other supplies for regulators that have
been registered.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/regulator/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 61d3918f329e..a0b52ef11c30 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3865,7 +3865,12 @@ static void rdev_init_debugfs(struct regulator_dev *rdev)
 
 static int regulator_register_resolve_supply(struct device *dev, void *data)
 {
-	return regulator_resolve_supply(dev_to_rdev(dev));
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	if (regulator_resolve_supply(rdev))
+		rdev_dbg(rdev, "unable to resolve supply\n");
+
+	return 0;
 }
 
 /**
-- 
2.1.4

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

#1384969 — Applied "regulator: core: Don't terminate supply resolution early" to the regulator tree

The patch

   regulator: core: Don't terminate supply resolution early

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 7ddede6a58a0bd26efcfd2a5055611195411f514 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jonathanh@nvidia.com>
Date: Thu, 21 Apr 2016 17:11:57 +0100
Subject: [PATCH] regulator: core: Don't terminate supply resolution early

The function regulator_register_resolve_supply() is called from the
context of class_for_each_dev() (during the regulator registration) to
resolve any supplies added. regulator_register_resolve_supply() will
return an error if a regulator's supply cannot be resolved and this will
terminate the loop in class_for_each_dev(). This means that we will not
attempt to resolve any other supplies after one has failed. Hence, this
may delay the resolution of other regulator supplies until the failing
one itself can be resolved.

Rather than terminating the loop early, don't return an error code and
keep attempting to resolve any other supplies for regulators that have
been registered.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index fd0e4e37f4e1..9922922ce6bd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3842,7 +3842,12 @@ static void rdev_init_debugfs(struct regulator_dev *rdev)
 
 static int regulator_register_resolve_supply(struct device *dev, void *data)
 {
-	return regulator_resolve_supply(dev_to_rdev(dev));
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	if (regulator_resolve_supply(rdev))
+		rdev_dbg(rdev, "unable to resolve supply\n");
+
+	return 0;
 }
 
 /**
-- 
2.8.0.rc3

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

#1384414 — [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

The call to set_machine_constraints() in regulator_register(), will
attempt to get the voltage for the regulator.

A regulator that is in bypass will fail to be registered because we will
attempt to get the voltage of the regulator (ie. it's bypass voltage)
before the supply for the regulator has been resolved. Therefore, when
getting the voltage for a bypassed regulator, if the supply has not been
resolved, then attempt to resolve it. Additionally, move the setup of
the regulator's supply name to before the call to
set_machine_constraints() so that it can be resolved.

Please note that regulator_resolve_supply() will call
regulator_dev_lookup() which may acquire the regulator_list_mutex. To
avoid any deadlocks we cannot hold the regulator_list_mutex when calling
regulator_resolve_supply(). Following this change because
set_machine_constraints() may now result in a call to
regulator_resolve_supply() for a bypassed regulator, we can no longer
hold the regulator_list_mutex around this call. To avoid this rather
than holding the lock around a large portion of the registration code,
just hold the lock when aquiring any GPIOs and setting up supplies
because these sections may add entries to the regulator_map_list and
regulator_ena_gpio_list, respectively.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/regulator/core.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 754f3b4c2218..4f57a1832079 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3127,8 +3127,13 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 			return ret;
 		if (bypassed) {
 			/* if bypassed the regulator must have a supply */
-			if (!rdev->supply)
-				return -EINVAL;
+			if (!rdev->supply) {
+				ret = regulator_resolve_supply(rdev);
+				if (ret < 0)
+					return ret;
+				if (!rdev->supply)
+					return -EINVAL;
+			}
 
 			return _regulator_get_voltage(rdev->supply->rdev);
 		}
@@ -3945,8 +3950,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		rdev->dev.of_node = of_node_get(config->of_node);
 	}
 
-	mutex_lock(&regulator_list_mutex);
-
 	mutex_init(&rdev->mutex);
 	rdev->reg_data = config->driver_data;
 	rdev->owner = regulator_desc->owner;
@@ -3971,7 +3974,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	if ((config->ena_gpio || config->ena_gpio_initialized) &&
 	    gpio_is_valid(config->ena_gpio)) {
+		mutex_lock(&regulator_list_mutex);
 		ret = regulator_ena_gpio_request(rdev, config);
+		mutex_unlock(&regulator_list_mutex);
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
@@ -3989,31 +3994,32 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (init_data)
 		constraints = &init_data->constraints;
 
-	ret = set_machine_constraints(rdev, constraints);
-	if (ret < 0)
-		goto wash;
-
 	if (init_data && init_data->supply_regulator)
 		rdev->supply_name = init_data->supply_regulator;
 	else if (regulator_desc->supply_name)
 		rdev->supply_name = regulator_desc->supply_name;
 
+	ret = set_machine_constraints(rdev, constraints);
+	if (ret < 0)
+		goto wash;
+
 	/* add consumers devices */
 	if (init_data) {
+		mutex_lock(&regulator_list_mutex);
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
 			ret = set_consumer_device_supply(rdev,
 				init_data->consumer_supplies[i].dev_name,
 				init_data->consumer_supplies[i].supply);
 			if (ret < 0) {
+				mutex_unlock(&regulator_list_mutex);
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
 				goto unset_supplies;
 			}
 		}
+		mutex_unlock(&regulator_list_mutex);
 	}
 
-	mutex_unlock(&regulator_list_mutex);
-
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
@@ -4030,13 +4036,16 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	return rdev;
 
 unset_supplies:
+	mutex_lock(&regulator_list_mutex);
 	unset_regulator_supplies(rdev);
+	mutex_unlock(&regulator_list_mutex);
 wash:
 	kfree(rdev->constraints);
+	mutex_lock(&regulator_list_mutex);
 	regulator_ena_gpio_free(rdev);
+	mutex_unlock(&regulator_list_mutex);
 clean:
 	kfree(rdev);
-	mutex_unlock(&regulator_list_mutex);
 	kfree(config);
 	return ERR_PTR(ret);
 }
-- 
2.1.4

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

#1384961 — Re: [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

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

On Thu, Apr 21, 2016 at 05:12:00PM +0100, Jon Hunter wrote:

> A regulator that is in bypass will fail to be registered because we will
> attempt to get the voltage of the regulator (ie. it's bypass voltage)
> before the supply for the regulator has been resolved. Therefore, when
> getting the voltage for a bypassed regulator, if the supply has not been
> resolved, then attempt to resolve it. Additionally, move the setup of
> the regulator's supply name to before the call to
> set_machine_constraints() so that it can be resolved.

The basic pattern here makes sense but rather than doing this
specifically in the case where we have a bypassed supply we didn't 
resolve yet I think we should instead always try to resolve the supply
but ignore the error unless we actively need the supply.  I'd be
surprised if we didn't run into other cases where we need to do this so
it seems better to try the resolution in one place.

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

#1385059 — Re: [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

On 22/04/16 11:48, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Apr 21, 2016 at 05:12:00PM +0100, Jon Hunter wrote:
> 
>> A regulator that is in bypass will fail to be registered because we will
>> attempt to get the voltage of the regulator (ie. it's bypass voltage)
>> before the supply for the regulator has been resolved. Therefore, when
>> getting the voltage for a bypassed regulator, if the supply has not been
>> resolved, then attempt to resolve it. Additionally, move the setup of
>> the regulator's supply name to before the call to
>> set_machine_constraints() so that it can be resolved.
> 
> The basic pattern here makes sense but rather than doing this
> specifically in the case where we have a bypassed supply we didn't 
> resolve yet I think we should instead always try to resolve the supply
> but ignore the error unless we actively need the supply.  I'd be
> surprised if we didn't run into other cases where we need to do this so
> it seems better to try the resolution in one place.

OK. Sorry if I have misunderstood you here, but this sounds more like
Thierry's initial proposal [0] but ignoring the any errors returned (and
we need to fix-up the locking in this patch). In the discussion that
followed I thought we agreed to only do this for the bypass case [1]. As
far as I am concerned either will work, but to confirm we should just
always try to resolve the supply early during regulator_register(), correct?

Cheers
Jon

[0] http://marc.info/?t=146003907800001&r=1&w=2
[1] http://marc.info/?l=linux-kernel&m=146038421710211&w=2	

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

#1385206 — Re: [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

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

On Fri, Apr 22, 2016 at 12:26:57PM +0100, Jon Hunter wrote:

> OK. Sorry if I have misunderstood you here, but this sounds more like
> Thierry's initial proposal [0] but ignoring the any errors returned (and
> we need to fix-up the locking in this patch). In the discussion that

Yes!

> followed I thought we agreed to only do this for the bypass case [1]. As
> far as I am concerned either will work, but to confirm we should just
> always try to resolve the supply early during regulator_register(), correct?

We need to only *fail* in the bypass case.

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

#1386549 — Re: [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

On 22/04/16 14:53, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Apr 22, 2016 at 12:26:57PM +0100, Jon Hunter wrote:
> 
>> OK. Sorry if I have misunderstood you here, but this sounds more like
>> Thierry's initial proposal [0] but ignoring the any errors returned (and
>> we need to fix-up the locking in this patch). In the discussion that
> 
> Yes!
> 
>> followed I thought we agreed to only do this for the bypass case [1]. As
>> far as I am concerned either will work, but to confirm we should just
>> always try to resolve the supply early during regulator_register(), correct?
> 
> We need to only *fail* in the bypass case.

OK. So this is what I have now. Is it weird to return EPROBE_DEFER in 
_regulator_get_voltage()? If so, I could add a test for bypass in the
regulator_register().

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5b46d907e61d..7a6b7f667bcb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3126,7 +3126,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 		if (bypassed) {
 			/* if bypassed the regulator must have a supply */
 			if (!rdev->supply)
-				return -EINVAL;
+				return -EPROBE_DEFER;
 
 			return _regulator_get_voltage(rdev->supply->rdev);
 		}
@@ -3943,8 +3943,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		rdev->dev.of_node = of_node_get(config->of_node);
 	}
 
-	mutex_lock(&regulator_list_mutex);
-
 	mutex_init(&rdev->mutex);
 	rdev->reg_data = config->driver_data;
 	rdev->owner = regulator_desc->owner;
@@ -3969,7 +3967,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	if ((config->ena_gpio || config->ena_gpio_initialized) &&
 	    gpio_is_valid(config->ena_gpio)) {
+		mutex_lock(&regulator_list_mutex);
 		ret = regulator_ena_gpio_request(rdev, config);
+		mutex_unlock(&regulator_list_mutex);
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
@@ -3987,31 +3987,40 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (init_data)
 		constraints = &init_data->constraints;
 
-	ret = set_machine_constraints(rdev, constraints);
-	if (ret < 0)
-		goto wash;
-
 	if (init_data && init_data->supply_regulator)
 		rdev->supply_name = init_data->supply_regulator;
 	else if (regulator_desc->supply_name)
 		rdev->supply_name = regulator_desc->supply_name;
 
+	/*
+	 * Attempt to resolve the regulator supply, if specified,
+	 * but don't return an error if we fail because we will try
+	 * to resolve it again later as more regulators are added.
+	 */
+	if (regulator_resolve_supply(rdev))
+		rdev_dbg(rdev, "unable to resolve supply\n");
+
+	ret = set_machine_constraints(rdev, constraints);
+	if (ret < 0)
+		goto wash;
+
 	/* add consumers devices */
 	if (init_data) {
+		mutex_lock(&regulator_list_mutex);
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
 			ret = set_consumer_device_supply(rdev,
 				init_data->consumer_supplies[i].dev_name,
 				init_data->consumer_supplies[i].supply);
 			if (ret < 0) {
+				mutex_unlock(&regulator_list_mutex);
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
 				goto unset_supplies;
 			}
 		}
+		mutex_unlock(&regulator_list_mutex);
 	}
 
-	mutex_unlock(&regulator_list_mutex);
-
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
@@ -4028,13 +4037,16 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	return rdev;
 
 unset_supplies:
+	mutex_lock(&regulator_list_mutex);
 	unset_regulator_supplies(rdev);
+	mutex_unlock(&regulator_list_mutex);
 wash:
 	kfree(rdev->constraints);
+	mutex_lock(&regulator_list_mutex);
 	regulator_ena_gpio_free(rdev);
+	mutex_unlock(&regulator_list_mutex);
 clean:
 	kfree(rdev);
-	mutex_unlock(&regulator_list_mutex);
 	kfree(config);
 	return ERR_PTR(ret);
 }
-- 
2.1.4

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

#1386675 — Re: [PATCH 4/5] regulator: core: Add early supply resolution for a bypassed regulator

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

On Mon, Apr 25, 2016 at 03:44:24PM +0100, Jon Hunter wrote:
> On 22/04/16 14:53, Mark Brown wrote:

> > We need to only *fail* in the bypass case.

> OK. So this is what I have now. Is it weird to return EPROBE_DEFER in 
> _regulator_get_voltage()? If so, I could add a test for bypass in the
> regulator_register().

It should be fine but perhaps print an error saying that it's a bypassed
regulator with no supply?  Deferring for no explicit reason can be
confusing.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web