[alsa-devel] [PATCH RFC 0/6] ASoC: sgtl5000: fix use of regulators and internal LDO
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
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, ®); - 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
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.
-----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-----
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.
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
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.
-----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-----
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
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
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.
-----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-----
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.
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
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
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.
-----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-----
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
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
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?
-----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-----
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.
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
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.
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.ht... 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
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.ht... 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
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.ht... 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.
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.
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.
participants (3)
-
Eric Nelson -
Mark Brown -
Russell King - ARM Linux