[alsa-devel] [PATCH v5 1/2] ASoC: sgtl5000: Read SGTL5000_CHIP_ID in i2c_probe()
From: Fabio Estevam fabio.estevam@freescale.com
The usual place for reading chip ID is inside i2c_probe, so move it there and also convert it to regmap.
sgtl5000_enable_regulators() needs to read the chip revision, so keep the revision check there.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- Changes since v4: - Just introduced in this version.
sound/soc/codecs/sgtl5000.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 327b443..af0c8aa 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1275,7 +1275,7 @@ static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec)
static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) { - u16 reg; + int reg; int ret; int rev; int i; @@ -1303,23 +1303,17 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) /* wait for all power rails bring up */ udelay(10);
- /* read chip information */ - reg = snd_soc_read(codec, SGTL5000_CHIP_ID); - if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) != - SGTL5000_PARTID_PART_ID) { - dev_err(codec->dev, - "Device with ID register %x is not a sgtl5000\n", reg); - ret = -ENODEV; - goto err_regulator_disable; - } - - rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT; - dev_info(codec->dev, "sgtl5000 revision 0x%x\n", rev); - /* * workaround for revision 0x11 and later, * roll back to use internal LDO */ + + ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, ®); + if (ret) + goto err_regulator_disable; + + rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT; + if (external_vddd && rev >= 0x11) { /* disable all regulator first */ regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies), @@ -1478,7 +1472,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct sgtl5000_priv *sgtl5000; - int ret; + int ret, reg, rev;
sgtl5000 = devm_kzalloc(&client->dev, sizeof(struct sgtl5000_priv), GFP_KERNEL); @@ -1492,6 +1486,21 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, return ret; }
+ /* read chip information */ + ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, ®); + if (ret) + return ret; + + if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) != + SGTL5000_PARTID_PART_ID) { + dev_err(&client->dev, + "Device with ID register %x is not a sgtl5000\n", reg); + return -ENODEV; + } + + rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT; + dev_info(&client->dev, "sgtl5000 revision 0x%x\n", rev); + i2c_set_clientdata(client, sgtl5000);
ret = snd_soc_register_codec(&client->dev,
From: Fabio Estevam fabio.estevam@freescale.com
After a 'reboot' command in Linux or after pressing the system's reset button the sgtl5000 driver fails to probe:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
sgtl5000 codec does not have a reset line, nor a reset command in software, so after a system reset the codec does not contain the default register values from sgtl5000_reg_defaults[] anymore, as these are only valid after a power-on-reset cycle.
Fix this issue by explicitly reading all the reset register values from sgtl5000_reg_defaults[] and writing them back into sgtl5000 to ensure a sane state.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- Changes since v4: - Only call sgtl5000_fill_defaults if we know we have a sgtl5000 Changes since v3: - Read sgtl5000_reg_defaults and write these values into sgtl5000 Changes since v2: - Do not use reg_defaults_raw as it is not the correct purpose - Manually build sgtl5000_reg_default - Improve commitlog Changes since v1: - Remove sgtl5000_reg_defaults array - Do not use num_reg_defaults_raw sound/soc/codecs/sgtl5000.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index af0c8aa..ec97636 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1468,6 +1468,31 @@ static const struct regmap_config sgtl5000_regmap = { .num_reg_defaults = ARRAY_SIZE(sgtl5000_reg_defaults), };
+/* + * Write all the default values from sgtl5000_reg_defaults[] array into the + * sgtl5000 registers, to make sure we always start with the sane registers + * values as stated in the datasheet. + * + * Since sgtl5000 does not have a reset line, nor a reset command in software, + * we follow this approach to guarantee we always start from the default values + * 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) +{ + int i, ret, val, index; + + for (i = 0; i < ARRAY_SIZE(sgtl5000_reg_defaults); i++) { + val = sgtl5000_reg_defaults[i].def; + index = sgtl5000_reg_defaults[i].reg; + ret = regmap_write(sgtl5000->regmap, index, val); + if (ret) + return ret; + } + + return 0; +} + static int sgtl5000_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1503,6 +1528,11 @@ 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) + return ret; + ret = snd_soc_register_codec(&client->dev, &sgtl5000_driver, &sgtl5000_dai, 1); return ret;
On 5/9/2013 5:15 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
After a 'reboot' command in Linux or after pressing the system's reset button the sgtl5000 driver fails to probe:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
sgtl5000 codec does not have a reset line, nor a reset command in software, so after a system reset the codec does not contain the default register values from sgtl5000_reg_defaults[] anymore, as these are only valid after a power-on-reset cycle.
Fix this issue by explicitly reading all the reset register values from sgtl5000_reg_defaults[] and writing them back into sgtl5000 to ensure a sane state.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
Changes since v4:
- Only call sgtl5000_fill_defaults if we know we have a sgtl5000
Changes since v3:
- Read sgtl5000_reg_defaults and write these values into sgtl5000
Changes since v2:
- Do not use reg_defaults_raw as it is not the correct purpose
- Manually build sgtl5000_reg_default
- Improve commitlog
Changes since v1:
- Remove sgtl5000_reg_defaults array
- Do not use num_reg_defaults_raw sound/soc/codecs/sgtl5000.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index af0c8aa..ec97636 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1468,6 +1468,31 @@ static const struct regmap_config sgtl5000_regmap = { .num_reg_defaults = ARRAY_SIZE(sgtl5000_reg_defaults), };
+/*
- Write all the default values from sgtl5000_reg_defaults[] array into the
- sgtl5000 registers, to make sure we always start with the sane registers
- values as stated in the datasheet.
- Since sgtl5000 does not have a reset line, nor a reset command in software,
- we follow this approach to guarantee we always start from the default values
- 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) +{
- int i, ret, val, index;
- for (i = 0; i < ARRAY_SIZE(sgtl5000_reg_defaults); i++) {
val = sgtl5000_reg_defaults[i].def;index = sgtl5000_reg_defaults[i].reg;ret = regmap_write(sgtl5000->regmap, index, val);if (ret)return ret;- }
- return 0;
+}
- static int sgtl5000_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) {
@@ -1503,6 +1528,11 @@ 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)
return ret;- ret = snd_soc_register_codec(&client->dev, &sgtl5000_driver, &sgtl5000_dai, 1); return ret;
Did you test a reset/reboot ? Since the fill_defaults now happens after the read of the device id, how does this fix the mentioned problem ?
Troy
On Thu, May 9, 2013 at 10:17 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Did you test a reset/reboot ? Since the fill_defaults now happens after the read of the device id, how does this fix the mentioned problem ?
Yes, tested several times :-)
The chip ID has to be read from the register and it always read correctly. In patch 1/2, I just moved the reading to a more standard location (in i2c_probe function), just like many other codecs do.
Patch 2/2 fixes the issue by ensuring that we start from sane values from power-on reset.
Let's look at the original error:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
If I move the chip ID reading to i2c_probe (ie, only apply patch 1/2), the device ID would be read correctly, but the fail would happen at a later point:
sgtl5000 0-000a: sgtl5000 revision 0x11 sgtl5000 0-000a: Failed to get supply 'VDDD': -517 mmcblk0: p1 0-000a: 1200 mV normal sgtl5000 0-000a: Using internal LDO instead of VDDD usb 2-1: new high-speed USB device number 2 using ci_hdrc hub 2-1:1.0: USB hub found hub 2-1:1.0: 3 ports detected sgtl5000 0-000a: ASoC: failed to probe CODEC -110 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -110 imx-sgtl5000 sound.12: snd_soc_register_card failed (-110) imx-sgtl5000: probe of sound.12 failed with error -110
So the original issue was not about reading the chip ID correctly.
The power related registers change from POR to reset (among others)
If the chip is not properly powered, then we are not able to read its ID and we get that original error.
On 5/9/2013 7:12 PM, Fabio Estevam wrote:
On Thu, May 9, 2013 at 10:17 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Did you test a reset/reboot ? Since the fill_defaults now happens after the read of the device id, how does this fix the mentioned problem ?
Yes, tested several times :-)
The chip ID has to be read from the register and it always read correctly. In patch 1/2, I just moved the reading to a more standard location (in i2c_probe function), just like many other codecs do.
Patch 2/2 fixes the issue by ensuring that we start from sane values from power-on reset.
Let's look at the original error:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
If I move the chip ID reading to i2c_probe (ie, only apply patch 1/2), the device ID would be read correctly, but the fail would happen at a later point:
sgtl5000 0-000a: sgtl5000 revision 0x11 sgtl5000 0-000a: Failed to get supply 'VDDD': -517 mmcblk0: p1 0-000a: 1200 mV normal sgtl5000 0-000a: Using internal LDO instead of VDDD usb 2-1: new high-speed USB device number 2 using ci_hdrc hub 2-1:1.0: USB hub found hub 2-1:1.0: 3 ports detected sgtl5000 0-000a: ASoC: failed to probe CODEC -110 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -110 imx-sgtl5000 sound.12: snd_soc_register_card failed (-110) imx-sgtl5000: probe of sound.12 failed with error -110
Shouldn't you update your commit log with this then ?
So the original issue was not about reading the chip ID correctly.
The power related registers change from POR to reset (among others)
If the chip is not properly powered, then we are not able to read its ID and we get that original error.
Thanks for fixing, I guess I don't need to understand how the fix works.
Troy
On 5/10/2013 12:11 PM, Troy Kisky wrote:
On 5/9/2013 7:12 PM, Fabio Estevam wrote:
On Thu, May 9, 2013 at 10:17 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Did you test a reset/reboot ? Since the fill_defaults now happens after the read of the device id, how does this fix the mentioned problem ?
Yes, tested several times :-)
Since Sabrelite uses "dummy" regulators (always on), have you tested with a boards that actually uses regulators?
I would think that some regulators would need to be on before you could even read the ID register.
The chip ID has to be read from the register and it always read correctly. In patch 1/2, I just moved the reading to a more standard location (in i2c_probe function), just like many other codecs do.
Patch 2/2 fixes the issue by ensuring that we start from sane values from power-on reset.
Let's look at the original error:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
If I move the chip ID reading to i2c_probe (ie, only apply patch 1/2), the device ID would be read correctly, but the fail would happen at a later point:
sgtl5000 0-000a: sgtl5000 revision 0x11 sgtl5000 0-000a: Failed to get supply 'VDDD': -517 mmcblk0: p1 0-000a: 1200 mV normal sgtl5000 0-000a: Using internal LDO instead of VDDD usb 2-1: new high-speed USB device number 2 using ci_hdrc hub 2-1:1.0: USB hub found hub 2-1:1.0: 3 ports detected sgtl5000 0-000a: ASoC: failed to probe CODEC -110 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -110 imx-sgtl5000 sound.12: snd_soc_register_card failed (-110) imx-sgtl5000: probe of sound.12 failed with error -110
Shouldn't you update your commit log with this then ?
So the original issue was not about reading the chip ID correctly.
The power related registers change from POR to reset (among others)
If the chip is not properly powered, then we are not able to read its ID and we get that original error.
Thanks for fixing, I guess I don't need to understand how the fix works.
Troy
On Fri, May 10, 2013 at 4:16 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Since Sabrelite uses "dummy" regulators (always on), have you tested with a boards that actually uses regulators?
I don't have any board that drives the sgtl5000 power supplies from a PMIC, for example.
I would think that some regulators would need to be on before you could even read the ID register.
Sure. That's why I placed the ID reading after turning on the regulators :-)
On Fri, May 10, 2013 at 4:18 PM, Fabio Estevam festevam@gmail.com wrote:
I would think that some regulators would need to be on before you could even read the ID register.
Sure. That's why I placed the ID reading after turning on the regulators :-)
Ops, actually I did not. Will fix it.
Thanks
On Fri, May 10, 2013 at 04:39:32PM -0300, Fabio Estevam wrote:
On Fri, May 10, 2013 at 4:18 PM, Fabio Estevam festevam@gmail.com wrote:
Sure. That's why I placed the ID reading after turning on the regulators :-)
Ops, actually I did not. Will fix it.
I'll wait for a new version.
On Fri, May 10, 2013 at 09:56:16PM +0100, Mark Brown wrote:
On Fri, May 10, 2013 at 04:39:32PM -0300, Fabio Estevam wrote:
Ops, actually I did not. Will fix it.
I'll wait for a new version.
Ah, actually that really only affects the already applied patch so...
On 5/10/2013 12:18 PM, Fabio Estevam wrote:
On Fri, May 10, 2013 at 4:16 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Since Sabrelite uses "dummy" regulators (always on), have you tested with a boards that actually uses regulators?
I don't have any board that drives the sgtl5000 power supplies from a PMIC, for example.
I would think that some regulators would need to be on before you could even read the ID register.
Sure. That's why I placed the ID reading after turning on the regulators :-)
Hmm. Maybe I need some remedial classes. This is what the flow looks like to me.
sgtl5000_i2c_probe regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, ®); snd_soc_register_codec(&client->dev,&sgtl5000_driver, &sgtl5000_dai, 1);
sgtl5000_probe sgtl5000_enable_regulators
On Fri, May 10, 2013 at 4:11 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
Shouldn't you update your commit log with this then ?
I provided the explanations in the commit log and in the source code in v5.
On Thu, May 09, 2013 at 09:15:47PM -0300, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
After a 'reboot' command in Linux or after pressing the system's reset button the sgtl5000 driver fails to probe:
Looks good, can everyone confirm that this fixes the reset issues please?
On 05/09/2013 05:15 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
After a 'reboot' command in Linux or after pressing the system's reset button the sgtl5000 driver fails to probe:
sgtl5000 0-000a: Device with ID register ffff is not a sgtl5000 sgtl5000 0-000a: ASoC: failed to probe CODEC -19 imx-sgtl5000 sound.12: ASoC: failed to instantiate card -19 imx-sgtl5000 sound.12: snd_soc_register_card failed (-19)
sgtl5000 codec does not have a reset line, nor a reset command in software, so after a system reset the codec does not contain the default register values from sgtl5000_reg_defaults[] anymore, as these are only valid after a power-on-reset cycle.
Fix this issue by explicitly reading all the reset register values from sgtl5000_reg_defaults[] and writing them back into sgtl5000 to ensure a sane state.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
Changes since v4:
- Only call sgtl5000_fill_defaults if we know we have a sgtl5000
Changes since v3:
- Read sgtl5000_reg_defaults and write these values into sgtl5000
Changes since v2:
- Do not use reg_defaults_raw as it is not the correct purpose
- Manually build sgtl5000_reg_default
- Improve commitlog
Changes since v1:
- Remove sgtl5000_reg_defaults array
- Do not use num_reg_defaults_raw sound/soc/codecs/sgtl5000.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
Tested-by: Eric Nelson eric.nelson@boundarydevices.com
On Thu, May 09, 2013 at 09:15:47PM -0300, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
After a 'reboot' command in Linux or after pressing the system's reset button the sgtl5000 driver fails to probe:
Applied, thanks.
On Thu, May 09, 2013 at 09:15:46PM -0300, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
The usual place for reading chip ID is inside i2c_probe, so move it there and also convert it to regmap.
Applied, thanks.
On 05/09/2013 05:15 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
The usual place for reading chip ID is inside i2c_probe, so move it there and also convert it to regmap.
sgtl5000_enable_regulators() needs to read the chip revision, so keep the revision check there.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
Changes since v4:
Just introduced in this version.
sound/soc/codecs/sgtl5000.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
Tested-by: Eric Nelson eric.nelson@boundarydevices.com
participants (4)
-
Eric Nelson -
Fabio Estevam -
Mark Brown -
Troy Kisky