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@xxxxxxxxx>

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

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
---
 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)