[alsa-devel] [PATCH v2 1/2] ASoC: tlv320aic32x4: Fix problem with first capture.
In its previous status, the first capture didn't work properly; nothing was actually recorded from the microphone. This behaviour was observed using a Visstrim M10 board.
In order to solve this BUG a workaround has been added that, during the initialization process of the codec, powers on and off the ADC.
The issue seems related to a HW BUG or some behavior that is not documented in the datasheet.
Signed-off-by: Javier Martin javier.martin@vista-silicon.com --- Changes since v1: - Use a subject line according to the ASoC subsystem.
--- sound/soc/codecs/tlv320aic32x4.c | 10 ++++++++++ sound/soc/codecs/tlv320aic32x4.h | 3 +++ 2 files changed, 13 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c index b0a73d3..aad92f9 100644 --- a/sound/soc/codecs/tlv320aic32x4.c +++ b/sound/soc/codecs/tlv320aic32x4.c @@ -675,6 +675,16 @@ static int aic32x4_probe(struct snd_soc_codec *codec) ARRAY_SIZE(aic32x4_snd_controls)); aic32x4_add_widgets(codec);
+ /* + * Workaround: for an unknown reason, the ADC needs to be powered up + * and down for the first capture to work properly. It seems related to + * a HW BUG or some kind of behavior not documented in the datasheet. + */ + tmp_reg = snd_soc_read(codec, AIC32X4_ADCSETUP); + snd_soc_write(codec, AIC32X4_ADCSETUP, tmp_reg | + AIC32X4_LADC_EN | AIC32X4_RADC_EN); + snd_soc_write(codec, AIC32X4_ADCSETUP, tmp_reg); + return 0; }
diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h index aae2b24..3577422 100644 --- a/sound/soc/codecs/tlv320aic32x4.h +++ b/sound/soc/codecs/tlv320aic32x4.h @@ -94,6 +94,9 @@ #define AIC32X4_WORD_LEN_24BITS 0x02 #define AIC32X4_WORD_LEN_32BITS 0x03
+#define AIC32X4_LADC_EN (1 << 7) +#define AIC32X4_RADC_EN (1 << 6) + #define AIC32X4_I2S_MODE 0x00 #define AIC32X4_DSP_MODE 0x01 #define AIC32X4_RIGHT_JUSTIFIED_MODE 0x02
Add the possibility to specify a gpio through platform data so that a HW reset can be issued to the codec.
Signed-off-by: Javier Martin javier.martin@vista-silicon.com --- Changes since v1: - Use a subject line according to the ASoC subsystem. - Use 'devm_gpio_request_one()' - Provide correct default value to the GPIO. --- include/sound/tlv320aic32x4.h | 1 + sound/soc/codecs/tlv320aic32x4.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/sound/tlv320aic32x4.h b/include/sound/tlv320aic32x4.h index c009f70..24e5d99 100644 --- a/include/sound/tlv320aic32x4.h +++ b/include/sound/tlv320aic32x4.h @@ -26,6 +26,7 @@ struct aic32x4_pdata { u32 power_cfg; u32 micpga_routing; bool swapdacs; + int rstn_gpio; };
#endif diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c index aad92f9..f188aa4 100644 --- a/sound/soc/codecs/tlv320aic32x4.c +++ b/sound/soc/codecs/tlv320aic32x4.c @@ -28,6 +28,7 @@ #include <linux/init.h> #include <linux/delay.h> #include <linux/pm.h> +#include <linux/gpio.h> #include <linux/i2c.h> #include <linux/cdev.h> #include <linux/slab.h> @@ -65,6 +66,7 @@ struct aic32x4_priv { u32 power_cfg; u32 micpga_routing; bool swapdacs; + int rstn_gpio; };
/* 0dB min, 1dB steps */ @@ -627,10 +629,20 @@ static int aic32x4_probe(struct snd_soc_codec *codec) { struct aic32x4_priv *aic32x4 = snd_soc_codec_get_drvdata(codec); u32 tmp_reg; + int ret;
codec->hw_write = (hw_write_t) i2c_master_send; codec->control_data = aic32x4->control_data;
+ if (aic32x4->rstn_gpio >= 0) { + ret = devm_gpio_request_one(codec->dev, aic32x4->rstn_gpio, + GPIOF_OUT_INIT_LOW, "tlv320aic32x4 rstn"); + if (ret != 0) + return ret; + ndelay(10); + gpio_set_value(aic32x4->rstn_gpio, 1); + } + snd_soc_write(codec, AIC32X4_RESET, 0x01);
/* Power platform configuration */ @@ -723,10 +735,12 @@ static __devinit int aic32x4_i2c_probe(struct i2c_client *i2c, aic32x4->power_cfg = pdata->power_cfg; aic32x4->swapdacs = pdata->swapdacs; aic32x4->micpga_routing = pdata->micpga_routing; + aic32x4->rstn_gpio = pdata->rstn_gpio; } else { aic32x4->power_cfg = 0; aic32x4->swapdacs = false; aic32x4->micpga_routing = 0; + aic32x4->rstn_gpio = -1; }
ret = snd_soc_register_codec(&i2c->dev,
On 10/31/2012 11:53 AM, Javier Martin wrote:
Add the possibility to specify a gpio through platform data so that a HW reset can be issued to the codec.
Signed-off-by: Javier Martin javier.martin@vista-silicon.com
Changes since v1:
- Use a subject line according to the ASoC subsystem.
- Use 'devm_gpio_request_one()'
- Provide correct default value to the GPIO.
Now that I see your patch - I modelled the RESET pins (and the power supply pins) as power supply inputs, and request them to be active when the bias goes into standby or higher. Keeping the chip in RESET is a very nice power saving feature. I just cache all the register settings up until that point (which required a bit of coding because of the "paging" of the I2C address space).
The reason that I also made RESET a "power" rather than a GPIO is that I have seven codecs, using three GPIO lines (in 3/3/1 configuration) to reset the chips. Having the RESET pin modelled as a powersupply allowed the codec instance to blindly turn it on, letting the powersupply keep track of the number of clients.
It was the best I could think off at the time, and it made me wonder why those reset lines aren't modelled this way more often.
If anyone's interested, I can upload the modified source. A patch is difficult, because I did horrible things to the code for our specific needs.
What's your view on this?
Mike.
Met vriendelijke groet / kind regards,
Mike Looijmans
TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: (+31) – (0)499 - 33.69.79 Telefax: (+31) - (0)499 - 33.69.70 E-mail: mike.looijmans@topic.nl Website: www.topic.nl
Dit e-mail bericht en de eventueel daarbij behorende bijlagen zijn uitsluitend bestemd voor de geadresseerde, zoals die blijkt uit het e-mail bericht en/of de bijlagen. Er kunnen gegevens met betrekking tot een derde instaan. Indien u als niet-geadresseerde dit bericht en de bijlagen ontvangt, terwijl u niet bevoegd of gemachtigd bent om dit bericht namens de geadresseerde te ontvangen, wordt u verzocht de afzender hierover direct te informeren en het e-mail bericht met de bijlagen te vernietigen. Ieder gebruik van de inhoud van het e-mail bericht, waaronder de daarbij behorende bijlagen, door een ander dan de geadresseerde is onrechtmatig jegens ons dan wel de eventueel in het e-mail bericht of de bijlagen voorkomende andere personen. TOPIC Embedded Systems is niet aansprakelijk voor enigerlei schade voortvloeiend uit het gebruik en/of acceptatie van dit e-mail bericht of de daarbij behorende bijlagen.
The contents of this message, as well as any enclosures, are addressed personally to, and thus solely intended for the addressee. They may contain information regarding a third party. A recipient who is neither the addressee, nor empowered to receive this message on behalf of the addressee, is kindly requested to immediately inform the sender of receipt, and to destroy the message and the enclosures. Any use of the contents of this message and/or the enclosures by any other person than the addressee or person who is empowered to receive this message, is illegal towards the sender and/or the aforementioned third party. TOPIC Embedded Systems is not liable for any damage as a result of the use and/or acceptance of this message and as well as any enclosures.
On Wed, Oct 31, 2012 at 03:40:17PM +0100, Mike Looijmans wrote:
is a very nice power saving feature. I just cache all the register settings up until that point (which required a bit of coding because of the "paging" of the I2C address space).
Probably easiest to just convert over to regmap, it handles paging fine.
It was the best I could think off at the time, and it made me wonder why those reset lines aren't modelled this way more often.
This breaks down if any of the drivers actually uses the reset pin to reset the chip - if the reset might not actually happen but the driver wants it to happen then things are going to go wrong.
On 11/01/2012 03:35 PM, Mark Brown wrote:
is a very nice power saving feature. I just cache all the register settings up until that point (which required a bit of coding because of the "paging" of the I2C address space).
Probably easiest to just convert over to regmap, it handles paging fine.
I've backported the driver to 2.6.37 so probably my regmap wasn't smart enough yet.
It was the best I could think off at the time, and it made me wonder why those reset lines aren't modelled this way more often.
This breaks down if any of the drivers actually uses the reset pin to reset the chip - if the reset might not actually happen but the driver wants it to happen then things are going to go wrong.
Good point. But in that case, the gpio cannot be "optional", the board file MUST supply it.
Most chips - if not all, and this one certainly has it - also have a software routine for resetting it.
From a hardware perspective, when using two codecs to get a 4-channel input, it makes perfect sense to use the same GPIO line to reset them both. This will never work with two instances of a driver that wants to control the GPIO line directly, but it works flawlessly when that line is modelled as a power supply. Each instance can request the reset to be relaxed, and each instance gets feedback from the supply core when the reset line is actually being altered (which can for example be used to update the codec's registers from the cache, or to indicate that the chip is no longer online and every write must go to the cache only).
Do I understand correctly that I'm totally alone in this and that there is no point in uploading the code?
Met vriendelijke groet / kind regards,
Mike Looijmans
TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: (+31) – (0)499 - 33.69.79 Telefax: (+31) - (0)499 - 33.69.70 E-mail: mike.looijmans@topic.nl Website: www.topic.nl
Dit e-mail bericht en de eventueel daarbij behorende bijlagen zijn uitsluitend bestemd voor de geadresseerde, zoals die blijkt uit het e-mail bericht en/of de bijlagen. Er kunnen gegevens met betrekking tot een derde instaan. Indien u als niet-geadresseerde dit bericht en de bijlagen ontvangt, terwijl u niet bevoegd of gemachtigd bent om dit bericht namens de geadresseerde te ontvangen, wordt u verzocht de afzender hierover direct te informeren en het e-mail bericht met de bijlagen te vernietigen. Ieder gebruik van de inhoud van het e-mail bericht, waaronder de daarbij behorende bijlagen, door een ander dan de geadresseerde is onrechtmatig jegens ons dan wel de eventueel in het e-mail bericht of de bijlagen voorkomende andere personen. TOPIC Embedded Systems is niet aansprakelijk voor enigerlei schade voortvloeiend uit het gebruik en/of acceptatie van dit e-mail bericht of de daarbij behorende bijlagen.
The contents of this message, as well as any enclosures, are addressed personally to, and thus solely intended for the addressee. They may contain information regarding a third party. A recipient who is neither the addressee, nor empowered to receive this message on behalf of the addressee, is kindly requested to immediately inform the sender of receipt, and to destroy the message and the enclosures. Any use of the contents of this message and/or the enclosures by any other person than the addressee or person who is empowered to receive this message, is illegal towards the sender and/or the aforementioned third party. TOPIC Embedded Systems is not liable for any damage as a result of the use and/or acceptance of this message and as well as any enclosures.
On Thu, Nov 01, 2012 at 04:28:59PM +0100, Mike Looijmans wrote:
This breaks down if any of the drivers actually uses the reset pin to reset the chip - if the reset might not actually happen but the driver wants it to happen then things are going to go wrong.
Good point. But in that case, the gpio cannot be "optional", the board file MUST supply it.
Well, what normally happens is that the driver has some alternative ways of doing what it wants but if there's a reset signal present it wants to actually use it and be able to rely on it doing the job.
Do I understand correctly that I'm totally alone in this and that there is no point in uploading the code?
If you want to do something like this you should explictly model reset lines like this rather than shoehorning them into a different subsystem.
Hi,
On 1 November 2012 15:35, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Oct 31, 2012 at 03:40:17PM +0100, Mike Looijmans wrote:
is a very nice power saving feature. I just cache all the register settings up until that point (which required a bit of coding because of the "paging" of the I2C address space).
Probably easiest to just convert over to regmap, it handles paging fine.
this is something I'd like to do when I have some spare time. Do you know any example of a codec driver that uses a similar approach for accessing registers?
On Mon, Nov 05, 2012 at 09:14:41AM +0100, javier Martin wrote:
On 1 November 2012 15:35, Mark Brown
Probably easiest to just convert over to regmap, it handles paging fine.
this is something I'd like to do when I have some spare time. Do you know any example of a codec driver that uses a similar approach for accessing registers?
The WM2200 uses paging, and grepping for regmap will turn up a bunch of other drivers which use it.
On Wed, Oct 31, 2012 at 11:53:33AM +0100, Javier Martin wrote:
In its previous status, the first capture didn't work properly; nothing was actually recorded from the microphone. This behaviour was observed using a Visstrim M10 board.
Applied both, thanks.
participants (3)
-
Javier Martin -
Mark Brown -
Mike Looijmans