Re: [PATCH] pcm: softvol: add support for S24_LE

["Jörg Krause" <joerg.krause@embedded.rocks>]

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?

Jörg Krause
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel