ASoC: rt5663: Fix error handling of regulator_set_load

Commit Message

Cheng-yi Chiang Nov. 16, 2018, 9:28 a.m. UTC

The default implementation of regulator_set_load returns
REGULATOR_MODE_NORMAL, which is positive.

rt5663_i2c_probe should only do error handling when return value of
regulator_set_load is negative.
In this case, rt5663_i2c_probe should return error.

Also, consolidate err_irq into err_enable.

Fix the missing goto for temporary regmap and rt5663->regmap.

Signed-off-by: Cheng-Yi Chiang <[e-mail]>
---
 This patch is the fixup of the patch
 e81a2a6d12e85 ASoC: rt5663: Add regulator support
 sound/soc/codecs/rt5663.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Mark Brown Nov. 17, 2018, 3:18 a.m. UTC | #1

On Fri, Nov 16, 2018 at 05:28:56PM +0800, Cheng-Yi Chiang wrote:
> The default implementation of regulator_set_load returns
> REGULATOR_MODE_NORMAL, which is positive.

The stub does indeed do that however that just looks like a bug - the
actual implementation returns 0 on success and the other users don't
seem to expect to get a mode value back.

> rt5663_i2c_probe should only do error handling when return value of
> regulator_set_load is negative.
> In this case, rt5663_i2c_probe should return error.

However that's also true so the patch is valid.

Patch

diff --git a/sound/soc/codecs/rt5663.c b/sound/soc/codecs/rt5663.c
index bb2c1ee4a836f..473a54d1ae2c5 100644
--- a/sound/soc/codecs/rt5663.c
+++ b/sound/soc/codecs/rt5663.c
@@ -3522,10 +3522,11 @@  static int rt5663_i2c_probe(struct i2c_client *i2c,
 	for (i = 0; i < ARRAY_SIZE(rt5663->supplies); i++) {
 		ret = regulator_set_load(rt5663->supplies[i].consumer,
 					 RT5663_SUPPLY_CURRENT_UA);
-		if (ret) {
+		if (ret < 0) {
 			dev_err(&i2c->dev,
-				"Failed to set regulator %s, ret: %d\n",
+				"Failed to set regulator load on %s, ret: %d\n",
 				rt5663->supplies[i].supply, ret);
+			return ret;
 		}
 	}
 
@@ -3543,7 +3544,7 @@  static int rt5663_i2c_probe(struct i2c_client *i2c,
 		ret = PTR_ERR(regmap);
 		dev_err(&i2c->dev, "Failed to allocate temp register map: %d\n",
 			ret);
-		return ret;
+		goto err_enable;
 	}
 
 	ret = regmap_read(regmap, RT5663_VENDOR_ID_2, &val);
@@ -3576,7 +3577,7 @@  static int rt5663_i2c_probe(struct i2c_client *i2c,
 		ret = PTR_ERR(rt5663->regmap);
 		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
 			ret);
-		return ret;
+		goto err_enable;
 	}
 
 	/* reset and calibrate */
@@ -3686,17 +3687,19 @@  static int rt5663_i2c_probe(struct i2c_client *i2c,
 			rt5663_dai, ARRAY_SIZE(rt5663_dai));
 
 	if (ret)
-		goto err_irq;
+		goto err_enable;
 
 	return 0;
 
-err_irq:
+
+	/*
+	 * Error after enabling regulators should goto err_enable
+	 * to disable regulators.
+	 */
+err_enable:
 	if (i2c->irq)
 		free_irq(i2c->irq, rt5663);
 
-err_enable:
-	dev_err(&i2c->dev,
-		"%s: Disable regulator after probe error\n", __func__);
 	regulator_bulk_disable(ARRAY_SIZE(rt5663->supplies), rt5663->supplies);
 	return ret;
 }