linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies
Date: Sat, 4 Feb 2017 10:13:18 -0800	[thread overview]
Message-ID: <20170204181318.GB12980@dtor-ws> (raw)
In-Reply-To: <20170204105614.ebbzeg7s3uhpgkrg@sirena.org.uk>

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)

  reply	other threads:[~2017-02-04 18:13 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-03 23:16 [PATCH 1/4] regulator: core: fix typo in regulator_bulk_disable() Dmitry Torokhov
2017-02-03 23:16 ` [PATCH 2/4] regulator: core: simplify regulator_bulk_force_disable() Dmitry Torokhov
2017-02-04 10:48   ` Applied "regulator: core: simplify regulator_bulk_force_disable()" to the regulator tree Mark Brown
2017-02-03 23:16 ` [PATCH 3/4] regulator: core: optimize devm_regulator_bulk_get() Dmitry Torokhov
2017-02-04 10:48   ` Applied "regulator: core: optimize devm_regulator_bulk_get()" to the regulator tree Mark Brown
2017-02-03 23:16 ` [PATCH 4/4] regulator: core: make bulk API support optional supplies Dmitry Torokhov
2017-02-04  7:53   ` kbuild test robot
2017-02-04 10:56   ` Mark Brown
2017-02-04 18:13     ` Dmitry Torokhov [this message]
2017-02-05 16:07       ` Mark Brown
2017-02-06  4:30         ` Dmitry Torokhov
2017-02-06 12:08           ` Mark Brown
2017-02-07  0:21   ` Bjorn Andersson
2017-02-07  0:47     ` Dmitry Torokhov
2017-02-04 10:47 ` Applied "regulator: core: fix typo in regulator_bulk_disable()" to the regulator tree Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170204181318.GB12980@dtor-ws \
    --to=dmitry.torokhov@gmail.com \
    --cc=broonie@kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).