[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.
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 470fbfb4b386..31b6ae2334bc 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, Dec 13, 2013 at 02:58:19PM +0200, Peter Ujfalusi wrote:
- case SNDRV_PCM_FORMAT_S24_LE:
- case SNDRV_PCM_FORMAT_S24_3LE: data |= (0x02 << 4); break;
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
On 12/13/2013 03:04 PM, Mark Brown wrote:
On Fri, Dec 13, 2013 at 02:58:19PM +0200, Peter Ujfalusi wrote:
- case SNDRV_PCM_FORMAT_S24_LE:
- case SNDRV_PCM_FORMAT_S24_3LE: data |= (0x02 << 4); break;
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
Not really since the codec has only field to specify the data format. The codec can not support S24_LE (S24_LE is basically S32_LE msbits==24) since we can not say to the codec to ignore the 8bit over the 24 bits of real data. In case of S24_3LE the I2S bus will have 24 clocks/per channel which can not be used to stream S24_LE either.
On Fri, Dec 13, 2013 at 03:29:33PM +0200, Peter Ujfalusi wrote:
On 12/13/2013 03:04 PM, Mark Brown wrote:
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
Not really since the codec has only field to specify the data format. The codec can not support S24_LE (S24_LE is basically S32_LE msbits==24) since we can not say to the codec to ignore the 8bit over the 24 bits of real data. In case of S24_3LE the I2S bus will have 24 clocks/per channel which can not be used to stream S24_LE either.
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
Like I say we ought to be handling this stuff in the core :/
At Fri, 13 Dec 2013 13:34:44 +0000, Mark Brown wrote:
On Fri, Dec 13, 2013 at 03:29:33PM +0200, Peter Ujfalusi wrote:
On 12/13/2013 03:04 PM, Mark Brown wrote:
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
Not really since the codec has only field to specify the data format. The codec can not support S24_LE (S24_LE is basically S32_LE msbits==24) since we can not say to the codec to ignore the 8bit over the 24 bits of real data. In case of S24_3LE the I2S bus will have 24 clocks/per channel which can not be used to stream S24_LE either.
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
Like I say we ought to be handling this stuff in the core :/
But this codec driver declares the available formats as:
#define AIC3X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
so the fix for inconsistency is needed anyway.
Takashi
On Fri, Dec 13, 2013 at 02:41:44PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
Like I say we ought to be handling this stuff in the core :/
But this codec driver declares the available formats as:
#define AIC3X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
so the fix for inconsistency is needed anyway.
Ah, indeed - and that one should go to stable. The changelog ought to be more accurate though.
On 12/13/2013 03:34 PM, Mark Brown wrote:
On Fri, Dec 13, 2013 at 03:29:33PM +0200, Peter Ujfalusi wrote:
On 12/13/2013 03:04 PM, Mark Brown wrote:
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
Not really since the codec has only field to specify the data format. The codec can not support S24_LE (S24_LE is basically S32_LE msbits==24) since we can not say to the codec to ignore the 8bit over the 24 bits of real data. In case of S24_3LE the I2S bus will have 24 clocks/per channel which can not be used to stream S24_LE either.
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
We can not change the HW... for example: twl4030/twl5030: 32 clock cycle/channel and 24 bits used out of that. tlv320aic3106: 24 clock cycle/channel for 24 bit audio.
The wire behavior is different and this need to be known by the CPU side as well.
On Fri, Dec 13, 2013 at 03:57:24PM +0200, Peter Ujfalusi wrote:
On 12/13/2013 03:34 PM, Mark Brown wrote:
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
We can not change the HW... for example:
This isn't a hardware issue, this is a software issue.
twl4030/twl5030: 32 clock cycle/channel and 24 bits used out of that.
These shouldn't be advertising themselves as supporting 24 bit format for ASoC then since they need 32 bit samples on the bus - this is actually pretty standard for things that say they support 32 bits, they usually just discard some of the lower bits but can cope with 32 bit on the interface.
The reason everything says 24_LE right now is that we're being dumb about how we do the constraints and CPUs do care about the memory layout, not because the CODECs require the blank cycles. Of course this means that almost anything that does I2S should be able to advertise arbatrary sample size support which is one of the resons we ought to be doing stuff in the core to make this nicer.
tlv320aic3106: 24 clock cycle/channel for 24 bit audio.
This is normal ASoC behaviour.
The wire behavior is different and this need to be known by the CPU side as well.
This isn't the way to do it, though - if the TWL drivers were ever used with other CPUs (admittedly unlikely) they'd run into trouble.
On 12/13/2013 04:18 PM, Mark Brown wrote:
On Fri, Dec 13, 2013 at 03:57:24PM +0200, Peter Ujfalusi wrote:
On 12/13/2013 03:34 PM, Mark Brown wrote:
No, I'd expect the wire behaviour to be identical for any 24 bit samples (that's certainly what most drivers are written for). The memory layout differences shouldn't be visible to CODEC drivers.
We can not change the HW... for example:
This isn't a hardware issue, this is a software issue.
twl4030/twl5030: 32 clock cycle/channel and 24 bits used out of that.
These shouldn't be advertising themselves as supporting 24 bit format for ASoC then since they need 32 bit samples on the bus - this is actually pretty standard for things that say they support 32 bits, they usually just discard some of the lower bits but can cope with 32 bit on the interface.
The reason everything says 24_LE right now is that we're being dumb about how we do the constraints and CPUs do care about the memory layout, not because the CODECs require the blank cycles. Of course this means that almost anything that does I2S should be able to advertise arbatrary sample size support which is one of the resons we ought to be doing stuff in the core to make this nicer.
Hrm, not sure about other SoCs but on OMAP most of the formats from memory ends up on the I2S bus without any bits added/removed so the format actually applies to the codec side as well since the data arriving to the codec will have the same layout as it had in the memory. But, yes some level of logic in the core might help.
tlv320aic3106: 24 clock cycle/channel for 24 bit audio.
This is normal ASoC behaviour.
The wire behavior is different and this need to be known by the CPU side as well.
This isn't the way to do it, though - if the TWL drivers were ever used with other CPUs (admittedly unlikely) they'd run into trouble.
The twl drivers (and the dac33) is doing the correct thing IMHO: reporting S32_LE with set .sig_bits = 24. Just to let the user space know that thay should not use the whole 32bit range.
On Fri, Dec 13, 2013 at 04:31:40PM +0200, Peter Ujfalusi wrote:
Hrm, not sure about other SoCs but on OMAP most of the formats from memory ends up on the I2S bus without any bits added/removed so the format actually applies to the codec side as well since the data arriving to the codec will have the same layout as it had in the memory. But, yes some level of logic in the core might help.
For every other CPU or CODEC where I know what it does the number of bits expected on the bus is the sample size.
This isn't the way to do it, though - if the TWL drivers were ever used with other CPUs (admittedly unlikely) they'd run into trouble.
The twl drivers (and the dac33) is doing the correct thing IMHO: reporting S32_LE with set .sig_bits = 24. Just to let the user space know that thay should not use the whole 32bit range.
Ah, OK - that's good and correct. Your mail made me think they were using one of the S24 formats.
On 12/13/2013 02:29 PM, Peter Ujfalusi wrote:
On 12/13/2013 03:04 PM, Mark Brown wrote:
On Fri, Dec 13, 2013 at 02:58:19PM +0200, Peter Ujfalusi wrote:
- case SNDRV_PCM_FORMAT_S24_LE:
- case SNDRV_PCM_FORMAT_S24_3LE: data |= (0x02 << 4); break;
This should be adding the case for the new format rather than replacing the old one shouldn't it? They ought to turn out the same on the AIF so the CODECs shouldn't care about the difference, ideally the core would hide the difference from them.
Not really since the codec has only field to specify the data format. The codec can not support S24_LE (S24_LE is basically S32_LE msbits==24) since we
S24_LE is a 24 bit sample in a 32 bit word, but it is right aligned, so there are 8 bits of leading padding, while with S32_LE msbits=24 you'd have 8bits of trailing padding.
can not say to the codec to ignore the 8bit over the 24 bits of real data. In case of S24_3LE the I2S bus will have 24 clocks/per channel which can not be used to stream S24_LE either.
Normally you'd expect the I2S core to only put the 24 bits of data onto the bus for both S24_3LE and S24_LE (it might add necessary trailing padding if the bit clocks per frame is > 24). A CODEC driver should really not have to care about the in memory layout of the data since all it will see is a serialized bit stream. I think ideally we wouldn't check for params_format() but rather for snd_pcm_format_width(params_format()).
- Lars
On Fri, Dec 13, 2013 at 02:42:32PM +0100, Lars-Peter Clausen wrote:
Normally you'd expect the I2S core to only put the 24 bits of data onto the bus for both S24_3LE and S24_LE (it might add necessary trailing padding if the bit clocks per frame is > 24). A CODEC driver should really not have to care about the in memory layout of the data since all it will see is a serialized bit stream. I think ideally we wouldn't check for params_format() but rather for snd_pcm_format_width(params_format()).
Yes, that would be a much better approach. In fact, let me go attack that...
participants (4)
-
Lars-Peter Clausen -
Mark Brown -
Peter Ujfalusi -
Takashi Iwai