Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 03/27/2014 10:39 AM, Daniel Mack wrote:
> Hi Peter,
> 
> Thanks for your review.
> 
> On 03/27/2014 09:16 AM, Peter Ujfalusi wrote:
>> On 03/26/2014 05:04 PM, Daniel Mack wrote:
>>> In DIT (S/PDIF) mode, program the transmitted user bits to reflect the
>>> configured sample rate, along with some other details.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 53 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
>>> index 712a7cd..ec0463a 100644
>>> --- a/sound/soc/davinci/davinci-mcasp.c
>>> +++ b/sound/soc/davinci/davinci-mcasp.c
>>> @@ -27,6 +27,7 @@
>>>  #include <linux/of_platform.h>
>>>  #include <linux/of_device.h>
>>>  
>>> +#include <sound/asoundef.h>
>>>  #include <sound/core.h>
>>>  #include <sound/pcm.h>
>>>  #include <sound/pcm_params.h>
>>> @@ -566,8 +567,11 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
>>>  }
>>>  
>>>  /* S/PDIF */
>>> -static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
>>> +static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp,
>>> +			      unsigned int rate)
>>>  {
>>> +	u32 val = 0;
>>> +
>>>  	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
>>>  	   and LSB first */
>>>  	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, TXROT(6) | TXSSZ(15));
>>> @@ -589,6 +593,53 @@ static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
>>>  	/* Enable the DIT */
>>>  	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
>>>  
>>> +	/* Set S/PDIF channel status bits */
>>> +	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 0,
>>> +		      IEC958_AES0_CON_NOT_COPYRIGHT);
>>
>> I think it would be safer to set the channel status bits like:
>> u32 val = 0;
>> u8 *bytes = (u8*) &val;
>>
>> byte[0] |= IEC958_AES0_CON_NOT_COPYRIGHT;
>> byte[1] |= IEC958_AES1_CON_PCM_CODER;
> 
> No, these defines are mapped on to 32-bit values, as seen in
> include/sound/asounddef.h. Over all 6 registers, 192 bits can be stored,
> which is the full length of channel status bits. Hence, they really need
> an individual mcasp_set_reg() each.

I don't think they are mapped for the 32bit:
#define IEC958_AES0_CON_NOT_COPYRIGHT   (1<<2)

#define IEC958_AES1_CON_DIGDIGCONV_ID   0x02
#define IEC958_AES1_CON_PCM_CODER       (IEC958_AES1_CON_DIGDIGCONV_ID|0x00)

#define IEC958_AES3_CON_FS_48000        (2<<0)  /* 48kHz */
#define IEC958_AES3_CON_FS_32000        (3<<0)  /* 32kHz */
#define IEC958_AES3_CON_FS_22050        (4<<0)  /* 22.05kHz */
#define IEC958_AES3_CON_FS_24000        (6<<0)  /* 24kHz */

From the AES3 spec (byte numbers are 1 based while bit numbers are 0 based for
some reason in the specs):
Copyright bit is on Byte1's bit 2
Byte2 has the category code
and Byte4's 0-3 bits for the sampling frequency.

All of the setting you are changing ar in the first 32bit of the whole
channels status array, which is 24 byte long.

> Also, I've measured the output of the S/PDIF port with external S/PDIF
> introspectors and the result is correct :)

Well you essentially writing to the correct byte offset but I don't think in a
correct way:

write to DAVINCI_MCASP_DITCSRA_REG + 0 is a write starting at Byte1
write to DAVINCI_MCASP_DITCSRA_REG + 1 is a write starting at Byte2
write to DAVINCI_MCASP_DITCSRA_REG + 3 is a write starting at Byte4

The interconnect I think is clever enough to figure out that you try to write
to non 32bit boundary and does corrects the access (shifting the data to it's
correct place) but I don't think we should rely on that. It is much simpler to
configure the bits first and write it as one 32bit.


-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel