ASoC and a codec that can't be controlled

ASoC and a codec that can't be controlled

[Timur Tabi]

I'm working on some ASoC drivers for a new board, and we're using a CS4270.  The CS4270 is 
interesting in that if it is connected in stand-alone mode, there is no way to configure 
it.  The board wirings determine all the parameters.

Therefore, my CS4270 codec driver will probably be very skimpy.

My question is: does this mean that my cs4270.c file will *never* call these functions:

snd_ctl_add
snd_soc_cnew
snd_soc_dapm_new_control
snd_soc_dapm_connect_input
snd_soc_dapm_new_widgets

These are the functions used to add new controls and widgets.

So how does ALSA know that it needs to call my I2S driver *instead* of my codec driver to 
do stuff like change volume?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Tue, 2007-05-22 at 10:47 -0500, Timur Tabi wrote:
> I'm working on some ASoC drivers for a new board, and we're using a CS4270.  The CS4270 is 
> interesting in that if it is connected in stand-alone mode, there is no way to configure 
> it.  The board wirings determine all the parameters.
> 
> Therefore, my CS4270 codec driver will probably be very skimpy.
> 
> My question is: does this mean that my cs4270.c file will *never* call these functions:
> 
> snd_ctl_add
> snd_soc_cnew

These functions are only required in the codec driver to change volume,
mixer settings etc. If your codec has no volume or mixers then you don't
need them.


> snd_soc_dapm_new_control
> snd_soc_dapm_connect_input
> snd_soc_dapm_new_widgets

These are used for setting up the dynamic audio power management and
won't be needed in stand alone mode.

> 
> These are the functions used to add new controls and widgets.
> 
> So how does ALSA know that it needs to call my I2S driver *instead* of my codec driver to 
> do stuff like change volume?

You probably want to create a volume kcontrol in your I2S driver. I
assume your I2S controller can digitally adjust the volume by altering
the PCM stream ? The kcontrols can be added in your I2S driver probe
function.


Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> These are used for setting up the dynamic audio power management and
> won't be needed in stand alone mode.

So if I don't care about power management, can I completely ignore anything with "dapm" in it?

Also, do I need a codec driver at all, even if I can't control the codec?  That is, can I 
do stuff like this:

static struct snd_soc_device mysoc_snd_devdata = {
	.machine = &snd_soc_machine_mysoc,
	.platform = &mysoc_soc_platform,
};

static struct snd_soc_dai_link mpc8610hpcd_dai = {
	.name = "CS4270",
	.stream_name = "CS4270",
	.cpu_dai = &mysoc_i2s_dai,
	.init = mpc8610hpcd_machine_init,
	.ops = &mpc8610hpcd_ops,
};

?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Fri, 2007-05-25 at 15:17 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > These are used for setting up the dynamic audio power management and
> > won't be needed in stand alone mode.
> 
> So if I don't care about power management, can I completely ignore anything with "dapm" in it?

Yes.

> 
> Also, do I need a codec driver at all, even if I can't control the codec?  That is, can I 
> do stuff like this:
> 
> static struct snd_soc_device mysoc_snd_devdata = {
> 	.machine = &snd_soc_machine_mysoc,
> 	.platform = &mysoc_soc_platform,
> };
> 
> static struct snd_soc_dai_link mpc8610hpcd_dai = {
> 	.name = "CS4270",
> 	.stream_name = "CS4270",
> 	.cpu_dai = &mysoc_i2s_dai,
> 	.init = mpc8610hpcd_machine_init,
> 	.ops = &mpc8610hpcd_ops,
> };

Yes, although my feeling is that a codec "driver" would still be needed
to define the capabilities of your codec within the audio subsystem.

e.g. supported sample rates, interface formats, etc

Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Yes, although my feeling is that a codec "driver" would still be needed
> to define the capabilities of your codec within the audio subsystem.
> 
> e.g. supported sample rates, interface formats, etc

I'm curious - why would the codec driver dictate all that?  On my board, the sample rates are determined by the I2S controller, not the codec, because in order to change the sample rate, I program new divisors into the I2S controller's registers.

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Mon, 2007-05-28 at 19:18 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > Yes, although my feeling is that a codec "driver" would still be needed
> > to define the capabilities of your codec within the audio subsystem.
> > 
> > e.g. supported sample rates, interface formats, etc
> 
> I'm curious - why would the codec driver dictate all that?  On my board, the sample rates are determined by the I2S controller, not the codec, because in order to change the sample rate, I program new divisors into the I2S controller's registers.


Some codecs can only support a small range of sample rates e.g. 8k &
48k, whilst your controller may support more. This just makes sure the
audio is set a rate the codec can handle, otherwise audio quality will
probably suffer.

Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Some codecs can only support a small range of sample rates e.g. 8k &
> 48k, whilst your controller may support more. This just makes sure the
> audio is set a rate the codec can handle, otherwise audio quality will
> probably suffer.

What about boards where the codec does *not* dictate the available sample rates?  That is, 
what if the SOC can't generate the frequencies necessary to drive the codec at all the 
rates it supports?  The codec driver would then need to be told by the machine driver 
which sample rates are going to be used.  When the codec driver is initialized, it needs 
to set the snd_soc_codec.dai field to point to the codec's DAI, and that structure 
contains the list of supported sample rates.

I guess in the PowerPC world, the codec driver can query the codec node in the device 
tree, but ideally there would need to be some standard interface.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:
> On Fri, 2007-05-25 at 15:17 -0500, Timur Tabi wrote:
>> Liam Girdwood wrote:
>>
>>> These are used for setting up the dynamic audio power management and
>>> won't be needed in stand alone mode.
>> So if I don't care about power management, can I completely ignore anything with "dapm" in it?
> 
> Yes.
> 
>> Also, do I need a codec driver at all, even if I can't control the codec?  That is, can I 
>> do stuff like this:
>>
>> static struct snd_soc_device mysoc_snd_devdata = {
>> 	.machine = &snd_soc_machine_mysoc,
>> 	.platform = &mysoc_soc_platform,
>> };
>>
>> static struct snd_soc_dai_link mpc8610hpcd_dai = {
>> 	.name = "CS4270",
>> 	.stream_name = "CS4270",
>> 	.cpu_dai = &mysoc_i2s_dai,
>> 	.init = mpc8610hpcd_machine_init,
>> 	.ops = &mpc8610hpcd_ops,
>> };
> 
> Yes, although my feeling is that a codec "driver" would still be needed
> to define the capabilities of your codec within the audio subsystem.

Another question:

Why do I need to specify the codec DAI in two different structures?

eti_b1_wm8731.c:

static struct snd_soc_dai_link eti_b1_dai = {
	.name = "WM8731",
	.stream_name = "WM8731",
	.cpu_dai = &at91_i2s_dai[1],
	.codec_dai = &wm8731_dai,	<----
	.init = eti_b1_wm8731_init,
	.ops = &eti_b1_ops,
};

wm8731.c:

static int wm8731_init(struct snd_soc_device *socdev)
{
	struct snd_soc_codec *codec = socdev->codec;
...
	int reg, ret = 0;

	codec->dai = &wm8731_dai;	<-----

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Yes, although my feeling is that a codec "driver" would still be needed
> to define the capabilities of your codec within the audio subsystem.

Another question:

Why is it the codec driver that calls snd_soc_new_pcms()?  Shouldn't the PCM driver be 
doing that?

Looking in snd_soc_new_pcms() itself, I see this:

	codec->card = snd_card_new(idx, xid, codec->owner, 0);

Here we create a new "sound card", and we assign it to the

I guess I just don't understand why the codec driver is acting like the "master" driver of 
ASOC.  IMHO, the codec driver should be doing two things:

1) Specifying the capabilities of the codec hardware itself, without any assumption that 
these capabilities dictate the capabilities of the system as a whole (e.g. the codec 
shouldn't assume that the system supports every sampling rate that the codec does).
2) Providing APIs for controlling the codec.

ASOC and the machine driver should then work in tandem to decide which driver will do what 
and which capabilities are *actually* supported.  *Something* needs to look at the entire 
system and say to each device, "Well, yes, I know about this little feature of yours, but 
we're just not going to support that today."

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Yes, although my feeling is that a codec "driver" would still be needed
> to define the capabilities of your codec within the audio subsystem.

Ok, I just noticed this:

at91-i2s.c:

#define AT91_I2S_RATES (SNDRV_PCM_RATE_8000  | SNDRV_PCM_RATE_11025 |\
			SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
			SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
			SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
			SNDRV_PCM_RATE_96000)

struct snd_soc_cpu_dai at91_i2s_dai[NUM_SSC_DEVICES] = {
	{	.name = "at91_ssc0/i2s",
		.id = 0,
		.type = SND_SOC_DAI_I2S,
		.suspend = at91_i2s_suspend,
		.resume = at91_i2s_resume,
		.playback = {
			.channels_min = 1,
			.channels_max = 2,
			.rates = AT91_I2S_RATES,
			.formats = SNDRV_PCM_FMTBIT_S16_LE,},
		.capture = {
			.channels_min = 1,
			.channels_max = 2,
			.rates = AT91_I2S_RATES,
			.formats = SNDRV_PCM_FMTBIT_S16_LE,},

wm8731.c:

#define WM8731_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
		SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
		SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
		SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
		SNDRV_PCM_RATE_96000)

#define WM8731_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
	SNDRV_PCM_FMTBIT_S24_LE)

struct snd_soc_codec_dai wm8731_dai = {
	.name = "WM8731",
	.playback = {
		.stream_name = "Playback",
		.channels_min = 1,
		.channels_max = 2,
		.rates = WM8731_RATES,
		.formats = WM8731_FORMATS,},
	.capture = {
		.stream_name = "Capture",
		.channels_min = 1,
		.channels_max = 2,
		.rates = WM8731_RATES,
		.formats = WM8731_FORMATS,},

So does this mean that ALSA looks at the rate and format capabilities of the I2S interface 
and the codec, and then only chooses ones that both support?

Also, what does it mean for the codec to support little-endian?  On PowerPC, all registers 
are big-endian, so my version of at91-i2s.c has to have SNDRV_PCM_FMTBIT_S24_BE.

Your codec driver says it's little-endian.  But the I2S standard is serial, so endianness 
doesn't apply.  Yet if I say the codec is little-endian, but the I2S interface is 
big-endian, how can ALSA resolve that?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Tue, 2007-05-29 at 13:47 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> > On Fri, 2007-05-25 at 15:17 -0500, Timur Tabi wrote:
> >> Liam Girdwood wrote:
> >>
> >>> These are used for setting up the dynamic audio power management and
> >>> won't be needed in stand alone mode.
> >> So if I don't care about power management, can I completely ignore anything with "dapm" in it?
> > 
> > Yes.
> > 
> >> Also, do I need a codec driver at all, even if I can't control the codec?  That is, can I 
> >> do stuff like this:
> >>
> >> static struct snd_soc_device mysoc_snd_devdata = {
> >> 	.machine = &snd_soc_machine_mysoc,
> >> 	.platform = &mysoc_soc_platform,
> >> };
> >>
> >> static struct snd_soc_dai_link mpc8610hpcd_dai = {
> >> 	.name = "CS4270",
> >> 	.stream_name = "CS4270",
> >> 	.cpu_dai = &mysoc_i2s_dai,
> >> 	.init = mpc8610hpcd_machine_init,
> >> 	.ops = &mpc8610hpcd_ops,
> >> };
> > 
> > Yes, although my feeling is that a codec "driver" would still be needed
> > to define the capabilities of your codec within the audio subsystem.
> 
> Another question:
> 
> Why do I need to specify the codec DAI in two different structures?
> 
> eti_b1_wm8731.c:
> 
> static struct snd_soc_dai_link eti_b1_dai = {
> 	.name = "WM8731",
> 	.stream_name = "WM8731",
> 	.cpu_dai = &at91_i2s_dai[1],
> 	.codec_dai = &wm8731_dai,	<----
> 	.init = eti_b1_wm8731_init,
> 	.ops = &eti_b1_ops,
> };
> 
> wm8731.c:
> 
> static int wm8731_init(struct snd_soc_device *socdev)
> {
> 	struct snd_soc_codec *codec = socdev->codec;
> ...
> 	int reg, ret = 0;
> 
> 	codec->dai = &wm8731_dai;	<-----
> 

Looks like some naming confusion on my part here, so his probably needs
a little refactoring in the codec struct.  

The codec struct contains a pointer to _all_ the digital audio
interfaces it supports (e.g. some codecs support more than 1 DAI),
whilst the link struct contains a mapping between codec interface n and
SoC interface n. In this example the WM8731 only has 1 DAI, so it does
look like duplication.

I'll refactor this in a future patch.

Liam

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Tue, 2007-05-29 at 13:10 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > Some codecs can only support a small range of sample rates e.g. 8k &
> > 48k, whilst your controller may support more. This just makes sure the
> > audio is set a rate the codec can handle, otherwise audio quality will
> > probably suffer.
> 
> What about boards where the codec does *not* dictate the available sample rates?  That is, 
> what if the SOC can't generate the frequencies necessary to drive the codec at all the 
> rates it supports?  The codec driver would then need to be told by the machine driver 
> which sample rates are going to be used.  When the codec driver is initialized, it needs 
> to set the snd_soc_codec.dai field to point to the codec's DAI, and that structure 
> contains the list of supported sample rates.
> 

ASoC creates a bitmask of supported sample rates from the DMA, I2S
controller and codec and then AND's them together. It then checks the
requested rate, etc against this bitmask before proceeding. This way the
whole audio system dictates the supported rate and not any single
component.

Liam

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Tue, 2007-05-29 at 18:05 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > Yes, although my feeling is that a codec "driver" would still be needed
> > to define the capabilities of your codec within the audio subsystem.
> 
> Ok, I just noticed this:
> 
> at91-i2s.c:
> 
> #define AT91_I2S_RATES (SNDRV_PCM_RATE_8000  | SNDRV_PCM_RATE_11025 |\
> 			SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> 			SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> 			SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> 			SNDRV_PCM_RATE_96000)
> 
> struct snd_soc_cpu_dai at91_i2s_dai[NUM_SSC_DEVICES] = {
> 	{	.name = "at91_ssc0/i2s",
> 		.id = 0,
> 		.type = SND_SOC_DAI_I2S,
> 		.suspend = at91_i2s_suspend,
> 		.resume = at91_i2s_resume,
> 		.playback = {
> 			.channels_min = 1,
> 			.channels_max = 2,
> 			.rates = AT91_I2S_RATES,
> 			.formats = SNDRV_PCM_FMTBIT_S16_LE,},
> 		.capture = {
> 			.channels_min = 1,
> 			.channels_max = 2,
> 			.rates = AT91_I2S_RATES,
> 			.formats = SNDRV_PCM_FMTBIT_S16_LE,},
> 
> wm8731.c:
> 
> #define WM8731_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
> 		SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> 		SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> 		SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> 		SNDRV_PCM_RATE_96000)
> 
> #define WM8731_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
> 	SNDRV_PCM_FMTBIT_S24_LE)
> 
> struct snd_soc_codec_dai wm8731_dai = {
> 	.name = "WM8731",
> 	.playback = {
> 		.stream_name = "Playback",
> 		.channels_min = 1,
> 		.channels_max = 2,
> 		.rates = WM8731_RATES,
> 		.formats = WM8731_FORMATS,},
> 	.capture = {
> 		.stream_name = "Capture",
> 		.channels_min = 1,
> 		.channels_max = 2,
> 		.rates = WM8731_RATES,
> 		.formats = WM8731_FORMATS,},
> 
> So does this mean that ALSA looks at the rate and format capabilities of the I2S interface 
> and the codec, and then only chooses ones that both support?
> 

Yes.

> Also, what does it mean for the codec to support little-endian?  On PowerPC, all registers 
> are big-endian, so my version of at91-i2s.c has to have SNDRV_PCM_FMTBIT_S24_BE.
> 
> Your codec driver says it's little-endian.  But the I2S standard is serial, so endianness 
> doesn't apply.  Yet if I say the codec is little-endian, but the I2S interface is 
> big-endian, how can ALSA resolve that?
> 

This was added as some controllers and codecs can shift out data in
different formats (e.g. LSB or MSB first). It's only really used atm for
data size (e.g. 16, 24 bits) as the endianess is currently not dealt
with correctly. I've logged bug :-

https://bugtrack.alsa-project.org/alsa-bug/view.php?id=3132

Fwiw, you should be ok in the mean time as most audio data is stored on
file in little endian format. Your media player should open such little
endian files as *_LE when it configures the ALSA pcm. This should work
if your I2S, DMA and codec are marked as supporting LE formats.

Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Fwiw, you should be ok in the mean time as most audio data is stored on
> file in little endian format. Your media player should open such little
> endian files as *_LE when it configures the ALSA pcm. This should work
> if your I2S, DMA and codec are marked as supporting LE formats.

What about this snippet in asound.h:

#ifdef  __LITTLE_ENDIAN
#define SNDRV_LITTLE_ENDIAN
#else
#ifdef __BIG_ENDIAN
#define SNDRV_BIG_ENDIAN

...

#ifdef SNDRV_LITTLE_ENDIAN
#define	SNDRV_PCM_FORMAT_S16		SNDRV_PCM_FORMAT_S16_LE
#define	SNDRV_PCM_FORMAT_U16		SNDRV_PCM_FORMAT_U16_LE
#define	SNDRV_PCM_FORMAT_S24		SNDRV_PCM_FORMAT_S24_LE
...
#endif
#ifdef SNDRV_BIG_ENDIAN
#define	SNDRV_PCM_FORMAT_S16		SNDRV_PCM_FORMAT_S16_BE
#define	SNDRV_PCM_FORMAT_U16		SNDRV_PCM_FORMAT_U16_BE
#define	SNDRV_PCM_FORMAT_S24		SNDRV_PCM_FORMAT_S24_BE
...
#endif

I could then do this:

#define CS4270_FORMATS SNDRV_PCM_FMTBIT_S24

On big-endian platforms like mine, CS4270_FORMATS will be SNDRV_PCM_FORMAT_S24_BE and on 
little-endian platforms it will be SNDRV_PCM_FORMAT_S24_LE.  Assuming the I2S registers 
are the same endian as the platform, will this work?

If not, then what about this:

#define CS4270_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)

That would tell ALSA that the CS4270 supports both formats, which isn't technically true, 
but it wouldn't matter because the I2S interface is what determines the actual 
"endianness" of the serial data.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

>> I guess I just don't understand why the codec driver is acting like the "master" driver of 
>> ASOC.  IMHO, the codec driver should be doing two things:
> 
> I guess this is due to an ordering issue we had during early
> development. We had to make sure that the I2C probe of the codec
> succeeded before registering the card, pcms, etc. Fwiw, I do intend to
> address this in future versions (I should probably add a road map to the
> wiki now).

In that case, I think we can solve this problem as well as the PowerPC device tree problem 
in one shot, because they're basically the same problem.

Anyway, let me see if I get this right:

1. The first function called is eti_b1_init().
2. eti_b1_init() calls platform_device_add() to add an soc-audio class device and register 
the eti_b1_snd_devdata structure.
3. The registration causes a number of things to happen, one of which is a call to 
soc_probe().
4. soc_probe() sees that the snd_soc_machine_eti_b1.probe is NULL, so that part is skipped.
3. soc_probe() sees that snd_soc_machine_eti_b1.dai_link->cpu_dai->probe is also NULL, so 
that part is skipped. (eti_b1_snd_devdata.machine == snd_soc_machine_eti_b1)
4. soc_probe() sees that soc_codec_dev_wm8731.probe is not NULL, so it calls wm8731_probe().
5. wm8731_probe() registers an I2C driver.
6. The I2C class driver calls wm8731_i2c_driver.attach_adapter, which is wm8731_i2c_attach().
7. wm8731_i2c_attach() calls i2c_probe(), which in turn calls wm8731_codec_probe()
8. wm8731_codec_probe() allocates a snd_soc_codec structure.
8. wm8731_codec_probe() calls wm8731_init().

This means that if there is no I2C support, wm8731_init() will never be called.

9. wm8731_init() initializes the snd_soc_codec structure.

Question: why doesn't wm8731_probe() initialize the non-I2C parts of the snd_soc_codec 
structure?  For example, snd_soc_codec.dai, snd_soc_codec.name, and snd_soc_codec.owner? 
That way, the snd_soc_codec structure will be properly initialized even when there's no I2C.

10. wm8731_init() calls snd_soc_new_pcms.
11. wm8731_init() exits, wm8731_codec_probe() exits, wm8731_i2c_attach() exits, and then 
wm8731_probe() exits.  Control returns to soc_probe().
12. soc_probe() notices that eti_b1_snd_devdata.platform.probe is NULL, so it skips that.

Question: why don't you define a function for at91_soc_platform.probe, and in that 
function you can call snd_soc_new_pcms()?  That way, you guarantee the codec driver's I2C 
interface is initialized before snd_soc_new_pcms() is called, and you avoid have PCM calls 
in the codec driver.

>> ASOC and the machine driver should then work in tandem to decide which driver will do what 
>> and which capabilities are *actually* supported.  *Something* needs to look at the entire 
>> system and say to each device, "Well, yes, I know about this little feature of yours, but 
>> we're just not going to support that today."
>>
> 
> The machine driver pretty much already does this. It can override valid
> hardware configurations and return -EINVAl if required. 

Do you have an example of that?  Would that be eti_b1_hw_params()?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Wed, 2007-05-30 at 13:10 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> >> I guess I just don't understand why the codec driver is acting like the "master" driver of 
> >> ASOC.  IMHO, the codec driver should be doing two things:
> > 
> > I guess this is due to an ordering issue we had during early
> > development. We had to make sure that the I2C probe of the codec
> > succeeded before registering the card, pcms, etc. Fwiw, I do intend to
> > address this in future versions (I should probably add a road map to the
> > wiki now).
> 

Wiki roadmap now added.

https://bugtrack.alsa-project.org/wiki/wikka.php?wakka=ASoCRoadMap

> In that case, I think we can solve this problem as well as the PowerPC device tree problem 
> in one shot, because they're basically the same problem.
> 
> Anyway, let me see if I get this right:
> 
> 1. The first function called is eti_b1_init().
> 2. eti_b1_init() calls platform_device_add() to add an soc-audio class device and register 
> the eti_b1_snd_devdata structure.
> 3. The registration causes a number of things to happen, one of which is a call to 
> soc_probe().
> 4. soc_probe() sees that the snd_soc_machine_eti_b1.probe is NULL, so that part is skipped.
> 3. soc_probe() sees that snd_soc_machine_eti_b1.dai_link->cpu_dai->probe is also NULL, so 
> that part is skipped. (eti_b1_snd_devdata.machine == snd_soc_machine_eti_b1)
> 4. soc_probe() sees that soc_codec_dev_wm8731.probe is not NULL, so it calls wm8731_probe().
> 5. wm8731_probe() registers an I2C driver.
> 6. The I2C class driver calls wm8731_i2c_driver.attach_adapter, which is wm8731_i2c_attach().
> 7. wm8731_i2c_attach() calls i2c_probe(), which in turn calls wm8731_codec_probe()
> 8. wm8731_codec_probe() allocates a snd_soc_codec structure.
> 8. wm8731_codec_probe() calls wm8731_init().
> 
> This means that if there is no I2C support, wm8731_init() will never be called.
> 
> 9. wm8731_init() initializes the snd_soc_codec structure.
> 
> Question: why doesn't wm8731_probe() initialize the non-I2C parts of the snd_soc_codec 
> structure?  For example, snd_soc_codec.dai, snd_soc_codec.name, and snd_soc_codec.owner? 
> That way, the snd_soc_codec structure will be properly initialized even when there's no I2C.
> 

This codec only supports I2C, so it shouldn't matter too much. But it
would be better for codecs using other IO mechanisms.

> 10. wm8731_init() calls snd_soc_new_pcms.
> 11. wm8731_init() exits, wm8731_codec_probe() exits, wm8731_i2c_attach() exits, and then 
> wm8731_probe() exits.  Control returns to soc_probe().
> 12. soc_probe() notices that eti_b1_snd_devdata.platform.probe is NULL, so it skips that.
> 
> Question: why don't you define a function for at91_soc_platform.probe, and in that 
> function you can call snd_soc_new_pcms()?  That way, you guarantee the codec driver's I2C 
> interface is initialized before snd_soc_new_pcms() is called, and you avoid have PCM calls 
> in the codec driver.
> 

I think the best place to call snd_soc_new_pcms is in the machine
driver. This means we don't have to add any pcms that are not used. 

> >> ASOC and the machine driver should then work in tandem to decide which driver will do what 
> >> and which capabilities are *actually* supported.  *Something* needs to look at the entire 
> >> system and say to each device, "Well, yes, I know about this little feature of yours, but 
> >> we're just not going to support that today."
> >>
> > 
> > The machine driver pretty much already does this. It can override valid
> > hardware configurations and return -EINVAl if required. 
> 
> Do you have an example of that?  Would that be eti_b1_hw_params()?

Yes, the hw_params could return error on anything the machine didn't
like. e.g. this could be used to workaround quirks. 

Liam

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Wed, 2007-05-30 at 10:46 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > Fwiw, you should be ok in the mean time as most audio data is stored on
> > file in little endian format. Your media player should open such little
> > endian files as *_LE when it configures the ALSA pcm. This should work
> > if your I2S, DMA and codec are marked as supporting LE formats.
> 
> What about this snippet in asound.h:
> 
> #ifdef  __LITTLE_ENDIAN
> #define SNDRV_LITTLE_ENDIAN
> #else
> #ifdef __BIG_ENDIAN
> #define SNDRV_BIG_ENDIAN
> 
> ...
> 
> #ifdef SNDRV_LITTLE_ENDIAN
> #define	SNDRV_PCM_FORMAT_S16		SNDRV_PCM_FORMAT_S16_LE
> #define	SNDRV_PCM_FORMAT_U16		SNDRV_PCM_FORMAT_U16_LE
> #define	SNDRV_PCM_FORMAT_S24		SNDRV_PCM_FORMAT_S24_LE
> ...
> #endif
> #ifdef SNDRV_BIG_ENDIAN
> #define	SNDRV_PCM_FORMAT_S16		SNDRV_PCM_FORMAT_S16_BE
> #define	SNDRV_PCM_FORMAT_U16		SNDRV_PCM_FORMAT_U16_BE
> #define	SNDRV_PCM_FORMAT_S24		SNDRV_PCM_FORMAT_S24_BE
> ...
> #endif
> 
> I could then do this:
> 
> #define CS4270_FORMATS SNDRV_PCM_FMTBIT_S24

I'll fix this in the core so we don't need to do this. Please use _LE
atm.

> 
> On big-endian platforms like mine, CS4270_FORMATS will be SNDRV_PCM_FORMAT_S24_BE and on 
> little-endian platforms it will be SNDRV_PCM_FORMAT_S24_LE.  Assuming the I2S registers 
> are the same endian as the platform, will this work?
> 

I think this probably depends on how your audio FIFO's align the data
received from memory/DMA.  They should do any bit reordering based on
the audio format, and size etc set in the control register. (Although,
ymmv.)


> If not, then what about this:
> 
> #define CS4270_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
> 
> That would tell ALSA that the CS4270 supports both formats, which isn't technically true, 
> but it wouldn't matter because the I2S interface is what determines the actual 
> "endianness" of the serial data.
> 

This looks fine as a workaround atm, I'll try and have a look at this
bug tomorrow so we don't need to do this.

Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

>> On big-endian platforms like mine, CS4270_FORMATS will be SNDRV_PCM_FORMAT_S24_BE and on 
>> little-endian platforms it will be SNDRV_PCM_FORMAT_S24_LE.  Assuming the I2S registers 
>> are the same endian as the platform, will this work?
>>
> 
> I think this probably depends on how your audio FIFO's align the data
> received from memory/DMA.  They should do any bit reordering based on
> the audio format, and size etc set in the control register. (Although,
> ymmv.)

Ok, I'm a little confused.  I was going to specify the SNDRV_PCM_FORMAT_S24_BE for my I2C 
device because the actual 32-bit memory-mapped registers are big-endian.  The structure 
definition even uses __be32 for each field.

My goal was to specify a single value (i.e. SNDRV_PCM_FORMAT_S24_BE) such that ALSA would 
give me a 32-bit quantity that exactly matches what my device expects.

I wish asound.h had more documentation.  I don't know what any of these macros *really* mean.

>> #define CS4270_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
>>
>> That would tell ALSA that the CS4270 supports both formats, which isn't technically true, 
>> but it wouldn't matter because the I2S interface is what determines the actual 
>> "endianness" of the serial data.
> 
> This looks fine as a workaround atm, I'll try and have a look at this
> bug tomorrow so we don't need to do this.

Ok, I'll do this for now.



-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> I think the best place to call snd_soc_new_pcms is in the machine
> driver. This means we don't have to add any pcms that are not used. 

True, but the machine driver is probed before the codec driver is probed.  So if you 
really need the codec driver to initialize the I2C bus first, then you've got a problem. 
Re-arranging the order of probes in soc_probe() is not really a solution.

So on your baord, why does the I2C interface need to be running before anything else can 
be probed?

> Yes, the hw_params could return error on anything the machine didn't
> like. e.g. this could be used to workaround quirks. 

Would ALSA know to try something else, if a particular combination was rejected?



-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: ASoC and a codec that can't be controlled

[Liam Girdwood]

On Thu, 2007-05-31 at 14:49 -0500, Timur Tabi wrote:
> Liam Girdwood wrote:
> 
> > I think the best place to call snd_soc_new_pcms is in the machine
> > driver. This means we don't have to add any pcms that are not used. 
> 
> True, but the machine driver is probed before the codec driver is probed.  So if you 
> really need the codec driver to initialize the I2C bus first, then you've got a problem. 
> Re-arranging the order of probes in soc_probe() is not really a solution.
> 
> So on your baord, why does the I2C interface need to be running before anything else can 
> be probed?

Imho, there is little point in probing or allocating any more resources
until we know the I2C codec probe succeeds. We would just have to free
any resources allocated up to that point. 


> 
> > Yes, the hw_params could return error on anything the machine didn't
> > like. e.g. this could be used to workaround quirks. 
> 
> Would ALSA know to try something else, if a particular combination was rejected?
> 

This would be up to your user space application. In general all common
sample rates and formats are supported by most drivers e.g. 44.1k
stereo. If some combinations are rejected, it's most likely due to
hardware limitations.

Liam

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> Imho, there is little point in probing or allocating any more resources
> until we know the I2C codec probe succeeds. We would just have to free
> any resources allocated up to that point. 

True, but if the I2C probe fails, then I would think that the whole system is shot, so this sounds like a very unlikely situation.  If you changed the code to clean up after an I2C failure, you could make the whole thing more modular, including eliminating the call to snd_soc_new() in the codec driver.

Since ASoC divides everything into four more-or-less independent drivers, it makes sense that it should be able to handle a situation where one of those drivers just fails to load.

Re: ASoC and a codec that can't be controlled

[Timur Tabi]

Liam Girdwood wrote:

> I think the best place to call snd_soc_new_pcms is in the machine
> driver. This means we don't have to add any pcms that are not used. 

Are you talking about the snd_soc_dai_link.init() function?  If so, then  where do you 
call snd_soc_free_pcms()?  The snd_soc_dai_link structure has no "exit" function.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale