[PATCH 1/4] regulator: core: fix typo in regulator_bulk_disable()

[PATCH 1/4] regulator: core: fix typo in regulator_bulk_disable()

[Dmitry Torokhov]

"re-enable" was misspelled as "reename".

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 1d3aff7e2bb1..8a469d7c00c2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3664,7 +3664,7 @@ int regulator_bulk_disable(int num_consumers,
 	for (++i; i < num_consumers; ++i) {
 		r = regulator_enable(consumers[i].consumer);
 		if (r != 0)
-			pr_err("Failed to reename %s: %d\n",
+			pr_err("Failed to re-enable %s: %d\n",
 			       consumers[i].supply, r);
 	}
 
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 2/4] regulator: core: simplify regulator_bulk_force_disable()

[Dmitry Torokhov]

There is no need to have two loops there, we can store error for subsequent
reporting.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8a469d7c00c2..9eb2e04f1219 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3690,21 +3690,17 @@ int regulator_bulk_force_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers)
 {
 	int i;
-	int ret;
+	int ret = 0;
 
-	for (i = 0; i < num_consumers; i++)
+	for (i = 0; i < num_consumers; i++) {
 		consumers[i].ret =
 			    regulator_force_disable(consumers[i].consumer);
 
-	for (i = 0; i < num_consumers; i++) {
-		if (consumers[i].ret != 0) {
+		/* Store first error for reporting */
+		if (consumers[i].ret && !ret)
 			ret = consumers[i].ret;
-			goto out;
-		}
 	}
 
-	return 0;
-out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_bulk_force_disable);
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 3/4] regulator: core: optimize devm_regulator_bulk_get()

[Dmitry Torokhov]

When performing this bulk operation, there is no need to track every supply
individually. It is more efficient to treat entire group as a single
managed resource.

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

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 965d1d31ec8c..784e3bf32210 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -120,6 +120,18 @@ void devm_regulator_put(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_put);
 
+struct regulator_bulk_devres {
+	struct regulator_bulk_data *consumers;
+	int num_consumers;
+};
+
+static void devm_regulator_bulk_release(struct device *dev, void *res)
+{
+	struct regulator_bulk_devres *devres = res;
+
+	regulator_bulk_free(devres->num_consumers, devres->consumers);
+}
+
 /**
  * devm_regulator_bulk_get - managed get multiple regulator consumers
  *
@@ -138,29 +150,22 @@ EXPORT_SYMBOL_GPL(devm_regulator_put);
 int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 			    struct regulator_bulk_data *consumers)
 {
-	int i;
+	struct regulator_bulk_devres *devres;
 	int ret;
 
-	for (i = 0; i < num_consumers; i++)
-		consumers[i].consumer = NULL;
-
-	for (i = 0; i < num_consumers; i++) {
-		consumers[i].consumer = devm_regulator_get(dev,
-							   consumers[i].supply);
-		if (IS_ERR(consumers[i].consumer)) {
-			ret = PTR_ERR(consumers[i].consumer);
-			dev_err(dev, "Failed to get supply '%s': %d\n",
-				consumers[i].supply, ret);
-			consumers[i].consumer = NULL;
-			goto err;
-		}
-	}
-
-	return 0;
+	devres = devres_alloc(devm_regulator_bulk_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
 
-err:
-	for (i = 0; i < num_consumers && consumers[i].consumer; i++)
-		devm_regulator_put(consumers[i].consumer);
+	ret = regulator_bulk_get(dev, num_consumers, consumers);
+	if (!ret) {
+		devres->consumers = consumers;
+		devres->num_consumers = num_consumers;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+	}
 
 	return ret;
 }
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 4/4] regulator: core: make bulk API support optional supplies

[Dmitry Torokhov]

Make it possible to use the bulk API with optional supplies, by allowing
the consumer to marking supplies as optional in the regulator_bulk_data.

Based on earlier patch by Bjorn Andersson <bjorn.andersson@sonymobile.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/regulator/core.c           | 41 ++++++++++++++++++++++++--------------
 include/linux/regulator/consumer.h |  3 +++
 2 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9eb2e04f1219..540d1dec6e06 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3546,20 +3546,22 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
 int regulator_bulk_get(struct device *dev, int num_consumers,
 		       struct regulator_bulk_data *consumers)
 {
+	struct regulator *r;
 	int i;
 	int ret;
 
-	for (i = 0; i < num_consumers; i++)
-		consumers[i].consumer = NULL;
-
 	for (i = 0; i < num_consumers; i++) {
-		consumers[i].consumer = regulator_get(dev,
-						      consumers[i].supply);
-		if (IS_ERR(consumers[i].consumer)) {
-			ret = PTR_ERR(consumers[i].consumer);
+		r = _regulator_get(dev, consumers[i].supply,
+				   consumers[i].optional ?
+					OPTIONAL_GET : NORMAL_GET);
+		ret = PTR_ERR_OR_ZERO(r);
+		if (!ret) {
+			consumers[i].consumer = r;
+		} else if (ret == -ENODEV && consumers[i].optional) {
+			consumers[i].consumer = NULL;
+		} else {
 			dev_err(dev, "Failed to get supply '%s': %d\n",
 				consumers[i].supply, ret);
-			consumers[i].consumer = NULL;
 			goto err;
 		}
 	}
@@ -3568,7 +3570,8 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 
 err:
 	while (--i >= 0)
-		regulator_put(consumers[i].consumer);
+		if (consumers[i].consumer)
+			regulator_put(consumers[i].consumer);
 
 	return ret;
 }
@@ -3601,7 +3604,7 @@ int regulator_bulk_enable(int num_consumers,
 	int ret = 0;
 
 	for (i = 0; i < num_consumers; i++) {
-		if (consumers[i].consumer->always_on)
+		if (!consumers[i].consumer || consumers[i].consumer->always_on)
 			consumers[i].ret = 0;
 		else
 			async_schedule_domain(regulator_bulk_enable_async,
@@ -3625,7 +3628,7 @@ int regulator_bulk_enable(int num_consumers,
 		if (consumers[i].ret < 0)
 			pr_err("Failed to enable %s: %d\n", consumers[i].supply,
 			       consumers[i].ret);
-		else
+		else if (consumers[i].consumer)
 			regulator_disable(consumers[i].consumer);
 	}
 
@@ -3652,6 +3655,9 @@ int regulator_bulk_disable(int num_consumers,
 	int ret, r;
 
 	for (i = num_consumers - 1; i >= 0; --i) {
+		if (!consumers[i].consumer)
+			continue;
+
 		ret = regulator_disable(consumers[i].consumer);
 		if (ret != 0)
 			goto err;
@@ -3662,6 +3668,9 @@ int regulator_bulk_disable(int num_consumers,
 err:
 	pr_err("Failed to disable %s: %d\n", consumers[i].supply, ret);
 	for (++i; i < num_consumers; ++i) {
+		if (!consumers[i].consumer)
+			continue;
+
 		r = regulator_enable(consumers[i].consumer);
 		if (r != 0)
 			pr_err("Failed to re-enable %s: %d\n",
@@ -3693,8 +3702,8 @@ int regulator_bulk_force_disable(int num_consumers,
 	int ret = 0;
 
 	for (i = 0; i < num_consumers; i++) {
-		consumers[i].ret =
-			    regulator_force_disable(consumers[i].consumer);
+		consumers[i].ret = consumers[i].consumer ?
+			regulator_force_disable(consumers[i].consumer) : 0;
 
 		/* Store first error for reporting */
 		if (consumers[i].ret && !ret)
@@ -3720,8 +3729,10 @@ void regulator_bulk_free(int num_consumers,
 	int i;
 
 	for (i = 0; i < num_consumers; i++) {
-		regulator_put(consumers[i].consumer);
-		consumers[i].consumer = NULL;
+		if (consumers[i].consumer) {
+			regulator_put(consumers[i].consumer);
+			consumers[i].consumer = NULL;
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(regulator_bulk_free);
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ea0fffa5faeb..acaeeec279af 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -159,6 +159,8 @@ struct regulator;
  *
  * @supply:   The name of the supply.  Initialised by the user before
  *            using the bulk regulator APIs.
+ * @optional: The supply should be considered optional. Initialised by the user
+ *            before using the bulk regulator APIs.
  * @consumer: The regulator consumer for the supply.  This will be managed
  *            by the bulk API.
  *
@@ -168,6 +170,7 @@ struct regulator;
  */
 struct regulator_bulk_data {
 	const char *supply;
+	bool optional;
 	struct regulator *consumer;
 
 	/* private: Internal use */
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[kbuild test robot]

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

Hi Dmitry,

[auto build test ERROR on regulator/for-next]
[also build test ERROR on v4.10-rc6 next-20170203]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/regulator-core-fix-typo-in-regulator_bulk_disable/20170204-145103
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-x008-201705 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/regulator/core.c: In function 'regulator_bulk_get':
>> drivers/regulator/core.c:3565:6: error: 'OPTIONAL_GET' undeclared (first use in this function)
         OPTIONAL_GET : NORMAL_GET);
         ^~~~~~~~~~~~
   drivers/regulator/core.c:3565:6: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/regulator/core.c:3565:21: error: 'NORMAL_GET' undeclared (first use in this function)
         OPTIONAL_GET : NORMAL_GET);
                        ^~~~~~~~~~
>> drivers/regulator/core.c:3563:7: error: too few arguments to function '_regulator_get'
      r = _regulator_get(dev, consumers[i].supply,
          ^~~~~~~~~~~~~~
   drivers/regulator/core.c:1596:26: note: declared here
    static struct regulator *_regulator_get(struct device *dev, const char *id,
                             ^~~~~~~~~~~~~~

vim +/OPTIONAL_GET +3565 drivers/regulator/core.c

  3557	{
  3558		struct regulator *r;
  3559		int i;
  3560		int ret;
  3561	
  3562		for (i = 0; i < num_consumers; i++) {
> 3563			r = _regulator_get(dev, consumers[i].supply,
  3564					   consumers[i].optional ?
> 3565						OPTIONAL_GET : NORMAL_GET);
  3566			ret = PTR_ERR_OR_ZERO(r);
  3567			if (!ret) {
  3568				consumers[i].consumer = r;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25574 bytes --]

Applied "regulator: core: fix typo in regulator_bulk_disable()" to the regulator tree

[Mark Brown]

The patch

   regulator: core: fix typo in regulator_bulk_disable()

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 d1642ea717be09039114dad57a8ae08d77f17dfb Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Fri, 3 Feb 2017 15:16:16 -0800
Subject: [PATCH] regulator: core: fix typo in regulator_bulk_disable()

"re-enable" was misspelled as "reename".

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 fe05923611ee..867756651544 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3661,7 +3661,7 @@ int regulator_bulk_disable(int num_consumers,
 	for (++i; i < num_consumers; ++i) {
 		r = regulator_enable(consumers[i].consumer);
 		if (r != 0)
-			pr_err("Failed to reename %s: %d\n",
+			pr_err("Failed to re-enable %s: %d\n",
 			       consumers[i].supply, r);
 	}
 
-- 
2.11.0

Applied "regulator: core: optimize devm_regulator_bulk_get()" to the regulator tree

[Mark Brown]

The patch

   regulator: core: optimize devm_regulator_bulk_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 3eaeb4756336ad884a2a8eef3ca9e12845fb5391 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Fri, 3 Feb 2017 15:16:18 -0800
Subject: [PATCH] regulator: core: optimize devm_regulator_bulk_get()

When performing this bulk operation, there is no need to track every supply
individually. It is more efficient to treat entire group as a single
managed resource.

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

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 965d1d31ec8c..784e3bf32210 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -120,6 +120,18 @@ void devm_regulator_put(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_put);
 
+struct regulator_bulk_devres {
+	struct regulator_bulk_data *consumers;
+	int num_consumers;
+};
+
+static void devm_regulator_bulk_release(struct device *dev, void *res)
+{
+	struct regulator_bulk_devres *devres = res;
+
+	regulator_bulk_free(devres->num_consumers, devres->consumers);
+}
+
 /**
  * devm_regulator_bulk_get - managed get multiple regulator consumers
  *
@@ -138,29 +150,22 @@ EXPORT_SYMBOL_GPL(devm_regulator_put);
 int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 			    struct regulator_bulk_data *consumers)
 {
-	int i;
+	struct regulator_bulk_devres *devres;
 	int ret;
 
-	for (i = 0; i < num_consumers; i++)
-		consumers[i].consumer = NULL;
-
-	for (i = 0; i < num_consumers; i++) {
-		consumers[i].consumer = devm_regulator_get(dev,
-							   consumers[i].supply);
-		if (IS_ERR(consumers[i].consumer)) {
-			ret = PTR_ERR(consumers[i].consumer);
-			dev_err(dev, "Failed to get supply '%s': %d\n",
-				consumers[i].supply, ret);
-			consumers[i].consumer = NULL;
-			goto err;
-		}
-	}
-
-	return 0;
+	devres = devres_alloc(devm_regulator_bulk_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
 
-err:
-	for (i = 0; i < num_consumers && consumers[i].consumer; i++)
-		devm_regulator_put(consumers[i].consumer);
+	ret = regulator_bulk_get(dev, num_consumers, consumers);
+	if (!ret) {
+		devres->consumers = consumers;
+		devres->num_consumers = num_consumers;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+	}
 
 	return ret;
 }
-- 
2.11.0

Applied "regulator: core: simplify regulator_bulk_force_disable()" to the regulator tree

[Mark Brown]

The patch

   regulator: core: simplify regulator_bulk_force_disable()

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 b8c77ff6902baa6ca93ca643bfff2d565801ea30 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Fri, 3 Feb 2017 15:16:17 -0800
Subject: [PATCH] regulator: core: simplify regulator_bulk_force_disable()

There is no need to have two loops there, we can store error for subsequent
reporting.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 206c274c0003..fe05923611ee 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3687,21 +3687,17 @@ int regulator_bulk_force_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers)
 {
 	int i;
-	int ret;
+	int ret = 0;
 
-	for (i = 0; i < num_consumers; i++)
+	for (i = 0; i < num_consumers; i++) {
 		consumers[i].ret =
 			    regulator_force_disable(consumers[i].consumer);
 
-	for (i = 0; i < num_consumers; i++) {
-		if (consumers[i].ret != 0) {
+		/* Store first error for reporting */
+		if (consumers[i].ret && !ret)
 			ret = consumers[i].ret;
-			goto out;
-		}
 	}
 
-	return 0;
-out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_bulk_force_disable);
-- 
2.11.0

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Mark Brown]

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

On Fri, Feb 03, 2017 at 03:16:19PM -0800, Dmitry Torokhov wrote:
> Make it possible to use the bulk API with optional supplies, by allowing
> the consumer to marking supplies as optional in the regulator_bulk_data.

So, I know I took the version Bjorn sent before (which was subsequently
reverted) but based on further reflection and having seen people trying
to use it I really don't think this is a good idea and that the revert
was the best thing to do.  The idiomatic use of bulk operations is to
treat the entire block of regulators en masse, this is not possible when
some of the regulators are optional.  You *can* peer into the structure
and special case things but it then makes further uses of the bulk API
on the same block of regulators not work which isn't good.

As I said earlier making it easy to just transparently mix optional
regulators in is something I'd expect to see commonly associated with
abuse of the optional API as a mechanism for not implementing sensible
error handling.

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

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Dmitry Torokhov]

On Sat, Feb 04, 2017 at 11:56:14AM +0100, Mark Brown wrote:
> On Fri, Feb 03, 2017 at 03:16:19PM -0800, Dmitry Torokhov wrote:
> > Make it possible to use the bulk API with optional supplies, by allowing
> > the consumer to marking supplies as optional in the regulator_bulk_data.
> 
> So, I know I took the version Bjorn sent before (which was subsequently
> reverted) but based on further reflection and having seen people trying
> to use it I really don't think this is a good idea and that the revert
> was the best thing to do.  The idiomatic use of bulk operations is to
> treat the entire block of regulators en masse, this is not possible when
> some of the regulators are optional.  You *can* peer into the structure
> and special case things but it then makes further uses of the bulk API
> on the same block of regulators not work which isn't good.

They should work with the version of the patch I sent. There you can use
regulator_bulk_enable() and regulator_bulk_disable() and others and they
will skip over optional missing regulators.

> 
> As I said earlier making it easy to just transparently mix optional
> regulators in is something I'd expect to see commonly associated with
> abuse of the optional API as a mechanism for not implementing sensible
> error handling.

Error handling is hard to get right and error paths ate rarely tested.
The more of it we can move away from boilerplate and into helper APIs,
the better we are off.

Consider the conversion patch below as an example. We are able to remove
forest of "if (IS_ERR(...))", checking and special handling of
-EPROBE_DEFER, and jumping to labels to disable regulators with 2 API
calls and much smaller checks to figure out the configuration we are
running with.

I think it also fixes bug with not handling deferrels from av/dv
regulators when ldoin is missing.

Thanks.

-- 
Dmitry


ASoC: tlv320aic32x4: use bulk regulator API

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Now that bulk regulator API supports optional regulators we can use it
here.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 sound/soc/codecs/tlv320aic32x4.c |  118 +++++++++++---------------------------
 1 file changed, 35 insertions(+), 83 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 28fdfc5ec544..5d8420f66f5c 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -46,6 +46,18 @@
 
 #include "tlv320aic32x4.h"
 
+#define SUPPLY_IOV	0
+#define SUPPLY_LDOIN	1
+#define SUPPLY_DV	2
+#define SUPPLY_AV	3
+
+static const struct regulator_bulk_data aic32x4_supplies[] = {
+	[SUPPLY_IOV]	= { .supply = "iov" },
+	[SUPPLY_LDOIN]	= { .supply = "ldoin", .optional = true },
+	[SUPPLY_DV]	= { .supply = "dv", .optional = true },
+	[SUPPLY_AV]	= { .supply = "av", .optional = true },
+};
+
 struct aic32x4_rate_divs {
 	u32 mclk;
 	u32 rate;
@@ -70,10 +82,7 @@ struct aic32x4_priv {
 	int rstn_gpio;
 	struct clk *mclk;
 
-	struct regulator *supply_ldo;
-	struct regulator *supply_iov;
-	struct regulator *supply_dv;
-	struct regulator *supply_av;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(aic32x4_supplies)];
 };
 
 /* 0dB min, 0.5dB steps */
@@ -819,102 +828,45 @@ static int aic32x4_parse_dt(struct aic32x4_priv *aic32x4,
 
 static void aic32x4_disable_regulators(struct aic32x4_priv *aic32x4)
 {
-	regulator_disable(aic32x4->supply_iov);
-
-	if (!IS_ERR(aic32x4->supply_ldo))
-		regulator_disable(aic32x4->supply_ldo);
-
-	if (!IS_ERR(aic32x4->supply_dv))
-		regulator_disable(aic32x4->supply_dv);
-
-	if (!IS_ERR(aic32x4->supply_av))
-		regulator_disable(aic32x4->supply_av);
+	regulator_bulk_disable(ARRAY_SIZE(aic32x4->supplies),
+			       aic32x4->supplies);
 }
 
 static int aic32x4_setup_regulators(struct device *dev,
 		struct aic32x4_priv *aic32x4)
 {
-	int ret = 0;
-
-	aic32x4->supply_ldo = devm_regulator_get_optional(dev, "ldoin");
-	aic32x4->supply_iov = devm_regulator_get(dev, "iov");
-	aic32x4->supply_dv = devm_regulator_get_optional(dev, "dv");
-	aic32x4->supply_av = devm_regulator_get_optional(dev, "av");
-
-	/* Check if the regulator requirements are fulfilled */
-
-	if (IS_ERR(aic32x4->supply_iov)) {
-		dev_err(dev, "Missing supply 'iov'\n");
-		return PTR_ERR(aic32x4->supply_iov);
-	}
-
-	if (IS_ERR(aic32x4->supply_ldo)) {
-		if (PTR_ERR(aic32x4->supply_ldo) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+	int ret;
 
-		if (IS_ERR(aic32x4->supply_dv)) {
-			dev_err(dev, "Missing supply 'dv' or 'ldoin'\n");
-			return PTR_ERR(aic32x4->supply_dv);
-		}
-		if (IS_ERR(aic32x4->supply_av)) {
-			dev_err(dev, "Missing supply 'av' or 'ldoin'\n");
-			return PTR_ERR(aic32x4->supply_av);
-		}
-	} else {
-		if (IS_ERR(aic32x4->supply_dv) &&
-				PTR_ERR(aic32x4->supply_dv) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		if (IS_ERR(aic32x4->supply_av) &&
-				PTR_ERR(aic32x4->supply_av) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-	}
+	memcpy(aic32x4->supplies, aic32x4_supplies, sizeof(aic32x4_supplies));
 
-	ret = regulator_enable(aic32x4->supply_iov);
-	if (ret) {
-		dev_err(dev, "Failed to enable regulator iov\n");
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(aic32x4->supplies),
+				      aic32x4->supplies);
+	if (ret)
 		return ret;
-	}
 
-	if (!IS_ERR(aic32x4->supply_ldo)) {
-		ret = regulator_enable(aic32x4->supply_ldo);
-		if (ret) {
-			dev_err(dev, "Failed to enable regulator ldo\n");
-			goto error_ldo;
+	/* Check if the regulator requirements are fulfilled */
+	if (!aic32x4->supplies[SUPPLY_LDOIN].consumer) {
+		if (!aic32x4->supplies[SUPPLY_DV].consumer) {
+			dev_err(dev, "Missing both 'dv' and 'ldoin' supplies\n");
+			return -ENODEV;
 		}
-	}
 
-	if (!IS_ERR(aic32x4->supply_dv)) {
-		ret = regulator_enable(aic32x4->supply_dv);
-		if (ret) {
-			dev_err(dev, "Failed to enable regulator dv\n");
-			goto error_dv;
+		if (!aic32x4->supplies[SUPPLY_AV].consumer) {
+			dev_err(dev, "Missing both 'av' and 'ldoin' supplies\n");
+			return -ENODEV;
 		}
+	} else if (!aic32x4->supplies[SUPPLY_AV].consumer) {
+		aic32x4->power_cfg |= AIC32X4_PWR_AIC32X4_LDO_ENABLE;
 	}
 
-	if (!IS_ERR(aic32x4->supply_av)) {
-		ret = regulator_enable(aic32x4->supply_av);
-		if (ret) {
-			dev_err(dev, "Failed to enable regulator av\n");
-			goto error_av;
-		}
+	ret = regulator_bulk_enable(ARRAY_SIZE(aic32x4->supplies),
+				    aic32x4->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulators\n");
+		return ret;
 	}
 
-	if (!IS_ERR(aic32x4->supply_ldo) && IS_ERR(aic32x4->supply_av))
-		aic32x4->power_cfg |= AIC32X4_PWR_AIC32X4_LDO_ENABLE;
-
 	return 0;
-
-error_av:
-	if (!IS_ERR(aic32x4->supply_dv))
-		regulator_disable(aic32x4->supply_dv);
-
-error_dv:
-	if (!IS_ERR(aic32x4->supply_ldo))
-		regulator_disable(aic32x4->supply_ldo);
-
-error_ldo:
-	regulator_disable(aic32x4->supply_iov);
-	return ret;
 }
 
 int aic32x4_probe(struct device *dev, struct regmap *regmap)

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Mark Brown]

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

On Sat, Feb 04, 2017 at 10:13:18AM -0800, Dmitry Torokhov wrote:
> On Sat, Feb 04, 2017 at 11:56:14AM +0100, Mark Brown wrote:

> > some of the regulators are optional.  You *can* peer into the structure
> > and special case things but it then makes further uses of the bulk API
> > on the same block of regulators not work which isn't good.

> They should work with the version of the patch I sent. There you can use
> regulator_bulk_enable() and regulator_bulk_disable() and others and they
> will skip over optional missing regulators.

So that bit's addressed but not the wider thing where more special case
code is going to be needed - it's unlikely to simply be a case of just
not operating on the optional regulator or regulators.  This is at least
as much of an issue, the way that this says that it's a normal thing to
just have some regulators that might be optional with no special
handling is a normal and standard thing.  

> Consider the conversion patch below as an example. We are able to remove
> forest of "if (IS_ERR(...))", checking and special handling of
> -EPROBE_DEFER, and jumping to labels to disable regulators with 2 API
> calls and much smaller checks to figure out the configuration we are
> running with.

The tlv320aic32x4 driver isn't a particularly well written driver in
this regard in the first place - I don't recall the details but I
strongly suspect that the driver is an example of abuse of the optional
API and that of the supplies possibly only ldoin is actually optional.
I would expect that this should look *much* more like sgtl5000, or
possibly handled more like arizona-ldo1.  I agree that there's lots of
room for cleanups and fixes here, frankly I don't quite remember why I
accepted the patch.

I'd be a lot happier if I were seeing examples where this helped a lot
and the original code looked good, I've not really been seeing those
though.  A lot of the examples of use of optional regulators that I see
(including both those in drivers/input FWIW) don't look like they are
for supplies that should be optional.

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

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Dmitry Torokhov]

On Sun, Feb 05, 2017 at 05:07:37PM +0100, Mark Brown wrote:
> On Sat, Feb 04, 2017 at 10:13:18AM -0800, Dmitry Torokhov wrote:
> > On Sat, Feb 04, 2017 at 11:56:14AM +0100, Mark Brown wrote:
> 
> > > some of the regulators are optional.  You *can* peer into the structure
> > > and special case things but it then makes further uses of the bulk API
> > > on the same block of regulators not work which isn't good.
> 
> > They should work with the version of the patch I sent. There you can use
> > regulator_bulk_enable() and regulator_bulk_disable() and others and they
> > will skip over optional missing regulators.
> 
> So that bit's addressed but not the wider thing where more special case
> code is going to be needed - it's unlikely to simply be a case of just
> not operating on the optional regulator or regulators.  This is at least
> as much of an issue, the way that this says that it's a normal thing to
> just have some regulators that might be optional with no special
> handling is a normal and standard thing.  

You would need some handling, but once you figured out the configuration
and set up the hardware, then you can just enable regulators that you
have in one go.

I guess there will be cases when that does not work, bit I think in many
cases it will.

> 
> > Consider the conversion patch below as an example. We are able to remove
> > forest of "if (IS_ERR(...))", checking and special handling of
> > -EPROBE_DEFER, and jumping to labels to disable regulators with 2 API
> > calls and much smaller checks to figure out the configuration we are
> > running with.
> 
> The tlv320aic32x4 driver isn't a particularly well written driver in
> this regard in the first place - I don't recall the details but I
> strongly suspect that the driver is an example of abuse of the optional
> API and that of the supplies possibly only ldoin is actually optional.
> I would expect that this should look *much* more like sgtl5000, or

OK, I think the optional support in regulator_bulk_get will help
sgtl5000 as well, as you would not have to go through contortions of
requesting the optional regulator first and then figuring out if it
should be included or excluded from the bulk supply list.

Please see the patch below.

> possibly handled more like arizona-ldo1.  I agree that there's lots of
> room for cleanups and fixes here, frankly I don't quite remember why I
> accepted the patch.
> 
> I'd be a lot happier if I were seeing examples where this helped a lot
> and the original code looked good, I've not really been seeing those
> though.

If you can point me at a few examples where original code looks good I
can see how it can be converter to bulk with optionals support.

>  A lot of the examples of use of optional regulators that I see
> (including both those in drivers/input FWIW) don't look like they are
> for supplies that should be optional.

Fair enough. The regulators there are not optional, rather we are trying
to avoid delays (i.e. doing msleep(N)) if regulators are not descried by
the firmware (incomplete DT in OF system, or ACPI system). Will you
accept:

bool regulator_is_dummy(struct regulator *r)
{
	return r == dummy_regulator_rdev;
}

or maybe even better

bool regulator_is_always_on(struct regulator *r)
{
	return r->always_on;
}

Then we can switch to straightforward (non-optional) regulator_get() in
input and still avoid waiting when we don't actually control regulators.

Thanks.

-- 
Dmitry


ASoC: sgtl5000: use bulk regulator API and more devm

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Now that bulk API supports optional regulators we can clean up the code a
bit. Also switch more resources to devm.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 sound/soc/codecs/sgtl5000.c |   96 ++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 1589325855bc..444a0fbd8348 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -105,7 +105,6 @@ struct sgtl5000_priv {
 	int master;	/* i2s master or not */
 	int fmt;	/* i2s data format */
 	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
-	int num_supplies;
 	struct regmap *regmap;
 	struct clk *mclk;
 	int revision;
@@ -939,7 +938,7 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 
 	vdda  = regulator_get_voltage(sgtl5000->supplies[VDDA].consumer);
 	vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO].consumer);
-	vddd  = (sgtl5000->num_supplies > VDDD)
+	vddd  = (sgtl5000->supplies[VDDD].consumer)
 		? regulator_get_voltage(sgtl5000->supplies[VDDD].consumer)
 		: LDO_VOLTAGE;
 
@@ -1047,43 +1046,46 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 	return 0;
 }
 
-static int sgtl5000_enable_regulators(struct i2c_client *client)
+static void sgtl5000_disable_regulators(void *_sgtl5000)
+{
+	struct sgtl5000_priv *sgtl5000 = _sgtl5000;
+
+	regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
+			       sgtl5000->supplies);
+}
+
+static int sgtl5000_enable_regulators(struct i2c_client *client,
+				      struct sgtl5000_priv *sgtl5000)
 {
 	int ret;
 	int i;
-	int external_vddd = 0;
-	struct regulator *vddd;
-	struct sgtl5000_priv *sgtl5000 = i2c_get_clientdata(client);
 
 	for (i = 0; i < ARRAY_SIZE(sgtl5000->supplies); i++)
 		sgtl5000->supplies[i].supply = supply_names[i];
 
-	vddd = regulator_get_optional(&client->dev, "VDDD");
-	if (IS_ERR(vddd)) {
-		/* See if it's just not registered yet */
-		if (PTR_ERR(vddd) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-	} else {
-		external_vddd = 1;
-		regulator_put(vddd);
-	}
+	sgtl5000->supplies[VDDD].optional = true;
 
-	sgtl5000->num_supplies = ARRAY_SIZE(sgtl5000->supplies)
-				 - 1 + external_vddd;
-	ret = regulator_bulk_get(&client->dev, sgtl5000->num_supplies,
-				 sgtl5000->supplies);
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(sgtl5000->supplies),
+				      sgtl5000->supplies);
 	if (ret)
 		return ret;
 
-	ret = regulator_bulk_enable(sgtl5000->num_supplies,
+	ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
 				    sgtl5000->supplies);
 	if (!ret)
 		usleep_range(10, 20);
-	else
-		regulator_bulk_free(sgtl5000->num_supplies,
-				    sgtl5000->supplies);
 
-	return ret;
+	ret = devm_add_action_or_reset(&client->dev,
+				       sgtl5000_disable_regulators,
+				       sgtl5000);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to add action to disable regulators\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 static int sgtl5000_probe(struct snd_soc_codec *codec)
@@ -1204,6 +1206,13 @@ static void sgtl5000_fill_defaults(struct i2c_client *client)
 	}
 }
 
+static void sgtl5000_disable_clk(void *_sgtl5000)
+{
+	struct sgtl5000_priv *sgtl5000 = _sgtl5000;
+
+	clk_disable_unprepare(sgtl5000->mclk);
+}
+
 static int sgtl5000_i2c_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1219,7 +1228,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, sgtl5000);
 
-	ret = sgtl5000_enable_regulators(client);
+	ret = sgtl5000_enable_regulators(client, sgtl5000);
 	if (ret)
 		return ret;
 
@@ -1227,7 +1236,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (IS_ERR(sgtl5000->regmap)) {
 		ret = PTR_ERR(sgtl5000->regmap);
 		dev_err(&client->dev, "Failed to allocate regmap: %d\n", ret);
-		goto disable_regs;
+		return ret;
 	}
 
 	sgtl5000->mclk = devm_clk_get(&client->dev, NULL);
@@ -1237,31 +1246,38 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 		/* Defer the probe to see if the clk will be provided later */
 		if (ret == -ENOENT)
 			ret = -EPROBE_DEFER;
-		goto disable_regs;
+		return ret;
 	}
 
 	ret = clk_prepare_enable(sgtl5000->mclk);
 	if (ret) {
 		dev_err(&client->dev, "Error enabling clock %d\n", ret);
-		goto disable_regs;
+		return ret;
 	}
 
 	/* Need 8 clocks before I2C accesses */
 	udelay(1);
 
+	ret = devm_add_action_or_reset(&client->dev,
+				       sgtl5000_disable_clk, sgtl5000);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to add action to disable clock\n");
+		return ret;
+	}
+
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
 	if (ret) {
 		dev_err(&client->dev, "Error reading chip id %d\n", ret);
-		goto disable_clk;
+		return ret;
 	}
 
 	if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=
 	    SGTL5000_PARTID_PART_ID) {
 		dev_err(&client->dev,
 			"Device with ID register %x is not a sgtl5000\n", reg);
-		ret = -ENODEV;
-		goto disable_clk;
+		return -ENODEV;
 	}
 
 	rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT;
@@ -1278,7 +1294,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 
 	/* Follow section 2.2.1.1 of AN3663 */
 	ana_pwr = SGTL5000_ANA_POWER_DEFAULT;
-	if (sgtl5000->num_supplies <= VDDD) {
+	if (!sgtl5000->supplies[VDDD].consumer) {
 		/* internal VDDD at 1.2V */
 		ret = regmap_update_bits(sgtl5000->regmap,
 					 SGTL5000_CHIP_LINREG_CTRL,
@@ -1353,28 +1369,14 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	ret = snd_soc_register_codec(&client->dev,
 			&sgtl5000_driver, &sgtl5000_dai, 1);
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	return 0;
-
-disable_clk:
-	clk_disable_unprepare(sgtl5000->mclk);
-
-disable_regs:
-	regulator_bulk_disable(sgtl5000->num_supplies, sgtl5000->supplies);
-	regulator_bulk_free(sgtl5000->num_supplies, sgtl5000->supplies);
-
-	return ret;
 }
 
 static int sgtl5000_i2c_remove(struct i2c_client *client)
 {
-	struct sgtl5000_priv *sgtl5000 = i2c_get_clientdata(client);
-
 	snd_soc_unregister_codec(&client->dev);
-	clk_disable_unprepare(sgtl5000->mclk);
-	regulator_bulk_disable(sgtl5000->num_supplies, sgtl5000->supplies);
-	regulator_bulk_free(sgtl5000->num_supplies, sgtl5000->supplies);
 
 	return 0;
 }

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Mark Brown]

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

On Sun, Feb 05, 2017 at 08:30:33PM -0800, Dmitry Torokhov wrote:
> On Sun, Feb 05, 2017 at 05:07:37PM +0100, Mark Brown wrote:

> > The tlv320aic32x4 driver isn't a particularly well written driver in
> > this regard in the first place - I don't recall the details but I
> > strongly suspect that the driver is an example of abuse of the optional
> > API and that of the supplies possibly only ldoin is actually optional.
> > I would expect that this should look *much* more like sgtl5000, or

> OK, I think the optional support in regulator_bulk_get will help
> sgtl5000 as well, as you would not have to go through contortions of
> requesting the optional regulator first and then figuring out if it
> should be included or excluded from the bulk supply list.

It does make things better, though I'm not sure that simply open coding
the optional regulators wouldn't be just as good.  I'm not 100% sure
that this is what the driver actually should be doing - there were lots
of problems with that driver figuring out what on earth it was supposed
to do as there were some older revisions of the chip with very serious
errata which used to be handled in the driver but broken the production
versions.  There's a degree of "it works, don't touch it" going on.

> > I'd be a lot happier if I were seeing examples where this helped a lot
> > and the original code looked good, I've not really been seeing those
> > though.

> If you can point me at a few examples where original code looks good I
> can see how it can be converter to bulk with optionals support.

The main one is the MMC core (which is what this was written for).

> >  A lot of the examples of use of optional regulators that I see
> > (including both those in drivers/input FWIW) don't look like they are
> > for supplies that should be optional.

> Fair enough. The regulators there are not optional, rather we are trying
> to avoid delays (i.e. doing msleep(N)) if regulators are not descried by
> the firmware (incomplete DT in OF system, or ACPI system). Will you
> accept:

> bool regulator_is_dummy(struct regulator *r)
> {
> 	return r == dummy_regulator_rdev;
> }

This is obviously a total failure of abstraction.

> or maybe even better

> bool regulator_is_always_on(struct regulator *r)
> {
> 	return r->always_on;
> }

> Then we can switch to straightforward (non-optional) regulator_get() in
> input and still avoid waiting when we don't actually control regulators.

This is also an abstraction failure, though it is better, and doesn't
quite do what you want since if the regulator is shared you may still
end up delaying when you don't need to.  What the drivers should be
doing is registering a notifier which they use to check when the
regulator was last enabled and using that to tell them if they need to
delay (with bonus points for adjusting the delay if we're part way
through the needed time).

It's probably worth having a helper which registers notifiers and
provides query functions that wraps this up, updates a timestamp for the
last enable and has a helper drivers can use to check if it was within a
given period, this is probably going to be a useful enough pattern for
that.  Drivers can then just enable and ask if how long ago the last
physical enable happened.

Depending on how severe the delays are and if anything else needs doing
(like resetting registers) a simpler thing can be to use a notifier to
determine if the power has been removed and reinitialize if that's
happened.

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

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Bjorn Andersson]

On Fri 03 Feb 15:16 PST 2017, Dmitry Torokhov wrote:

> Make it possible to use the bulk API with optional supplies, by allowing
> the consumer to marking supplies as optional in the regulator_bulk_data.
> 
> Based on earlier patch by Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 

Hi Dmitry,

Rather than fixing my broken patch, I reverted it after talking to Mark.
This as I realized that I got things backwards.

When a given component has an "optional" supply this can mean two
things:
1) The component has a supply pin that is connected, but the kernel has
no information about what it's connected to (in essence a "broken" DT).
In this case regulator_get() will return a dummy regulator.

2) The component has a supply pin that might or not might be connected
and depending on this the component needs to be configured differently.
In this case you use regulator_get_optional() which will return an error
in the event that no supply is specified.


With this in mind, supporting optional supplies in the bulk operations
makes less sense. (And after further review my case was a clear #1)

Regards,
Bjorn

Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Dmitry Torokhov]

On Mon, Feb 06, 2017 at 04:21:18PM -0800, Bjorn Andersson wrote:
> On Fri 03 Feb 15:16 PST 2017, Dmitry Torokhov wrote:
> 
> > Make it possible to use the bulk API with optional supplies, by allowing
> > the consumer to marking supplies as optional in the regulator_bulk_data.
> > 
> > Based on earlier patch by Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > 
> 
> Hi Dmitry,
> 
> Rather than fixing my broken patch, I reverted it after talking to Mark.
> This as I realized that I got things backwards.
> 
> When a given component has an "optional" supply this can mean two
> things:
> 1) The component has a supply pin that is connected, but the kernel has
> no information about what it's connected to (in essence a "broken" DT).
> In this case regulator_get() will return a dummy regulator.
> 
> 2) The component has a supply pin that might or not might be connected
> and depending on this the component needs to be configured differently.
> In this case you use regulator_get_optional() which will return an error
> in the event that no supply is specified.

Right, I'm after the case #2 and I believe there are cases, where, after
configuring the chip based on the regulators available, we can continue
handling (enabling and disabling) all regulators en-masse. It does
simplify the code in certain cases (tlv320aic32x4 and sgtl5000, and I am
sure other drivers, where configuration is "static"), but will not help
for others (like Mark's MMC example, where optional regulator is managed
actively - switching voltage, etc).

Thanks.

-- 
Dmitry