ASoC: PCM formats for AC97

ASoC: PCM formats for AC97

[Jon Smirl]

Tell me if this is right and I'll do a patch....

-----------------------------------------------------------
The current mpc5200 code has:

static const struct snd_pcm_hardware psc_dma_hardware = {
	.formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE |
		SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE,

struct snd_soc_dai psc_ac97_dai[] = {
{
	.playback = {
		.formats = SNDRV_PCM_FMTBIT_S32_BE,

I think I have this backwards, the ac97 driver can take 8/16/24/32
it's the Bestcomm program that doesn't know about anything except 32b.
So if we wrote new Bestcomm programs that expanded bytes from memory
out into 32b DMA register writes, we could support 8/16b formats.

The correct formats would be:
static const struct snd_pcm_hardware psc_dma_hardware = {
	.formats = SNDRV_PCM_FMTBIT_S32_BE,

struct snd_soc_dai psc_ac97_dai[] = {
{
	.playback = {
		.formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE |
			SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE,

After implementing new Bestcomm programs....

static const struct snd_pcm_hardware psc_dma_hardware = {
	.formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE |
		SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE,

-----------------------------------------------------------
A related issue in soc-dai.h:

#define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16_LE |\
                               SNDRV_PCM_FMTBIT_S32_LE |\
                               SNDRV_PCM_FMTBIT_S32_BE)

shouldn't this be
#define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16 |\
                               SNDRV_PCM_FMTBIT_S32)

The first form says the AC97 hardware can take both endians, but it
doesn't take both endians it takes the endian matching the machine it
is attached to.
Don't the standard AC97 formats also include 8 and 24b?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: ASoC: PCM formats for AC97

[Mark Brown]

On Sat, Aug 08, 2009 at 02:44:48PM -0400, Jon Smirl wrote:

> I think I have this backwards, the ac97 driver can take 8/16/24/32
> it's the Bestcomm program that doesn't know about anything except 32b.

Remember, you need to consider the I2S driver as well.

> #define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16_LE |\
>                                SNDRV_PCM_FMTBIT_S32_LE |\
>                                SNDRV_PCM_FMTBIT_S32_BE)
> shouldn't this be

> #define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16 |\
>                                SNDRV_PCM_FMTBIT_S32)

> The first form says the AC97 hardware can take both endians, but it
> doesn't take both endians it takes the endian matching the machine it
> is attached to.

No, it can take any endianess - the wire format is fixed.  The layout of
the data in memory is of little interest to the CODEC.  The formats used
will be restricted by the CPU driver, the values there are essentially
functioning as a wildcard match.

> Don't the standard AC97 formats also include 8 and 24b?

They'll include whatever the controller is happy to render down onto the
bus.  That's just the ones that people have needed so far.

Re: ASoC: PCM formats for AC97

[Jon Smirl]

On Sun, Aug 9, 2009 at 6:48 AM, Mark Brown<broonie@sirena.org.uk> wrote:
> On Sat, Aug 08, 2009 at 02:44:48PM -0400, Jon Smirl wrote:
>
>> I think I have this backwards, the ac97 driver can take 8/16/24/32
>> it's the Bestcomm program that doesn't know about anything except 32b.
>
> Remember, you need to consider the I2S driver as well.
>
>> #define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16_LE |\
>>                                SNDRV_PCM_FMTBIT_S32_LE |\
>>                                SNDRV_PCM_FMTBIT_S32_BE)
>> shouldn't this be
>
>> #define SND_SOC_STD_AC97_FMTS (SNDRV_PCM_FMTBIT_S16 |\
>>                                SNDRV_PCM_FMTBIT_S32)
>
>> The first form says the AC97 hardware can take both endians, but it
>> doesn't take both endians it takes the endian matching the machine it
>> is attached to.
>
> No, it can take any endianess - the wire format is fixed.  The layout of
> the data in memory is of little interest to the CODEC.  The formats used
> will be restricted by the CPU driver, the values there are essentially
> functioning as a wildcard match.

I agree with you, but I think the second form better indicates the
wildcard than the first form. It makes it look like endianess is not a
factor.

If you had used the second form I wouldn't have had a problem bringing
up a powerpc driver.

>> Don't the standard AC97 formats also include 8 and 24b?
>
> They'll include whatever the controller is happy to render down onto the
> bus.  That's just the ones that people have needed so far.

We could add 8/24b now so to make thing easier on the next driver developer.


>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: ASoC: PCM formats for AC97

[Mark Brown]

On Sun, Aug 09, 2009 at 09:32:22AM -0400, Jon Smirl wrote:

> I agree with you, but I think the second form better indicates the
> wildcard than the first form. It makes it look like endianess is not a
> factor.

You need to specify both - the controller may be able to handle data in
either endianness, not just the native format.

Re: ASoC: PCM formats for AC97

[Jon Smirl]

On Sun, Aug 9, 2009 at 10:37 AM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, Aug 09, 2009 at 09:32:22AM -0400, Jon Smirl wrote:
>
>> I agree with you, but I think the second form better indicates the
>> wildcard than the first form. It makes it look like endianess is not a
>> factor.
>
> You need to specify both - the controller may be able to handle data in
> either endianness, not just the native format.

I thought endianess was fixed in the wire protocol standard. It's the
host AC97 controller that is converting the endianess.

I struggled with this when first writing the host driver for my BE
powerpc chip. I kept trying to encode the AC97 wire protocol
definition into the AC97 codec. The trick is that you don't want to
encode it since it is fixed by the standard.

If we declared the AC97 codecs like this, which they really are:
.formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE |
		SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,

Then I would need a way to say my powerpc host AC97 controller needs
BE on the input and that it converts it to LE on the output. ALSA
doesn't have a way to say that.

Leaving the endianess off from the AC97 codec definition makes it implicit.
.formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16 |
		SNDRV_PCM_FMTBIT_S24 | SNDRV_PCM_FMTBIT_S32,
We don't need to declare an endianess because it is fixed in the standard.
Now the ifdefs in the h file make this work for both BE and LE.

This also hides another implicit conversion. The AC97 codecs only take
24b. Again it is the host controller that is converting 8/16/32 to
24b.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: ASoC: PCM formats for AC97

[Mark Brown]

On Sun, Aug 09, 2009 at 11:10:24AM -0400, Jon Smirl wrote:

> We don't need to declare an endianess because it is fixed in the standard.
> Now the ifdefs in the h file make this work for both BE and LE.

The ifdefs in the header file define only the native endianness.
There's no reason why an AC97 controller can't do byte swapping for
itself but if we use the non-specific constants only native endianness
will be advertised by the CODEC.

> This also hides another implicit conversion. The AC97 codecs only take
> 24b. Again it is the host controller that is converting 8/16/32 to
> 24b.

This is exactly why we're defining this standard format for AC97
devices.  The formats are just placeholders to make the standard
matching stuff work without having to teach it about AC97, they have no
influence on the behaviour of the CODEC.

Re: ASoC: PCM formats for AC97

[Jon Smirl]

On Sun, Aug 9, 2009 at 3:03 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, Aug 09, 2009 at 11:10:24AM -0400, Jon Smirl wrote:
>
>> We don't need to declare an endianess because it is fixed in the standard.
>> Now the ifdefs in the h file make this work for both BE and LE.
>
> The ifdefs in the header file define only the native endianness.
> There's no reason why an AC97 controller can't do byte swapping for
> itself but if we use the non-specific constants only native endianness
> will be advertised by the CODEC.

So the BE/LE are there to support the case of an AC97 host that can
handle both endians.  I think the mpc5200 can do that for I2S but not
AC97.

>> This also hides another implicit conversion. The AC97 codecs only take
>> 24b. Again it is the host controller that is converting 8/16/32 to
>> 24b.
>
> This is exactly why we're defining this standard format for AC97
> devices.  The formats are just placeholders to make the standard
> matching stuff work without having to teach it about AC97, they have no
> influence on the behaviour of the CODEC.
>

-- 
Jon Smirl
jonsmirl@gmail.com