[alsa-devel] [RFC 0/4] Add support for DAI link addition dynamically
Following patches are based on for-next branch from broonie tree. First 2 patches include changes in existing soc, core framework to prepare for adding support for dynamic DAI link addition/ removal
Patch 3 & 4 contains actual changes to enable dynamic DAI link support
NOTE: Currently, the code is tested on Pandaboad ES revB3 for playback usecase.
Vaibhav Agarwal (4): ASoc: Use ref_count for soc DAI & component alsa: add locked variant for snd_ctl_remove_id ASoC: Enable dynamic DAIlink insertion & removal ASoC: Change soc-card codec_conf array to a list
include/sound/control.h | 1 + include/sound/soc-dai.h | 1 + include/sound/soc-dapm.h | 7 +- include/sound/soc-dpcm.h | 1 + include/sound/soc.h | 18 ++- sound/core/control.c | 23 +++ sound/soc/Kconfig | 4 + sound/soc/soc-core.c | 359 ++++++++++++++++++++++++++++++++++++++++++++--- sound/soc/soc-dapm.c | 105 +++++++++++--- sound/soc/soc-pcm.c | 25 ++++ 10 files changed, 501 insertions(+), 43 deletions(-)
This is preperation to allow dynamic DAI link insertion & removal.
Currently, DAI links are added/removed once during soc-card instatiate & removal. Thus, irrespective of usage count by multiple DAI links, DAIs and components are probed or removed only once while maintaining 'probed' flag. However, in case of dynamic DAI link insertion/removal we need to ensure DAI/components are not unnecessarily probed multiple & not removed mistakenly while in use by any other existing DAI link. Thus, ref_count is used to maintain their usage count.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- include/sound/soc-dai.h | 1 + include/sound/soc.h | 2 ++ sound/soc/soc-core.c | 47 ++++++++++++++++++++++++++++++----------------- 3 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 964b7de..03c2c7a 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -270,6 +270,7 @@ struct snd_soc_dai { unsigned int symmetric_samplebits:1; unsigned int active; unsigned char probed:1; + int ref_count;
struct snd_soc_dapm_widget *playback_widget; struct snd_soc_dapm_widget *capture_widget; diff --git a/include/sound/soc.h b/include/sound/soc.h index 7afb72c..3dda0c4 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -822,6 +822,8 @@ struct snd_soc_component { struct dentry *debugfs_root; #endif
+ int ref_count; + /* * DO NOT use any of the fields below in drivers, they are temporary and * are going to be removed again soon. If you use them in driver code the diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 790ee2b..2b83814 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1060,6 +1060,11 @@ static void soc_remove_component(struct snd_soc_component *component) if (!component->card) return;
+ component->ref_count--; + + if (component->ref_count) + return; + /* This is a HACK and will be removed soon */ if (component->codec) list_del(&component->codec->card_list); @@ -1080,14 +1085,17 @@ static void soc_remove_dai(struct snd_soc_dai *dai, int order)
if (dai && dai->probed && dai->driver->remove_order == order) { - if (dai->driver->remove) { - err = dai->driver->remove(dai); - if (err < 0) - dev_err(dai->dev, - "ASoC: failed to remove %s: %d\n", - dai->name, err); + dai->ref_count--; + if (!dai->ref_count) { + if (dai->driver->remove) { + err = dai->driver->remove(dai); + if (err < 0) + dev_err(dai->dev, + "ASoC: failed to remove %s: %d\n", + dai->name, err); + } + dai->probed = 0; } - dai->probed = 0; } }
@@ -1367,6 +1375,7 @@ static int soc_probe_component(struct snd_soc_card *card, card->name, component->card->name); return -ENODEV; } + component->ref_count++; return 0; }
@@ -1436,6 +1445,7 @@ static int soc_probe_component(struct snd_soc_card *card, if (component->codec) list_add(&component->codec->card_list, &card->codec_dev_list);
+ component->ref_count++; return 0;
err_probe: @@ -1523,18 +1533,21 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order) { int ret;
- if (!dai->probed && dai->driver->probe_order == order) { - if (dai->driver->probe) { - ret = dai->driver->probe(dai); - if (ret < 0) { - dev_err(dai->dev, - "ASoC: failed to probe DAI %s: %d\n", - dai->name, ret); - return ret; + if (dai->driver->probe_order == order) { + if (!dai->probed) { + if (dai->driver->probe) { + ret = dai->driver->probe(dai); + if (ret < 0) { + dev_err(dai->dev, + "ASoC: failed to probe DAI %s: %d\n", + dai->name, ret); + return ret; + } } - }
- dai->probed = 1; + dai->probed = 1; + } + dai->ref_count++; }
return 0;
On Mon, Feb 15, 2016 at 05:49:29PM +0530, Vaibhav Agarwal wrote:
This is preperation to allow dynamic DAI link insertion & removal.
Which there is no proposed user of here...
Currently, DAI links are added/removed once during soc-card instatiate & removal. Thus, irrespective of usage count by multiple DAI links, DAIs and components are probed or removed only once while maintaining 'probed' flag. However, in case of dynamic DAI link insertion/removal we need to ensure DAI/components are not unnecessarily probed multiple & not removed mistakenly while in use by any other existing DAI link. Thus, ref_count is used to maintain their usage count.
Please use normal changelog formatting - either consistent line lengths or blank lines between paragraphs depending on what you're trying to accomplish here.
- if (!dai->probed && dai->driver->probe_order == order) {
if (dai->driver->probe) {ret = dai->driver->probe(dai);if (ret < 0) {dev_err(dai->dev,"ASoC: failed to probe DAI %s: %d\n",dai->name, ret);return ret;
- if (dai->driver->probe_order == order) {
if (!dai->probed) {if (dai->driver->probe) {ret = dai->driver->probe(dai);if (ret < 0) {dev_err(dai->dev,"ASoC: failed to probe DAI %s: %d\n",dai->name, ret);return ret;} }
}dai->probed = 1;
dai->probed = 1;}dai->ref_count++;
I don't understand this at all - why the complete reindent and change of condition, don't we just increase the refcount when we flag as probed (and why do we still have the probed flag after this)?
On 15 February 2016 at 19:29, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:29PM +0530, Vaibhav Agarwal wrote:
This is preperation to allow dynamic DAI link insertion & removal.
Which there is no proposed user of here...
Currently, DAI links are added/removed once during soc-card instatiate & removal. Thus, irrespective of usage count by multiple DAI links, DAIs and components are probed or removed only once while maintaining 'probed' flag. However, in case of dynamic DAI link insertion/removal we need to ensure DAI/components are not unnecessarily probed multiple & not removed mistakenly while in use by any other existing DAI link. Thus, ref_count is used to maintain their usage count.
Please use normal changelog formatting - either consistent line lengths or blank lines between paragraphs depending on what you're trying to accomplish here.
Sure, I'll take care while submitting future patches.
if (!dai->probed && dai->driver->probe_order == order) {if (dai->driver->probe) {ret = dai->driver->probe(dai);if (ret < 0) {dev_err(dai->dev,"ASoC: failed to probe DAI %s: %d\n",dai->name, ret);return ret;
if (dai->driver->probe_order == order) {if (!dai->probed) {if (dai->driver->probe) {ret = dai->driver->probe(dai);if (ret < 0) {dev_err(dai->dev,"ASoC: failed to probe DAI %s: %d\n",dai->name, ret);return ret;} }
}dai->probed = 1;
dai->probed = 1;}dai->ref_count++; }I don't understand this at all - why the complete reindent and change of condition, don't we just increase the refcount when we flag as probed (and why do we still have the probed flag after this)?
yes, non-zero check would also solve the purpose of probed flag. will make this change in next patchset.
While removing kcontrols due to dynamic dai_link/codec removal, mutex/semaphore for soc-card/sound card is already acquired. Thus, added lock already acquired variant for ctl_remove_id.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- include/sound/control.h | 1 + sound/core/control.c | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+)
diff --git a/include/sound/control.h b/include/sound/control.h index 21d047f..8faea10 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -125,6 +125,7 @@ int snd_ctl_add(struct snd_card * card, struct snd_kcontrol * kcontrol); int snd_ctl_remove(struct snd_card * card, struct snd_kcontrol * kcontrol); int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, bool add_on_replace); int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id); +int snd_ctl_remove_id_locked(struct snd_card *card, struct snd_ctl_elem_id *id); int snd_ctl_rename_id(struct snd_card * card, struct snd_ctl_elem_id *src_id, struct snd_ctl_elem_id *dst_id); int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, int active); diff --git a/sound/core/control.c b/sound/core/control.c index a85d455..dca1998 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -530,6 +530,29 @@ int snd_ctl_remove_id(struct snd_card *card, struct snd_ctl_elem_id *id) EXPORT_SYMBOL(snd_ctl_remove_id);
/** + * snd_ctl_remove_id_locked - remove the control of the given id and release it + * @card: the card instance + * @id: the control id to remove + * + * Finds the control instance with the given id, removes it from the + * card list and releases it. + * + * Return: 0 if successful, or a negative error code on failure. + */ +int snd_ctl_remove_id_locked(struct snd_card *card, struct snd_ctl_elem_id *id) +{ + struct snd_kcontrol *kctl; + int ret; + + kctl = snd_ctl_find_id(card, id); + if (kctl == NULL) + return -ENOENT; + ret = snd_ctl_remove(card, kctl); + return ret; +} +EXPORT_SYMBOL(snd_ctl_remove_id_locked); + +/** * snd_ctl_remove_user_ctl - remove and release the unlocked user control * @file: active control handle * @id: the control id to remove
On Mon, Feb 15, 2016 at 05:49:30PM +0530, Vaibhav Agarwal wrote:
While removing kcontrols due to dynamic dai_link/codec removal, mutex/semaphore for soc-card/sound card is already acquired. Thus, added lock already acquired variant for ctl_remove_id.
Please use subject lines matching the style for the subsystem and remember to CC maintainers on patch submission.
+Takashi
On 15 February 2016 at 19:32, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:30PM +0530, Vaibhav Agarwal wrote:
While removing kcontrols due to dynamic dai_link/codec removal, mutex/semaphore for soc-card/sound card is already acquired. Thus, added lock already acquired variant for ctl_remove_id.
Please use subject lines matching the style for the subsystem and remember to CC maintainers on patch submission.
Sure, will follow the convention for subject lines as well.
On Tue, 16 Feb 2016 14:54:57 +0100, Vaibhav Agarwal wrote:
+Takashi
On 15 February 2016 at 19:32, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:30PM +0530, Vaibhav Agarwal wrote:
While removing kcontrols due to dynamic dai_link/codec removal, mutex/semaphore for soc-card/sound card is already acquired. Thus, added lock already acquired variant for ctl_remove_id.
Please use subject lines matching the style for the subsystem and remember to CC maintainers on patch submission.
Sure, will follow the convention for subject lines as well.
I prefer reducing snd_ctl_remove_id() as well. Just wrap the call of the unlocked one with down/up_write().
Takashi
On 16 February 2016 at 19:43, Takashi Iwai tiwai@suse.de wrote:
On Tue, 16 Feb 2016 14:54:57 +0100, Vaibhav Agarwal wrote:
+Takashi
On 15 February 2016 at 19:32, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:30PM +0530, Vaibhav Agarwal wrote:
While removing kcontrols due to dynamic dai_link/codec removal, mutex/semaphore for soc-card/sound card is already acquired. Thus, added lock already acquired variant for ctl_remove_id.
Please use subject lines matching the style for the subsystem and remember to CC maintainers on patch submission.
Sure, will follow the convention for subject lines as well.
I prefer reducing snd_ctl_remove_id() as well. Just wrap the call of the unlocked one with down/up_write().
Sure, will make the change in next patchset.
Takashi
This patch enables dynamic DAI link insertion & removal from machine driver. With the evolvement of modularized platforms, codecs can be dynamically added to/removed from a platform. Thus, there is a need to add FE/BE DAIs to an existing sound card in response to codec inserted/removed.
Another kconfig option SND_SOC_DYNAMIC_DAILINK (default set to y) is added to avoid compilation issues for client (machine, codec) drivers with other kernel versions.
Limitations: This patch enables support for new DAI links in response to new codec driver & DAIs only. The same can be extended for new platform drivers added/removed as well.
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- include/sound/soc-dapm.h | 7 +- include/sound/soc-dpcm.h | 1 + include/sound/soc.h | 6 ++ sound/soc/Kconfig | 4 + sound/soc/soc-core.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++- sound/soc/soc-dapm.c | 105 +++++++++++++++---- sound/soc/soc-pcm.c | 25 +++++ 7 files changed, 391 insertions(+), 21 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 9706946..f680e3a 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -384,7 +384,10 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai); int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card); -void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card); +int snd_soc_dapm_link_dai_widgets_component(struct snd_soc_card *card, + struct snd_soc_dapm_context *dapm); +void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd); int snd_soc_dapm_new_pcm(struct snd_soc_card *card, const struct snd_soc_pcm_stream *params, unsigned int num_params, @@ -620,6 +623,8 @@ struct snd_soc_dapm_context { unsigned int idle_bias_off:1; /* Use BIAS_OFF instead of STANDBY */ /* Go to BIAS_OFF in suspend if the DAPM context is idle */ unsigned int suspend_bias_off:1; + /* registered dynamically in response to dynamic DAI links */ + unsigned int dynamic_registered:1; void (*seq_notifier)(struct snd_soc_dapm_context *, enum snd_soc_dapm_type, int);
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h index 8060590..ffac57d 100644 --- a/include/sound/soc-dpcm.h +++ b/include/sound/soc-dpcm.h @@ -144,6 +144,7 @@ int dpcm_process_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list **list, int new); int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream); int dpcm_be_dai_shutdown(struct snd_soc_pcm_runtime *fe, int stream); +void dpcm_fe_disconnect(struct snd_soc_pcm_runtime *be, int stream); void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream); void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream); int dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream); diff --git a/include/sound/soc.h b/include/sound/soc.h index 3dda0c4..44d8568 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -796,6 +796,8 @@ struct snd_soc_component {
unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */ unsigned int registered_as_component:1; + /* registered dynamically in response to dynamic DAI links */ + unsigned int dynamic_registered:1;
struct list_head list; struct list_head list_aux; /* for auxiliary component of the card */ @@ -1682,6 +1684,10 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, void snd_soc_remove_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link);
+int snd_soc_add_dailink(struct snd_soc_card *card, + struct snd_soc_dai_link *dai_link); +void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name); + int snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv);
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 7ea66ee..a8bb03c 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -22,6 +22,10 @@ menuconfig SND_SOC
if SND_SOC
+config SND_SOC_DYNAMIC_DAILINK + bool + default y + config SND_SOC_AC97_BUS bool
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 2b83814..7049f9b 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -67,6 +67,13 @@ static int pmdown_time = 5000; module_param(pmdown_time, int, 0); MODULE_PARM_DESC(pmdown_time, "DAPM stream powerdown time (msecs)");
+static int snd_soc_init_codec_cache(struct snd_soc_codec *codec); +static int soc_probe_link_components(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd, + int order); +static int soc_probe_link_dais(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd, int order); + /* returns the minimum number of bytes needed to represent * a particular given value */ static int min_bytes_needed(unsigned long val) @@ -1055,8 +1062,41 @@ _err_defer: return -EPROBE_DEFER; }
+static void soc_remove_component_controls(struct snd_soc_component *component) +{ + int i, ret, name_len; + const struct snd_kcontrol_new *controls = component->controls; + struct snd_card *card = component->card->snd_card; + const char *prefix, *long_name; + + for (i = 0; i < component->num_controls; i++) { + const struct snd_kcontrol_new *control = &controls[i]; + struct snd_ctl_elem_id id; + + long_name = control->name; + prefix = component->name_prefix; + name_len = sizeof(id.name); + if (prefix) + snprintf(id.name, name_len, "%s %s", prefix, + long_name); + else + strlcpy(id.name, long_name, sizeof(id.name)); + id.numid = 0; + id.iface = control->iface; + id.device = control->device; + id.subdevice = control->subdevice; + id.index = control->index; + ret = snd_ctl_remove_id_locked(card, &id); + if (ret < 0) { + dev_err(component->dev, "%d: Failed to remove %s\n", + ret, control->name); + } + } +} + static void soc_remove_component(struct snd_soc_component *component) { + struct snd_soc_dapm_context *dapm; if (!component->card) return;
@@ -1065,6 +1105,17 @@ static void soc_remove_component(struct snd_soc_component *component) if (component->ref_count) return;
+ /* + * should be done, only in case + * component probed after card instantiation + * assumptions: + * relevant DAI links are already removed + * mutex acquired for soc-card + * semaphore acquired for sound card + */ + if (component->controls && component->dynamic_registered) + soc_remove_component_controls(component); + /* This is a HACK and will be removed soon */ if (component->codec) list_del(&component->codec->card_list); @@ -1072,9 +1123,12 @@ static void soc_remove_component(struct snd_soc_component *component) if (component->remove) component->remove(component);
- snd_soc_dapm_free(snd_soc_component_get_dapm(component)); + dapm = snd_soc_component_get_dapm(component); + snd_soc_dapm_free(dapm);
soc_cleanup_component_debugfs(component); + component->dynamic_registered = 0; + dapm->dynamic_registered = 0; component->card = NULL; module_put(component->dev->driver->owner); } @@ -1143,6 +1197,28 @@ static void soc_remove_link_components(struct snd_soc_card *card, } }
+static void soc_remove_dai_link(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd) +{ + int order; + struct snd_soc_dai_link *link; + + for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; + order++) + soc_remove_link_dais(card, rtd, order); + + for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; + order++) + soc_remove_link_components(card, rtd, order); + + link = rtd->dai_link; + if (link->dobj.type == SND_SOC_DOBJ_DAI_LINK) + dev_warn(card->dev, "Topology forgot to remove link %s?\n", + link->name); + list_del(&link->list); + card->num_dai_links--; +} + static void soc_remove_dai_links(struct snd_soc_card *card) { int order; @@ -1339,6 +1415,175 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card, } EXPORT_SYMBOL_GPL(snd_soc_remove_dai_link);
+/** + * snd_soc_add_dailink - add DAI link to an instantiated sound card. + * + * @card: Sound card identifier to add DAI link to + * @dai_link: dai_link configuration to add + * + * Return 0 for success, else error. + */ +int snd_soc_add_dailink(struct snd_soc_card *card, + struct snd_soc_dai_link *dai_link) +{ + int ret, order; + struct snd_soc_pcm_runtime *rtd; + struct snd_soc_codec *codec; + struct snd_soc_dapm_context *dapm; + + if (!card) + return -EINVAL; + + mutex_lock(&card->mutex); + /* init check DAI link */ + ret = soc_init_dai_link(card, dai_link); + if (ret) + goto init_error; + + /* bind DAIs */ + ret = soc_bind_dai_link(card, dai_link); + if (ret) + goto init_error; + rtd = snd_soc_get_pcm_runtime(card, dai_link->name); + + ret = snd_soc_add_dai_link(card, dai_link); + if (ret) + goto base_error; + + if (!card->instantiated) { + dev_info(card->dev, + "ASoC: card not yet instantiated, can exit here\n"); + mutex_unlock(&card->mutex); + return 0; + } + + /* initialize the register cache for each available codec */ + list_for_each_entry(codec, &codec_list, list) { + if (codec->cache_init) + continue; + ret = snd_soc_init_codec_cache(codec); + if (ret < 0) + goto probe_dai_err; + } + + /* probe all components used by DAI link on this card */ + for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; + order++) { + ret = soc_probe_link_components(card, rtd, order); + if (ret < 0) { + dev_err(card->dev, "ASoC: failed to probe %s link components, %d\n", + dai_link->name, ret); + goto probe_dai_err; + } + } + + /* Find new DAI links added during probing components and bind them. + * Components with topology may bring new DAIs and DAI links. + */ + list_for_each_entry(dai_link, &card->dai_link_list, list) { + if (soc_is_dai_link_bound(card, dai_link)) + continue; + + ret = soc_init_dai_link(card, dai_link); + if (ret) + goto probe_dai_err; + ret = soc_bind_dai_link(card, dai_link); + if (ret) + goto probe_dai_err; + } + + + /* probe DAI links on this card */ + for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; + order++) { + ret = soc_probe_link_dais(card, rtd, order); + if (ret < 0) { + dev_err(card->dev, "ASoC: failed to probe %s dai link,%d\n", + dai_link->name, ret); + goto probe_dai_err; + } + } + + dapm = &rtd->codec->component.dapm; + snd_soc_dapm_new_widgets(card); + snd_soc_dapm_link_dai_widgets_component(card, dapm); + snd_soc_dapm_connect_dai_link_widgets(card, rtd); + snd_device_register(rtd->card->snd_card, rtd->pcm); + snd_soc_dapm_sync(&card->dapm); + mutex_unlock(&card->mutex); + + return 0; + +probe_dai_err: + soc_remove_dai_link(card, rtd); +base_error: + list_del(&rtd->list); + card->num_rtd--; + soc_free_pcm_runtime(rtd); +init_error: + mutex_unlock(&card->mutex); + return ret; +} +EXPORT_SYMBOL_GPL(snd_soc_add_dailink); + +/** + * snd_soc_remove_dailink - Remove DAI link from the active sound card + * + * @card_name: Sound card identifier to remove DAI link from + * @link_name: DAI link identfier to remove + */ +void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name) +{ + int ret; + struct snd_soc_dai_link *dai_link; + struct snd_soc_pcm_runtime *rtd; + struct snd_card *sndcard = card->snd_card; + + if (!card) + return; + + rtd = snd_soc_get_pcm_runtime(card, link_name); + if (!rtd) { + dev_err(card->dev, "DAI link not found\n"); + return; + } + + dai_link = rtd->dai_link; + + /* check if link is active */ + if (rtd->codec_dai->active) { + mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); + if (rtd->codec_dai->playback_active) + dpcm_dapm_stream_event(rtd, SNDRV_PCM_STREAM_PLAYBACK, + SND_SOC_DAPM_STREAM_STOP); + if (rtd->codec_dai->capture_active) + dpcm_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE, + SND_SOC_DAPM_STREAM_STOP); + mutex_unlock(&rtd->pcm_mutex); + ret = soc_dpcm_runtime_update(rtd->card); + } + cancel_delayed_work_sync(&rtd->delayed_work); + + down_write(&sndcard->controls_rwsem); + mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_RUNTIME); + /* in case of BE DAI, update fe_clients list */ + if (dai_link->no_pcm) { + dpcm_fe_disconnect(rtd, SNDRV_PCM_STREAM_PLAYBACK); + dpcm_fe_disconnect(rtd, SNDRV_PCM_STREAM_CAPTURE); + } + + /* free associated PCM device */ + snd_device_free(rtd->card->snd_card, rtd->pcm); + soc_remove_dai_link(card, rtd); + list_del(&rtd->list); + card->num_rtd--; + soc_free_pcm_runtime(rtd); + + mutex_unlock(&card->mutex); + up_write(&sndcard->controls_rwsem); +} +EXPORT_SYMBOL_GPL(snd_soc_remove_dailink); + static void soc_set_name_prefix(struct snd_soc_card *card, struct snd_soc_component *component) { @@ -1439,6 +1684,11 @@ static int soc_probe_component(struct snd_soc_card *card, snd_soc_dapm_add_routes(dapm, component->dapm_routes, component->num_dapm_routes);
+ if (card->instantiated) { + component->dynamic_registered = 1; + dapm->dynamic_registered = 1; + } + list_add(&dapm->list, &card->dapm_list);
/* This is a HACK and will be removed soon */ @@ -1968,7 +2218,17 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) }
snd_soc_dapm_link_dai_widgets(card); - snd_soc_dapm_connect_dai_link_widgets(card); + /* for each BE DAI link... */ + list_for_each_entry(rtd, &card->rtd_list, list) { + /* + * dynamic FE links have no fixed DAI mapping. + * CODEC<->CODEC links have no direct connection. + */ + if (rtd->dai_link->dynamic || rtd->dai_link->params) + continue; + + snd_soc_dapm_connect_dai_link_widgets(card, rtd); + }
if (card->controls) snd_soc_add_card_controls(card, card->controls, card->num_controls); diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 0d37079..dedc0ba 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -2272,6 +2272,34 @@ static void dapm_free_path(struct snd_soc_dapm_path *path) kfree(path); }
+static void snd_soc_dapm_remove_kcontrols(struct snd_soc_dapm_widget *w) +{ + int i, ret; + struct snd_kcontrol *kctl; + struct snd_soc_dapm_context *dapm = w->dapm; + struct snd_card *card = dapm->card->snd_card; + + if (!w->num_kcontrols) + return; + + for (i = 0; i < w->num_kcontrols; i++) { + kctl = w->kcontrols[i]; + if (!kctl) { + dev_err(dapm->dev, "%s: Failed to find %d kcontrol\n", + w->name, i); + continue; + } + dev_dbg(dapm->dev, "Remove %d: %s\n", kctl->id.numid, + kctl->id.name); + ret = snd_ctl_remove_id_locked(card, &kctl->id); + if (ret < 0) { + dev_err(dapm->dev, "Err %d: while remove %s:%s\n", ret, + w->name, kctl->id.name); + } + } + w->num_kcontrols = 0; +} + void snd_soc_dapm_free_widget(struct snd_soc_dapm_widget *w) { struct snd_soc_dapm_path *p, *next_p; @@ -2288,6 +2316,12 @@ void snd_soc_dapm_free_widget(struct snd_soc_dapm_widget *w) dapm_free_path(p); }
+ /* + * remove associated kcontrols, + * in case added dynamically + */ + if (w->dapm->dynamic_registered && w->num_kcontrols) + snd_soc_dapm_remove_kcontrols(w); kfree(w->kcontrols); kfree_const(w->name); kfree(w); @@ -3778,6 +3812,58 @@ int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, return 0; }
+int snd_soc_dapm_link_dai_widgets_component(struct snd_soc_card *card, + struct snd_soc_dapm_context *dapm) +{ + struct snd_soc_dapm_widget *dai_w, *w; + struct snd_soc_dapm_widget *src, *sink; + struct snd_soc_dai *dai; + + /* For each DAI widget... */ + list_for_each_entry(dai_w, &card->widgets, list) { + if (dai_w->dapm != dapm) + continue; + switch (dai_w->id) { + case snd_soc_dapm_dai_in: + case snd_soc_dapm_dai_out: + break; + default: + continue; + } + + dai = dai_w->priv; + + /* ...find all widgets with the same stream and link them */ + list_for_each_entry(w, &card->widgets, list) { + if (w->dapm != dai_w->dapm) + continue; + + switch (w->id) { + case snd_soc_dapm_dai_in: + case snd_soc_dapm_dai_out: + continue; + default: + break; + } + + if (!w->sname || !strstr(w->sname, dai_w->sname)) + continue; + + if (dai_w->id == snd_soc_dapm_dai_in) { + src = dai_w; + sink = w; + } else { + src = w; + sink = dai_w; + } + dev_dbg(dai->dev, "%s -> %s\n", src->name, sink->name); + snd_soc_dapm_add_path(w->dapm, src, sink, NULL, NULL); + } + } + + return 0; +} + int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) { struct snd_soc_dapm_widget *dai_w, *w; @@ -3827,7 +3913,7 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) return 0; }
-static void dapm_connect_dai_link_widgets(struct snd_soc_card *card, +void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card, struct snd_soc_pcm_runtime *rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; @@ -3903,23 +3989,6 @@ static void soc_dapm_dai_stream_event(struct snd_soc_dai *dai, int stream, } }
-void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) -{ - struct snd_soc_pcm_runtime *rtd; - - /* for each BE DAI link... */ - list_for_each_entry(rtd, &card->rtd_list, list) { - /* - * dynamic FE links have no fixed DAI mapping. - * CODEC<->CODEC links have no direct connection. - */ - if (rtd->dai_link->dynamic || rtd->dai_link->params) - continue; - - dapm_connect_dai_link_widgets(card, rtd); - } -} - static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream, int event) { diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index aa99dac..903cfd5 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1192,6 +1192,31 @@ static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe, }
/* disconnect a BE and FE */ +void dpcm_fe_disconnect(struct snd_soc_pcm_runtime *be, int stream) +{ + struct snd_soc_dpcm *dpcm, *d; + + list_for_each_entry_safe(dpcm, d, &be->dpcm[stream].fe_clients, + list_fe) { + dev_dbg(be->dev, "ASoC: BE %s disconnect check for %s\n", + stream ? "capture" : "playback", + dpcm->fe->dai_link->name); + + dev_dbg(be->dev, " freed DSP %s path %s %s %s\n", + stream ? "capture" : "playback", be->dai_link->name, + stream ? "<-" : "->", dpcm->fe->dai_link->name); + +#ifdef CONFIG_DEBUG_FS + debugfs_remove(dpcm->debugfs_state); +#endif + /* FE still alive, update it's BE client list */ + list_del(&dpcm->list_be); + list_del(&dpcm->list_fe); + kfree(dpcm); + } +} + +/* disconnect a BE and FE */ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream) { struct snd_soc_dpcm *dpcm, *d;
On Mon, Feb 15, 2016 at 05:49:31PM +0530, Vaibhav Agarwal wrote:
With the evolvement of modularized platforms, codecs can be dynamically added to/removed from a platform. Thus, there is a need to add FE/BE DAIs to an existing sound card in response to codec inserted/removed.
Like I say please format your changelogs normally. It makes things much easier to read. I'm still not seeing a user or how this will work on the client side here.
Another kconfig option SND_SOC_DYNAMIC_DAILINK (default set to y) is added to avoid compilation issues for client (machine, codec) drivers with other kernel versions.
No, don't do this. We don't care about random other kernel versions, if we did the whole kernel would be full of ifdefs. We have config options for things like adding substantial size to the kernel.
+int snd_soc_dapm_link_dai_widgets_component(struct snd_soc_card *card,
struct snd_soc_dapm_context *dapm);+void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card,
struct snd_soc_pcm_runtime *rtd);
Why void?
+void dpcm_fe_disconnect(struct snd_soc_pcm_runtime *be, int stream); void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream);
This seems like it should be a separate patch, it's not strongly tied to the rest of the code.
index 3dda0c4..44d8568 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -796,6 +796,8 @@ struct snd_soc_component {
unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */ unsigned int registered_as_component:1;
- /* registered dynamically in response to dynamic DAI links */
- unsigned int dynamic_registered:1;
dynamically.
+int snd_soc_add_dailink(struct snd_soc_card *card,
struct snd_soc_dai_link *dai_link);+void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name);
Everywhere else we write dai_link.
+config SND_SOC_DYNAMIC_DAILINK
- bool
default y
Like I say I don't see a need for this but note also that the indentation seems broken - might be worth checking this isn't creeping in elsewhere.
+static void soc_remove_component_controls(struct snd_soc_component *component)
This is only used once which means we're going to end up with two different ways of removing controls, one of which is essentially never used and hence likely to break. It would be better to ensure that the removal path is the same as much of the time as possible so that things are more maintainable. This is an issue through the patch, it feels like it'd be clearer and easier to review if it first rearranged things to split up things for reuse by dynamic addition and then separately added new interfaces that do the dyanmic stuff.
long_name = control->name;prefix = component->name_prefix;name_len = sizeof(id.name);
name_len is completely pointless here...
if (prefix)snprintf(id.name, name_len, "%s %s", prefix,long_name);elsestrlcpy(id.name, long_name, sizeof(id.name));
...it's not even used here. Just don't bother with the intermediate variables, they make things harder to follow.
- /*
* should be done, only in case* component probed after card instantiation* assumptions:* relevant DAI links are already removed* mutex acquired for soc-card* semaphore acquired for sound card*/
Please fix the formatting of this comment so it looks more like normal kernel code.
On 15 February 2016 at 22:32, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:31PM +0530, Vaibhav Agarwal wrote:
With the evolvement of modularized platforms, codecs can be dynamically added to/removed from a platform. Thus, there is a need to add FE/BE DAIs to an existing sound card in response to codec inserted/removed.
Like I say please format your changelogs normally. It makes things much easier to read. I'm still not seeing a user or how this will work on the client side here.
Will take care of formatting. I'll also add some more details in commit message to show possible usage
Another kconfig option SND_SOC_DYNAMIC_DAILINK (default set to y) is added to avoid compilation issues for client (machine, codec) drivers with other kernel versions.
No, don't do this. We don't care about random other kernel versions, if we did the whole kernel would be full of ifdefs. We have config options for things like adding substantial size to the kernel.
Agreed.
+int snd_soc_dapm_link_dai_widgets_component(struct snd_soc_card *card,
struct snd_soc_dapm_context *dapm);+void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card,
struct snd_soc_pcm_runtime *rtd);Why void?
No functional change in snd_soc_dapm_connect_dai_link_widgets(). I have refactored it for individual pcm_runtime instead of complete soc_card.
+void dpcm_fe_disconnect(struct snd_soc_pcm_runtime *be, int stream); void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream);
This seems like it should be a separate patch, it's not strongly tied to the rest of the code.
Sure, will share separate patch for this change.
index 3dda0c4..44d8568 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -796,6 +796,8 @@ struct snd_soc_component {
unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */ unsigned int registered_as_component:1;
/* registered dynamically in response to dynamic DAI links */unsigned int dynamic_registered:1;dynamically.
will make the change in next patchset
+int snd_soc_add_dailink(struct snd_soc_card *card,
struct snd_soc_dai_link *dai_link);+void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name);
Everywhere else we write dai_link.
Another API snd_soc_add_dai_link() already exists (added by Mengdong). How about renaming it to snd_soc_create_dai_link() ?
+config SND_SOC_DYNAMIC_DAILINK
booldefault yLike I say I don't see a need for this but note also that the indentation seems broken - might be worth checking this isn't creeping in elsewhere.
Got the point. I'll remove this completely.
+static void soc_remove_component_controls(struct snd_soc_component *component)
This is only used once which means we're going to end up with two different ways of removing controls, one of which is essentially never used and hence likely to break. It would be better to ensure that the removal path is the same as much of the time as possible so that things are more maintainable. This is an issue through the patch, it feels like it'd be clearer and easier to review if it first rearranged things to split up things for reuse by dynamic addition and then separately added new interfaces that do the dyanmic stuff.
Will split this patch further between preparation & actual new interfaces.
long_name = control->name;prefix = component->name_prefix;name_len = sizeof(id.name);name_len is completely pointless here...
if (prefix)snprintf(id.name, name_len, "%s %s", prefix,long_name);elsestrlcpy(id.name, long_name, sizeof(id.name));...it's not even used here. Just don't bother with the intermediate variables, they make things harder to follow.
/** should be done, only in case* component probed after card instantiation* assumptions:* relevant DAI links are already removed* mutex acquired for soc-card* semaphore acquired for sound card*/Please fix the formatting of this comment so it looks more like normal kernel code.
will modify the comment formatting.
On Tue, Feb 16, 2016 at 08:04:23PM +0530, Vaibhav Agarwal wrote:
As I said with regard to your changlogs please leave blank lines between paragraphs, it makes things much easier to read.
On 15 February 2016 at 22:32, Mark Brown broonie@kernel.org wrote:
On Mon, Feb 15, 2016 at 05:49:31PM +0530, Vaibhav Agarwal wrote:
+int snd_soc_add_dailink(struct snd_soc_card *card,
struct snd_soc_dai_link *dai_link);+void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name);
Everywhere else we write dai_link.
Another API snd_soc_add_dai_link() already exists (added by Mengdong). How about renaming it to snd_soc_create_dai_link() ?
No. Think about what you are saying here. You're having problems because you're trying to create a new function where the obvious names collide with existing functions. This suggests that hacking around with the naming isn't going to lead to a clear interface or implementation, users will be confused about what to do and we will most likely have code duplication in the implementation. Instead this should be addressed through looking at the structure of the code.
Currently, number & type of codec(s) available on a platform is fixed. Thus, machine driver knows in advance the corresponding device name and can easily define codec_configuration (name_prefix, compress_type) statically.
However, with the scope of adding codec devices dynamically there is a need to add codec configuration dynamically as well. Now we can add/remove codec configuration dynamically in response to any codec device added/removed.
For backward compatibility, there is a provision to create list for predefined codec configuration as well.
ToDo: For snd_soc_remove_codec_config(), possible argument should be of_node as well to identify codec_conf
Signed-off-by: Vaibhav Agarwal vaibhav.agarwal@linaro.org --- include/sound/soc.h | 10 +++++++++- sound/soc/soc-core.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 44d8568..4074ec3 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -396,6 +396,7 @@ struct snd_soc_dai_link; struct snd_soc_platform_driver; struct snd_soc_codec; struct snd_soc_codec_driver; +struct snd_soc_codec_conf; struct snd_soc_component; struct snd_soc_component_driver; struct soc_enum; @@ -1074,6 +1075,7 @@ struct snd_soc_codec_conf { * associated per device */ const char *name_prefix; + struct list_head list; /* codec_conf list of the soc card */ };
struct snd_soc_aux_dev { @@ -1140,8 +1142,9 @@ struct snd_soc_card { int num_rtd;
/* optional codec specific configuration */ - struct snd_soc_codec_conf *codec_conf; + struct snd_soc_codec_conf *codec_conf; /* predefined configs only */ int num_configs; + struct list_head codec_conf_list; /* all configs */
/* * optional auxiliary devices such as amplifiers or codecs with DAI @@ -1688,6 +1691,11 @@ int snd_soc_add_dailink(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link); void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name);
+void snd_soc_add_codec_config(struct snd_soc_card *card, + struct snd_soc_codec_conf *codec_conf); +void snd_soc_remove_codec_config(struct snd_soc_card *card, + const char *dev_name); + int snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7049f9b..57ce151 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1584,16 +1584,51 @@ void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name) } EXPORT_SYMBOL_GPL(snd_soc_remove_dailink);
+/** + * snd_soc_add_codec_config - add codec configuration to a sound card. + * + * @card: Sound card identifier to add codec configuration to + * @codec_conf: codec configuration to add + */ +void snd_soc_add_codec_config(struct snd_soc_card *card, + struct snd_soc_codec_conf *codec_conf) +{ + mutex_lock(&client_mutex); + list_add_tail(&codec_conf->list, &card->codec_conf_list); + mutex_unlock(&client_mutex); +} +EXPORT_SYMBOL(snd_soc_add_codec_config); + +/** + * snd_soc_remove_codec_config - remove codec configuration from a sound card. + * + * @card: Sound card identifier to remove codec configuration from + * @dev_name: codec configuration identifier + */ +void snd_soc_remove_codec_config(struct snd_soc_card *card, + const char *dev_name) +{ + struct snd_soc_codec_conf *codec_conf; + + mutex_lock(&client_mutex); + list_for_each_entry(codec_conf, &card->codec_conf_list, list) + if (!strcmp(codec_conf->dev_name, dev_name)) { + list_del(&codec_conf->list); + break; + } + mutex_unlock(&client_mutex); +} +EXPORT_SYMBOL(snd_soc_remove_codec_config); + static void soc_set_name_prefix(struct snd_soc_card *card, struct snd_soc_component *component) { - int i; + struct snd_soc_codec_conf *map, *_map;
- if (card->codec_conf == NULL) + if (list_empty(&card->codec_conf_list)) return;
- for (i = 0; i < card->num_configs; i++) { - struct snd_soc_codec_conf *map = &card->codec_conf[i]; + list_for_each_entry_safe(map, _map, &card->codec_conf_list, list) { if (map->of_node && component->dev->of_node != map->of_node) continue; if (map->dev_name && strcmp(component->name, map->dev_name)) @@ -2119,6 +2154,10 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) for (i = 0; i < card->num_links; i++) snd_soc_add_dai_link(card, card->dai_link+i);
+ /* add predefined codec_configs to the list */ + for (i = 0; i < card->num_configs; i++) + snd_soc_add_codec_config(card, card->codec_conf+i); + /* initialize the register cache for each available codec */ list_for_each_entry(codec, &codec_list, list) { if (codec->cache_init) @@ -2899,6 +2938,7 @@ int snd_soc_register_card(struct snd_soc_card *card) INIT_LIST_HEAD(&card->rtd_list); card->num_rtd = 0;
+ INIT_LIST_HEAD(&card->codec_conf_list); INIT_LIST_HEAD(&card->dapm_dirty); INIT_LIST_HEAD(&card->dobj_list); card->instantiated = 0;
On Mon, Feb 15, 2016 at 05:49:32PM +0530, Vaibhav Agarwal wrote:
+void snd_soc_add_codec_config(struct snd_soc_card *card,
struct snd_soc_codec_conf *codec_conf)+{
- mutex_lock(&client_mutex);
- list_add_tail(&codec_conf->list, &card->codec_conf_list);
- mutex_unlock(&client_mutex);
+}
We add a configuration but we don't act on it? That's surprising... I don't really have a sense of how in concrete terms this interface is intended to work.
+EXPORT_SYMBOL(snd_soc_add_codec_config);
All ASoC APIs are _GPL.
- /* add predefined codec_configs to the list */
- for (i = 0; i < card->num_configs; i++)
snd_soc_add_codec_config(card, card->codec_conf+i);
Coding style.
On 02/15/2016 01:19 PM, Vaibhav Agarwal wrote:
Following patches are based on for-next branch from broonie tree. First 2 patches include changes in existing soc, core framework to prepare for adding support for dynamic DAI link addition/ removal
Patch 3 & 4 contains actual changes to enable dynamic DAI link support
NOTE: Currently, the code is tested on Pandaboad ES revB3 for playback usecase.
Can you outline how exactly that would work?
Thanks, - Lars
On 02/15/2016 10:22 PM, Lars-Peter Clausen wrote:
On 02/15/2016 01:19 PM, Vaibhav Agarwal wrote:
Following patches are based on for-next branch from broonie tree. First 2 patches include changes in existing soc, core framework to prepare for adding support for dynamic DAI link addition/ removal
Patch 3 & 4 contains actual changes to enable dynamic DAI link support
NOTE: Currently, the code is tested on Pandaboad ES revB3 for playback usecase.
Can you outline how exactly that would work?
Thanks,
- Lars
In Agarwal's use case, it seems a removable codec driver can be registered after the sound card is registered. And the Pandaboard machine driver need to add new BE links brought by the new codec component, which will further trigger probing of the codec components.
How can the machine driver know a new codec component is registered automatically?
Can we add a notification ops like "new_component" to a soc card? A machine driver can implement this ops. So when a new component is registered to the ASoC core, all instantiated soc cards can get notified and the machine driver can check if the new component brings some new links to the soc card in this ops. e.g. the Pandaboard machine driver can add new BE links for the new codec.
Thanks Mengdong
On 02/17/2016 01:52 PM, Mengdong Lin wrote:
On 02/15/2016 10:22 PM, Lars-Peter Clausen wrote:
On 02/15/2016 01:19 PM, Vaibhav Agarwal wrote:
Following patches are based on for-next branch from broonie tree. First 2 patches include changes in existing soc, core framework to prepare for adding support for dynamic DAI link addition/ removal
Patch 3 & 4 contains actual changes to enable dynamic DAI link support
NOTE: Currently, the code is tested on Pandaboad ES revB3 for playback usecase.
Can you outline how exactly that would work?
Thanks,
- Lars
In Agarwal's use case, it seems a removable codec driver can be registered after the sound card is registered. And the Pandaboard machine driver need to add new BE links brought by the new codec component, which will further trigger probing of the codec components.
How can the machine driver know a new codec component is registered automatically?
Can we add a notification ops like "new_component" to a soc card? A machine driver can implement this ops. So when a new component is registered to the ASoC core, all instantiated soc cards can get notified and the machine driver can check if the new component brings some new links to the soc card in this ops. e.g. the Pandaboard machine driver can add new BE links for the new codec.
Thanks Mengdong
Sorry, I missed the case that the codec component can be still be registered before the machine driver register the card. In this situation, the machine driver cannot get the notification.
I wonder if the machine driver can still predefine these links but flag them as "dynamically_registered". For such links, asoc core will not abort instantiating the card, i.e. the sound card will be created even if these links are not bound because lack of some DAIs (e.g. the DAIs of the codec component not registered yet).
Then when the missing component is added, the ASOC core will automatically check these the unbound links and bind them. When the component is unregistered, ASoC will try to remove the affected links and then remove the components.
Thus we may not need add new APIs or let the machine driver monitor when the codec component is registered.
Thanks Mengdong
On 17 February 2016 at 13:55, Mengdong Lin mengdong.lin@linux.intel.com wrote:
On 02/17/2016 01:52 PM, Mengdong Lin wrote:
On 02/15/2016 10:22 PM, Lars-Peter Clausen wrote:
On 02/15/2016 01:19 PM, Vaibhav Agarwal wrote:
Following patches are based on for-next branch from broonie tree. First 2 patches include changes in existing soc, core framework to prepare for adding support for dynamic DAI link addition/ removal
Patch 3 & 4 contains actual changes to enable dynamic DAI link support
NOTE: Currently, the code is tested on Pandaboad ES revB3 for playback usecase.
Can you outline how exactly that would work?
Thanks,
- Lars
In Agarwal's use case, it seems a removable codec driver can be registered after the sound card is registered. And the Pandaboard machine driver need to add new BE links brought by the new codec component, which will further trigger probing of the codec components.
How can the machine driver know a new codec component is registered automatically?
Can we add a notification ops like "new_component" to a soc card? A machine driver can implement this ops. So when a new component is registered to the ASoC core, all instantiated soc cards can get notified and the machine driver can check if the new component brings some new links to the soc card in this ops. e.g. the Pandaboard machine driver can add new BE links for the new codec.
Thanks Mengdong
Sorry, I missed the case that the codec component can be still be registered before the machine driver register the card. In this situation, the machine driver cannot get the notification.
Yes, the codec component can be registered before the sound card registration or even later. All we need is, to add & bind DAI link to an already registered/active soc card dynamically.
Also, w.r.t .new_component() callback approach proposed, it can be a good solution assuming any device would have limited no. of soc cards registered (with support for dynamic codec modules). In this case, can a client codec choose to which soc card, it can register DAI links?
I wonder if the machine driver can still predefine these links but flag them as "dynamically_registered". For such links, asoc core will not abort instantiating the card, i.e. the sound card will be created even if these links are not bound because lack of some DAIs (e.g. the DAIs of the codec component not registered yet).
Pre-defining links can be a good idea, but honestly, I'm not convinced with that. Since, I can have N no. of codec modules to be added dynamically. And each may have different number of DAIs, corresponding to which I can have different no. of DAI links.
Then when the missing component is added, the ASOC core will automatically check these the unbound links and bind them. When the component is unregistered, ASoC will try to remove the affected links and then remove the components.
I would prefer defining & binding DAI links at the same time as I mentioned above. Similarly, cleanup all resources during removal path.
Thus we may not need add new APIs or let the machine driver monitor when the codec component is registered.
My original idea was: Let machine driver export APIs to register/unregister DAI links to its registered card. Now, client codec driver can use those exported APIs to register DAI link with the card.
In case codec driver is supposed to register dynamically, during probe itself it can use machine driver APIs to register relevant DAI links.
And in case codec driver is already registered, a separate mechanism can be used to register relevant DAI link using machine driver's exposed API.
Also, I plan to update card->codec_conf dynamically in response to recently added codec module. This would help to update .prefix_name while adding mixer controls for the codec and uniquely identify them. The same is required in case of multiple codec modules of similar type attached to a device.
-- Thanks, ./va
Thanks Mengdong
On 02/17/2016 05:31 PM, Vaibhav Agarwal wrote:
On 17 February 2016 at 13:55, Mengdong Lin mengdong.lin@linux.intel.com wrote:
On 02/17/2016 01:52 PM, Mengdong Lin wrote:
On 02/15/2016 10:22 PM, Lars-Peter Clausen wrote:
Can you outline how exactly that would work?
Thanks,
- Lars
In Agarwal's use case, it seems a removable codec driver can be registered after the sound card is registered. And the Pandaboard machine driver need to add new BE links brought by the new codec component, which will further trigger probing of the codec components.
How can the machine driver know a new codec component is registered automatically?
Can we add a notification ops like "new_component" to a soc card? A machine driver can implement this ops. So when a new component is registered to the ASoC core, all instantiated soc cards can get notified and the machine driver can check if the new component brings some new links to the soc card in this ops. e.g. the Pandaboard machine driver can add new BE links for the new codec.
Thanks Mengdong
Sorry, I missed the case that the codec component can be still be registered before the machine driver register the card. In this situation, the machine driver cannot get the notification.
Yes, the codec component can be registered before the sound card registration or even later. All we need is, to add & bind DAI link to an already registered/active soc card dynamically.
Also, w.r.t .new_component() callback approach proposed, it can be a good solution assuming any device would have limited no. of soc cards registered (with support for dynamic codec modules). In this case, can a client codec choose to which soc card, it can register DAI links?
Maybe we should not let a client codec driver choose the soc card, but let the machine driver (owner of the soc card) to specify a removable codec.
Since the codec driver is generic and can be used for many machines so I feel we should avoid adding too machine-specific code to it (e.g. the soc card name) as much as possible.
I think the machine driver can understand all use cases for a platform, including static or removable connections between the SOC and codecs. So it's able to specify the removable codec and the codec DAIs needed by the machine, as well as these removable links. Please correct me if my understanding is wrong.
I wonder if the machine driver can still predefine these links but flag them as "dynamically_registered". For such links, asoc core will not abort instantiating the card, i.e. the sound card will be created even if these links are not bound because lack of some DAIs (e.g. the DAIs of the codec component not registered yet).
Pre-defining links can be a good idea, but honestly, I'm not convinced with that. Since, I can have N no. of codec modules to be added dynamically. And each may have different number of DAIs, corresponding to which I can have different no. of DAI links.
Do you mean the machine driver has no idea about these removal codecs? And could you share more info about the removal codec in your use case?
If we let the codec driver to add the links, there is gap: a dai link needs info from both cpu (soc) side and codec side. A generic codec driver only knows the codec info, but has no idea about the cpu side or the connection between the codec & cpu. E.g. a codec may have 2 i2s interface, but maybe only 1 is connected to the soc.
Then when the missing component is added, the ASOC core will automatically check these the unbound links and bind them. When the component is unregistered, ASoC will try to remove the affected links and then remove the components.
I would prefer defining & binding DAI links at the same time as I mentioned above. Similarly, cleanup all resources during removal path.
Thus we may not need add new APIs or let the machine driver monitor when the codec component is registered.
My original idea was: Let machine driver export APIs to register/unregister DAI links to its registered card. Now, client codec driver can use those exported APIs to register DAI link with the card.
In case codec driver is supposed to register dynamically, during probe itself it can use machine driver APIs to register relevant DAI links.
And in case codec driver is already registered, a separate mechanism can be used to register relevant DAI link using machine driver's exposed API.
Is it unavoidable to define a private API of a machine driver and let a generic codec driver call this API to add/remove BE DAI links?
If it's really unavoidable, maybe this machine API can just let the codec driver to add/remove codec DAIs. And the machine driver can decide by itself what links to add/remove for the soc card by ASoC API. Maybe you could extend the existing API snd_soc_add_dai_link() for your case.
But IMHO I hope we can avoid this. So I suggested let machine driver define some 'removable' links and ASoC core can bind/unbind them automatically with the registration/unregistration of the codec component. I think most of your code for ASoC core can still be reused.
Thanks Mengdong
On 18 February 2016 at 10:53, Mengdong Lin mengdong.lin@linux.intel.com wrote:
On 02/17/2016 05:31 PM, Vaibhav Agarwal wrote:
On 17 February 2016 at 13:55, Mengdong Lin mengdong.lin@linux.intel.com wrote:
On 02/17/2016 01:52 PM, Mengdong Lin wrote:
On 02/15/2016 10:22 PM, Lars-Peter Clausen wrote:
Can you outline how exactly that would work?
Thanks,
- Lars
In Agarwal's use case, it seems a removable codec driver can be registered after the sound card is registered. And the Pandaboard machine driver need to add new BE links brought by the new codec component, which will further trigger probing of the codec components.
How can the machine driver know a new codec component is registered automatically?
Can we add a notification ops like "new_component" to a soc card? A machine driver can implement this ops. So when a new component is registered to the ASoC core, all instantiated soc cards can get notified and the machine driver can check if the new component brings some new links to the soc card in this ops. e.g. the Pandaboard machine driver can add new BE links for the new codec.
Thanks Mengdong
Sorry, I missed the case that the codec component can be still be registered before the machine driver register the card. In this situation, the machine driver cannot get the notification.
Yes, the codec component can be registered before the sound card registration or even later. All we need is, to add & bind DAI link to an already registered/active soc card dynamically.
Also, w.r.t .new_component() callback approach proposed, it can be a good solution assuming any device would have limited no. of soc cards registered (with support for dynamic codec modules). In this case, can a client codec choose to which soc card, it can register DAI links?
Maybe we should not let a client codec driver choose the soc card, but let the machine driver (owner of the soc card) to specify a removable codec.
Yes, I agree machine driver is the real owner of soc card & should decide/choose on possible capabilities of codec. Also, codec may wish to choose from different soc cards registered based on the functionality supported. Say, performance mode using I2S interface, otherwise feature mode (supporting multichannel, etc.) using PCM interface or may be via USB interface.
I agree, as of today, we would rarely find a device with multiple sound cards exposing different interface. And in case, this kind of usecase makes the architecture way complex, let's drop that for now and re-visit as & when need really comes.
Since the codec driver is generic and can be used for many machines so I feel we should avoid adding too machine-specific code to it (e.g. the soc card name) as much as possible.
We are not adding any machine-specific code in generic codec driver. In case we are using generic codec driver (existing in sound/soc/codecs), it would need a helper driver to glue it to soc card dynamically. Otherwise, for a specific platform, we can have a wrapper codec driver that can fetch capabilities of removable codec (may be via binary data) and expose them to already known soc cards for that device.
I think the machine driver can understand all use cases for a platform, including static or removable connections between the SOC and codecs. So it's able to specify the removable codec and the codec DAIs needed by the machine, as well as these removable links. Please correct me if my understanding is wrong.
As per my opinion, in case a platform supports say I2S interface, then any removable codec module utilizing I2S mode should work with that platform. So, knowing complete info about the codec may not be possible.
I wonder if the machine driver can still predefine these links but flag them as "dynamically_registered". For such links, asoc core will not abort instantiating the card, i.e. the sound card will be created even if these links are not bound because lack of some DAIs (e.g. the DAIs of the codec component not registered yet).
Pre-defining links can be a good idea, but honestly, I'm not convinced with that. Since, I can have N no. of codec modules to be added dynamically. And each may have different number of DAIs, corresponding to which I can have different no. of DAI links.
Do you mean the machine driver has no idea about these removal codecs? And could you share more info about the removal codec in your use case?
A removable codec can be of any manufacturer using xyz codec. As mentioned before, it may not have info about possible codec to be connected. But, it can definitely choose the functionality supported based on it own capabilities.
If we let the codec driver to add the links, there is gap: a dai link needs info from both cpu (soc) side and codec side. A generic codec driver only knows the codec info, but has no idea about the cpu side or the connection between the codec & cpu. E.g. a codec may have 2 i2s interface, but maybe only 1 is connected to the soc.
Yes, definitely soc card should define capabilities supported on its platform.
Then when the missing component is added, the ASOC core will automatically check these the unbound links and bind them. When the component is unregistered, ASoC will try to remove the affected links and then remove the components.
I would prefer defining & binding DAI links at the same time as I mentioned above. Similarly, cleanup all resources during removal path.
Thus we may not need add new APIs or let the machine driver monitor when the codec component is registered.
My original idea was: Let machine driver export APIs to register/unregister DAI links to its registered card. Now, client codec driver can use those exported APIs to register DAI link with the card.
In case codec driver is supposed to register dynamically, during probe itself it can use machine driver APIs to register relevant DAI links.
And in case codec driver is already registered, a separate mechanism can be used to register relevant DAI link using machine driver's exposed API.
Is it unavoidable to define a private API of a machine driver and let a generic codec driver call this API to add/remove BE DAI links?
I'm still not clear about the thought here. Machine driver is platform/device specific. And since we are defining it to be capable of supporting removable codecs, it should be possible to expose an API for this purpose(i guess... )
If it's really unavoidable, maybe this machine API can just let the codec driver to add/remove codec DAIs. And the machine driver can decide by itself what links to add/remove for the soc card by ASoC API. Maybe you could extend the existing API snd_soc_add_dai_link() for your case.
I'm unable to catch your thought here :(
But IMHO I hope we can avoid this. So I suggested let machine driver define some 'removable' links and ASoC core can bind/unbind them automatically with the registration/unregistration of the codec component. I think most of your code for ASoC core can still be reused.
I wonder, if we can extend snd_soc_add_dai_link() to perform complete binding as well, with a conditional check for (!card->instantiated) to exit early. This would enable us using this API itself for the purpose we intend too. Any thoughts/suggestions?
Also, keeping in mind that we don't have codec & codec_dai info in advance, I can't see any value addition of keeping place holders for DAI link based on platform capability.
-- Thanks, ./va
Thanks Mengdong
On Thu, Feb 18, 2016 at 01:27:26PM +0530, Vaibhav Agarwal wrote:
Yes, I agree machine driver is the real owner of soc card & should decide/choose on possible capabilities of codec. Also, codec may wish to choose from different soc cards registered based on the functionality supported. Say, performance mode using I2S interface, otherwise feature mode (supporting multichannel, etc.) using PCM interface or may be via USB interface.
No, the driver should offer whatever the hardware is capable of doing and let userspace make any policy decisions.
In case we are using generic codec driver (existing in sound/soc/codecs), it would need a helper driver to glue it to soc card dynamically. Otherwise, for a specific platform, we can have a wrapper codec driver that can fetch capabilities of removable codec (may be via binary data) and expose them to already known soc cards for that device.
It sounds like there might be some review concerns with some of this stuff...
On 02/18/2016 03:57 PM, Vaibhav Agarwal wrote:
On 18 February 2016 at 10:53, Mengdong Lin mengdong.lin@linux.intel.com wrote:
Since the codec driver is generic and can be used for many machines so I feel we should avoid adding too machine-specific code to it (e.g. the soc card name) as much as possible.
We are not adding any machine-specific code in generic codec driver. In case we are using generic codec driver (existing in sound/soc/codecs), it would need a helper driver to glue it to soc card dynamically. Otherwise, for a specific platform, we can have a wrapper codec driver that can fetch capabilities of removable codec (may be via binary data) and expose them to already known soc cards for that device.
I think the machine driver can understand all use cases for a platform, including static or removable connections between the SOC and codecs. So it's able to specify the removable codec and the codec DAIs needed by the machine, as well as these removable links. Please correct me if my understanding is wrong.
As per my opinion, in case a platform supports say I2S interface, then any removable codec module utilizing I2S mode should work with that platform. So, knowing complete info about the codec may not be possible.
Okay. Then the machine driver has no way to pre-define the removal codec and the BE links. Thanks for your clarification.
Can the machine driver leave the codec and codec dai empty in the removable BE links? When a compatible codec is registered, the ASoC core will fill these fields and bind the links. When the codec is removed, the ASoC will unbind the links and clear the fields.
Do you mean the machine driver has no idea about these removal codecs? And could you share more info about the removal codec in your use case?
A removable codec can be of any manufacturer using xyz codec. As mentioned before, it may not have info about possible codec to be connected. But, it can definitely choose the functionality supported based on it own capabilities.
But IMHO I hope we can avoid this. So I suggested let machine driver define some 'removable' links and ASoC core can bind/unbind them automatically with the registration/unregistration of the codec component. I think most of your code for ASoC core can still be reused.
I wonder, if we can extend snd_soc_add_dai_link() to perform complete binding as well, with a conditional check for (!card->instantiated) to exit early. This would enable us using this API itself for the purpose we intend too. Any thoughts/suggestions?
Yes. I think we can extend this function like this if need.
Thanks Mengdong
Also, keeping in mind that we don't have codec & codec_dai info in advance, I can't see any value addition of keeping place holders for DAI link based on platform capability.
-- Thanks, ./va
participants (5)
-
Lars-Peter Clausen -
Mark Brown -
Mengdong Lin -
Takashi Iwai -
Vaibhav Agarwal