[PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Eric Nelson]

This patch set addresses a structural problem in the handling of regulators
for VDDIO, VDDA, and VDDD in the SGTL5000 driver.

The first two of these power rails must be powered on prior to any I2C
communication, and yet the regulators were tied to the codec, which is
instantiated only after a fair amount of I2C communication takes place.

In other words, these regulators could never have function, and we can
surmise that no user of this driver has switched power supply rails
connected to them.

The third power rail (VDDD) can be derived internally (by using I2C registers)
though the data sheet says that if an external VDDD is used, it should be
enabled before MCLK is started and I2C activity begins.

Patch 1 moves the regulators to the I2C device and initializes them before
	I2C activity.
Patch 2 addresses an issue found during development of the patch and is
        somewhat unrelated to regulators. It is included for discussion
Patch 3 removes the root cause of patch 2
Patch 4 adds return value checks when setting the LINREG and ANA_POWER
	registers
Patch 5 addresses the cause of follow-on failures and the source of the
	error in patch 3.
Patch 6 removes the naive use of the supplies to lower power consumption
	and replaces it with something more modest.

The patch set is being sent as an RFC for discussion, and should probably
be squashed into two or three patches covering the regulator updates
(patches 1, 3-5 and perhaps 6), initialization error handling (patch 2).

Eric Nelson (6):
  ASoC: sgtl5000: fix regulator support
  ASoC: sgtl5000: write all default registers
  ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults
  ASoC: sgtl5000: check return values
  ASoC: sgtl5000: disable internal PLL early
  ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF

 sound/soc/codecs/sgtl5000.c | 417 +++++++++++---------------------------------
 sound/soc/codecs/sgtl5000.h |   2 +
 2 files changed, 102 insertions(+), 317 deletions(-)

-- 
1.9.1

[RFC PATCH 1/6] ASoC: sgtl5000: fix regulator support

[Eric Nelson]

Regulator support on SGTL5000 is broken because the VDDIO and
VDDA and VDDD should be powered on before enabling MCLK as
shown in Figure 4 of [1]. This requires moving control of the
regulators from the codec block to the I2C block of the driver.

The bulk of this patch consists of swapping the codec device with
the i2c client. The new field num_supplies in struct sgtl5000_priv
is used instead of ARRAY_SIZE(supplies) to handle the special case
when the internal LDO is used instead of external VDDD.

Note that ER1 in the SGTL5000 Errata document [2] suggests that
all designs should use external VDDD.

Since the internal LDO can only be enabled after I2C is available,
there's no benefit in treating it as a regulator, so this patch
removes the regulator structure surrounding it.

Instead, a single block of code in sgtl5000_i2c_probe() performs
the initialization sequence in section 2.2.1.1 of [3] and page
26 of [1].

References:
[1] SGTL5000 data sheet
    http://cache.freescale.com/files/analog/doc/data_sheet/SGTL5000.pdf
[2] SGTL5000 errata
    http://cache.freescale.com/files/analog/doc/errata/SGTL5000ER.pdf
[3] SGTL5000 Initialization and programming app note
    http://cache.freescale.com/files/analog/doc/app_note/AN3663.pdf

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 343 ++++++++++----------------------------------
 1 file changed, 78 insertions(+), 265 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index e182e65..8f755e8 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -92,36 +92,8 @@ static const char *supply_names[SGTL5000_SUPPLY_NUM] = {
 	"VDDD"
 };
 
-#define LDO_CONSUMER_NAME	"VDDD_LDO"
 #define LDO_VOLTAGE		1200000
 
-static struct regulator_consumer_supply ldo_consumer[] = {
-	REGULATOR_SUPPLY(LDO_CONSUMER_NAME, NULL),
-};
-
-static struct regulator_init_data ldo_init_data = {
-	.constraints = {
-		.min_uV                 = 1200000,
-		.max_uV                 = 1200000,
-		.valid_modes_mask       = REGULATOR_MODE_NORMAL,
-		.valid_ops_mask         = REGULATOR_CHANGE_STATUS,
-	},
-	.num_consumer_supplies = 1,
-	.consumer_supplies = &ldo_consumer[0],
-};
-
-/*
- * sgtl5000 internal ldo regulator,
- * enabled when VDDD not provided
- */
-struct ldo_regulator {
-	struct regulator_desc desc;
-	struct regulator_dev *dev;
-	int voltage;
-	void *codec_data;
-	bool enabled;
-};
-
 enum sgtl5000_micbias_resistor {
 	SGTL5000_MICBIAS_OFF = 0,
 	SGTL5000_MICBIAS_2K = 2,
@@ -135,7 +107,7 @@ struct sgtl5000_priv {
 	int master;	/* i2s master or not */
 	int fmt;	/* i2s data format */
 	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
-	struct ldo_regulator *ldo;
+	int num_supplies;
 	struct regmap *regmap;
 	struct clk *mclk;
 	int revision;
@@ -778,155 +750,6 @@ static int sgtl5000_pcm_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-#ifdef CONFIG_REGULATOR
-static int ldo_regulator_is_enabled(struct regulator_dev *dev)
-{
-	struct ldo_regulator *ldo = rdev_get_drvdata(dev);
-
-	return ldo->enabled;
-}
-
-static int ldo_regulator_enable(struct regulator_dev *dev)
-{
-	struct ldo_regulator *ldo = rdev_get_drvdata(dev);
-	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
-	int reg;
-
-	if (ldo_regulator_is_enabled(dev))
-		return 0;
-
-	/* set regulator value firstly */
-	reg = (1600 - ldo->voltage / 1000) / 50;
-	reg = clamp(reg, 0x0, 0xf);
-
-	/* amend the voltage value, unit: uV */
-	ldo->voltage = (1600 - reg * 50) * 1000;
-
-	/* set voltage to register */
-	snd_soc_update_bits(codec, SGTL5000_CHIP_LINREG_CTRL,
-				SGTL5000_LINREG_VDDD_MASK, reg);
-
-	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_LINEREG_D_POWERUP,
-				SGTL5000_LINEREG_D_POWERUP);
-
-	/* when internal ldo is enabled, simple digital power can be disabled */
-	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_LINREG_SIMPLE_POWERUP,
-				0);
-
-	ldo->enabled = 1;
-	return 0;
-}
-
-static int ldo_regulator_disable(struct regulator_dev *dev)
-{
-	struct ldo_regulator *ldo = rdev_get_drvdata(dev);
-	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
-
-	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_LINEREG_D_POWERUP,
-				0);
-
-	/* clear voltage info */
-	snd_soc_update_bits(codec, SGTL5000_CHIP_LINREG_CTRL,
-				SGTL5000_LINREG_VDDD_MASK, 0);
-
-	ldo->enabled = 0;
-
-	return 0;
-}
-
-static int ldo_regulator_get_voltage(struct regulator_dev *dev)
-{
-	struct ldo_regulator *ldo = rdev_get_drvdata(dev);
-
-	return ldo->voltage;
-}
-
-static struct regulator_ops ldo_regulator_ops = {
-	.is_enabled = ldo_regulator_is_enabled,
-	.enable = ldo_regulator_enable,
-	.disable = ldo_regulator_disable,
-	.get_voltage = ldo_regulator_get_voltage,
-};
-
-static int ldo_regulator_register(struct snd_soc_codec *codec,
-				struct regulator_init_data *init_data,
-				int voltage)
-{
-	struct ldo_regulator *ldo;
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
-	struct regulator_config config = { };
-
-	ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
-
-	if (!ldo)
-		return -ENOMEM;
-
-	ldo->desc.name = kstrdup(dev_name(codec->dev), GFP_KERNEL);
-	if (!ldo->desc.name) {
-		kfree(ldo);
-		dev_err(codec->dev, "failed to allocate decs name memory\n");
-		return -ENOMEM;
-	}
-
-	ldo->desc.type  = REGULATOR_VOLTAGE;
-	ldo->desc.owner = THIS_MODULE;
-	ldo->desc.ops   = &ldo_regulator_ops;
-	ldo->desc.n_voltages = 1;
-
-	ldo->codec_data = codec;
-	ldo->voltage = voltage;
-
-	config.dev = codec->dev;
-	config.driver_data = ldo;
-	config.init_data = init_data;
-
-	ldo->dev = regulator_register(&ldo->desc, &config);
-	if (IS_ERR(ldo->dev)) {
-		int ret = PTR_ERR(ldo->dev);
-
-		dev_err(codec->dev, "failed to register regulator\n");
-		kfree(ldo->desc.name);
-		kfree(ldo);
-
-		return ret;
-	}
-	sgtl5000->ldo = ldo;
-
-	return 0;
-}
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
-	struct ldo_regulator *ldo = sgtl5000->ldo;
-
-	if (!ldo)
-		return 0;
-
-	regulator_unregister(ldo->dev);
-	kfree(ldo->desc.name);
-	kfree(ldo);
-
-	return 0;
-}
-#else
-static int ldo_regulator_register(struct snd_soc_codec *codec,
-				struct regulator_init_data *init_data,
-				int voltage)
-{
-	dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
-	return -EINVAL;
-}
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
-	return 0;
-}
-#endif
-
 /*
  * set dac bias
  * common state changes:
@@ -950,7 +773,7 @@ static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
 	case SND_SOC_BIAS_STANDBY:
 		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
 			ret = regulator_bulk_enable(
-						ARRAY_SIZE(sgtl5000->supplies),
+						sgtl5000->num_supplies,
 						sgtl5000->supplies);
 			if (ret)
 				return ret;
@@ -964,7 +787,7 @@ static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
 					"Failed to restore cache: %d\n", ret);
 
 				regcache_cache_only(sgtl5000->regmap, true);
-				regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
+				regulator_bulk_disable(sgtl5000->num_supplies,
 						       sgtl5000->supplies);
 
 				return ret;
@@ -974,8 +797,8 @@ static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
 		break;
 	case SND_SOC_BIAS_OFF:
 		regcache_cache_only(sgtl5000->regmap, true);
-		regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
-					sgtl5000->supplies);
+		regulator_bulk_disable(sgtl5000->num_supplies,
+				       sgtl5000->supplies);
 		break;
 	}
 
@@ -1115,7 +938,9 @@ 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  = regulator_get_voltage(sgtl5000->supplies[VDDD].consumer);
+	vddd  = (sgtl5000->num_supplies > VDDD)
+		? regulator_get_voltage(sgtl5000->supplies[VDDD].consumer)
+		: LDO_VOLTAGE;
 
 	vdda  = vdda / 1000;
 	vddio = vddio / 1000;
@@ -1224,78 +1049,43 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 	return 0;
 }
 
-static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec)
-{
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
-	int ret;
-
-	/* set internal ldo to 1.2v */
-	ret = ldo_regulator_register(codec, &ldo_init_data, LDO_VOLTAGE);
-	if (ret) {
-		dev_err(codec->dev,
-			"Failed to register vddd internal supplies: %d\n", ret);
-		return ret;
-	}
-
-	sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
-
-	dev_info(codec->dev, "Using internal LDO instead of VDDD\n");
-	return 0;
-}
-
-static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
+static int sgtl5000_enable_regulators(struct i2c_client *client)
 {
 	int ret;
 	int i;
 	int external_vddd = 0;
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
 	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];
 
-	/* External VDDD only works before revision 0x11 */
-	if (sgtl5000->revision < 0x11) {
-		vddd = regulator_get_optional(codec->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);
-		}
-	}
-
-	if (!external_vddd) {
-		ret = sgtl5000_replace_vddd_with_ldo(codec);
-		if (ret)
-			return ret;
+	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);
 	}
 
-	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
+	sgtl5000->num_supplies = ARRAY_SIZE(sgtl5000->supplies)
+				 - 1 + external_vddd;
+	ret = regulator_bulk_get(&client->dev, sgtl5000->num_supplies,
 				 sgtl5000->supplies);
 	if (ret)
-		goto err_ldo_remove;
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
-					sgtl5000->supplies);
-	if (ret)
-		goto err_regulator_free;
-
-	/* wait for all power rails bring up */
-	udelay(10);
+		return ret;
 
-	return 0;
+	ret = regulator_bulk_enable(sgtl5000->num_supplies,
+				    sgtl5000->supplies);
+	if (!ret)
+		usleep_range(10, 20);
+	else
+		regulator_bulk_free(sgtl5000->num_supplies,
+				    sgtl5000->supplies);
 
-err_regulator_free:
-	regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),
-				sgtl5000->supplies);
-err_ldo_remove:
-	if (!external_vddd)
-		ldo_regulator_remove(codec);
 	return ret;
-
 }
 
 static int sgtl5000_probe(struct snd_soc_codec *codec)
@@ -1303,10 +1093,6 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
 	int ret;
 	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
 
-	ret = sgtl5000_enable_regulators(codec);
-	if (ret)
-		return ret;
-
 	/* power up sgtl5000 */
 	ret = sgtl5000_set_power_regs(codec);
 	if (ret)
@@ -1357,25 +1143,11 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
 	return 0;
 
 err:
-	regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
-						sgtl5000->supplies);
-	regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),
-				sgtl5000->supplies);
-	ldo_regulator_remove(codec);
-
 	return ret;
 }
 
 static int sgtl5000_remove(struct snd_soc_codec *codec)
 {
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
-
-	regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
-						sgtl5000->supplies);
-	regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),
-				sgtl5000->supplies);
-	ldo_regulator_remove(codec);
-
 	return 0;
 }
 
@@ -1443,11 +1215,17 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (!sgtl5000)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, sgtl5000);
+
+	ret = sgtl5000_enable_regulators(client);
+	if (ret)
+		return ret;
+
 	sgtl5000->regmap = devm_regmap_init_i2c(client, &sgtl5000_regmap);
 	if (IS_ERR(sgtl5000->regmap)) {
 		ret = PTR_ERR(sgtl5000->regmap);
 		dev_err(&client->dev, "Failed to allocate regmap: %d\n", ret);
-		return ret;
+		goto disable_regs;
 	}
 
 	sgtl5000->mclk = devm_clk_get(&client->dev, NULL);
@@ -1456,21 +1234,25 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 		dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
 		/* Defer the probe to see if the clk will be provided later */
 		if (ret == -ENOENT)
-			return -EPROBE_DEFER;
-		return ret;
+			ret = -EPROBE_DEFER;
+		goto disable_regs;
 	}
 
 	ret = clk_prepare_enable(sgtl5000->mclk);
-	if (ret)
-		return ret;
+	if (ret) {
+		dev_err(&client->dev, "Error  enabling clock %d\n", ret);
+		goto disable_regs;
+	}
 
 	/* Need 8 clocks before I2C accesses */
 	udelay(1);
 
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Error reading chip id %d\n", ret);
 		goto disable_clk;
+	}
 
 	if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=
 	    SGTL5000_PARTID_PART_ID) {
@@ -1484,6 +1266,31 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	dev_info(&client->dev, "sgtl5000 revision 0x%x\n", rev);
 	sgtl5000->revision = rev;
 
+	/* Follow section 2.2.1.1 of AN3663 */
+	if (sgtl5000->num_supplies <= VDDD) {
+		/* internal VDDD at 1.2V */
+		regmap_update_bits(sgtl5000->regmap,
+				   SGTL5000_CHIP_LINREG_CTRL,
+				   SGTL5000_LINREG_VDDD_MASK, 8);
+		regmap_update_bits(sgtl5000->regmap,
+				   SGTL5000_CHIP_ANA_POWER,
+				   SGTL5000_LINEREG_D_POWERUP
+				   | SGTL5000_LINREG_SIMPLE_POWERUP,
+				   SGTL5000_LINEREG_D_POWERUP);
+		dev_info(&client->dev, "Using internal LDO instead of VDDD: check ER1\n");
+	} else {
+		/* using external LDO for VDDD
+		 * Clear startup powerup and simple powerup
+		 * bits to save power
+		 */
+		regmap_update_bits(sgtl5000->regmap,
+				   SGTL5000_CHIP_ANA_POWER,
+				   SGTL5000_STARTUP_POWERUP
+				   | SGTL5000_LINREG_SIMPLE_POWERUP,
+				   0);
+		dev_dbg(&client->dev, "Using external VDDD\n");
+	}
+
 	if (np) {
 		if (!of_property_read_u32(np,
 			"micbias-resistor-k-ohms", &value)) {
@@ -1525,8 +1332,6 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 		}
 	}
 
-	i2c_set_clientdata(client, sgtl5000);
-
 	/* Ensure sgtl5000 will start with sane register values */
 	ret = sgtl5000_fill_defaults(sgtl5000);
 	if (ret)
@@ -1541,6 +1346,11 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 
 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;
 }
 
@@ -1550,6 +1360,9 @@ static int sgtl5000_i2c_remove(struct i2c_client *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;
 }
 
-- 
1.9.1

Re: [RFC PATCH 1/6] ASoC: sgtl5000: fix regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2591 bytes --]

On Thu, Feb 26, 2015 at 03:54:28PM -0700, Eric Nelson wrote:

> Since the internal LDO can only be enabled after I2C is available,
> there's no benefit in treating it as a regulator, so this patch
> removes the regulator structure surrounding it.

The expected benefit would be that it is possible to then have the code
that uses the regulator be constant:

>  	case SND_SOC_BIAS_STANDBY:
>  		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
>  			ret = regulator_bulk_enable(
> -						ARRAY_SIZE(sgtl5000->supplies),
> +						sgtl5000->num_supplies,
>  						sgtl5000->supplies);

so we avoid stuff like this.

> @@ -1115,7 +938,9 @@ 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  = regulator_get_voltage(sgtl5000->supplies[VDDD].consumer);
> +	vddd  = (sgtl5000->num_supplies > VDDD)
> +		? regulator_get_voltage(sgtl5000->supplies[VDDD].consumer)
> +		: LDO_VOLTAGE;

Please write a normal if statement, this is not legible (and the test is
more than a little obscure).

> -	/* External VDDD only works before revision 0x11 */
> -	if (sgtl5000->revision < 0x11) {
> -		vddd = regulator_get_optional(codec->dev, "VDDD");

It'd be good to keep at least a warning about this (not that there's one
now but it's a good idea).

> -	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
> +	sgtl5000->num_supplies = ARRAY_SIZE(sgtl5000->supplies)
> +				 - 1 + external_vddd;

This is a bit obscure, why not just do this sooner (with a comment for
the - 1) and increment num_supplies when we add the external regulator.
A comment on the supply list saying that VDDD must be last would be good
too.

> +	ret = regulator_bulk_enable(sgtl5000->num_supplies,
> +				    sgtl5000->supplies);
> +	if (!ret)
> +		usleep_range(10, 20);

This is a separate change...

> +	else
> +		regulator_bulk_free(sgtl5000->num_supplies,
> +				    sgtl5000->supplies);

Convert to using devm_ since you're doing all this stuff.


>  	ret = clk_prepare_enable(sgtl5000->mclk);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		dev_err(&client->dev, "Error  enabling clock %d\n", ret);
> +		goto disable_regs;
> +	}

This is a separate fix as far as I can tell?

> +	/* Follow section 2.2.1.1 of AN3663 */
> +	if (sgtl5000->num_supplies <= VDDD) {
> +		/* internal VDDD at 1.2V */

These checks are just far too obscure, please set a flag or something.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 1/6] ASoC: sgtl5000: fix regulator support

[Eric Nelson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark,

Thanks for the review.

On 03/06/2015 01:04 PM, Mark Brown wrote:
> On Thu, Feb 26, 2015 at 03:54:28PM -0700, Eric Nelson wrote:
> 
>> Since the internal LDO can only be enabled after I2C is
>> available, there's no benefit in treating it as a regulator, so
>> this patch removes the regulator structure surrounding it.
> 
> The expected benefit would be that it is possible to then have the
> code that uses the regulator be constant:
> 

That's a nice plan, but doesn't fit, since the internal regulator
requires I2C access and can only be used after the rest of the
power-up sequence has completed.

>> case SND_SOC_BIAS_STANDBY: if (codec->dapm.bias_level ==
>> SND_SOC_BIAS_OFF) { ret = regulator_bulk_enable( -
>> ARRAY_SIZE(sgtl5000->supplies), +						sgtl5000->num_supplies, 
>> sgtl5000->supplies);
> 
> so we avoid stuff like this.
> 

I understand the intent, but that doesn't work. If the internal LDO
is wrapped in a regulator and placed here, the sequence needs to be:
	enable VDDIO and VDDA regulators
	re-enable the clock
	wait 8 cycles
	enable the LDO for VDDD

>> @@ -1115,7 +938,9 @@ 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
>> = regulator_get_voltage(sgtl5000->supplies[VDDD].consumer); +
>> vddd  = (sgtl5000->num_supplies > VDDD) +		?
>> regulator_get_voltage(sgtl5000->supplies[VDDD].consumer) +		:
>> LDO_VOLTAGE;
> 
> Please write a normal if statement, this is not legible (and the
> test is more than a little obscure).
> 

Will do (in a V2).

>> -	/* External VDDD only works before revision 0x11 */ -	if
>> (sgtl5000->revision < 0x11) { -		vddd =
>> regulator_get_optional(codec->dev, "VDDD");
> 
> It'd be good to keep at least a warning about this (not that
> there's one now but it's a good idea).
> 

I haven't been able to find the origin of this test, but it's in
conflict with ER1.

>> -	ret = regulator_bulk_get(codec->dev,
>> ARRAY_SIZE(sgtl5000->supplies), +	sgtl5000->num_supplies =
>> ARRAY_SIZE(sgtl5000->supplies) +				 - 1 + external_vddd;
> 
> This is a bit obscure, why not just do this sooner (with a comment
> for the - 1) and increment num_supplies when we add the external
> regulator. A comment on the supply list saying that VDDD must be
> last would be good too.
> 

Agreed. I'll rework it.

>> +	ret = regulator_bulk_enable(sgtl5000->num_supplies, +
>> sgtl5000->supplies); +	if (!ret) +		usleep_range(10, 20);
> 
> This is a separate change...
> 

I changed udelay() to usleep_range() in order to keep
checkpatch happy.

>> +	else +		regulator_bulk_free(sgtl5000->num_supplies, +
>> sgtl5000->supplies);
> 
> Convert to using devm_ since you're doing all this stuff.
> 
> 

Will do.

>> ret = clk_prepare_enable(sgtl5000->mclk); -	if (ret) -		return
>> ret; +	if (ret) { +		dev_err(&client->dev, "Error  enabling clock
>> %d\n", ret); +		goto disable_regs; +	}
> 
> This is a separate fix as far as I can tell?
> 

Not really. Once regulators are moved into the I2C device, we
can't just return because that would leave the regulators enabled.

>> +	/* Follow section 2.2.1.1 of AN3663 */ +	if
>> (sgtl5000->num_supplies <= VDDD) { +		/* internal VDDD at 1.2V
>> */
> 
> These checks are just far too obscure, please set a flag or
> something.
> 

Got it. I'll fix this in V2 (not RFC).

Regards,


Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU+heAAAoJEFUqXmm9AiVrx+0H/3juYAWgD0su7nwPLvw8nhPH
8rGVc/laHcSTC/GBAjomkhmGWTL3Kb/gJo38hEo3GNeq9E6rlqPjP5qQMorcqVSo
agy+GGaPZ6W8AvRoEqyG3r/pN/MrwmT8JtkmMp3iBlPtB9cDfksFmeych697jPuU
gWj9xSuWyrnGcAZPJMrI2xJDfPHQ2/TStNhnR3bvVIAqfdfS8MKbqeUwQJ12gLhM
GeEMTfHwx2P5IBPDucieJtMnl+pX4v+s/IoJdU8gIz1IC4k9jz9OQIFxYocLWcpe
m6DhlTY0gMUdfnMIQ+WnDSk6dzNINglvFUqNsm0sMyXI+2MKZGFD+Dx33qOQX24=
=1he2
-----END PGP SIGNATURE-----

Re: [RFC PATCH 1/6] ASoC: sgtl5000: fix regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1252 bytes --]

On Fri, Mar 06, 2015 at 02:09:20PM -0700, Eric Nelson wrote:
> On 03/06/2015 01:04 PM, Mark Brown wrote:

> >> case SND_SOC_BIAS_STANDBY: if (codec->dapm.bias_level ==
> >> SND_SOC_BIAS_OFF) { ret = regulator_bulk_enable( -
> >> ARRAY_SIZE(sgtl5000->supplies), +						sgtl5000->num_supplies, 
> >> sgtl5000->supplies);

> > so we avoid stuff like this.

> I understand the intent, but that doesn't work. If the internal LDO
> is wrapped in a regulator and placed here, the sequence needs to be:
> 	enable VDDIO and VDDA regulators
> 	re-enable the clock
> 	wait 8 cycles
> 	enable the LDO for VDDD

So make the changelog actually say that - I'm commenting on the fact
that your changelog appears to misunderstand the current code.

> >> -	/* External VDDD only works before revision 0x11 */ -	if
> >> (sgtl5000->revision < 0x11) { -		vddd =
> >> regulator_get_optional(codec->dev, "VDDD");

> > It'd be good to keep at least a warning about this (not that
> > there's one now but it's a good idea).

> I haven't been able to find the origin of this test, but it's in
> conflict with ER1.

My reading of the situation is that early silicon had one set of bugs,
current silicon fixed those but introduced a different set.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[RFC PATCH 2/6] ASoC: sgtl5000: write all default registers

[Eric Nelson]

If an error occurs writing defaults, produce an error message but
continue writing other registers. The failure of a single write should
not cause catastrophic device failure.

In at least one occurrence, I2C writes of CHIP_ANA_POWER was nacked,
though continuing allowed the device to operate properly.

A follow-up patch will remove CHIP_ANA_POWER from the set of default
registers.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 8f755e8..2ac4db5 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1188,8 +1188,9 @@ static const struct regmap_config sgtl5000_regmap = {
  * and avoid problems like, not being able to probe after an audio playback
  * followed by a system reset or a 'reboot' command in Linux
  */
-static int sgtl5000_fill_defaults(struct sgtl5000_priv *sgtl5000)
+static void sgtl5000_fill_defaults(struct i2c_client *client)
 {
+	struct sgtl5000_priv *sgtl5000 = i2c_get_clientdata(client);
 	int i, ret, val, index;
 
 	for (i = 0; i < ARRAY_SIZE(sgtl5000_reg_defaults); i++) {
@@ -1197,10 +1198,10 @@ static int sgtl5000_fill_defaults(struct sgtl5000_priv *sgtl5000)
 		index = sgtl5000_reg_defaults[i].reg;
 		ret = regmap_write(sgtl5000->regmap, index, val);
 		if (ret)
-			return ret;
+			dev_err(&client->dev,
+				"%s: error %d setting reg 0x%02x to 0x%04x\n",
+				__func__, ret, index, val);
 	}
-
-	return 0;
 }
 
 static int sgtl5000_i2c_probe(struct i2c_client *client,
@@ -1333,9 +1334,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	}
 
 	/* Ensure sgtl5000 will start with sane register values */
-	ret = sgtl5000_fill_defaults(sgtl5000);
-	if (ret)
-		goto disable_clk;
+	sgtl5000_fill_defaults(client);
 
 	ret = snd_soc_register_codec(&client->dev,
 			&sgtl5000_driver, &sgtl5000_dai, 1);
-- 
1.9.1

Re: [RFC PATCH 2/6] ASoC: sgtl5000: write all default registers

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]

On Thu, Feb 26, 2015 at 03:54:29PM -0700, Eric Nelson wrote:
> If an error occurs writing defaults, produce an error message but
> continue writing other registers. The failure of a single write should
> not cause catastrophic device failure.
> 
> In at least one occurrence, I2C writes of CHIP_ANA_POWER was nacked,
> though continuing allowed the device to operate properly.

I'm a bit nervous about this.  Generally problems physically writing to
the device are pretty bad and lead to followon hard to debug problems as
the chip state drifts from the state the software thinks it has.  If
this is a good idea I'd really like to see more analysis as to why this
is expected to happen and why it's OK.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 2/6] ASoC: sgtl5000: write all default registers

[Eric Nelson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark,

On 03/06/2015 01:14 PM, Mark Brown wrote:
> On Thu, Feb 26, 2015 at 03:54:29PM -0700, Eric Nelson wrote:
>> If an error occurs writing defaults, produce an error message
>> but continue writing other registers. The failure of a single
>> write should not cause catastrophic device failure.
>> 
>> In at least one occurrence, I2C writes of CHIP_ANA_POWER was
>> nacked, though continuing allowed the device to operate
>> properly.
> 
> I'm a bit nervous about this.  Generally problems physically
> writing to the device are pretty bad and lead to followon hard to
> debug problems as the chip state drifts from the state the software
> thinks it has.  If this is a good idea I'd really like to see more
> analysis as to why this is expected to happen and why it's OK.
> 

I included this patch in the set because I found that the driver
would fail through a soft re-boot until I found the dependencies
of CHIP_CLK_CTRL and LINREG_CTRL in setting the initial value for
ANA_POWER.

By ignoring the failed register write, I found that I had functioning
audio.

Which begs the question: should we force complete failure on what may
be an intermittent problem.

My preference is no.

This is really un-related to the rest of the patch set.

Regards,


Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU+ik1AAoJEFUqXmm9AiVrRxYIAKvfJooQbhFu0um3bcX2e4sj
0AcBhn8P1brHv9gGkq68iEpQUA7E4nzSeOlmjlE6wLiC2tzHPdRsUFfHpAtrIMk5
CSdw3Qs9ti7jZkj0bRPVo0yINO7PxtCujvClfXETPNuyn78xy9lFK5PTX1sjZEso
Apeiz7smduItcFfBAGlcARK29b86cqJApKGNkzLL6J+NpmQptvrl3ijgohEPMB1/
4ubyt4K/0dCGON1UqFyMr5LKAlTjG2ctMM+10O7b2JpULwQqUpXSrobvT1syPSKZ
haa16ir8AYD5IWYMsUy/vWKDTrN2Vj+7pYdrpnW3ermVyqnSY7B0jx73REnjbbY=
=gk5O
-----END PGP SIGNATURE-----

Applied "ASoC: sgtl5000: Write all default registers" to the asoc tree

[Mark Brown]

The patch

   ASoC: sgtl5000: Write all default registers

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.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 f219b16959ee3df2fd49f09493b3f6b28487c416 Mon Sep 17 00:00:00 2001
From: Eric Nelson <eric@nelint.com>
Date: Tue, 7 Jun 2016 01:14:49 +0200
Subject: [PATCH] ASoC: sgtl5000: Write all default registers

If an error occurs writing defaults, produce an error message but
continue writing other registers. The failure of a single write should
not cause catastrophic device failure.

In at least one occurrence, I2C writes of CHIP_ANA_POWER were nacked,
though continuing allowed the device to operate properly.

Signed-off-by: Eric Nelson <eric@nelint.com>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/sgtl5000.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 77bdd1daa322..56d61a212083 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1219,8 +1219,9 @@ static const struct regmap_config sgtl5000_regmap = {
  * and avoid problems like, not being able to probe after an audio playback
  * followed by a system reset or a 'reboot' command in Linux
  */
-static int sgtl5000_fill_defaults(struct sgtl5000_priv *sgtl5000)
+static void sgtl5000_fill_defaults(struct i2c_client *client)
 {
+	struct sgtl5000_priv *sgtl5000 = i2c_get_clientdata(client);
 	int i, ret, val, index;
 
 	for (i = 0; i < ARRAY_SIZE(sgtl5000_reg_defaults); i++) {
@@ -1228,10 +1229,10 @@ static int sgtl5000_fill_defaults(struct sgtl5000_priv *sgtl5000)
 		index = sgtl5000_reg_defaults[i].reg;
 		ret = regmap_write(sgtl5000->regmap, index, val);
 		if (ret)
-			return ret;
+			dev_err(&client->dev,
+				"%s: error %d setting reg 0x%02x to 0x%04x\n",
+				__func__, ret, index, val);
 	}
-
-	return 0;
 }
 
 static int sgtl5000_i2c_probe(struct i2c_client *client,
@@ -1364,9 +1365,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	}
 
 	/* Ensure sgtl5000 will start with sane register values */
-	ret = sgtl5000_fill_defaults(sgtl5000);
-	if (ret)
-		goto disable_clk;
+	sgtl5000_fill_defaults(client);
 
 	ret = snd_soc_register_codec(&client->dev,
 			&sgtl5000_driver, &sgtl5000_dai, 1);
-- 
2.8.1

[RFC PATCH 3/6] ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults

[Eric Nelson]

Initialize CHIP_ANA_POWER to match power on defaults, which disables
ADC, DAC, and charge pumps.

In the process, remove references to the following register/bitfields
from the sgttl5000_set_power_regs routine:
	CHIP_ANA_POWER/LINREG_SIMPLE_POWERUP and
	CHIP_LINREG_CTRL/LINREG_VDD_MASK

And remove CHIP_ANA_POWER and CHIP_LINREG_CTRL from the set of default
registers so they don't get clobbered by sgtl5000_fill_defaults().

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 41 +++++++++--------------------------------
 sound/soc/codecs/sgtl5000.h |  1 +
 2 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 2ac4db5..0a7b96d7 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -47,12 +47,10 @@ static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_CHIP_ANA_ADC_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_ANA_HP_CTRL,		0x1818 },
 	{ SGTL5000_CHIP_ANA_CTRL,		0x0111 },
-	{ SGTL5000_CHIP_LINREG_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_REF_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_MIC_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_LINE_OUT_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_LINE_OUT_VOL,		0x0404 },
-	{ SGTL5000_CHIP_ANA_POWER,		0x7060 },
 	{ SGTL5000_CHIP_PLL_CTRL,		0x5000 },
 	{ SGTL5000_CHIP_CLK_TOP_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_ANA_STATUS,		0x0000 },
@@ -93,6 +91,7 @@ static const char *supply_names[SGTL5000_SUPPLY_NUM] = {
 };
 
 #define LDO_VOLTAGE		1200000
+#define LINREG_VDDD	((1600 - LDO_VOLTAGE / 1000) / 50)
 
 enum sgtl5000_micbias_resistor {
 	SGTL5000_MICBIAS_OFF = 0,
@@ -993,25 +992,6 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 
 	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, ana_pwr);
 
-	/* set voltage to register */
-	snd_soc_update_bits(codec, SGTL5000_CHIP_LINREG_CTRL,
-				SGTL5000_LINREG_VDDD_MASK, 0x8);
-
-	/*
-	 * if vddd linear reg has been enabled,
-	 * simple digital supply should be clear to get
-	 * proper VDDD voltage.
-	 */
-	if (ana_pwr & SGTL5000_LINEREG_D_POWERUP)
-		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_LINREG_SIMPLE_POWERUP,
-				0);
-	else
-		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_LINREG_SIMPLE_POWERUP |
-				SGTL5000_STARTUP_POWERUP,
-				0);
-
 	/*
 	 * set ADC/DAC VAG to vdda / 2,
 	 * should stay in range (0.8v, 1.575v)
@@ -1211,6 +1191,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	int ret, reg, rev;
 	struct device_node *np = client->dev.of_node;
 	u32 value;
+	u16 ana_pwr;
 
 	sgtl5000 = devm_kzalloc(&client->dev, sizeof(*sgtl5000), GFP_KERNEL);
 	if (!sgtl5000)
@@ -1268,29 +1249,25 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	sgtl5000->revision = rev;
 
 	/* Follow section 2.2.1.1 of AN3663 */
+	ana_pwr = SGTL5000_ANA_POWER_DEFAULT;
 	if (sgtl5000->num_supplies <= VDDD) {
 		/* internal VDDD at 1.2V */
 		regmap_update_bits(sgtl5000->regmap,
 				   SGTL5000_CHIP_LINREG_CTRL,
-				   SGTL5000_LINREG_VDDD_MASK, 8);
-		regmap_update_bits(sgtl5000->regmap,
-				   SGTL5000_CHIP_ANA_POWER,
-				   SGTL5000_LINEREG_D_POWERUP
-				   | SGTL5000_LINREG_SIMPLE_POWERUP,
-				   SGTL5000_LINEREG_D_POWERUP);
+				   SGTL5000_LINREG_VDDD_MASK,
+				   LINREG_VDDD);
+		ana_pwr |= SGTL5000_LINEREG_D_POWERUP;
 		dev_info(&client->dev, "Using internal LDO instead of VDDD: check ER1\n");
 	} else {
 		/* using external LDO for VDDD
 		 * Clear startup powerup and simple powerup
 		 * bits to save power
 		 */
-		regmap_update_bits(sgtl5000->regmap,
-				   SGTL5000_CHIP_ANA_POWER,
-				   SGTL5000_STARTUP_POWERUP
-				   | SGTL5000_LINREG_SIMPLE_POWERUP,
-				   0);
+		ana_pwr &= ~(SGTL5000_STARTUP_POWERUP
+			     | SGTL5000_LINREG_SIMPLE_POWERUP);
 		dev_dbg(&client->dev, "Using external VDDD\n");
 	}
+	regmap_write(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER, ana_pwr);
 
 	if (np) {
 		if (!of_property_read_u32(np,
diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
index bd7a344..6fdc589 100644
--- a/sound/soc/codecs/sgtl5000.h
+++ b/sound/soc/codecs/sgtl5000.h
@@ -325,6 +325,7 @@
 /*
  * SGTL5000_CHIP_ANA_POWER
  */
+#define SGTL5000_ANA_POWER_DEFAULT		0x7060
 #define SGTL5000_DAC_STEREO			0x4000
 #define SGTL5000_LINREG_SIMPLE_POWERUP		0x2000
 #define SGTL5000_STARTUP_POWERUP		0x1000
-- 
1.9.1

Re: [RFC PATCH 3/6] ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --]

On Thu, Feb 26, 2015 at 03:54:30PM -0700, Eric Nelson wrote:
> Initialize CHIP_ANA_POWER to match power on defaults, which disables
> ADC, DAC, and charge pumps.

> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -47,12 +47,10 @@ static const struct reg_default sgtl5000_reg_defaults[] = {
>  	{ SGTL5000_CHIP_ANA_ADC_CTRL,		0x0000 },
>  	{ SGTL5000_CHIP_ANA_HP_CTRL,		0x1818 },
>  	{ SGTL5000_CHIP_ANA_CTRL,		0x0111 },
> -	{ SGTL5000_CHIP_LINREG_CTRL,		0x0000 },
>  	{ SGTL5000_CHIP_REF_CTRL,		0x0000 },
>  	{ SGTL5000_CHIP_MIC_CTRL,		0x0000 },
>  	{ SGTL5000_CHIP_LINE_OUT_CTRL,		0x0000 },
>  	{ SGTL5000_CHIP_LINE_OUT_VOL,		0x0404 },
> -	{ SGTL5000_CHIP_ANA_POWER,		0x7060 },

Two big problems here.  One is that this appears to also affect
LINREG_CTRL which your changelog didn't mention.  The other is that
contrary to what the changelog says this isn't fixing the default, it's
removing it.  The whole point of the register defaults table is to
contain the default power on register values, if it contains other
things that is a bug and should be fixed by changing the values not
removing them.

Given that confusion I'm really having a hard time understanding the
rest of the commit.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 3/6] ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults

[Eric Nelson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark,

On 03/06/2015 01:12 PM, Mark Brown wrote:
> On Thu, Feb 26, 2015 at 03:54:30PM -0700, Eric Nelson wrote:
>> Initialize CHIP_ANA_POWER to match power on defaults, which
>> disables ADC, DAC, and charge pumps.
> 
>> +++ b/sound/soc/codecs/sgtl5000.c @@ -47,12 +47,10 @@ static
>> const struct reg_default sgtl5000_reg_defaults[] = { {
>> SGTL5000_CHIP_ANA_ADC_CTRL,		0x0000 }, {
>> SGTL5000_CHIP_ANA_HP_CTRL,		0x1818 }, { SGTL5000_CHIP_ANA_CTRL,
>> 0x0111 }, -	{ SGTL5000_CHIP_LINREG_CTRL,		0x0000 }, {
>> SGTL5000_CHIP_REF_CTRL,		0x0000 }, { SGTL5000_CHIP_MIC_CTRL,
>> 0x0000 }, { SGTL5000_CHIP_LINE_OUT_CTRL,		0x0000 }, {
>> SGTL5000_CHIP_LINE_OUT_VOL,		0x0404 }, -	{
>> SGTL5000_CHIP_ANA_POWER,		0x7060 },
> 
> Two big problems here.  One is that this appears to also affect 
> LINREG_CTRL which your changelog didn't mention.

It did mention the change:

	> Initialize CHIP_ANA_POWER to match power on defaults, which
	> disables ADC, DAC, and charge pumps.
	>
	> In the process, remove references to the following
	> register/bitfields from the sgttl5000_set_power_regs routine:
	> 	CHIP_ANA_POWER/LINREG_SIMPLE_POWERUP and
	> 	CHIP_LINREG_CTRL/LINREG_VDD_MASK
	>
	> And remove CHIP_ANA_POWER and CHIP_LINREG_CTRL from the set
	> of default registers so they don't get clobbered by
	> sgtl5000_fill_defaults().

The CHIP_LINREG_CTRL value needs to be set based on the presence
or absence of external VDDD, so writing a default (power-on) value
doesn't help much, and this certainly shouldn't happen after
the proper value is written.

> The other is that contrary to what the changelog says this isn't 
> fixing the default, it's removing it.
> 

You're right about the commit message. It should be re-worded.

> The whole point of the register defaults table is to contain the 
> default power on register values, if it contains other things that 
> is a bug and should be fixed by changing the values not removing
> them.
> 

I understand that, but the crux of the problem is that these
registers need to be set early, their order is critical, and
some of them need to be written based on the internal/external
VDDD decision.

In a soft reboot on a machine that doesn't actually control
the power to the SGTL5000 (which is all supported boards at
the moment), a write to the ANA_POWER register with stale
values in either LINREG_CTRL or CLK_CTRL (from patch 5)
will fail and cause the chip to lock up.

Discussion of this is the primary reason I sent this patch
set as an RFC: to highlight the issues.

> Given that confusion I'm really having a hard time understanding
> the rest of the commit.
> 

Some of this (and some of your expressed concerns) could be
alleviated by moving the call to sgtl5000_fill_defaults()
before the newly-added code to initialize ANA_POWER based on
whether an external VDDD is supplied or the internal LDO is
used, but then the dependencies would be hidden in the order
of registers in the table.

This is just more explicit.

I hope that clarifies things.

Regards,


Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU+ia3AAoJEFUqXmm9AiVrU6AH/iu2bxDRrhBPaBBLMnXQUqiG
w49zAs74PA3LcFAvR0zPhBeRjbw3NOFjAVqsr2nejpq4jtNnlxG0aYYiX+bsWA4H
52vplw8BaJxRUnR/pwa+cj7HzUwK637t8/19Zk3mfxbeqaUMX6zDS1w5k8c8U5DE
1497cxLXnX5OVjClzCyrLiZMUhLqu2BCXFgxJLHe9315MFz5T+Nd1tFXDFSVmNB8
oO85GSMBvdz2CkQ9X6wjMVVS9QnISoisEjBOhoqzfGP6A5C3p2f2zi+Sm/2Rote7
E3diMCmrH22q+IruCfQbzzVVB0B3SiU+ckQvvEWtCXWNr0RVaJwzuzdonD51Zkk=
=9hrU
-----END PGP SIGNATURE-----

Re: [RFC PATCH 3/6] ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1427 bytes --]

On Fri, Mar 06, 2015 at 03:14:16PM -0700, Eric Nelson wrote:
> On 03/06/2015 01:12 PM, Mark Brown wrote:

> > Two big problems here.  One is that this appears to also affect 
> > LINREG_CTRL which your changelog didn't mention.

> It did mention the change:

> 	> Initialize CHIP_ANA_POWER to match power on defaults, which
> 	> disables ADC, DAC, and charge pumps.
> 	>
> 	> In the process, remove references to the following
> 	> register/bitfields from the sgttl5000_set_power_regs routine:

That's burying the lead a bit - that just looked like a list of
bitfields within the register (hardware designers often call ndividual
bitfields registers).

> The CHIP_LINREG_CTRL value needs to be set based on the presence
> or absence of external VDDD, so writing a default (power-on) value
> doesn't help much, and this certainly shouldn't happen after
> the proper value is written.

This indicates that there is a greater confusion in the code which
should be fixed, this sounds more like a patch of some kind at this
point.  At the very least this needs to be renamed, or there needs to be
handling in the sync code for the more sensitive registers.

My understanding was that this was being done to regain control of the
chip after driver start because there's no reset feature in the chip.
That's a reasonable thing to want to do but apparently the current code
isn't very well thought out.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[RFC PATCH 4/6] ASoC: sgtl5000: check return values

[Eric Nelson]

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 0a7b96d7..c373991 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1252,12 +1252,17 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	ana_pwr = SGTL5000_ANA_POWER_DEFAULT;
 	if (sgtl5000->num_supplies <= VDDD) {
 		/* internal VDDD at 1.2V */
-		regmap_update_bits(sgtl5000->regmap,
-				   SGTL5000_CHIP_LINREG_CTRL,
-				   SGTL5000_LINREG_VDDD_MASK,
-				   LINREG_VDDD);
+		ret = regmap_update_bits(sgtl5000->regmap,
+					 SGTL5000_CHIP_LINREG_CTRL,
+					 SGTL5000_LINREG_VDDD_MASK,
+					 LINREG_VDDD);
+		if (ret)
+			dev_err(&client->dev,
+				"error %d setting LINREG_VDDD\n", ret);
+
 		ana_pwr |= SGTL5000_LINEREG_D_POWERUP;
-		dev_info(&client->dev, "Using internal LDO instead of VDDD: check ER1\n");
+		dev_info(&client->dev,
+			 "Using internal LDO instead of VDDD: check ER1\n");
 	} else {
 		/* using external LDO for VDDD
 		 * Clear startup powerup and simple powerup
@@ -1267,7 +1272,11 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 			     | SGTL5000_LINREG_SIMPLE_POWERUP);
 		dev_dbg(&client->dev, "Using external VDDD\n");
 	}
-	regmap_write(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER, ana_pwr);
+	ret = regmap_write(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER, ana_pwr);
+	if (ret)
+		dev_err(&client->dev,
+			"error %d setting CHIP_ANA_POWER to %04x\n",
+			ret, ana_pwr);
 
 	if (np) {
 		if (!of_property_read_u32(np,
-- 
1.9.1

[RFC PATCH 5/6] ASoC: sgtl5000: disable internal PLL early

[Eric Nelson]

To handle the soft reboot case, the internal PLL must be
disabled in SGTL5000_CHIP_CLK_CTRL before clearing bits
SGTL5000_VCOAMP_POWERUP and SGTL5000_PLL_POWERUP in
register SGTL5000_CHIP_ANA_POWER.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 9 ++++++++-
 sound/soc/codecs/sgtl5000.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index c373991..5f7dd5d 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -38,7 +38,6 @@
 /* default value of sgtl5000 registers */
 static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_CHIP_DIG_POWER,		0x0000 },
-	{ SGTL5000_CHIP_CLK_CTRL,		0x0008 },
 	{ SGTL5000_CHIP_I2S_CTRL,		0x0010 },
 	{ SGTL5000_CHIP_SSS_CTRL,		0x0010 },
 	{ SGTL5000_CHIP_ADCDAC_CTRL,		0x020c },
@@ -1248,6 +1247,14 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	dev_info(&client->dev, "sgtl5000 revision 0x%x\n", rev);
 	sgtl5000->revision = rev;
 
+	/* reconfigure the clocks in case we're using the PLL */
+	ret = regmap_write(sgtl5000->regmap,
+			   SGTL5000_CHIP_CLK_CTRL,
+			   SGTL5000_CHIP_CLK_CTRL_DEFAULT);
+	if (ret)
+		dev_err(&client->dev,
+			"Error %d initializing CHIP_CLK_CTRL\n", ret);
+
 	/* Follow section 2.2.1.1 of AN3663 */
 	ana_pwr = SGTL5000_ANA_POWER_DEFAULT;
 	if (sgtl5000->num_supplies <= VDDD) {
diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
index 6fdc589..a97e3f4 100644
--- a/sound/soc/codecs/sgtl5000.h
+++ b/sound/soc/codecs/sgtl5000.h
@@ -92,6 +92,7 @@
 /*
  * SGTL5000_CHIP_CLK_CTRL
  */
+#define SGTL5000_CHIP_CLK_CTRL_DEFAULT		0x0008
 #define SGTL5000_RATE_MODE_MASK			0x0030
 #define SGTL5000_RATE_MODE_SHIFT		4
 #define SGTL5000_RATE_MODE_WIDTH		2
-- 
1.9.1

Re: [RFC PATCH 5/6] ASoC: sgtl5000: disable internal PLL early

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 319 bytes --]

On Thu, Feb 26, 2015 at 03:54:32PM -0700, Eric Nelson wrote:

>  static const struct reg_default sgtl5000_reg_defaults[] = {
>  	{ SGTL5000_CHIP_DIG_POWER,		0x0000 },
> -	{ SGTL5000_CHIP_CLK_CTRL,		0x0008 },

Again, you shouldn't be changing the register defaults table unless the
defaults don't match power up values.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 5/6] ASoC: sgtl5000: disable internal PLL early

[Eric Nelson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark,

On 03/06/2015 01:15 PM, Mark Brown wrote:
> On Thu, Feb 26, 2015 at 03:54:32PM -0700, Eric Nelson wrote:
> 
>> static const struct reg_default sgtl5000_reg_defaults[] = { {
>> SGTL5000_CHIP_DIG_POWER,		0x0000 }, -	{ SGTL5000_CHIP_CLK_CTRL,
>> 0x0008 },
> 
> Again, you shouldn't be changing the register defaults table unless
> the defaults don't match power up values.
> 

I described the rational in response to patch 2: there's a
dependency on the PLLs being disabled before a write to
ANA_POWER.

Regards,


Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU+ic1AAoJEFUqXmm9AiVra7wIAJ26meDf/UaeEjWcDgPdFPEU
68kZeNWXh6M1LliJCLwxtdp/WQLErJOh51vtT3foTcm1DF2WUIaqggX5J1BFccJT
Nu3//Q4SWf2yQhFQvsE0Vu+c2edQdh2JKvs3dN8HOxrlNXu5wfS0o2hhoKoqHisT
ROzipaV/MsHwA36l8iK383vpmz3cZjzTJdKWl7Pnr24LffWwPbtqpRUgKFuccl2B
bL61s8YujMgDCMC8D8k/p3h3EaOGNq0NOQUSiL+ISIpQlqsI7jLEgo9BtVTjpg0S
3fyykr1Wi93C1k1xag5ZAEdckvo1YemVyUdNUrX1aVStkarM+5l57rnncHnjUmM=
=+E9A
-----END PGP SIGNATURE-----

Applied "ASoC: sgtl5000: Disable internal PLL early" to the asoc tree

[Mark Brown]

The patch

   ASoC: sgtl5000: Disable internal PLL early

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.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 08dea16e0960ea5caf7876045b747145cb677096 Mon Sep 17 00:00:00 2001
From: Eric Nelson <eric@nelint.com>
Date: Tue, 7 Jun 2016 01:14:51 +0200
Subject: [PATCH] ASoC: sgtl5000: Disable internal PLL early

To handle the soft reboot case, the internal PLL must be
disabled in SGTL5000_CHIP_CLK_CTRL before clearing bits
SGTL5000_VCOAMP_POWERUP and SGTL5000_PLL_POWERUP in
register SGTL5000_CHIP_ANA_POWER.

Signed-off-by: Eric Nelson <eric@nelint.com>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/sgtl5000.c | 9 ++++++++-
 sound/soc/codecs/sgtl5000.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 42f2eb62664e..0916bb46ccf2 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -38,7 +38,6 @@
 /* default value of sgtl5000 registers */
 static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_CHIP_DIG_POWER,		0x0000 },
-	{ SGTL5000_CHIP_CLK_CTRL,		0x0008 },
 	{ SGTL5000_CHIP_I2S_CTRL,		0x0010 },
 	{ SGTL5000_CHIP_SSS_CTRL,		0x0010 },
 	{ SGTL5000_CHIP_ADCDAC_CTRL,		0x020c },
@@ -1279,6 +1278,14 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	dev_info(&client->dev, "sgtl5000 revision 0x%x\n", rev);
 	sgtl5000->revision = rev;
 
+	/* reconfigure the clocks in case we're using the PLL */
+	ret = regmap_write(sgtl5000->regmap,
+			   SGTL5000_CHIP_CLK_CTRL,
+			   SGTL5000_CHIP_CLK_CTRL_DEFAULT);
+	if (ret)
+		dev_err(&client->dev,
+			"Error %d initializing CHIP_CLK_CTRL\n", ret);
+
 	/* Follow section 2.2.1.1 of AN3663 */
 	ana_pwr = SGTL5000_ANA_POWER_DEFAULT;
 	if (sgtl5000->num_supplies <= VDDD) {
diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
index 1be82379c689..22f3442af982 100644
--- a/sound/soc/codecs/sgtl5000.h
+++ b/sound/soc/codecs/sgtl5000.h
@@ -92,6 +92,7 @@
 /*
  * SGTL5000_CHIP_CLK_CTRL
  */
+#define SGTL5000_CHIP_CLK_CTRL_DEFAULT		0x0008
 #define SGTL5000_RATE_MODE_MASK			0x0030
 #define SGTL5000_RATE_MODE_SHIFT		4
 #define SGTL5000_RATE_MODE_WIDTH		2
-- 
2.8.1

[RFC PATCH 6/6] ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF

[Eric Nelson]

Disabling the SGTL5000 through regulators would certainly save
power than simply disabling the reference voltages as described
in the data sheet, but won't properly restore things on resume.

As patch 1 in the series shows, no user of this driver has support
for active regulators yet, so this hasn't shown up.

This patch is much more conservative and simply disables the
reference bias currents.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 32 +++++---------------------------
 1 file changed, 5 insertions(+), 27 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 5f7dd5d..d4d793f 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -767,36 +767,14 @@ static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
 	switch (level) {
 	case SND_SOC_BIAS_ON:
 	case SND_SOC_BIAS_PREPARE:
-		break;
 	case SND_SOC_BIAS_STANDBY:
-		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
-			ret = regulator_bulk_enable(
-						sgtl5000->num_supplies,
-						sgtl5000->supplies);
-			if (ret)
-				return ret;
-			udelay(10);
-
-			regcache_cache_only(sgtl5000->regmap, false);
-
-			ret = regcache_sync(sgtl5000->regmap);
-			if (ret != 0) {
-				dev_err(codec->dev,
-					"Failed to restore cache: %d\n", ret);
-
-				regcache_cache_only(sgtl5000->regmap, true);
-				regulator_bulk_disable(sgtl5000->num_supplies,
-						       sgtl5000->supplies);
-
-				return ret;
-			}
-		}
-
+		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
+				    SGTL5000_REFTOP_POWERUP,
+				    SGTL5000_REFTOP_POWERUP);
 		break;
 	case SND_SOC_BIAS_OFF:
-		regcache_cache_only(sgtl5000->regmap, true);
-		regulator_bulk_disable(sgtl5000->num_supplies,
-				       sgtl5000->supplies);
+		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
+				    SGTL5000_REFTOP_POWERUP, 0);
 		break;
 	}
 
-- 
1.9.1

Re: [RFC PATCH 6/6] ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 337 bytes --]

On Thu, Feb 26, 2015 at 03:54:33PM -0700, Eric Nelson wrote:
> Disabling the SGTL5000 through regulators would certainly save
> power than simply disabling the reference voltages as described
> in the data sheet, but won't properly restore things on resume.

Why not just fix the code, reinitializing the chip is usually pretty
trivial?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 6/6] ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF

[Eric Nelson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark,

On 03/06/2015 01:16 PM, Mark Brown wrote:
> On Thu, Feb 26, 2015 at 03:54:33PM -0700, Eric Nelson wrote:
>> Disabling the SGTL5000 through regulators would certainly save 
>> power than simply disabling the reference voltages as described 
>> in the data sheet, but won't properly restore things on resume.
> 
> Why not just fix the code, reinitializing the chip is usually
> pretty trivial?
> 

Apparently not.

I think it's fair to say that nobody has hooked up switching regulators
using this driver, since they can't function without some form of this
patch set.

Once we have working regulator support, adding code to save power
additional power here might make sense, but not until then.

In other words, these are really elaborate no-ops now.

Regards,


Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU+iu4AAoJEFUqXmm9AiVrdlEH+gKLTIaT8ozcfbh1vgNWZQyz
lTd6wlMK9InRxZU2UPXtcCUIyK+z0YnNNoZEaoIP9YYLadOmpC+Gw/lF+8bAXInt
pEZqa17rLE7Q2xC1IhyjsIspQMP3Zpnvc/Ay51aq1I1M/wD4+G8kk6fhqrSAjBMo
DqYCNmP5AbLNGL1yePp/Zt5G/mgApC3QtE4vdBdi/4HIFxMNwySEmVFQD0/WV2aI
stZ8Sh5jc8pIQeLBRy1a0DJUF29jC7+nhVVMJQlXt8yWprnn6IgoM0dJTkzSif01
5HMRYLPbzemzJNOTcvIF15yOczGw4lqUmgbtR8Rz0wNQ3cghhjTk41HU5XasFJY=
=/NEm
-----END PGP SIGNATURE-----

Re: [RFC PATCH 6/6] ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 785 bytes --]

On Fri, Mar 06, 2015 at 03:35:36PM -0700, Eric Nelson wrote:
> On 03/06/2015 01:16 PM, Mark Brown wrote:

> > Why not just fix the code, reinitializing the chip is usually
> > pretty trivial?

> Apparently not.

Well, I'm not sure if that's actually true or not - the current driver
code is pretty bad and there's been a history of worrying submissions
that people assure me are working well and address problems.  All of
this is indicating to me that the complexity here is largely due to code
quality issues and that if we address those then things might get a lot
easier.

Requiring a funny power on sequence shouldn't be hard and is needed for
suspend and resume anyway, all having the regulator support does is mean
we can potentially take advantage of that code path at runtime.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Eric Nelson]

Hi Mark,

On 02/26/2015 03:54 PM, Eric Nelson wrote:
> This patch set addresses a structural problem in the handling of regulators
> for VDDIO, VDDA, and VDDD in the SGTL5000 driver.
> 
> The first two of these power rails must be powered on prior to any I2C
> communication, and yet the regulators were tied to the codec, which is
> instantiated only after a fair amount of I2C communication takes place.
> 
> In other words, these regulators could never have function, and we can
> surmise that no user of this driver has switched power supply rails
> connected to them.
> 
> The third power rail (VDDD) can be derived internally (by using I2C registers)
> though the data sheet says that if an external VDDD is used, it should be
> enabled before MCLK is started and I2C activity begins.
> 
> Patch 1 moves the regulators to the I2C device and initializes them before
> 	I2C activity.
> Patch 2 addresses an issue found during development of the patch and is
>         somewhat unrelated to regulators. It is included for discussion
> Patch 3 removes the root cause of patch 2
> Patch 4 adds return value checks when setting the LINREG and ANA_POWER
> 	registers
> Patch 5 addresses the cause of follow-on failures and the source of the
> 	error in patch 3.
> Patch 6 removes the naive use of the supplies to lower power consumption
> 	and replaces it with something more modest.
> 
> The patch set is being sent as an RFC for discussion, and should probably
> be squashed into two or three patches covering the regulator updates
> (patches 1, 3-5 and perhaps 6), initialization error handling (patch 2).
> 
> Eric Nelson (6):
>   ASoC: sgtl5000: fix regulator support
>   ASoC: sgtl5000: write all default registers
>   ASoC: sgtl5000: initialize CHIP_ANA_POWER to power-on defaults
>   ASoC: sgtl5000: check return values
>   ASoC: sgtl5000: disable internal PLL early
>   ASoC: sgtl5000: Don't disable regulators in SND_SOC_BIAS_OFF
> 

Based on your review, it seems like the following is needed for a
V2 patch (set):

- Move call to sgtl5000_fill_defaults() earlier in the
  sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
  This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
  back into the default	register list.
- Move regulators from codec to I2C device
- switch to devm_regulator api
- various code cleanups to simplify logic
- Adjust regulator usage in SND_SOC_BIAS_OFF

I'll leave out the "write all default" registers for the moment.

Let me know if you'd like to see this in multiple patches or a
single patch.

I'll wait to see if there's other feedback before prepping V2.

Regards,


Eric

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote:
> Based on your review, it seems like the following is needed for a
> V2 patch (set):
> 
> - Move call to sgtl5000_fill_defaults() earlier in the
>   sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
>   This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
>   back into the default	register list.
> - Move regulators from codec to I2C device
> - switch to devm_regulator api
> - various code cleanups to simplify logic
> - Adjust regulator usage in SND_SOC_BIAS_OFF
> 
> I'll leave out the "write all default" registers for the moment.

I'll also interject here to say that, against mainline, I need this
patch to make the sgtl5k work on SolidRun's Hummingboard hardware.
Without this, the internal regulator seems to get powered down, and
we lose access to the device until the entire board is power cycled.

 sound/soc/codecs/sgtl5000.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index e182e6569bbd..79d5cbd65f36 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev)
 	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
 
 	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
+				SGTL5000_LINREG_SIMPLE_POWERUP,
+				SGTL5000_LINREG_SIMPLE_POWERUP);
+
+	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
 				SGTL5000_LINEREG_D_POWERUP,
 				0);
 
@@ -1155,8 +1159,11 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 		 * if vddio and vddd > 3.1v,
 		 * charge pump should be clean before set ana_pwr
 		 */
-		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
-				SGTL5000_VDDC_CHRGPMP_POWERUP, 0);
+// FIXME: this is total crap - we have read this register above into
+// ana_pwr, which we then modify (above), and then write back to the
+// register below.  This modification just gets completely overwritten.
+//		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
+//				SGTL5000_VDDC_CHRGPMP_POWERUP, 0);
 
 		/* VDDC use VDDIO rail */
 		lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD;


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Eric Nelson]

Hi Russell,

On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote:
> On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote:
>> Based on your review, it seems like the following is needed for a
>> V2 patch (set):
>>
>> - Move call to sgtl5000_fill_defaults() earlier in the
>>   sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
>>   This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
>>   back into the default	register list.
>> - Move regulators from codec to I2C device
>> - switch to devm_regulator api
>> - various code cleanups to simplify logic
>> - Adjust regulator usage in SND_SOC_BIAS_OFF
>>
>> I'll leave out the "write all default" registers for the moment.
> 
> I'll also interject here to say that, against mainline, I need this
> patch to make the sgtl5k work on SolidRun's Hummingboard hardware.
> Without this, the internal regulator seems to get powered down, and
> we lose access to the device until the entire board is power cycled.
> 

Mark applied essentially the patch below separately from
the LDO support:
	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html
	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html

>  sound/soc/codecs/sgtl5000.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index e182e6569bbd..79d5cbd65f36 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev)
>  	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
>  
>  	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> +				SGTL5000_LINREG_SIMPLE_POWERUP,
> +				SGTL5000_LINREG_SIMPLE_POWERUP);
> +
> +	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
>  				SGTL5000_LINEREG_D_POWERUP,
>  				0);
>  
> @@ -1155,8 +1159,11 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
>  		 * if vddio and vddd > 3.1v,
>  		 * charge pump should be clean before set ana_pwr
>  		 */
> -		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> -				SGTL5000_VDDC_CHRGPMP_POWERUP, 0);
> +// FIXME: this is total crap - we have read this register above into
> +// ana_pwr, which we then modify (above), and then write back to the
> +// register below.  This modification just gets completely overwritten.
> +//		snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> +//				SGTL5000_VDDC_CHRGPMP_POWERUP, 0);
>  
>  		/* VDDC use VDDIO rail */
>  		lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD;
> 
> 

The bulk of this patch set tries to make the VDDIO and VDDA
regulators work and prep for the use of external VDDD, which
errata ER1 says is best practice (and which nobody is following).

Regards,


Eric

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Eric Nelson]

Hi Russell,

On 03/06/2015 03:58 PM, Eric Nelson wrote:
> Hi Russell,
> 
> On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote:
>> On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote:
>>> Based on your review, it seems like the following is needed for a
>>> V2 patch (set):
>>>
>>> - Move call to sgtl5000_fill_defaults() earlier in the
>>>   sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
>>>   This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
>>>   back into the default	register list.
>>> - Move regulators from codec to I2C device
>>> - switch to devm_regulator api
>>> - various code cleanups to simplify logic
>>> - Adjust regulator usage in SND_SOC_BIAS_OFF
>>>
>>> I'll leave out the "write all default" registers for the moment.
>>
>> I'll also interject here to say that, against mainline, I need this
>> patch to make the sgtl5k work on SolidRun's Hummingboard hardware.
>> Without this, the internal regulator seems to get powered down, and
>> we lose access to the device until the entire board is power cycled.
>>
> 
> Mark applied essentially the patch below separately from
> the LDO support:
> 	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html
> 	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html
> 
>>  sound/soc/codecs/sgtl5000.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
>> index e182e6569bbd..79d5cbd65f36 100644
>> --- a/sound/soc/codecs/sgtl5000.c
>> +++ b/sound/soc/codecs/sgtl5000.c
>> @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev)
>>  	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
>>  

Sorry. This part of the patch you sent wasn't included in the
patch I mentioned above:

>>  	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
>> +				SGTL5000_LINREG_SIMPLE_POWERUP,
>> +				SGTL5000_LINREG_SIMPLE_POWERUP);
>> +
>> +	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
>>  				SGTL5000_LINEREG_D_POWERUP,
>>  				0);
>>  

In the full patch-set, initialization of ANA_POWER is placed inside
the sgtl5000_i2c_probe routine and the ldo_regulator is discarded.

I'm not sure why the code above is needed though.

Do you know what sequence of events causes this to fail?

Please advise,


Eric

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 04:16:18PM -0700, Eric Nelson wrote:
> Hi Russell,
> 
> On 03/06/2015 03:58 PM, Eric Nelson wrote:
> > Hi Russell,
> > 
> > On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote:
> >> On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote:
> >>> Based on your review, it seems like the following is needed for a
> >>> V2 patch (set):
> >>>
> >>> - Move call to sgtl5000_fill_defaults() earlier in the
> >>>   sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
> >>>   This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
> >>>   back into the default	register list.
> >>> - Move regulators from codec to I2C device
> >>> - switch to devm_regulator api
> >>> - various code cleanups to simplify logic
> >>> - Adjust regulator usage in SND_SOC_BIAS_OFF
> >>>
> >>> I'll leave out the "write all default" registers for the moment.
> >>
> >> I'll also interject here to say that, against mainline, I need this
> >> patch to make the sgtl5k work on SolidRun's Hummingboard hardware.
> >> Without this, the internal regulator seems to get powered down, and
> >> we lose access to the device until the entire board is power cycled.
> >>
> > 
> > Mark applied essentially the patch below separately from
> > the LDO support:
> > 	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html
> > 	http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html
> > 
> >>  sound/soc/codecs/sgtl5000.c | 11 +++++++++--
> >>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> >> index e182e6569bbd..79d5cbd65f36 100644
> >> --- a/sound/soc/codecs/sgtl5000.c
> >> +++ b/sound/soc/codecs/sgtl5000.c
> >> @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev)
> >>  	struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data;
> >>  
> 
> Sorry. This part of the patch you sent wasn't included in the
> patch I mentioned above:
> 
> >>  	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> >> +				SGTL5000_LINREG_SIMPLE_POWERUP,
> >> +				SGTL5000_LINREG_SIMPLE_POWERUP);
> >> +
> >> +	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> >>  				SGTL5000_LINEREG_D_POWERUP,
> >>  				0);
> >>  
> 
> In the full patch-set, initialization of ANA_POWER is placed inside
> the sgtl5000_i2c_probe routine and the ldo_regulator is discarded.
> 
> I'm not sure why the code above is needed though.
> 
> Do you know what sequence of events causes this to fail?

All that I remember is that as the mainline code stood in 3.19 etc,
the SGTL5k would be partially detected and then I2C communication
with it was lost.  Probing the pins with a scope showed that it had
basically lost power.

The patch above is over a year old now, and I don't remember much
more detail than that.

I'm not going to be physically in a state where I can look at it for
about a week and a bit, sorry.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Eric Nelson]

Hi Russell,

On 03/06/2015 04:24 PM, Russell King - ARM Linux wrote:
> On Fri, Mar 06, 2015 at 04:16:18PM -0700, Eric Nelson wrote:
>> Hi Russell,
>>
>> <snip>
>>
>> I'm not sure why the code above is needed though.
>>
>> Do you know what sequence of events causes this to fail?
> 
> All that I remember is that as the mainline code stood in 3.19 etc,
> the SGTL5k would be partially detected and then I2C communication
> with it was lost.  Probing the pins with a scope showed that it had
> basically lost power.
> 
> The patch above is over a year old now, and I don't remember much
> more detail than that.
> 
> I'm not going to be physically in a state where I can look at it for
> about a week and a bit, sorry.
> 

No worries. I appreciate the feedback.

Re: [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote:

> Based on your review, it seems like the following is needed for a
> V2 patch (set):

> - Move call to sgtl5000_fill_defaults() earlier in the
>   sgtl5000_i2c_probe routine, before adjusting ANA_POWER.
>   This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER
>   back into the default	register list.

I think that whole area of code also needs some revisions to make the
code look like it's doing what it's supposed to be doing - the fact that
it is described as register defaults at the minute but aren't really
what a driver would normally mean by register defaults seems to be a big
source of confusion.

> Let me know if you'd like to see this in multiple patches or a
> single patch.

Definitely multiple patches, the level of splitting you've got now is
pretty good.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]