| Message ID | 1394302616-15991-1-git-send-email-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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?
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
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.
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.
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().
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
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.
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
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).
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, }, };
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(-)