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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
