Re: [PATCH] ASoC: tlv320aic3x: Correct S24_3LE support

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

On 06/23/2014 06:18 PM, Mark Brown wrote:
> On Mon, Jun 23, 2014 at 04:01:13PM +0300, Peter Ujfalusi wrote:
> 
>> AIC3X_FORMATS is correct:
>> #define AIC3X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
>> 			SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
> 
>> While in hw_params the switch was not handling the S24_3LE but it was defined
>> as S24_LE. So the code was not correct: It advertised that it supportsd
>> S24_3LE and was not handling it, but it was handling S24_LE which would not
>> happen with this codec driver since the format is not supported - as it was
>> defined by the AIC3X_FORMATS.
> 
>> This is a fix for the current driver.
> 
> Yes, but the fix should be to support both.

I would rather fix the S24_3LE with this patch and add the S24_LE with a
separate one. But IMHO the S24_LE support is not obvious.

>>> I really need to get round to doing
>>> the rest of the conversions to use params_width() which should ensure
>>> this.
> 
>> Not sure how this would work at the end to be honest for all platforms and codecs.
>> How should the bitclock need to be configured for S24_3LE, S24_LE and S32_LE
>> msbits=24? They are at the end have 24bits audio data but in memory they are
>> stored in different ways.
> 
> The number of bits on the wire should be the number specified in the
> format.

So you are saying that for S24_3LE, S24_LE and S32_LE (msbits=24) we should
have 24 bits on the wire?
At least twl4030/5030 expects the 24bit data on the wire with 32 clocks (24
bits data + 8bits padding).
OMAP McBSP can only support 24bit audio with S32_LE (msbits=24) and it needs
to have 32 clock cycle on the bus to do this due to system constraints.

So if we say that S32_LE (msbits=24) should have 24 bits on the wire neither
OMAP McBSP nor twl4030/5030 could do that.

Not sure about other SoCs, but I would not be so surprised that they also want
to shift out all the bits they have got form the main memory, ie 24 bits in
case of S24_3LE, 32bits in case of S24_LE or S32_LE (msbits=24).

>> Aic3x for example need to be configured to 24bit mode (24 clocks/channel if it
>> is master) so it is going to 'pull' that amount from the CPU side. This can
>> not be used with OMAP's sDMA/McBSP but davinci's eDMA/McASP can support it. I
>> can even get daVinci to 'extract' the valid 24 bits from the 32bit sample
>> coming via eDMA (mask/rotate/reverse operation).
> 
> Sure, so it only supports exactly 24 bit sample formats in master mode.
> That's what's expected by default.
> 
>> So I still think that this is a bit more complicated than it looks
> 
> I'm not seeing anything particularly hard here.  The main thing is that
> we ought to do something for dealing with extra BCLKs (eg, a device that
> can tolerate extra BCLKs can have the other end generating more data
> quite happily).
> 


-- 
Péter