[1/3] ASoC: omap-abe-twl6040: No need to register DMIC routes seperatly
diff mbox

Message ID 1394302616-15991-1-git-send-email-lars@metafoo.de
State Changes Requested
Headers show

Commit Message

Lars-Peter Clausen March 8, 2014, 6:16 p.m. UTC
When using table based DAPM setup there is no need to register DAPM elements for
different sub-components separately. The widgets will be registered before the
first sub-component is initialized, the routes are only added after the last
sub-component has been initialized, meaning everything will be available when it
is needed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/omap/omap-abe-twl6040.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

Comments

Peter Ujfalusi March 10, 2014, 8:13 a.m. UTC | #1
Hi,

On 03/08/2014 08:16 PM, Lars-Peter Clausen wrote:
> When using table based DAPM setup there is no need to register DAPM elements for
> different sub-components separately. The widgets will be registered before the
> first sub-component is initialized, the routes are only added after the last
> sub-component has been initialized, meaning everything will be available when it
> is needed.

The reason why we add the DMIC routes in the way we do is that not all boards
have DMIC in use. PandaBoards does not have DMIC while SDP/Blaze have. On
PandaBoard we do not register the dmic dai link so the widgets are not going
to be added and also the dmic DAI and codec will be not loaded on PandaBoards.
I think this will cause some warning because of missing "DMic" widget?
Lars-Peter Clausen March 10, 2014, 8:18 a.m. UTC | #2
On 03/10/2014 09:13 AM, Peter Ujfalusi wrote:
> Hi,
>
> On 03/08/2014 08:16 PM, Lars-Peter Clausen wrote:
>> When using table based DAPM setup there is no need to register DAPM elements for
>> different sub-components separately. The widgets will be registered before the
>> first sub-component is initialized, the routes are only added after the last
>> sub-component has been initialized, meaning everything will be available when it
>> is needed.
>
> The reason why we add the DMIC routes in the way we do is that not all boards
> have DMIC in use. PandaBoards does not have DMIC while SDP/Blaze have. On
> PandaBoard we do not register the dmic dai link so the widgets are not going
> to be added and also the dmic DAI and codec will be not loaded on PandaBoards.
> I think this will cause some warning because of missing "DMic" widget?
>

Hm, ok, missed that part. Makes sense. I'll respin the patch to just use the 
card's DAPM context when registering the DMIC DAPM routes.

- Lars
Mark Brown March 10, 2014, 9:12 a.m. UTC | #3
On Mon, Mar 10, 2014 at 09:18:50AM +0100, Lars-Peter Clausen wrote:

> Hm, ok, missed that part. Makes sense. I'll respin the patch to just
> use the card's DAPM context when registering the DMIC DAPM routes.

In general anything keying this stuff off DT is going to have that sort
of thing going on - most if not all of the things that are registering
in several chunks are doing so because some of it is conditional.
Lars-Peter Clausen March 10, 2014, 9:24 a.m. UTC | #4
On 03/10/2014 10:12 AM, Mark Brown wrote:
> On Mon, Mar 10, 2014 at 09:18:50AM +0100, Lars-Peter Clausen wrote:
>
>> Hm, ok, missed that part. Makes sense. I'll respin the patch to just
>> use the card's DAPM context when registering the DMIC DAPM routes.
>
> In general anything keying this stuff off DT is going to have that sort
> of thing going on - most if not all of the things that are registering
> in several chunks are doing so because some of it is conditional.
>

Before we had the support for card table based setup you'd had to register 
them in chunks, because the components became available one after another 
and you couldn't register DAPM elements for one component in the callback of 
another one. When using the table based setup the widgets are registered 
before all components are registered and the routes are registered after all 
the components have been registered.
Mark Brown March 10, 2014, 10:10 a.m. UTC | #5
On Mon, Mar 10, 2014 at 10:24:49AM +0100, Lars-Peter Clausen wrote:
> On 03/10/2014 10:12 AM, Mark Brown wrote:

> >In general anything keying this stuff off DT is going to have that sort
> >of thing going on - most if not all of the things that are registering
> >in several chunks are doing so because some of it is conditional.

> Before we had the support for card table based setup you'd had to
> register them in chunks, because the components became available one
> after another and you couldn't register DAPM elements for one
> component in the callback of another one. When using the table based
> setup the widgets are registered before all components are
> registered and the routes are registered after all the components
> have been registered.

No, that's not the case at all - what makes you think that's the case?
All table based setup did was move things from code to data, I'm not
sure what you mean by registering DAPM elements from one component in
the callback for another.  The only thing that should be registering
things for other components is the card and the general idea was to
register everything in late_probe().
Lars-Peter Clausen March 10, 2014, 10:27 a.m. UTC | #6
On 03/10/2014 11:10 AM, Mark Brown wrote:
> On Mon, Mar 10, 2014 at 10:24:49AM +0100, Lars-Peter Clausen wrote:
>> On 03/10/2014 10:12 AM, Mark Brown wrote:
>
>>> In general anything keying this stuff off DT is going to have that sort
>>> of thing going on - most if not all of the things that are registering
>>> in several chunks are doing so because some of it is conditional.
>
>> Before we had the support for card table based setup you'd had to
>> register them in chunks, because the components became available one
>> after another and you couldn't register DAPM elements for one
>> component in the callback of another one. When using the table based
>> setup the widgets are registered before all components are
>> registered and the routes are registered after all the components
>> have been registered.
>
> No, that's not the case at all - what makes you think that's the case?

The code ;)

> All table based setup did was move things from code to data, I'm not
> sure what you mean by registering DAPM elements from one component in
> the callback for another.

I meant the card level DAPM elements that are related to a certain 
component. Not the DAPM elements of the component itself.

> The only thing that should be registering
> things for other components is the card and the general idea was to
> register everything in late_probe().
>

What most machine drivers did before card table based setup is to have a 
per-component callback in which they did register the card DAPM elements 
associated with that component. E.g. routes from the CODEC output to a 
speaker, etc. You couldn't register the card level DAPM elements for one 
component in the init callback component of another one since the routes 
depend on the widgets being registered first. So those board drivers kept 
separate routes and widget tables for separate components. With table based 
setup for the card the routes are registered after all components have been 
registered, which means you can register all the routes via one table since 
all the dependencies are ready. Same is true if you use the card's 
late_probe callback to register the DAPM routes and widgets.

- Lars
Mark Brown March 10, 2014, 11:05 a.m. UTC | #7
On Mon, Mar 10, 2014 at 11:27:39AM +0100, Lars-Peter Clausen wrote:
> On 03/10/2014 11:10 AM, Mark Brown wrote:

> >The only thing that should be registering
> >things for other components is the card and the general idea was to
> >register everything in late_probe().

> What most machine drivers did before card table based setup is to
> have a per-component callback in which they did register the card
> DAPM elements associated with that component. E.g. routes from the
> CODEC output to a speaker, etc. You couldn't register the card level
> DAPM elements for one component in the init callback component of
> another one since the routes depend on the widgets being registered
> first. So those board drivers kept separate routes and widget tables
> for separate components. With table based setup for the card the

I'm sorry but this just isn't what was happening at all.  Remember that
most of the code that you're looking at only ever had one component so
it really never made any difference which callback people happened to
pick so long as it was one of the ones that ran after the CODEC was up.

The only driver that I can think of which did anything like what you
describe was smdk-wm8580 and that was still only using one component,
it's just that Jassi preferred to split the input and output paths since
the DAIs were separate on the CODEC and he felt that was clearer.  It
didn't make any practical difference, it certainly wasn't due to startup
ordering.

> routes are registered after all components have been registered,
> which means you can register all the routes via one table since all
> the dependencies are ready. Same is true if you use the card's
> late_probe callback to register the DAPM routes and widgets.

Right, anything that actually cared would be using the late_probe()
callback.
Lars-Peter Clausen March 10, 2014, 11:29 a.m. UTC | #8
On 03/10/2014 12:05 PM, Mark Brown wrote:
> On Mon, Mar 10, 2014 at 11:27:39AM +0100, Lars-Peter Clausen wrote:
>> On 03/10/2014 11:10 AM, Mark Brown wrote:
>
>>> The only thing that should be registering
>>> things for other components is the card and the general idea was to
>>> register everything in late_probe().
>
>> What most machine drivers did before card table based setup is to
>> have a per-component callback in which they did register the card
>> DAPM elements associated with that component. E.g. routes from the
>> CODEC output to a speaker, etc. You couldn't register the card level
>> DAPM elements for one component in the init callback component of
>> another one since the routes depend on the widgets being registered
>> first. So those board drivers kept separate routes and widget tables
>> for separate components. With table based setup for the card the
>
> I'm sorry but this just isn't what was happening at all.  Remember that
> most of the code that you're looking at only ever had one component so
> it really never made any difference which callback people happened to
> pick so long as it was one of the ones that ran after the CODEC was up.
>
> The only driver that I can think of which did anything like what you
> describe was smdk-wm8580 and that was still only using one component,
> it's just that Jassi preferred to split the input and output paths since
> the DAIs were separate on the CODEC and he felt that was clearer.  It
> didn't make any practical difference, it certainly wasn't due to startup
> ordering.

Take a look at e.g. omap/rx51.c it's doing what I'm describing and I presume 
it does this for the reasons I described.

But it doesn't really matter. The important thing about this series is that 
card level DAPM elements and controls should be registered with the card not 
the CODEC and I think we can agree on that one.

- Lars
Mark Brown March 10, 2014, 12:09 p.m. UTC | #9
On Mon, Mar 10, 2014 at 12:29:33PM +0100, Lars-Peter Clausen wrote:
> On 03/10/2014 12:05 PM, Mark Brown wrote:

> >The only driver that I can think of which did anything like what you
> >describe was smdk-wm8580 and that was still only using one component,
> >it's just that Jassi preferred to split the input and output paths since
> >the DAIs were separate on the CODEC and he felt that was clearer.  It
> >didn't make any practical difference, it certainly wasn't due to startup
> >ordering.

> Take a look at e.g. omap/rx51.c it's doing what I'm describing and I
> presume it does this for the reasons I described.

Oh, rx51 is just generally fun - I'd be a bit surprised if it still
works.  It's actually the out of ASoC probe stuff that's causing
problems there, it was trying to do multi-component prior that being
supported.

> But it doesn't really matter. The important thing about this series
> is that card level DAPM elements and controls should be registered
> with the card not the CODEC and I think we can agree on that one.

Right, it's mostly just an alarm bell for review (I'd not looked at the
series yet, I tend to defer things until the people working on the
driver have had a chance to look).

Patch
diff mbox

diff --git a/sound/soc/omap/omap-abe-twl6040.c b/sound/soc/omap/omap-abe-twl6040.c
index ebb1390..5011bfa 100644
--- a/sound/soc/omap/omap-abe-twl6040.c
+++ b/sound/soc/omap/omap-abe-twl6040.c
@@ -163,6 +163,10 @@  static const struct snd_soc_dapm_route audio_map[] = {
 
 	{"AFML", NULL, "Line In"},
 	{"AFMR", NULL, "Line In"},
+
+	/* DMIC routing */
+	{"DMic", NULL, "Digital Mic"},
+	{"Digital Mic", NULL, "Digital Mic1 Bias"},
 };
 
 static int omap_abe_twl6040_init(struct snd_soc_pcm_runtime *rtd)
@@ -196,20 +200,6 @@  static int omap_abe_twl6040_init(struct snd_soc_pcm_runtime *rtd)
 	return ret;
 }
 
-static const struct snd_soc_dapm_route dmic_audio_map[] = {
-	{"DMic", NULL, "Digital Mic"},
-	{"Digital Mic", NULL, "Digital Mic1 Bias"},
-};
-
-static int omap_abe_dmic_init(struct snd_soc_pcm_runtime *rtd)
-{
-	struct snd_soc_codec *codec = rtd->codec;
-	struct snd_soc_dapm_context *dapm = &codec->dapm;
-
-	return snd_soc_dapm_add_routes(dapm, dmic_audio_map,
-				ARRAY_SIZE(dmic_audio_map));
-}
-
 /* Digital audio interface glue - connects codec <--> CPU */
 static struct snd_soc_dai_link abe_twl6040_dai_links[] = {
 	{
@@ -229,7 +219,6 @@  static struct snd_soc_dai_link abe_twl6040_dai_links[] = {
 		.codec_dai_name = "dmic-hifi",
 		.platform_name = "omap-pcm-audio",
 		.codec_name = "dmic-codec",
-		.init = omap_abe_dmic_init,
 		.ops = &omap_abe_dmic_ops,
 	},
 };