[PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[Charles Keepax]

Currently DAI playback and capture widgets are created during
soc_probe_codec and soc_probe_platform, using the device associated with
the DAI to check which widgets should be created. If a device registers
both a CODEC and platform driver this leads the CODEC playback and
capture widgets being overwritten by the widgets created by the platform
probe.

It is more sensible to retain the CODEC widgets as the most common use
case for registering both a CODEC and platform driver on the same chip
is a CODEC which contains a DSP for compressed playback. In this
situation it is more sensible to attach the routing information to the
CODEC and add a thin platform driver interface to link into the
compressed API.

So this patch will check for existing widgets during soc_probe_platform
and only create new widgets if no existing ones exist.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 04af2a6..53efe1d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1159,7 +1159,8 @@ static int soc_probe_platform(struct snd_soc_card *card,
 
 	/* Create DAPM widgets for each DAI stream */
 	list_for_each_entry(dai, &dai_list, list) {
-		if (dai->dev != platform->dev)
+		if (dai->dev != platform->dev ||
+		    dai->playback_widget || dai->capture_widget)
 			continue;
 
 		snd_soc_dapm_new_dai_widgets(&platform->dapm, dai);
-- 
1.7.2.5

Re: [PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 317 bytes --]

On Thu, Jan 24, 2013 at 09:49:11AM +0000, Charles Keepax wrote:

> So this patch will check for existing widgets during soc_probe_platform
> and only create new widgets if no existing ones exist.

Why not do these extra checks in new_dai_widgets() itself?  That'd be
cover more cases and generally seems more robust.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 332 bytes --]

On Thu, Jan 24, 2013 at 09:49:11AM +0000, Charles Keepax wrote:

> So this patch will check for existing widgets during soc_probe_platform
> and only create new widgets if no existing ones exist.

...or as I was sending that it occurred to me that it'd be even neater
to share the DAPM context, though that's much more refactoring.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[Charles Keepax]

On Sat, Jan 26, 2013 at 05:20:21PM +0800, Mark Brown wrote:
> On Thu, Jan 24, 2013 at 09:49:11AM +0000, Charles Keepax wrote:
> 
> > So this patch will check for existing widgets during soc_probe_platform
> > and only create new widgets if no existing ones exist.
> 
> ...or as I was sending that it occurred to me that it'd be even neater
> to share the DAPM context, though that's much more refactoring.

Looking at this in more detail sharing the DAPM context doesn't
fix the issue. The problem is related to overwriting the widgets
on the DAI which means any routes added to the old widgets are
no longer considered when DAPM processes the DAI. So I will sent
in a patch which does the check in snd_soc_dapm_new_dai_widgets
as that is indeed a more robust and general solution.

However, that said there is some argument for sharing the context
anyway as there is no need for the one device to have two
contexts associated with it, however I am not sure it is worth
it. The most sensible way I can see to do so is to replace the
dapm structs in snd_soc_platform and snd_soc_codec with pointers
and dynamically allocate the dapm contexts. However this is a
fair amount of dynamic allocation and leaves a lot of user code
that needs updating (albiet trivially), all just to avoid a
pointlessly duplicated context that does no harm. I do have some
patches moving down this direction whilst I was investigating it
so let me know if this is something we might be interested in
seeing and I will look to find some time to get them into a state
they could be pushed out for comments.

Charles

[PATCH v2] ASoC: dapm: Add support for platform and CODEC drivers on same device

[Charles Keepax]

Currently DAI playback and capture widgets are created during
soc_probe_codec and soc_probe_platform using the device associated
with the platform/codec to locate the DAI upon which the widgets
should be created. If a device registers both a CODEC and platform
driver this leads the widgets created during CODEC probe being
overwritten by the widgets created during the platform probe.

In general we want to retain the existing playback and capture widgets
because routes may have already been added to them. This patch will
check for existing widgets during snd_soc_dapm_new_dai_widgets and
only create new widgets if none exist.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-dapm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 33acd8b..6959304 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3351,7 +3351,7 @@ int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm,
 	memset(&template, 0, sizeof(template));
 	template.reg = SND_SOC_NOPM;
 
-	if (dai->driver->playback.stream_name) {
+	if (dai->driver->playback.stream_name && !dai->playback_widget) {
 		template.id = snd_soc_dapm_dai;
 		template.name = dai->driver->playback.stream_name;
 		template.sname = dai->driver->playback.stream_name;
@@ -3369,7 +3369,7 @@ int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm,
 		dai->playback_widget = w;
 	}
 
-	if (dai->driver->capture.stream_name) {
+	if (dai->driver->capture.stream_name && !dai->capture_widget) {
 		template.id = snd_soc_dapm_dai;
 		template.name = dai->driver->capture.stream_name;
 		template.sname = dai->driver->capture.stream_name;
-- 
1.7.2.5

Re: [PATCH v2] ASoC: dapm: Add support for platform and CODEC drivers on same device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1759 bytes --]

On Fri, Mar 15, 2013 at 03:38:21PM +0000, Charles Keepax wrote:
> Currently DAI playback and capture widgets are created during
> soc_probe_codec and soc_probe_platform using the device associated

Don't bury patches in the middle of threads, especially if it's several
months after the fact - it just makes it more likely that the patch will
be missed or ignored.

Anyway now I remember the full context...

> with the platform/codec to locate the DAI upon which the widgets
> should be created. If a device registers both a CODEC and platform
> driver this leads the widgets created during CODEC probe being
> overwritten by the widgets created during the platform probe.

> In general we want to retain the existing playback and capture widgets
> because routes may have already been added to them. This patch will
> check for existing widgets during snd_soc_dapm_new_dai_widgets and
> only create new widgets if none exist.

This still seems like it's bodging things in the wrong place.  The issue
is that we're trying to instantiate the DAI multiple times because we've
not got a firm idea of the chip as a whole.  Adding checks like this to
the code is just going to make everything more and more complicated to
understand over time without addressing the source of complexity.

This is especially true now that Morimoto-san has added component
objects which exist to provide that sort of grouping, we should be
addressing things like this by pulling them up to the component level
so that we know we're only doing them once per chip.  I think the first
step here is to add the ability to create DAIs based on components, then
to let components sit where CODECs sit in terms of setting up DAIs so we
can start moving CODEC drivers over into components.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: core: Add support for platform and CODEC drivers on same device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1646 bytes --]

On Fri, Mar 15, 2013 at 03:35:40PM +0000, Charles Keepax wrote:
> On Sat, Jan 26, 2013 at 05:20:21PM +0800, Mark Brown wrote:
> > On Thu, Jan 24, 2013 at 09:49:11AM +0000, Charles Keepax wrote:

> > > So this patch will check for existing widgets during soc_probe_platform
> > > and only create new widgets if no existing ones exist.

> > ...or as I was sending that it occurred to me that it'd be even neater
> > to share the DAPM context, though that's much more refactoring.

> Looking at this in more detail sharing the DAPM context doesn't
> fix the issue. The problem is related to overwriting the widgets
> on the DAI which means any routes added to the old widgets are
> no longer considered when DAPM processes the DAI. So I will sent

I don't think you've fully understood my suggestion, or what's involved
in much more refactoring.  I can't fully recall the context since it's
been several months since the discussion came up but I suspect the thing
here is that the widgets are mostly a function of the DAPM context so if
we share the context we share the widgets and don't end up creating
multiple copies.

> However, that said there is some argument for sharing the context
> anyway as there is no need for the one device to have two
> contexts associated with it, however I am not sure it is worth
> it. The most sensible way I can see to do so is to replace the
> dapm structs in snd_soc_platform and snd_soc_codec with pointers
> and dynamically allocate the dapm contexts. However this is a

Why would we need to dynamically allocate anything here?  The DAIs
should all be owned by something (the CODEC or, now, the component).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]