[PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Daniel Mack]

Signed-off-by: Daniel Mack <zonque@gmail.com>
Cc: Hebbar, Gururaja <gururaja.hebbar@ti.com>
Cc: Matt Porter <mporter@ti.com>
---
v2: also enable 24bit in the DAI

 sound/soc/davinci/davinci-mcasp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 1252bad..76728be 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -839,6 +839,14 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		word_length = DAVINCI_AUDIO_WORD_16;
 		break;
 
+	case SNDRV_PCM_FORMAT_U24_3LE:
+	case SNDRV_PCM_FORMAT_S24_3LE:
+	case SNDRV_PCM_FORMAT_U24_LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+		dma_params->data_type = 3;
+		word_length = DAVINCI_AUDIO_WORD_24;
+		break;
+
 	case SNDRV_PCM_FORMAT_U32_LE:
 	case SNDRV_PCM_FORMAT_S32_LE:
 		dma_params->data_type = 4;
@@ -917,6 +925,10 @@ static const struct snd_soc_dai_ops davinci_mcasp_dai_ops = {
 				SNDRV_PCM_FMTBIT_U8 | \
 				SNDRV_PCM_FMTBIT_S16_LE | \
 				SNDRV_PCM_FMTBIT_U16_LE | \
+				SNDRV_PCM_FMTBIT_S24_LE | \
+				SNDRV_PCM_FMTBIT_U24_LE | \
+				SNDRV_PCM_FMTBIT_S24_3LE | \
+				SNDRV_PCM_FMTBIT_U24_3LE | \
 				SNDRV_PCM_FMTBIT_S32_LE | \
 				SNDRV_PCM_FMTBIT_U32_LE)
 
-- 
1.7.11.4

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mark Brown]

On Tue, Oct 09, 2012 at 09:35:16AM +0200, Daniel Mack wrote:
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Cc: Hebbar, Gururaja <gururaja.hebbar@ti.com>
> Cc: Matt Porter <mporter@ti.com>

Applied, thanks.

[PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mike Looijmans]

Sorry for the lack of quoting, but I onle get the digest.

These are wrong:

+       case SNDRV_PCM_FORMAT_U24_LE:
+       case SNDRV_PCM_FORMAT_S24_LE:

These pack a 24-bit sample value in a 32-bit word. The codec will send 32 bits to the McASP, and you should transfer 32 bits to the user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated just like SNDRV_PCM_FORMAT_S32_LE.

I've tested that on a DA850-alike board with several TLV320AIC3256 codecs, treating them as 3-byte samples will reasult in invalid data.


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 enig
 erlei 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.

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Daniel Mack]

On 09.10.2012 11:41, Mike Looijmans wrote:
> Sorry for the lack of quoting, but I onle get the digest.
> 
> These are wrong:
> 
> +       case SNDRV_PCM_FORMAT_U24_LE: +       case
> SNDRV_PCM_FORMAT_S24_LE:
> 
> These pack a 24-bit sample value in a 32-bit word. The codec will
> send 32 bits to the McASP, and you should transfer 32 bits to the
> user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated
> just like SNDRV_PCM_FORMAT_S32_LE.
> 
> I've tested that on a DA850-alike board with several TLV320AIC3256
> codecs, treating them as 3-byte samples will reasult in invalid
> data.

Ok, thanks for reporting this. Would like to send a patch or want me to
fix it?


Daniel

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mike Looijmans]

Please just fix it, the fix is simply to move those "cases" a few lines down. The mcasp code in my tree is severely bashed because i'm using it to record from up to 8 codecs simultaneously...



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 enig
 erlei 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.
________________________________________
Van: Daniel Mack [zonque@gmail.com]
Verzonden: dinsdag 9 oktober 2012 11:47
Aan: Mike Looijmans
CC: alsa-devel@alsa-project.org; Mark Brown
Onderwerp: Re: [alsa-devel] [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

On 09.10.2012 11:41, Mike Looijmans wrote:
> Sorry for the lack of quoting, but I onle get the digest.
>
> These are wrong:
>
> +       case SNDRV_PCM_FORMAT_U24_LE: +       case
> SNDRV_PCM_FORMAT_S24_LE:
>
> These pack a 24-bit sample value in a 32-bit word. The codec will
> send 32 bits to the McASP, and you should transfer 32 bits to the
> user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated
> just like SNDRV_PCM_FORMAT_S32_LE.
>
> I've tested that on a DA850-alike board with several TLV320AIC3256
> codecs, treating them as 3-byte samples will reasult in invalid
> data.

Ok, thanks for reporting this. Would like to send a patch or want me to
fix it?


Daniel

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mark Brown]

On Tue, Oct 09, 2012 at 11:47:57AM +0200, Daniel Mack wrote:
> On 09.10.2012 11:41, Mike Looijmans wrote:
> > Sorry for the lack of quoting, but I onle get the digest.

> > These are wrong:

> > +       case SNDRV_PCM_FORMAT_U24_LE: +       case
> > SNDRV_PCM_FORMAT_S24_LE:

> > These pack a 24-bit sample value in a 32-bit word. The codec will
> > send 32 bits to the McASP, and you should transfer 32 bits to the
> > user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated
> > just like SNDRV_PCM_FORMAT_S32_LE.

> > I've tested that on a DA850-alike board with several TLV320AIC3256
> > codecs, treating them as 3-byte samples will reasult in invalid
> > data.

> Ok, thanks for reporting this. Would like to send a patch or want me to
> fix it?

The above explanation isn't quite right.  For 24 bit the CODEC should be
working with 24 bits on the wire (though obviously extra BCLKs are
allowed) and the AP should be working with 32 bit words in memory.  The
issue here is probably that you're working with real 24 bit data in RAM
too.

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Daniel Mack]

On 09.10.2012 12:24, Mark Brown wrote:
> On Tue, Oct 09, 2012 at 11:47:57AM +0200, Daniel Mack wrote:
>> On 09.10.2012 11:41, Mike Looijmans wrote:
>>> Sorry for the lack of quoting, but I onle get the digest.
> 
>>> These are wrong:
> 
>>> +       case SNDRV_PCM_FORMAT_U24_LE: +       case
>>> SNDRV_PCM_FORMAT_S24_LE:
> 
>>> These pack a 24-bit sample value in a 32-bit word. The codec will
>>> send 32 bits to the McASP, and you should transfer 32 bits to the
>>> user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated
>>> just like SNDRV_PCM_FORMAT_S32_LE.
> 
>>> I've tested that on a DA850-alike board with several TLV320AIC3256
>>> codecs, treating them as 3-byte samples will reasult in invalid
>>> data.
> 
>> Ok, thanks for reporting this. Would like to send a patch or want me to
>> fix it?
> 
> The above explanation isn't quite right.  For 24 bit the CODEC should be
> working with 24 bits on the wire (though obviously extra BCLKs are
> allowed) and the AP should be working with 32 bit words in memory.  The
> issue here is probably that you're working with real 24 bit data in RAM
> too.

Hmm, I don't understand. I thought S24_3LE exists to denote a 3-byte
representation, because S24_LE uses 32 bits? Hence Mike would be right
that S24_LE has to use a 4-byte dma transfer size, no?

I'm using S24_3LE here, hence that didn't hit me.

Where's the confusion? :)

Daniel

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Clemens Ladisch]

Daniel Mack wrote:
> On 09.10.2012 12:24, Mark Brown wrote:
>>> On 09.10.2012 11:41, Mike Looijmans wrote:
>>>> Sorry for the lack of quoting, but I onle get the digest.
>>
>>>> These are wrong:
>>
>>>> +       case SNDRV_PCM_FORMAT_U24_LE:
>>>> +       case SNDRV_PCM_FORMAT_S24_LE:
>>
>>>> These pack a 24-bit sample value in a 32-bit word. The codec will
>>>> send 32 bits to the McASP, and you should transfer 32 bits to the
>>>> user, not just 24. Hence, SNDRV_PCM_FORMAT_S24_LE must be treated
>>>> just like SNDRV_PCM_FORMAT_S32_LE.
>>
>> The above explanation isn't quite right.  For 24 bit the CODEC should be
>> working with 24 bits on the wire (though obviously extra BCLKs are
>> allowed) and the AP should be working with 32 bit words in memory.  The
>> issue here is probably that you're working with real 24 bit data in RAM
>> too.
>
> Hmm, I don't understand. I thought S24_3LE exists to denote a 3-byte
> representation

Yes.

The three possibilities to store little-endian signed 24-bit samples in
memory are (where L=LSB, M=MSB):

  0padding LLLLLLLL MMMMMMMM HHHHHHHH  S32_LE
  LLLLLLLL MMMMMMMM HHHHHHHH 0padding  S24_LE
  LLLLLLLL MMMMMMMM HHHHHHHH           S24_3LE

The first two require the DMA controller to transfer four bytes, the
last one, three bytes.

All three use (at least) 24 bits on the wire.

Mark is right, unless your DMA controller actually happens to support
three-byte samples, with all the alignment problems that entails.
S24_3LE was introduced for USB controllers that are too dumb to
differentiate between memory and on-the-wire formats; nobody else
should want to use it.  (But if the hardware supports it, you might
as well allow it, if you implement it correctly.)

> Hence Mike would be right that S24_LE has to use a 4-byte dma transfer
> size, no?

Yes.


Regards,
Clemens

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mark Brown]

On Tue, Oct 09, 2012 at 02:21:48PM +0200, Clemens Ladisch wrote:

> > Hence Mike would be right that S24_LE has to use a 4-byte dma transfer
> > size, no?

> Yes.

That's what Mike meant but it wasn't quite what he said - he said the
CODEC would send 32 bits which isn't the case, the 32 bits thing only
applies inside the CPU.

Re: [PATCH v2] ALSA: ASoC: McASP: add support for 24 bit samples

[Mike Looijmans]

On 10/09/2012 02:21 PM, Clemens Ladisch wrote:
> The three possibilities to store little-endian signed 24-bit samples in
> memory are (where L=LSB, M=MSB):
>
>    0padding LLLLLLLL MMMMMMMM HHHHHHHH  S32_LE
>    LLLLLLLL MMMMMMMM HHHHHHHH 0padding  S24_LE
>    LLLLLLLL MMMMMMMM HHHHHHHH           S24_3LE
>
> The first two require the DMA controller to transfer four bytes, the
> last one, three bytes.
>
> All three use (at least) 24 bits on the wire.
Oh, I always assumed that S32_LE was just a 32-bit format. That's what 
I've been assuming so far, and the resulting data files from "arecord -f 
{S32_LE|S24_LE|S24_3LE}" were perfectly understood by audio programs.
Basically, what you're claiming here is that S24_LE must store the audio 
data in the lower 3 bytes? Because in my experiments so far I concluded 
I had to fill the high order bits for both S24_LE and S32_LE formats.

> >Hence Mike would be right that S24_LE has to use a 4-byte dma transfer
> >size, no?
> Yes.
It's not strictly what I wrote, but it's what I meant, yes.

Now we're on the topic of DMA controllers:
When reading multiple channels (e.g. 2 or more codecs on the serializers 
either using TDM or multiple AXR pins), the DMA controller must always 
read 32 bits per sample from the serializer, regardless of the sample 
format used.
Currently I implement this by enforcing S32_LE or S24_LE formats, and 
denying all others when multiple serializers are involved.
Would it be possible to instruct the davinci's DMA controller to read 32 
bits but write only 8, 16 or 24 bits to memory for each sample?


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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel