[alsa-devel] dev_* output functions and ASoC codecs
am i doing something wrong or are the dev_* output functions kind of useless with ASoC codecs ? it seems like the error output is prefixed by the generic "soc-audio" but no info related to the exact codec is included.
as an example, the typical behavior is for the machine driver to call platform_device_alloc("soc-audio", -1). this struct device is then passed down to the relevant subsystem driver register/probe (i2c/spi/etc...) to the codec driver. these functions typically then set the snd_soc_codec's dev member to the allocated platform device. which means the snd_soc_codec's name member isnt included in decoding. so now any dev_* output in the codec driver looks like: soc-audio soc-audio: some message
if there are multiple codecs, things obviously fall apart quickly. the question is how to address this. werent there snd_* output funcs before ? or are those now deprecated/dead ? or perhaps we should encourage people to stop doing platform_device_alloc("soc-audio") and start doing platform_device_alloc(snd_soc_card.name) ?
also, there seems to be a semi-common bug in the codec/machine drivers where they dont actually initialize the dev member. perhaps it's time to upgrade the snd_soc_register_codec() check from a KERN_WARNING to a BUG_ON() ? clearly the current warning isnt getting its message through ... -mike
On Mon, Oct 05, 2009 at 08:18:03PM -0400, Mike Frysinger wrote:
am i doing something wrong or are the dev_* output functions kind of useless with ASoC codecs ? it seems like the error output is prefixed by the generic "soc-audio" but no info related to the exact codec is included.
You're missing something here.
as an example, the typical behavior is for the machine driver to call platform_device_alloc("soc-audio", -1). this struct device is then
This is the device for the ASoC card, not for the CODEC. The ASoC card is built up from several different devices, the main ones being the devices for the CPU and for the CODEC.
passed down to the relevant subsystem driver register/probe (i2c/spi/etc...) to the codec driver. these functions typically then set the snd_soc_codec's dev member to the allocated platform device.
No, nothing should ever be setting codec->dev to the soc-audio device. If anything is that's a bug and should be fixed. That should be set to whatever the device model struct device is for the CODEC, normally an I2C or SPI device.
if there are multiple codecs, things obviously fall apart quickly. the question is how to address this. werent there snd_* output funcs before ? or are those now deprecated/dead ? or perhaps we should
Those have never really been used within ASoC.
encourage people to stop doing platform_device_alloc("soc-audio") and start doing platform_device_alloc(snd_soc_card.name) ?
At some point, probably before the end of the year, it should be possible to have any device registered as the card. However, this is a separate issue to the device used for the CODEC itself.
also, there seems to be a semi-common bug in the codec/machine drivers where they dont actually initialize the dev member. perhaps it's time to upgrade the snd_soc_register_codec() check from a KERN_WARNING to a BUG_ON() ? clearly the current warning isnt getting its message through ...
That would make all the existing drivers instabuggy which isn't a helpful way of dealing with things. At some point where this becomes a blocker to doing further core work someone will have to go through and convert all the remaining CODEC drivers over to use the device model. The main problem here is that most of the affected CODEC drivers are essentially unmaintained.
We use many global variables in asoc now, except defining two/multi groups of global variables to fulfill two/multi codecs work at the same time in one system, is there any other way?
On Tue, Oct 6, 2009 at 6:20 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Oct 05, 2009 at 08:18:03PM -0400, Mike Frysinger wrote:
am i doing something wrong or are the dev_* output functions kind of useless with ASoC codecs ? it seems like the error output is prefixed by the generic "soc-audio" but no info related to the exact codec is included.
You're missing something here.
as an example, the typical behavior is for the machine driver to call platform_device_alloc("soc-audio", -1). this struct device is then
This is the device for the ASoC card, not for the CODEC. The ASoC card is built up from several different devices, the main ones being the devices for the CPU and for the CODEC.
passed down to the relevant subsystem driver register/probe (i2c/spi/etc...) to the codec driver. these functions typically then set the snd_soc_codec's dev member to the allocated platform device.
No, nothing should ever be setting codec->dev to the soc-audio device. If anything is that's a bug and should be fixed. That should be set to whatever the device model struct device is for the CODEC, normally an I2C or SPI device.
if there are multiple codecs, things obviously fall apart quickly. the question is how to address this. werent there snd_* output funcs before ? or are those now deprecated/dead ? or perhaps we should
Those have never really been used within ASoC.
encourage people to stop doing platform_device_alloc("soc-audio") and start doing platform_device_alloc(snd_soc_card.name) ?
At some point, probably before the end of the year, it should be possible to have any device registered as the card. However, this is a separate issue to the device used for the CODEC itself.
also, there seems to be a semi-common bug in the codec/machine drivers where they dont actually initialize the dev member. perhaps it's time to upgrade the snd_soc_register_codec() check from a KERN_WARNING to a BUG_ON() ? clearly the current warning isnt getting its message through ...
That would make all the existing drivers instabuggy which isn't a helpful way of dealing with things. At some point where this becomes a blocker to doing further core work someone will have to go through and convert all the remaining CODEC drivers over to use the device model. The main problem here is that most of the affected CODEC drivers are essentially unmaintained.
On Mon, Oct 12, 2009 at 10:26:09AM +0800, Barry Song wrote:
Please don't top post and please start new threads for new topics.
We use many global variables in asoc now, except defining two/multi groups of global variables to fulfill two/multi codecs work at the same time in one system, is there any other way?
Could you be more specific about which variables you are talking about here? In general you should just use the struct device driver data or a private data field in one of the ASoC structures for any per-device data, in much the same way as you would do for any other system.
On Mon, Oct 12, 2009 at 5:36 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Oct 12, 2009 at 10:26:09AM +0800, Barry Song wrote:
Please don't top post and please start new threads for new topics.
We use many global variables in asoc now, except defining two/multi groups of global variables to fulfill two/multi codecs work at the same time in one system, is there any other way?
Could you be more specific about which variables you are talking about here? In general you should just use the struct device driver data or a private data field in one of the ASoC structures for any per-device data, in much the same way as you would do for any other system.
Let me take wm8903 as an example. In codec: "static struct snd_soc_codec *wm8903_codec" is a global variable to describe a codec. The global variable limit the codec driver can only support one to work. If we use num_links = 2, it seems codec_dai for wm8903 should be duplicated too.
In CPU dai: in case there are two same I2S interfaces connecting two same wm8903, then an array with two elements for CPU dai is needed. For the two struct snd_soc_dai, all fields(like probe, remove) are same except private_data.
In machin driver: snd_soc_dai_link connect the multi same CPU dai and codec dai together.
If we don't use num_links =2, we need to call platform_device_alloc("soc-audio",...)/platform_device_add() twice with duplicated struct snd_soc_device.
Don't know whether I lost something.
On Mon, Oct 12, 2009 at 06:34:56PM +0800, Barry Song wrote:
On Mon, Oct 12, 2009 at 5:36 PM, Mark Brown
In codec: "static struct snd_soc_codec *wm8903_codec" is a global variable to describe a codec. The global variable limit the codec driver can only support one to work.
Right, currently the WM8903 driver only allows one instance - this will be addressed in the future but for now the core doesn't really support this. Addressing this is the main reason why the drivers need to be converted away from the old style of probing to using the device model and supplying struct deviecs - without doing that it's not possible to support more than one instance of the same CODEC.
In CPU dai: in case there are two same I2S interfaces connecting two same wm8903, then an array with two elements for CPU dai is needed.
I'm sorry, I can't quite parse what you're saying here. If you have two CPU DAIs then you need an array to hold them but that seems self evident so is presumably not what you mean?
In machin driver: snd_soc_dai_link connect the multi same CPU dai and codec dai together.
If we don't use num_links =2, we need to call platform_device_alloc("soc-audio",...)/platform_device_add() twice with duplicated struct snd_soc_device.
Again, I'm not 100% clear what you're saying here. If you have a CODEC with two DAIs which you need to connect to two CPU drivers then you will need two dai_links. If you have two unrelated sound subsystems in your system then you should end up with two separate sound devices at the ALSA level. I understand that this currently works OK, though it's not really supported.
On Mon, Oct 12, 2009 at 6:48 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Oct 12, 2009 at 06:34:56PM +0800, Barry Song wrote:
On Mon, Oct 12, 2009 at 5:36 PM, Mark Brown
In codec: "static struct snd_soc_codec *wm8903_codec" is a global variable to describe a codec. The global variable limit the codec driver can only support one to work.
Right, currently the WM8903 driver only allows one instance - this will be addressed in the future but for now the core doesn't really support this. Addressing this is the main reason why the drivers need to be converted away from the old style of probing to using the device model and supplying struct deviecs - without doing that it's not possible to support more than one instance of the same CODEC.
In CPU dai: in case there are two same I2S interfaces connecting two same wm8903, then an array with two elements for CPU dai is needed.
I'm sorry, I can't quite parse what you're saying here. If you have two CPU DAIs then you need an array to hold them but that seems self evident so is presumably not what you mean?
In machin driver: snd_soc_dai_link connect the multi same CPU dai and codec dai together.
If we don't use num_links =2, we need to call platform_device_alloc("soc-audio",...)/platform_device_add() twice with duplicated struct snd_soc_device.
Again, I'm not 100% clear what you're saying here. If you have a CODEC with two DAIs which you need to connect to two CPU drivers then you will need two dai_links. If you have two unrelated sound subsystems in your system then you should end up with two separate sound devices at the ALSA level. I understand that this currently works OK, though it's not really supported.
Thanks a lot. Fortunately, You parsed my meaning well. I just want to say it seems not too economic to have arrays with almost same members while CPU interfaces and codecs are same for all dai_links. dai[0] and dai[1] in array are just same. It seems that will make more sense if we can dynamically probe multi same devices with one DAI information but not several repeated DAIs just as other bus/device/driver framework probes multi-instances by matching. In case we can provide a way to match and bind cpu DAI and codec DAI dynamically, for example placing cpu and codec dai name wit an index in dai_link but not referring C variables directly, then one DAI will support multi instances while matching happens. And it will also eliminate the compiling dependency that the current mach driver is accessing cpu/codec variables directly.
Of course I think the current way is clear too. But people maybe feel strange if they see completely same elements in DAI arrays :-)
On Mon, Oct 12, 2009 at 09:20:18PM +0800, Barry Song wrote:
Thanks a lot. Fortunately, You parsed my meaning well. I just want to say it seems not too economic to have arrays with almost same members while CPU interfaces and codecs are same for all dai_links. dai[0] and dai[1] in array are just same. It seems that will make more sense if
The current plan is to allow a dev_name plus DAI index instead of the pointer to be used to establish the link, though I can't guarantee that particular approach until it's implemented.
On Mon, Oct 12, 2009 at 10:20 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Oct 12, 2009 at 09:20:18PM +0800, Barry Song wrote:
Thanks a lot. Fortunately, You parsed my meaning well. I just want to say it seems not too economic to have arrays with almost same members while CPU interfaces and codecs are same for all dai_links. dai[0] and dai[1] in array are just same. It seems that will make more sense if
The current plan is to allow a dev_name plus DAI index instead of the pointer to be used to establish the link, though I can't guarantee that particular approach until it's implemented.
Another issue is that there is only one global soc_ac97_ops in the whole system. In case there are two different ac97 ports, how to handle? Even though the two ac97 ports are same, how could CPU DAI related private data can be given to snd_ac97 to let it use that data to execute different operations? It seems snd_ac97 is only attached to codec.
On Tue, Oct 13, 2009 at 06:09:07PM +0800, Barry Song wrote:
Another issue is that there is only one global soc_ac97_ops in the whole system. In case there are two different ac97 ports, how to handle?
Even though the two ac97 ports are same, how could CPU DAI related private data can be given to snd_ac97 to let it use that data to execute different operations? It seems snd_ac97 is only attached to codec.
I've no current plan to work on multiple AC97 controllers support within ASoC except in so far as it falls out of other work - I've never encountered an embedded system with multiple AC97 controllers so it's a somewhat academic issue. Patches welcome, though I'd expect it'd be sensible to wait until multi-card support is present.
On Tue, Oct 13, 2009 at 6:14 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Oct 13, 2009 at 06:09:07PM +0800, Barry Song wrote:
Another issue is that there is only one global soc_ac97_ops in the whole system. In case there are two different ac97 ports, how to handle?
Even though the two ac97 ports are same, how could CPU DAI related private data can be given to snd_ac97 to let it use that data to execute different operations? It seems snd_ac97 is only attached to codec.
I've no current plan to work on multiple AC97 controllers support within ASoC except in so far as it falls out of other work - I've never encountered an embedded system with multiple AC97 controllers so it's a somewhat academic issue. Patches welcome, though I'd expect it'd be sensible to wait until multi-card support is present.
Currently, this patch should can help people to set platform_data to snd_ac97, and different operation can be fulfilled based on different ac97_pdata even with only one ac97 ops instance.
author Marek Vasut marek.vasut@gmail.com Wed, 22 Jul 2009 11:01:03 +0000 (13:01 +0200) commit 474828a40f6ddab6e2a3475a19c5c84aa3ec7d60
ALSA: Allow passing platform_data to devices attached to AC97 bus
This patch allows passing platform_data to devices attached to AC97 bus (like touchscreens, battery measurement chips ...).
Signed-off-by: Marek Vasut marek.vasut@gmail.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
On Tue, Oct 20, 2009 at 01:27:14PM +0800, Barry Song wrote:
On Tue, Oct 13, 2009 at 6:14 PM, Mark Brown
I've no current plan to work on multiple AC97 controllers support within ASoC except in so far as it falls out of other work - I've never encountered an embedded system with multiple AC97 controllers so it's a somewhat academic issue. Patches welcome, though I'd expect it'd be sensible to wait until multi-card support is present.
Currently, this patch should can help people to set platform_data to snd_ac97, and different operation can be fulfilled based on different ac97_pdata even with only one ac97 ops instance.
It's possible that this would work, though it would need explicit support in any driver that were to implement it and I'd not guarantee that all the special cases for AC97 would cope properly. Hopefully it'd be relatively straightforward to fix up any issues, though.
participants (3)
-
Barry Song -
Mark Brown -
Mike Frysinger