[alsa-devel] [PATCH] ASoC: tlv320aic3x: Correct S24_3LE support
Correct the hw_params callback to configure the codec correctly in case of S24_3LE format. S24_LE is not defined as supported format for the codec.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/tlv320aic3x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index e12fafbb1e09..5360772bc1ad 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -879,7 +879,7 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream, case SNDRV_PCM_FORMAT_S20_3LE: data |= (0x01 << 4); break; - case SNDRV_PCM_FORMAT_S24_LE: + case SNDRV_PCM_FORMAT_S24_3LE: data |= (0x02 << 4); break; case SNDRV_PCM_FORMAT_S32_LE:
On Fri, Jun 06, 2014 at 04:05:31PM +0300, Peter Ujfalusi wrote:
Correct the hw_params callback to configure the codec correctly in case of S24_3LE format. S24_LE is not defined as supported format for the codec.
These should both be supported by the time things get down to the wire - the memory layout shouldn't matter. I really need to get round to doing the rest of the conversions to use params_width() which should ensure this.
On 06/09/2014 11:24 PM, Mark Brown wrote:
On Fri, Jun 06, 2014 at 04:05:31PM +0300, Peter Ujfalusi wrote:
Correct the hw_params callback to configure the codec correctly in case of S24_3LE format. S24_LE is not defined as supported format for the codec.
These should both be supported by the time things get down to the wire - the memory layout shouldn't matter.
The codec itself has support for 16, 20, 24 and 32 bits data. The 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.
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. 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).
So I still think that this is a bit more complicated than it looks
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 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.
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).
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).
On Tue, Jun 24, 2014 at 09:23:28AM +0300, Peter Ujfalusi wrote:
On 06/23/2014 06:18 PM, Mark Brown wrote:
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?
No - when the format is S32_LE then that's 32 bits.
participants (2)
-
Mark Brown -
Peter Ujfalusi