dev_* output functions and ASoC codecs

dev_* output functions and ASoC codecs

[Mike Frysinger]

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

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.

Re: dev_* output functions and ASoC codecs

[Barry Song]

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

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.

Re: dev_* output functions and ASoC codecs

[Barry Song]

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

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.

Re: dev_* output functions and ASoC codecs

[Barry Song]

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

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.

Re: dev_* output functions and ASoC codecs

[Barry Song]

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.

>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.

Re: dev_* output functions and ASoC codecs

[Barry Song]

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>

>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: dev_* output functions and ASoC codecs

[Mark Brown]

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.