| Message ID | bb343fe20b14af90f695939d8faae50fae1ba97a.1449036307.git.mengdong.lin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 02, 2015 at 02:11:56PM +0800, mengdong.lin@linux.intel.com wrote: > From: Mengdong Lin <mengdong.lin@linux.intel.com> > > Machine drivers can set the name of one or more auxiliary components. > And the ASoC core will find and probe the auxiliary components. > > Machine drivers can use this to specify the components with topology. > Then when the components are probed, topology info will be loaded to > the core. This makes sense as a replacement for the existing aux_devs but this doesn't feel terribly strongly related to the rest of the series and we're not actually doing the transition yet. What's the relationship with dynamic links and what's the plan for transition? There's CODECs that could be transitioned relatively straightforwardly since they have no DAIs and can only be used as aux_devs, it's them I'm mainly thinking of here - the rest will need to wait I fear.
On 12/09/2015 02:58 AM, Mark Brown wrote: > On Wed, Dec 02, 2015 at 02:11:56PM +0800, mengdong.lin@linux.intel.com wrote: >> From: Mengdong Lin <mengdong.lin@linux.intel.com> >> >> Machine drivers can set the name of one or more auxiliary components. >> And the ASoC core will find and probe the auxiliary components. >> >> Machine drivers can use this to specify the components with topology. >> Then when the components are probed, topology info will be loaded to >> the core. > > This makes sense as a replacement for the existing aux_devs but this > doesn't feel terribly strongly related to the rest of the series and > we're not actually doing the transition yet. What's the relationship > with dynamic links and what's the plan for transition? There's CODECs > that could be transitioned relatively straightforwardly since they have > no DAIs and can only be used as aux_devs, it's them I'm mainly thinking > of here - the rest will need to wait I fear. > Thanks for your comments! I have no intention to replace the existing aux_devs with aux_components. The aux_components is not for codecs and has no rtd like aux_dev. The purpose to introduce aux_components here is to let a soc card to load topology info easier: E.g. The machine driver may not define any FE DAI links explicitly but specify an aux component. And the platform driver can implement the aux component and let its probe ops to load the topology info. When the ASoC core is instantiating the soc card, it must to find the aux component and probe it (the platform component). Thus the topology info for the platform is loaded and topology core will create widgets/controls/routes/DAIs/DAI links. So after probing the aux components, the ASoC need to check if any new links are created, bind them and create pcm runtimes for them. Regards Mengdong
On Wed, Dec 09, 2015 at 05:09:31PM +0800, Mengdong Lin wrote: > I have no intention to replace the existing aux_devs with aux_components. > The aux_components is not for codecs and has no rtd like aux_dev. OK, but I do think that's something we *should* be doing as part of the overall move of CODECs to components and it's something that having this change implies we should be doing as an immediate thing since it's the more obvious direct use of the code (as Lars said in reply to the early draft you posted IIRC). > The purpose to introduce aux_components here is to let a soc card to load > topology info easier: > E.g. The machine driver may not define any FE DAI links explicitly but > specify an aux component. And the platform driver can implement the aux > component and let its probe ops to load the topology info. When the ASoC > core is instantiating the soc card, it must to find the aux component and > probe it (the platform component). Thus the topology info for the platform > is loaded and topology core will create widgets/controls/routes/DAIs/DAI > links. So after probing the aux components, the ASoC need to check if any > new links are created, bind them and create pcm runtimes for them. I think I'd need to see the code you're proposing here to really comment but I think it sounds like it comes back to the same thing as the above case where we're overall trying to move everything towards uniformly being components.
On 12/10/2015 04:38 AM, Mark Brown wrote: > On Wed, Dec 09, 2015 at 05:09:31PM +0800, Mengdong Lin wrote: > >> I have no intention to replace the existing aux_devs with aux_components. >> The aux_components is not for codecs and has no rtd like aux_dev. > > OK, but I do think that's something we *should* be doing as part of the > overall move of CODECs to components and it's something that having this > change implies we should be doing as an immediate thing since it's the > more obvious direct use of the code (as Lars said in reply to the early > draft you posted IIRC). My early draft didn't use the aux components, so I'm not sure where to find Lars's comments on this idea. Please check if my understanding is right? I guess you want me to replace the "aux_dev" array from the struct snd_soc_card, by an "aux_components" array. And we may replace soc_bind_aux_dev() by soc_find_components(), replace soc_probe/remove_aux_dev() by soc_probe/remove_components. Probably soc_find/prove/remove_components need some adjustment for the the aux devices (DAIless codecs). And device driver of the these aux_dev need to use snd_soc_register_component() to make it as a component. >> The purpose to introduce aux_components here is to let a soc card to load >> topology info easier: > >> E.g. The machine driver may not define any FE DAI links explicitly but >> specify an aux component. And the platform driver can implement the aux >> component and let its probe ops to load the topology info. When the ASoC >> core is instantiating the soc card, it must to find the aux component and >> probe it (the platform component). Thus the topology info for the platform >> is loaded and topology core will create widgets/controls/routes/DAIs/DAI >> links. So after probing the aux components, the ASoC need to check if any >> new links are created, bind them and create pcm runtimes for them. > > I think I'd need to see the code you're proposing here to really comment > but I think it sounds like it comes back to the same thing as the above > case where we're overall trying to move everything towards uniformly > being components. In my test cases, the Broadwell machine driver (sound/soc/intel/boards/broadwell.c) specifies the name an aux component for the soc card: +static struct snd_soc_aux_component broadwell_topology_components[] = { + /* Platform topology component */ + { + .name = "haswell-pcm-audio", + }, +}; /* broadwell audio machine driver for WPT + RT286S */ static struct snd_soc_card broadwell_rt286 = { .name = "broadwell-rt286", .owner = THIS_MODULE, + .aux_components = broadwell_topology_components, + .num_aux_components = ARRAY_SIZE(broadwell_topology_components), .dai_link = broadwell_rt286_dais, .num_links = ARRAY_SIZE(broadwell_rt286_dais), .controls = broadwell_controls, Actually, the component "haswell-pcm-audio" is the component name for the platform (sound/soc/intel/haswell/sst-haswell-pcm.c). When the ASoC core instantiates the soc card, it will find the aux component at first, which is available only after the platform is registered. Then the ASoC will probe the aux component which triggers the platform probe ops and this ops loads the topology info. Thanks Mengdong > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Thu, Dec 10, 2015 at 06:05:30PM +0800, Mengdong Lin wrote: > On 12/10/2015 04:38 AM, Mark Brown wrote: > >OK, but I do think that's something we *should* be doing as part of the > >overall move of CODECs to components and it's something that having this > >change implies we should be doing as an immediate thing since it's the > >more obvious direct use of the code (as Lars said in reply to the early > >draft you posted IIRC). > My early draft didn't use the aux components, so I'm not sure where to find > Lars's comments on this idea. He replied to some thing you posted by mistake and immediately retracted. Can't remember the subject, sorry. > Please check if my understanding is right? > I guess you want me to replace the "aux_dev" array from the struct > snd_soc_card, by an "aux_components" array. And we may > replace soc_bind_aux_dev() by soc_find_components(), > replace soc_probe/remove_aux_dev() by soc_probe/remove_components. > Probably soc_find/prove/remove_components need some adjustment for the the > aux devices (DAIless codecs). > And device driver of the these aux_dev need to use > snd_soc_register_component() to make it as a component. Yeah, pretty much. I think we'll have a period where we support both though as any CODEC *could* be used in this way and we're not ready for that yet. Let me have a look at converting some of the drivers over the weekend.
On 12/12/2015 04:22 AM, Mark Brown wrote: > On Thu, Dec 10, 2015 at 06:05:30PM +0800, Mengdong Lin wrote: >> On 12/10/2015 04:38 AM, Mark Brown wrote: > >>> OK, but I do think that's something we *should* be doing as part of the >>> overall move of CODECs to components and it's something that having this >>> change implies we should be doing as an immediate thing since it's the >>> more obvious direct use of the code (as Lars said in reply to the early >>> draft you posted IIRC). > >> My early draft didn't use the aux components, so I'm not sure where to find >> Lars's comments on this idea. > > He replied to some thing you posted by mistake and immediately > retracted. Can't remember the subject, sorry. Thanks, I found his comments. > >> Please check if my understanding is right? > >> I guess you want me to replace the "aux_dev" array from the struct >> snd_soc_card, by an "aux_components" array. And we may >> replace soc_bind_aux_dev() by soc_find_components(), >> replace soc_probe/remove_aux_dev() by soc_probe/remove_components. >> Probably soc_find/prove/remove_components need some adjustment for the the >> aux devices (DAIless codecs). > >> And device driver of the these aux_dev need to use >> snd_soc_register_component() to make it as a component. > > Yeah, pretty much. I think we'll have a period where we support both > though as any CODEC *could* be used in this way and we're not ready for > that yet. > > Let me have a look at converting some of the drivers over the weekend. > I still have some basic questions: 1. What are the typical usages for aux_dev? For CODEC<->CODEC link or external headset detection chip? In samsung/neo1973_wm8753.c, the aux_dev "dfbmcs320" is to involve the BT sco codec. And this codec provides dai "bt-sco-pcm" for voice via BT. This seems to be the codec-codec link case, the CODEC can have DAIs. And this seems to be the external headset chip case (DAIless): In rockchip/rockchip_max98090.c, the aux_devs static struct snd_soc_aux_dev rk_98090_headset_dev = { .name = "Headset Chip", .init = rk_98090_headset_init, }; But how can ASoC find the right component registered by codecs/ts3a227e.c? This aux device does not give a codec node or name. Any other typical usage cases? 2. Why we need the rtd array 'rtd_aux' for the aux_devs? If the codec has DAIs and used by a DAI link, the ASoC will create a rtd for the link. Thanks Mengdong ts3a227e.c Thanks Mengdong
On Tue, Dec 15, 2015 at 04:06:14PM +0800, Mengdong Lin wrote: > I still have some basic questions: > 1. What are the typical usages for aux_dev? > For CODEC<->CODEC link or external headset detection chip? Neither, it's for analogue devices. > 2. Why we need the rtd array 'rtd_aux' for the aux_devs? > If the codec has DAIs and used by a DAI link, the ASoC will create a rtd > for the link. There are (or were at the time) assumptions in drivers that there will be a rtd there so it was easier to provide a stub.
On 12/15/2015 07:23 PM, Mark Brown wrote: > On Tue, Dec 15, 2015 at 04:06:14PM +0800, Mengdong Lin wrote: > >> I still have some basic questions: > >> 1. What are the typical usages for aux_dev? >> For CODEC<->CODEC link or external headset detection chip? > > Neither, it's for analogue devices. Got it. Thanks! > >> 2. Why we need the rtd array 'rtd_aux' for the aux_devs? >> If the codec has DAIs and used by a DAI link, the ASoC will create a rtd >> for the link. > > There are (or were at the time) assumptions in drivers that there will > be a rtd there so it was easier to provide a stub. > Can we remove this rtd array if current driver does not need the stub? For the replacement of aux_dev by aux_component, can we define like below? struct snd_soc_aux_component { const char *comp_name; ... no longer assume they're only codecs const char *comp_of_node; /* machine specific init */ int (*init)(struct snd_soc_component *componnent); }; The ASoC can use the comp_name or comp_of_node to find the components and probe them. Thanks Mengdong
On Wed, Dec 16, 2015 at 04:33:32PM +0800, Mengdong Lin wrote: > On 12/15/2015 07:23 PM, Mark Brown wrote: > >>2. Why we need the rtd array 'rtd_aux' for the aux_devs? > >> If the codec has DAIs and used by a DAI link, the ASoC will create a rtd > >>for the link. > >There are (or were at the time) assumptions in drivers that there will > >be a rtd there so it was easier to provide a stub. > Can we remove this rtd array if current driver does not need the stub? If it's unneeded we should probably remove it. As might be obvious by now I've not yet had time to dig into the code, sorry. I've got two weeks of vacation after today though. > For the replacement of aux_dev by aux_component, can we define like below? > struct snd_soc_aux_component { > const char *comp_name; ... no longer assume they're only codecs > const char *comp_of_node; > > /* machine specific init */ > int (*init)(struct snd_soc_component *componnent); > }; > The ASoC can use the comp_name or comp_of_node to find the components and > probe them. Well, that's just the existing struct with a rename now I look at it - init() already takes a component. Given that can you not just have whatever code was going to use this work with aux_devs as they are now?
Revise the subject. Could someone clarify if the rtd_aux in struct snd_soc_card is still needed? We want to replace the aux_dev by aux_component. If the rtd_aux is no longer needed, we can remove the array and it will become easier to handle an aux_dev as a generic component. Thanks Mengdong On 12/16/2015 04:33 PM, Mengdong Lin wrote: > > > On 12/15/2015 07:23 PM, Mark Brown wrote: >> On Tue, Dec 15, 2015 at 04:06:14PM +0800, Mengdong Lin wrote: >> >>> I still have some basic questions: >> >>> 1. What are the typical usages for aux_dev? >>> For CODEC<->CODEC link or external headset detection chip? >> >> Neither, it's for analogue devices. > > Got it. Thanks! > >> >>> 2. Why we need the rtd array 'rtd_aux' for the aux_devs? >>> If the codec has DAIs and used by a DAI link, the ASoC will >>> create a rtd >>> for the link. >> >> There are (or were at the time) assumptions in drivers that there will >> be a rtd there so it was easier to provide a stub. >> > > Can we remove this rtd array if current driver does not need the stub? > > For the replacement of aux_dev by aux_component, can we define like below? > > struct snd_soc_aux_component { > const char *comp_name; ... no longer assume they're only codecs > const char *comp_of_node; > > /* machine specific init */ > int (*init)(struct snd_soc_component *componnent); > }; > > The ASoC can use the comp_name or comp_of_node to find the components > and probe them. > > Thanks > Mengdong >
On Tue, Dec 22, 2015 at 04:15:26PM +0800, Mengdong Lin wrote: > Revise the subject. > Could someone clarify if the rtd_aux in struct snd_soc_card is still needed? > We want to replace the aux_dev by aux_component. because...? > If the rtd_aux is no longer needed, we can remove the array and it will > become easier to handle an aux_dev as a generic component. We are still using it in the core but nothing outside the core appears to be using it.
diff --git a/include/sound/soc.h b/include/sound/soc.h index f28b36e..46c6492 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1074,6 +1074,11 @@ struct snd_soc_aux_dev { int (*init)(struct snd_soc_component *component); }; +struct snd_soc_aux_component { + const char *name; + struct snd_soc_component *comp; +}; + /* SoC card */ struct snd_soc_card { const char *name; @@ -1120,6 +1125,10 @@ struct snd_soc_card { struct list_head dai_link_list; /* all links */ int num_dai_links; + /* auxiliary components, e.g. topology */ + struct snd_soc_aux_component *aux_components; + int num_aux_components; + struct list_head rtd_list; int num_rtd; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 19f7486..4ddc5d5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1826,6 +1826,18 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) goto base_error; } + /* find auxiliary components */ + for (i = 0; i < card->num_aux_components; i++) { + card->aux_components[i].comp = + soc_find_component(NULL, card->aux_components[i].name); + if (!card->aux_components[i].comp) { + dev_err(card->dev, "ASoC: Aux component %s not registered\n", + card->aux_components[i].name); + ret = -EPROBE_DEFER; + goto base_error; + } + } + /* add predefined DAI links to the list */ for (i = 0; i < card->num_links; i++) snd_soc_add_dai_link(card, card->dai_link+i); @@ -1880,6 +1892,21 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) goto card_probe_error; } + /* probe auxiliary components */ + for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; + order++) { + for (i = 0; i < card->num_aux_components; i++) { + ret = soc_probe_component(card, + card->aux_components[i].comp); + if (ret < 0) { + dev_err(card->dev, + "ASoC: failed to probe aux component %s %d\n", + card->aux_components[i].name, ret); + goto probe_dai_err; + } + } + } + /* probe all components used by DAI links on this card */ for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; order++) {