On Tue, 12 Sep 2017 16:15:58 +0200, Jörg Krause wrote:
On Tue, 2017-09-12 at 09:12 +0200, Takashi Iwai wrote:
On Tue, 12 Sep 2017 09:02:47 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
This indeed is S24_LE.
Which is an extremely uncommon format. If possible, the driver should be changed to align to the sample's MSB to the memory word's MSB, and then label it as S32_LE.
S24_LE cannot be handled with the same algorithm as S32_LE.
To be precise: the sign in bit 23 must be preserved.
I guess the simple calculation with S24_LE using the current code would work as long as the upper 8 bits are ignored by hardware. The upper 8 bits would hold bogus bits, and clearing this would be an extra action we'd need in addition.
Thanks! I've copied the area convertion macro for S24_3LE and explicitly set the upper 8 bits to 0:
""" #define CONVERT_AREA_S24_LE() do { \ unsigned int ch, fr; \ unsigned char *src, *dst; \ int tmp; \ for (ch = 0; ch < channels; ch++) { \ src_area = &src_areas[ch]; \ dst_area = &dst_areas[ch]; \ src = snd_pcm_channel_area_addr(src_area, src_offset); \ dst = snd_pcm_channel_area_addr(dst_area, dst_offset); \ src_step = snd_pcm_channel_area_step(src_area); \ dst_step = snd_pcm_channel_area_step(dst_area); \ GET_VOL_SCALE; \ fr = frames; \ if (! vol_scale) { \ while (fr--) { \ dst[0] = dst[1] = dst[2] = dst[3] = 0; \ dst += dst_step; \ } \ } else if (vol_scale == 0xffff) { \ while (fr--) { \ dst[0] = src[0]; \ dst[1] = src[1]; \ dst[2] = src[2]; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } else { \ while (fr--) { \ tmp = src[0] | \ (src[1] << 8) | \ (((signed char *) src)[2] << 16); \ tmp = MULTI_DIV_24(tmp, vol_scale); \ dst[0] = tmp; \ dst[1] = tmp >> 8; \ dst[2] = tmp >> 16; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } \ } \ } while (0) """
Is that okay?
You can simply mask 0xffffff (or 0xffffff00, depending on CPU-native or not) to the int value instead of assigning / calculating each byte. And it's needed only when vol_scale !=0 && vol_scale != 0xffff.
thanks,
Takashi