Message-ID: <20170206043033.GA1900@dtor-ws>
Date:   Sun, 5 Feb 2017 20:30:33 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] regulator: core: make bulk API support optional
 supplies

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@...il.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@...il.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;
 }

Powered by blists - more mailing lists