[RFC 0/4] Add support for DAI link addition dynamically

[RFC 0/4] Add support for DAI link addition dynamically

[Vaibhav Agarwal]

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(-)

-- 
2.1.4

[RFC 1/4] ASoc: Use ref_count for soc DAI & component

[Vaibhav Agarwal]

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;
-- 
2.1.4

Re: [RFC 1/4] ASoc: Use ref_count for soc DAI & component

[Mark Brown]

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

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)?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [RFC 1/4] ASoc: Use ref_count for soc DAI & component

[Vaibhav Agarwal]

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.

[RFC 2/4] alsa: add locked variant for snd_ctl_remove_id

[Vaibhav Agarwal]

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
-- 
2.1.4

Re: [RFC 2/4] alsa: add locked variant for snd_ctl_remove_id

[Mark Brown]

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

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [RFC 2/4] alsa: add locked variant for snd_ctl_remove_id

[Vaibhav Agarwal]

+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.

Re: [RFC 2/4] alsa: add locked variant for snd_ctl_remove_id

[Takashi Iwai]

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

Re: [RFC 2/4] alsa: add locked variant for snd_ctl_remove_id

[Vaibhav Agarwal]

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

[RFC 3/4] ASoC: Enable dynamic DAIlink insertion & removal

[Vaibhav Agarwal]

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;
-- 
2.1.4

Re: [RFC 3/4] ASoC: Enable dynamic DAIlink insertion & removal

[Mark Brown]

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

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);
> +		else
> +			strlcpy(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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [RFC 3/4] ASoC: Enable dynamic DAIlink insertion & removal

[Vaibhav Agarwal]

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
>> +     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.
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);
>> +             else
>> +                     strlcpy(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.

Re: [RFC 3/4] ASoC: Enable dynamic DAIlink insertion & removal

[Mark Brown]

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

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[RFC 4/4] ASoC: Change soc-card codec_conf array to a list

[Vaibhav Agarwal]

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;
-- 
2.1.4

Re: [RFC 4/4] ASoC: Change soc-card codec_conf array to a list

[Mark Brown]

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

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Lars-Peter Clausen]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Mengdong Lin]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Mengdong Lin]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Vaibhav Agarwal]

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
>
>

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Mengdong Lin]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Vaibhav Agarwal]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Mark Brown]

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

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...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [RFC 0/4] Add support for DAI link addition dynamically

[Mengdong Lin]

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
>