Subject: Re: [PATCH 1/5] regulator: core: Resolve supply earlier
To: Mark Brown <broonie@kernel.org>, Thierry Reding <thierry.reding@gmail.com>
References: <1460038959-21592-1-git-send-email-thierry.reding@gmail.com>
 <20160411140300.GH3351@sirena.org.uk> <20160411141101.GB27400@ulmo.ba.sec>
 <20160411141621.GI3351@sirena.org.uk>
CC: Liam Girdwood <lgirdwood@gmail.com>, <linux-kernel@vger.kernel.org>
From: Jon Hunter <jonathanh@nvidia.com>
Message-ID: <5716059B.3080503@nvidia.com>
Date: Tue, 19 Apr 2016 11:16:59 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160411141621.GI3351@sirena.org.uk>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2996
Lines: 99


On 11/04/16 15:16, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
>> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> 
>>> This shouldn't be a hard dependency: most regulators won't be in bypass
>>> mode or otherwise depend on their parents enough to need this.
> 
>> I had initially proposed to resolve the supply only when necessary
>> during regulator_get_voltage() when checking for bypass, perhaps that
>> would after all be more appropriate here?
> 
> Yes, that had been what I'd expected.

So the following seems to work, but only item I am uncertain about
is if it is ok to move the mutex_lock to after the
machine_set_constraints()?

Jon

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 61d3918f329e..742d10371e2d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3126,8 +3126,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);
 		}
@@ -3939,8 +3944,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;
@@ -3983,23 +3986,26 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (init_data)
 		constraints = &init_data->constraints;
 
+	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;
 
+	mutex_lock(&regulator_list_mutex);
+
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
+		mutex_unlock(&regulator_list_mutex);
 		goto wash;
 	}
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	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;
-
 	/* add consumers devices */
 	if (init_data) {
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
@@ -4009,6 +4015,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 			if (ret < 0) {
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
+				mutex_unlock(&regulator_list_mutex);
 				goto unset_supplies;
 			}
 		}
@@ -4036,7 +4043,6 @@ wash:
 clean:
 	kfree(rdev);
 out:
-	mutex_unlock(&regulator_list_mutex);
 	kfree(config);
 	return ERR_PTR(ret);
 }