[PATCH v6 0/3] ASoC: Add Multi CPU DAI support

[PATCH v6 0/3] ASoC: Add Multi CPU DAI support

[Shreyas NC]

As discussed in [1], ASoC core supports multi codec DAIs
on a DAI link. However it does not do so for CPU DAIs.

So, add support for multi CPU DAIs on a DAI Link by adding
multi CPU DAI in Card instantiation, suspend and resume
functions, PCM ops, stream handling functions and DAPM.

[1]: https://www.spinics.net/lists/alsa-devel/msg71369.html

changes in v6:
 - Addressed review comments from Pierre in calculating delay
for CPU DAI, add check for compress DAIs and other nits
 - add proper exit check in dpcm_prune_path() for CPU DAIs
 - rebased on latest for-next branch

Shreyas NC (3):
  ASoC: Add initial support for multiple CPU DAIs
  ASoC: Add multiple CPU DAI support for PCM ops
  ASoC: Add multiple CPU DAI support in DAPM

 include/sound/soc.h  |   6 +
 sound/soc/soc-core.c | 290 +++++++++++++++++++++---------
 sound/soc/soc-dapm.c |  71 +++++---
 sound/soc/soc-pcm.c  | 491 ++++++++++++++++++++++++++++++++++-----------------
 4 files changed, 589 insertions(+), 269 deletions(-)

-- 
2.7.4

[PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Shreyas NC]

ASoC core supports multiple codec DAIs but supports only a CPU DAI.
To support multiple cpu DAIs, add cpu_dai and num_cpu_dai in
snd_soc_dai_link and snd_soc_pcm_runtime structures similar to
support for codec_dai. This is intended as a preparatory patch to
eventually support the unification of the Codec and CPU DAI.

Inline with multiple codec DAI approach, add support to allocate,
init, bind and probe multiple cpu_dai on init if driver specifies
that. Also add support to loop over multiple cpu_dai during
suspend and resume.

This is intended as a preparatory patch to eventually unify the CPU
and Codec DAI into DAI components.

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 include/sound/soc.h  |   6 ++
 sound/soc/soc-core.c | 290 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 211 insertions(+), 85 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 1378dcd..2f46009 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -903,6 +903,9 @@ struct snd_soc_dai_link {
 	struct snd_soc_dai_link_component *codecs;
 	unsigned int num_codecs;
 
+	struct snd_soc_dai_link_component *cpu_dai;
+	unsigned int num_cpu_dai;
+
 	/*
 	 * You MAY specify the link's platform/PCM/DMA driver, either by
 	 * device name, or by DT/OF node, but not both. Some forms of link
@@ -1125,6 +1128,9 @@ struct snd_soc_pcm_runtime {
 	struct snd_soc_dai **codec_dais;
 	unsigned int num_codecs;
 
+	struct snd_soc_dai **cpu_dais;
+	unsigned int num_cpu_dai;
+
 	struct delayed_work delayed_work;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_dpcm_root;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4663de3..b42a0d5 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -381,12 +381,21 @@ static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
 		return NULL;
 	}
 
+	rtd->cpu_dais = kzalloc(sizeof(struct snd_soc_dai *) *
+				dai_link->num_cpu_dai, GFP_KERNEL);
+	if (!rtd->cpu_dais) {
+		kfree(rtd->codec_dais);
+		kfree(rtd);
+		return NULL;
+	}
+
 	return rtd;
 }
 
 static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
 {
 	kfree(rtd->codec_dais);
+	kfree(rtd->cpu_dais);
 	snd_soc_rtdcom_del_all(rtd);
 	kfree(rtd);
 }
@@ -482,13 +491,17 @@ int snd_soc_suspend(struct device *dev)
 		card->suspend_pre(card);
 
 	list_for_each_entry(rtd, &card->rtd_list, list) {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		struct snd_soc_dai *cpu_dai;
 
 		if (rtd->dai_link->ignore_suspend)
 			continue;
 
-		if (cpu_dai->driver->suspend && !cpu_dai->driver->bus_control)
-			cpu_dai->driver->suspend(cpu_dai);
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+			if (cpu_dai->driver->suspend &&
+					!cpu_dai->driver->bus_control)
+				cpu_dai->driver->suspend(cpu_dai);
+		}
 	}
 
 	/* close any waiting streams */
@@ -552,16 +565,21 @@ int snd_soc_suspend(struct device *dev)
 	}
 
 	list_for_each_entry(rtd, &card->rtd_list, list) {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		struct snd_soc_dai *cpu_dai;
 
 		if (rtd->dai_link->ignore_suspend)
 			continue;
 
-		if (cpu_dai->driver->suspend && cpu_dai->driver->bus_control)
-			cpu_dai->driver->suspend(cpu_dai);
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
 
-		/* deactivate pins to sleep state */
-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+			if (cpu_dai->driver->suspend &&
+					cpu_dai->driver->bus_control)
+				cpu_dai->driver->suspend(cpu_dai);
+
+			/* deactivate pins to sleep state */
+			pinctrl_pm_select_sleep_state(cpu_dai->dev);
+		}
 	}
 
 	if (card->suspend_post)
@@ -596,13 +614,18 @@ static void soc_resume_deferred(struct work_struct *work)
 
 	/* resume control bus DAIs */
 	list_for_each_entry(rtd, &card->rtd_list, list) {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		struct snd_soc_dai *cpu_dai;
 
 		if (rtd->dai_link->ignore_suspend)
 			continue;
 
-		if (cpu_dai->driver->resume && cpu_dai->driver->bus_control)
-			cpu_dai->driver->resume(cpu_dai);
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+
+			if (cpu_dai->driver->resume &&
+					cpu_dai->driver->bus_control)
+				cpu_dai->driver->resume(cpu_dai);
+		}
 	}
 
 	list_for_each_entry(component, &card->component_dev_list, card_list) {
@@ -648,8 +671,13 @@ static void soc_resume_deferred(struct work_struct *work)
 		if (rtd->dai_link->ignore_suspend)
 			continue;
 
-		if (cpu_dai->driver->resume && !cpu_dai->driver->bus_control)
-			cpu_dai->driver->resume(cpu_dai);
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+
+			if (cpu_dai->driver->resume &&
+					!cpu_dai->driver->bus_control)
+				cpu_dai->driver->resume(cpu_dai);
+		}
 	}
 
 	if (card->resume_post)
@@ -671,6 +699,7 @@ int snd_soc_resume(struct device *dev)
 	struct snd_soc_card *card = dev_get_drvdata(dev);
 	bool bus_control = false;
 	struct snd_soc_pcm_runtime *rtd;
+	int i;
 
 	/* If the card is not initialized yet there is nothing to do */
 	if (!card->instantiated)
@@ -679,11 +708,16 @@ int snd_soc_resume(struct device *dev)
 	/* activate pins from sleep state */
 	list_for_each_entry(rtd, &card->rtd_list, list) {
 		struct snd_soc_dai **codec_dais = rtd->codec_dais;
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
+		struct snd_soc_dai *cpu_dai;
 		int j;
 
-		if (cpu_dai->active)
-			pinctrl_pm_select_default_state(cpu_dai->dev);
+		for (j = 0; j < rtd->num_cpu_dai; j++) {
+			cpu_dai = cpu_dais[j];
+
+			if (cpu_dai->active)
+				pinctrl_pm_select_default_state(cpu_dai->dev);
+		}
 
 		for (j = 0; j < rtd->num_codecs; j++) {
 			struct snd_soc_dai *codec_dai = codec_dais[j];
@@ -699,8 +733,11 @@ int snd_soc_resume(struct device *dev)
 	 * due to I/O costs and anti-pop so handle them out of line.
 	 */
 	list_for_each_entry(rtd, &card->rtd_list, list) {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-		bus_control |= cpu_dai->driver->bus_control;
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			struct snd_soc_dai *cpu_dai = rtd->cpu_dais[i];
+
+			bus_control |= cpu_dai->driver->bus_control;
+		}
 	}
 	if (bus_control) {
 		dev_dbg(dev, "ASoC: Resuming control bus master immediately\n");
@@ -845,35 +882,47 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
 {
 	struct snd_soc_pcm_runtime *rtd;
 	struct snd_soc_dai_link_component *codecs = dai_link->codecs;
-	struct snd_soc_dai_link_component cpu_dai_component;
+	struct snd_soc_dai_link_component *cpu_dai_component;
 	struct snd_soc_component *component;
-	struct snd_soc_dai **codec_dais;
+	struct snd_soc_dai **codec_dais, **cpu_dais;
 	struct device_node *platform_of_node;
 	const char *platform_name;
 	int i;
 
 	dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name);
 
+	cpu_dai_component = dai_link->cpu_dai;
+
 	if (soc_is_dai_link_bound(card, dai_link)) {
 		dev_dbg(card->dev, "ASoC: dai link %s already bound\n",
 			dai_link->name);
 		return 0;
 	}
 
+	if (dai_link->dynamic && dai_link->num_cpu_dai > 1) {
+		dev_err(card->dev, "ASoC: Multi CPU DAI not supported for FE");
+		return -EINVAL;
+	}
+
 	rtd = soc_new_pcm_runtime(card, dai_link);
 	if (!rtd)
 		return -ENOMEM;
 
-	cpu_dai_component.name = dai_link->cpu_name;
-	cpu_dai_component.of_node = dai_link->cpu_of_node;
-	cpu_dai_component.dai_name = dai_link->cpu_dai_name;
-	rtd->cpu_dai = snd_soc_find_dai(&cpu_dai_component);
-	if (!rtd->cpu_dai) {
-		dev_info(card->dev, "ASoC: CPU DAI %s not registered\n",
-			 dai_link->cpu_dai_name);
-		goto _err_defer;
+	rtd->num_cpu_dai = dai_link->num_cpu_dai;
+
+	cpu_dais = rtd->cpu_dais;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dais[i] = snd_soc_find_dai(&cpu_dai_component[i]);
+		if (!cpu_dais[i]) {
+			dev_err(card->dev, "ASoC: CPU DAI %s not registered\n",
+				cpu_dai_component[i].dai_name);
+			goto _err_defer;
+		}
+		snd_soc_rtdcom_add(rtd, cpu_dais[i]->component);
 	}
-	snd_soc_rtdcom_add(rtd, rtd->cpu_dai->component);
+
+	/* Fill cpu_dai in the runtime data */
+	rtd->cpu_dai = cpu_dais[0];
 
 	rtd->num_codecs = dai_link->num_codecs;
 
@@ -971,7 +1020,8 @@ static void soc_remove_link_dais(struct snd_soc_card *card,
 	for (i = 0; i < rtd->num_codecs; i++)
 		soc_remove_dai(rtd->codec_dais[i], order);
 
-	soc_remove_dai(rtd->cpu_dai, order);
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		soc_remove_dai(rtd->cpu_dais[i], order);
 }
 
 static void soc_remove_link_components(struct snd_soc_card *card,
@@ -1043,6 +1093,30 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card,
 	return 0;
 }
 
+static int snd_soc_init_single_cpu_dai(struct snd_soc_card *card,
+				   struct snd_soc_dai_link *dai_link)
+{
+	if (dai_link->cpu_name || dai_link->cpu_of_node ||
+					dai_link->cpu_dai_name) {
+		dai_link->num_cpu_dai = 1;
+		dai_link->cpu_dai = devm_kzalloc(card->dev,
+				sizeof(struct snd_soc_dai_link_component),
+				GFP_KERNEL);
+
+		if (!dai_link->cpu_dai)
+			return -ENOMEM;
+
+		dai_link->cpu_dai[0].name = dai_link->cpu_name;
+		dai_link->cpu_dai[0].of_node = dai_link->cpu_of_node;
+		dai_link->cpu_dai[0].dai_name = dai_link->cpu_dai_name;
+	} else if (!dai_link->cpu_dai) {
+		dev_err(card->dev, "ASoC: DAI link has no DAIs\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int soc_init_dai_link(struct snd_soc_card *card,
 				   struct snd_soc_dai_link *link)
 {
@@ -1054,6 +1128,12 @@ static int soc_init_dai_link(struct snd_soc_card *card,
 		return ret;
 	}
 
+	ret = snd_soc_init_single_cpu_dai(card, link);
+	if (ret) {
+		dev_err(card->dev, "ASoC: failed to init cpu\n");
+		return ret;
+	}
+
 	for (i = 0; i < link->num_codecs; i++) {
 		/*
 		 * Codec must be specified by 1 of name or OF node,
@@ -1089,24 +1169,28 @@ static int soc_init_dai_link(struct snd_soc_card *card,
 	 * can be left unspecified, and will be matched based on DAI
 	 * name alone..
 	 */
-	if (link->cpu_name && link->cpu_of_node) {
-		dev_err(card->dev,
-			"ASoC: Neither/both cpu name/of_node are set for %s\n",
-			link->name);
-		return -EINVAL;
-	}
-	/*
-	 * At least one of CPU DAI name or CPU device name/node must be
-	 * specified
-	 */
-	if (!link->cpu_dai_name &&
-	    !(link->cpu_name || link->cpu_of_node)) {
-		dev_err(card->dev,
-			"ASoC: Neither cpu_dai_name nor cpu_name/of_node are set for %s\n",
-			link->name);
-		return -EINVAL;
-	}
 
+	for (i = 0; i < link->num_cpu_dai; i++) {
+		if (link->cpu_dai[i].name &&
+			link->cpu_dai[i].of_node) {
+			dev_err(card->dev,
+			    "ASoC: Neither/both cpu name/of_node are set for %s\n",
+					link->cpu_dai[i].name);
+			return -EINVAL;
+		}
+
+		/*
+		 * At least one of CPU DAI name or CPU device name/node must be
+		 * specified
+		 */
+		if (!link->cpu_dai[i].dai_name &&
+			!(link->cpu_dai[i].name || link->cpu_dai[i].of_node)) {
+			dev_err(card->dev,
+			    "ASoC: Neither cpu_dai_name nor cpu_name/of_node are set for %s\n",
+				link->name);
+			return -EINVAL;
+		}
+	}
 	return 0;
 }
 
@@ -1426,6 +1510,9 @@ static int soc_link_dai_widgets(struct snd_soc_card *card,
 	if (rtd->num_codecs > 1)
 		dev_warn(card->dev, "ASoC: Multiple codecs not supported yet\n");
 
+	if (rtd->num_cpu_dai > 1)
+		dev_warn(card->dev, "ASoC: Multiple CPU DAIs not supported yet\n");
+
 	/* link the DAI widgets */
 	sink = codec_dai->playback_widget;
 	source = cpu_dai->capture_widget;
@@ -1456,12 +1543,24 @@ static int soc_link_dai_widgets(struct snd_soc_card *card,
 	return 0;
 }
 
+static bool soc_check_compress_dais(struct snd_soc_pcm_runtime *rtd)
+{
+	int i;
+
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		if (!rtd->cpu_dais[i]->driver->compress_new)
+			return false;
+	}
+
+	return true;
+}
+
 static int soc_probe_link_dais(struct snd_soc_card *card,
 		struct snd_soc_pcm_runtime *rtd, int order)
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	int i, ret;
+	bool is_compress;
 
 	dev_dbg(card->dev, "ASoC: probe %s dai link %d late %d\n",
 			card->name, rtd->num, order);
@@ -1469,9 +1568,11 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 	/* set default power off timeout */
 	rtd->pmdown_time = pmdown_time;
 
-	ret = soc_probe_dai(cpu_dai, order);
-	if (ret)
-		return ret;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		ret = soc_probe_dai(rtd->cpu_dais[i], order);
+		if (ret)
+			return ret;
+	}
 
 	/* probe the CODEC DAI */
 	for (i = 0; i < rtd->num_codecs; i++) {
@@ -1507,9 +1608,14 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 		soc_dpcm_debugfs_add(rtd);
 #endif
 
-	if (cpu_dai->driver->compress_new) {
+	is_compress = soc_check_compress_dais(rtd);
+	if (is_compress) {
+		if (rtd->num_cpu_dai > 1)
+			dev_warn(card->dev,
+				"ASoC: multi-cpu compress dais not supported");
+
 		/*create compress_device"*/
-		ret = cpu_dai->driver->compress_new(rtd, rtd->num);
+		ret = rtd->cpu_dais[0]->driver->compress_new(rtd, rtd->num);
 		if (ret < 0) {
 			dev_err(card->dev, "ASoC: can't create compress %s\n",
 					 dai_link->stream_name);
@@ -1525,7 +1631,8 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 				       dai_link->stream_name, ret);
 				return ret;
 			}
-			ret = soc_link_dai_pcm_new(&cpu_dai, 1, rtd);
+			ret = soc_link_dai_pcm_new(rtd->cpu_dais,
+					rtd->num_cpu_dai, rtd);
 			if (ret < 0)
 				return ret;
 			ret = soc_link_dai_pcm_new(rtd->codec_dais,
@@ -1644,7 +1751,7 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 	unsigned int dai_fmt)
 {
 	struct snd_soc_dai **codec_dais = rtd->codec_dais;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
 	unsigned int i;
 	int ret;
 
@@ -1659,35 +1766,44 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 		}
 	}
 
-	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
 	/* the component which has non_legacy_dai_naming is Codec */
-	if (cpu_dai->component->driver->non_legacy_dai_naming) {
-		unsigned int inv_dai_fmt;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		struct snd_soc_dai *cpu_dai = cpu_dais[i];
+		unsigned int inv_dai_fmt, temp_dai_fmt;
 
-		inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
-		switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
-		case SND_SOC_DAIFMT_CBM_CFM:
-			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
-			break;
-		case SND_SOC_DAIFMT_CBM_CFS:
-			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
-			break;
-		case SND_SOC_DAIFMT_CBS_CFM:
-			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
-			break;
-		case SND_SOC_DAIFMT_CBS_CFS:
-			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
-			break;
-		}
+		temp_dai_fmt = dai_fmt;
+		if (cpu_dai->component->driver->non_legacy_dai_naming) {
 
-		dai_fmt = inv_dai_fmt;
-	}
+			inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
 
-	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
-	if (ret != 0 && ret != -ENOTSUPP) {
-		dev_warn(cpu_dai->dev,
-			 "ASoC: Failed to set DAI format: %d\n", ret);
-		return ret;
+			switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+			case SND_SOC_DAIFMT_CBM_CFM:
+				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
+				break;
+
+			case SND_SOC_DAIFMT_CBM_CFS:
+				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
+				break;
+
+			case SND_SOC_DAIFMT_CBS_CFM:
+				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
+				break;
+
+			case SND_SOC_DAIFMT_CBS_CFS:
+				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
+				break;
+
+			}
+
+			temp_dai_fmt = inv_dai_fmt;
+		}
+
+		ret = snd_soc_dai_set_fmt(cpu_dai, temp_dai_fmt);
+		if (ret != 0 && ret != -ENOTSUPP) {
+			dev_warn(cpu_dai->dev,
+				"ASoC: Failed to set DAI format: %d\n", ret);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -2121,10 +2237,11 @@ int snd_soc_poweroff(struct device *dev)
 
 	/* deactivate pins to sleep state */
 	list_for_each_entry(rtd, &card->rtd_list, list) {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 		int i;
 
-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+		for (i = 0; i < rtd->num_cpu_dai; i++)
+			pinctrl_pm_select_sleep_state(rtd->cpu_dais[i]->dev);
+
 		for (i = 0; i < rtd->num_codecs; i++) {
 			struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
 			pinctrl_pm_select_sleep_state(codec_dai->dev);
@@ -2609,7 +2726,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
 
 	/* deactivate pins to sleep state */
 	list_for_each_entry(rtd, &card->rtd_list, list)  {
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		struct snd_soc_dai *cpu_dai;
 		int j;
 
 		for (j = 0; j < rtd->num_codecs; j++) {
@@ -2618,8 +2735,11 @@ int snd_soc_register_card(struct snd_soc_card *card)
 				pinctrl_pm_select_sleep_state(codec_dai->dev);
 		}
 
-		if (!cpu_dai->active)
-			pinctrl_pm_select_sleep_state(cpu_dai->dev);
+		for (j = 0; j < rtd->num_cpu_dai; j++) {
+			cpu_dai = rtd->cpu_dais[j];
+			if (!cpu_dai->active)
+				pinctrl_pm_select_sleep_state(cpu_dai->dev);
+		}
 	}
 
 	return ret;
-- 
2.7.4

[PATCH v6 2/3] ASoC: Add multiple CPU DAI support for PCM ops

[Shreyas NC]

Add support in PCM operations to invoke multiple cpu dais as we do
for multiple codec dais. Also the symmetry calculations are updated to
reflect multiple cpu dais.

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 sound/soc/soc-pcm.c | 491 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 332 insertions(+), 159 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 5e7ae47..cdcbc4f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
  */
 void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
 {
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	int i;
 
 	lockdep_assert_held(&rtd->pcm_mutex);
 
 	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		cpu_dai->playback_active++;
+		for (i = 0; i < rtd->num_cpu_dai; i++)
+			rtd->cpu_dais[i]->playback_active++;
 		for (i = 0; i < rtd->num_codecs; i++)
 			rtd->codec_dais[i]->playback_active++;
 	} else {
-		cpu_dai->capture_active++;
+		for (i = 0; i < rtd->num_cpu_dai; i++)
+			rtd->cpu_dais[i]->capture_active++;
 		for (i = 0; i < rtd->num_codecs; i++)
 			rtd->codec_dais[i]->capture_active++;
 	}
 
-	cpu_dai->active++;
-	cpu_dai->component->active++;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		rtd->cpu_dais[i]->component->active++;
+		rtd->cpu_dais[i]->active++;
+	}
+
 	for (i = 0; i < rtd->num_codecs; i++) {
 		rtd->codec_dais[i]->active++;
 		rtd->codec_dais[i]->component->active++;
@@ -99,23 +103,27 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
  */
 void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
 {
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	int i;
 
 	lockdep_assert_held(&rtd->pcm_mutex);
 
 	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		cpu_dai->playback_active--;
+		for (i = 0; i < rtd->num_cpu_dai; i++)
+			rtd->cpu_dais[i]->playback_active--;
 		for (i = 0; i < rtd->num_codecs; i++)
 			rtd->codec_dais[i]->playback_active--;
 	} else {
-		cpu_dai->capture_active--;
+		for (i = 0; i < rtd->num_cpu_dai; i++)
+			rtd->cpu_dais[i]->capture_active--;
 		for (i = 0; i < rtd->num_codecs; i++)
 			rtd->codec_dais[i]->capture_active--;
 	}
 
-	cpu_dai->active--;
-	cpu_dai->component->active--;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		rtd->cpu_dais[i]->component->active--;
+		rtd->cpu_dais[i]->active--;
+	}
+
 	for (i = 0; i < rtd->num_codecs; i++) {
 		rtd->codec_dais[i]->component->active--;
 		rtd->codec_dais[i]->active--;
@@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
 				struct snd_pcm_hw_params *params)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	unsigned int rate, channels, sample_bits, symmetry, i;
 
 	rate = params_rate(params);
@@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
 	sample_bits = snd_pcm_format_physical_width(params_format(params));
 
 	/* reject unmatched parameters when applying symmetry */
-	symmetry = cpu_dai->driver->symmetric_rates ||
-		rtd->dai_link->symmetric_rates;
+	symmetry = rtd->dai_link->symmetric_rates;
+
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
 
 	for (i = 0; i < rtd->num_codecs; i++)
 		symmetry |= rtd->codec_dais[i]->driver->symmetric_rates;
 
-	if (symmetry && cpu_dai->rate && cpu_dai->rate != rate) {
-		dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
-				cpu_dai->rate, rate);
-		return -EINVAL;
-	}
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		if (symmetry && rtd->cpu_dais[i]->rate &&
+					rtd->cpu_dais[i]->rate != rate) {
+			dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
+					rtd->cpu_dais[i]->rate, rate);
+			return -EINVAL;
+		}
 
-	symmetry = cpu_dai->driver->symmetric_channels ||
-		rtd->dai_link->symmetric_channels;
+	symmetry = rtd->dai_link->symmetric_channels;
+
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		symmetry |= rtd->cpu_dais[i]->driver->symmetric_channels;
 
 	for (i = 0; i < rtd->num_codecs; i++)
 		symmetry |= rtd->codec_dais[i]->driver->symmetric_channels;
 
-	if (symmetry && cpu_dai->channels && cpu_dai->channels != channels) {
-		dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
-				cpu_dai->channels, channels);
-		return -EINVAL;
-	}
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		if (symmetry && rtd->cpu_dais[i]->channels &&
+				rtd->cpu_dais[i]->channels != channels) {
+			dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
+					rtd->cpu_dais[i]->channels, channels);
+			return -EINVAL;
+		}
 
-	symmetry = cpu_dai->driver->symmetric_samplebits ||
-		rtd->dai_link->symmetric_samplebits;
+	symmetry = rtd->dai_link->symmetric_samplebits;
+
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		symmetry |= rtd->cpu_dais[i]->driver->symmetric_samplebits;
 
 	for (i = 0; i < rtd->num_codecs; i++)
 		symmetry |= rtd->codec_dais[i]->driver->symmetric_samplebits;
 
-	if (symmetry && cpu_dai->sample_bits && cpu_dai->sample_bits != sample_bits) {
-		dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
-				cpu_dai->sample_bits, sample_bits);
-		return -EINVAL;
-	}
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		if (symmetry && rtd->cpu_dais[i]->sample_bits &&
+				rtd->cpu_dais[i]->sample_bits != sample_bits) {
+			dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
+						rtd->cpu_dais[i]->sample_bits,
+								sample_bits);
+			return -EINVAL;
+		}
 
 	return 0;
 }
@@ -308,13 +328,18 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
 static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai_driver *cpu_driver = rtd->cpu_dai->driver;
 	struct snd_soc_dai_link *link = rtd->dai_link;
 	unsigned int symmetry, i;
 
-	symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
-		cpu_driver->symmetric_channels || link->symmetric_channels ||
-		cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
+	symmetry = link->symmetric_rates || link->symmetric_channels ||
+			link->symmetric_samplebits;
+
+	/* Apply symmetery for multiple cpu dais */
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		symmetry = symmetry ||
+			rtd->cpu_dais[i]->driver->symmetric_rates ||
+			rtd->cpu_dais[i]->driver->symmetric_channels ||
+			rtd->cpu_dais[i]->driver->symmetric_samplebits;
 
 	for (i = 0; i < rtd->num_codecs; i++)
 		symmetry = symmetry ||
@@ -342,10 +367,10 @@ static void soc_pcm_set_msb(struct snd_pcm_substream *substream, int bits)
 static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	int i;
-	unsigned int bits = 0, cpu_bits;
+	unsigned int bits = 0, cpu_bits = 0;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		for (i = 0; i < rtd->num_codecs; i++) {
@@ -356,7 +381,16 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
 			}
 			bits = max(codec_dai->driver->playback.sig_bits, bits);
 		}
-		cpu_bits = cpu_dai->driver->playback.sig_bits;
+
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+			if (cpu_dai->driver->playback.sig_bits == 0) {
+				cpu_bits = 0;
+				break;
+			}
+
+			cpu_bits = max(cpu_dai->driver->playback.sig_bits, cpu_bits);
+		}
 	} else {
 		for (i = 0; i < rtd->num_codecs; i++) {
 			codec_dai = rtd->codec_dais[i];
@@ -366,7 +400,15 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
 			}
 			bits = max(codec_dai->driver->capture.sig_bits, bits);
 		}
-		cpu_bits = cpu_dai->driver->capture.sig_bits;
+
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+			if (cpu_dai->driver->capture.sig_bits == 0) {
+				cpu_bits = 0;
+				break;
+			}
+			cpu_bits = max(cpu_dai->driver->capture.sig_bits, bits);
+		}
 	}
 
 	soc_pcm_set_msb(substream, bits);
@@ -378,21 +420,18 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_pcm_hardware *hw = &runtime->hw;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai_driver *cpu_dai_drv = rtd->cpu_dai->driver;
+	struct snd_soc_dai_driver *cpu_dai_drv;
 	struct snd_soc_dai_driver *codec_dai_drv;
 	struct snd_soc_pcm_stream *codec_stream;
 	struct snd_soc_pcm_stream *cpu_stream;
 	unsigned int chan_min = 0, chan_max = UINT_MAX;
+	unsigned int cpu_chan_min = 0, cpu_chan_max = UINT_MAX;
 	unsigned int rate_min = 0, rate_max = UINT_MAX;
-	unsigned int rates = UINT_MAX;
+	unsigned int cpu_rate_min = 0, cpu_rate_max = UINT_MAX;
+	unsigned int rates = UINT_MAX, cpu_rates = UINT_MAX;
 	u64 formats = ULLONG_MAX;
 	int i;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		cpu_stream = &cpu_dai_drv->playback;
-	else
-		cpu_stream = &cpu_dai_drv->capture;
-
 	/* first calculate min/max only for CODECs in the DAI link */
 	for (i = 0; i < rtd->num_codecs; i++) {
 
@@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
 	}
 
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai_drv = rtd->cpu_dais[i]->driver;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			cpu_stream = &cpu_dai_drv->playback;
+		else
+			cpu_stream = &cpu_dai_drv->capture;
+
+		cpu_chan_min = max(cpu_chan_min,
+					cpu_stream->channels_min);
+		cpu_chan_max = min(cpu_chan_max,
+					cpu_stream->channels_max);
+
+		if (hw->formats)
+			hw->formats &= cpu_stream->formats;
+		else
+			hw->formats = cpu_stream->formats;
+
+		cpu_rates = snd_pcm_rate_mask_intersect(cpu_rates,
+						cpu_stream->rates);
+
+		cpu_rate_min = max(cpu_rate_min, cpu_stream->rate_min);
+		cpu_rate_max = min_not_zero(cpu_rate_max, cpu_stream->rate_max);
+	}
+
 	/*
-	 * chan min/max cannot be enforced if there are multiple CODEC DAIs
-	 * connected to a single CPU DAI, use CPU DAI's directly and let
-	 * channel allocation be fixed up later
+	 * chan min/max cannot be enforced if there are multiple
+	 * CODEC DAIs connected to CPU DAI(s), use CPU DAI's
+	 * directly and let channel allocation be fixed up later
 	 */
 	if (rtd->num_codecs > 1) {
-		chan_min = cpu_stream->channels_min;
-		chan_max = cpu_stream->channels_max;
+		chan_min = cpu_chan_min;
+		chan_max = cpu_chan_max;
 	}
 
-	hw->channels_min = max(chan_min, cpu_stream->channels_min);
-	hw->channels_max = min(chan_max, cpu_stream->channels_max);
-	if (hw->formats)
-		hw->formats &= formats & cpu_stream->formats;
-	else
-		hw->formats = formats & cpu_stream->formats;
-	hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_stream->rates);
+	hw->channels_min = max(cpu_chan_min, chan_min);
+
+	hw->channels_max = min(cpu_chan_max, chan_max);
+
+	hw->rate_min = max(cpu_rate_min, rate_min);
+	hw->rate_max = min_not_zero(cpu_rate_max, rate_max);
+	hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_rates);
 
 	snd_pcm_limit_hw_rates(runtime);
 
-	hw->rate_min = max(hw->rate_min, cpu_stream->rate_min);
-	hw->rate_min = max(hw->rate_min, rate_min);
-	hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max);
-	hw->rate_max = min_not_zero(hw->rate_max, rate_max);
+	if (hw->formats)
+		hw->formats &= formats;
+	else
+		hw->formats = formats;
 }
 
 /*
@@ -459,12 +523,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	const char *codec_dai_name = "multicodec";
-	int i, ret = 0, __ret;
+	const char *cpu_dai_name = "multicpu";
+	int i, ret = 0, __ret, j;
+
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		pinctrl_pm_select_default_state(rtd->cpu_dais[i]->dev);
 
-	pinctrl_pm_select_default_state(cpu_dai->dev);
 	for (i = 0; i < rtd->num_codecs; i++)
 		pinctrl_pm_select_default_state(rtd->codec_dais[i]->dev);
 
@@ -477,12 +544,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
 	/* startup the audio subsystem */
-	if (cpu_dai->driver->ops->startup) {
-		ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
-		if (ret < 0) {
-			dev_err(cpu_dai->dev, "ASoC: can't open interface"
-				" %s: %d\n", cpu_dai->name, ret);
-			goto out;
+	for (j = 0; j < rtd->num_cpu_dai; j++) {
+		cpu_dai = rtd->cpu_dais[j];
+		if (cpu_dai->driver->ops->startup) {
+			ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
+			if (ret < 0) {
+				dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n",
+							cpu_dai->name, ret);
+				goto interface_err;
+			}
 		}
 	}
 
@@ -543,34 +613,40 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	if (rtd->num_codecs == 1)
 		codec_dai_name = rtd->codec_dai->name;
 
+	if (rtd->num_cpu_dai == 1)
+		cpu_dai_name = rtd->cpu_dai->name;
+
 	if (soc_pcm_has_symmetry(substream))
 		runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
 
 	ret = -EINVAL;
 	if (!runtime->hw.rates) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n",
-			codec_dai_name, cpu_dai->name);
+			codec_dai_name, cpu_dai_name);
 		goto config_err;
 	}
 	if (!runtime->hw.formats) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching formats\n",
-			codec_dai_name, cpu_dai->name);
+			codec_dai_name, cpu_dai_name);
 		goto config_err;
 	}
 	if (!runtime->hw.channels_min || !runtime->hw.channels_max ||
 	    runtime->hw.channels_min > runtime->hw.channels_max) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching channels\n",
-				codec_dai_name, cpu_dai->name);
+				codec_dai_name, cpu_dai_name);
 		goto config_err;
 	}
 
 	soc_pcm_apply_msb(substream);
 
 	/* Symmetry only applies if we've already got an active stream. */
-	if (cpu_dai->active) {
-		ret = soc_pcm_apply_symmetry(substream, cpu_dai);
-		if (ret != 0)
-			goto config_err;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		if (rtd->cpu_dais[i]->active) {
+			ret = soc_pcm_apply_symmetry(substream,
+							rtd->cpu_dais[i]);
+			if (ret != 0)
+				goto config_err;
+		}
 	}
 
 	for (i = 0; i < rtd->num_codecs; i++) {
@@ -583,7 +659,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	}
 
 	pr_debug("ASoC: %s <-> %s info:\n",
-			codec_dai_name, cpu_dai->name);
+			codec_dai_name, cpu_dai_name);
 	pr_debug("ASoC: rate mask 0x%x\n", runtime->hw.rates);
 	pr_debug("ASoC: min ch %d max ch %d\n", runtime->hw.channels_min,
 		 runtime->hw.channels_max);
@@ -622,9 +698,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		component->driver->ops->close(substream);
 	}
 
-	if (cpu_dai->driver->ops->shutdown)
-		cpu_dai->driver->ops->shutdown(substream, cpu_dai);
-out:
+	j = rtd->num_cpu_dai;
+
+interface_err:
+	while (--j >= 0) {
+		cpu_dai = rtd->cpu_dais[j];
+		if (cpu_dai->driver->ops->shutdown)
+			cpu_dai->driver->ops->shutdown(substream, cpu_dai);
+	}
+
 	mutex_unlock(&rtd->pcm_mutex);
 
 	for_each_rtdcom(rtd, rtdcom) {
@@ -638,8 +720,11 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		if (!rtd->codec_dais[i]->active)
 			pinctrl_pm_select_sleep_state(rtd->codec_dais[i]->dev);
 	}
-	if (!cpu_dai->active)
-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		if (!rtd->cpu_dais[i]->active)
+			pinctrl_pm_select_sleep_state(rtd->cpu_dais[i]->dev);
+	}
 
 	return ret;
 }
@@ -682,7 +767,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	int i;
 
@@ -691,8 +776,11 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	snd_soc_runtime_deactivate(rtd, substream->stream);
 
 	/* clear the corresponding DAIs rate when inactive */
-	if (!cpu_dai->active)
-		cpu_dai->rate = 0;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (!cpu_dai->active)
+			cpu_dai->rate = 0;
+	}
 
 	for (i = 0; i < rtd->num_codecs; i++) {
 		codec_dai = rtd->codec_dais[i];
@@ -700,10 +788,16 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 			codec_dai->rate = 0;
 	}
 
-	snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
+	}
 
-	if (cpu_dai->driver->ops->shutdown)
-		cpu_dai->driver->ops->shutdown(substream, cpu_dai);
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->driver->ops->shutdown)
+			cpu_dai->driver->ops->shutdown(substream, cpu_dai);
+	}
 
 	for (i = 0; i < rtd->num_codecs; i++) {
 		codec_dai = rtd->codec_dais[i];
@@ -756,8 +850,11 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 		if (!rtd->codec_dais[i]->active)
 			pinctrl_pm_select_sleep_state(rtd->codec_dais[i]->dev);
 	}
-	if (!cpu_dai->active)
-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		if (!rtd->cpu_dais[i]->active)
+			pinctrl_pm_select_sleep_state(rtd->cpu_dais[i]->dev);
+	}
 
 	return 0;
 }
@@ -772,7 +869,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	int i, ret = 0;
 
@@ -816,12 +913,16 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
 		}
 	}
 
-	if (cpu_dai->driver->ops->prepare) {
-		ret = cpu_dai->driver->ops->prepare(substream, cpu_dai);
-		if (ret < 0) {
-			dev_err(cpu_dai->dev,
-				"ASoC: cpu DAI prepare error: %d\n", ret);
-			goto out;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->driver->ops->prepare) {
+			ret = cpu_dai->driver->ops->prepare(substream, cpu_dai);
+			if (ret < 0) {
+				dev_err(cpu_dai->dev,
+					"ASoC: cpu DAI prepare error: %d\n",
+					ret);
+				goto out;
+			}
 		}
 	}
 
@@ -838,7 +939,10 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
 	for (i = 0; i < rtd->num_codecs; i++)
 		snd_soc_dai_digital_mute(rtd->codec_dais[i], 0,
 					 substream->stream);
-	snd_soc_dai_digital_mute(cpu_dai, 0, substream->stream);
+
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		snd_soc_dai_digital_mute(rtd->cpu_dais[i], 0,
+					substream->stream);
 
 out:
 	mutex_unlock(&rtd->pcm_mutex);
@@ -885,8 +989,8 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int i, ret = 0, __ret;
+	struct snd_soc_dai *cpu_dai;
+	int i, ret = 0, __ret, j;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 	if (rtd->dai_link->ops->hw_params) {
@@ -940,9 +1044,12 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 						params_format(&codec_params));
 	}
 
-	ret = soc_dai_hw_params(substream, params, cpu_dai);
-	if (ret < 0)
-		goto interface_err;
+	for (j = 0; j < rtd->num_cpu_dai; j++) {
+		cpu_dai = rtd->cpu_dais[j];
+		ret = soc_dai_hw_params(substream, params, cpu_dai);
+		if (ret < 0)
+			goto interface_err;
+	}
 
 	ret = 0;
 	for_each_rtdcom(rtd, rtdcom) {
@@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 	if (ret < 0)
 		goto component_err;
 
-	/* store the parameters for each DAIs */
-	cpu_dai->rate = params_rate(params);
-	cpu_dai->channels = params_channels(params);
-	cpu_dai->sample_bits =
-		snd_pcm_format_physical_width(params_format(params));
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		/* store the parameters for each DAIs */
+		cpu_dai = rtd->cpu_dais[i];
+		cpu_dai->rate = params_rate(params);
+		cpu_dai->channels = params_channels(params);
+		cpu_dai->sample_bits =
+			snd_pcm_format_physical_width(params_format(params));
+	}
 
 	ret = soc_pcm_params_symmetry(substream, params);
         if (ret)
@@ -987,10 +1097,16 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		component->driver->ops->hw_free(substream);
 	}
 
-	if (cpu_dai->driver->ops->hw_free)
-		cpu_dai->driver->ops->hw_free(substream, cpu_dai);
+	j = rtd->num_cpu_dai;
 
 interface_err:
+	while (--j >= 0) {
+		cpu_dai = rtd->cpu_dais[j];
+
+		if (cpu_dai->driver->ops && cpu_dai->driver->ops->hw_free)
+			cpu_dai->driver->ops->hw_free(substream, cpu_dai);
+	}
+
 	i = rtd->num_codecs;
 
 codec_err:
@@ -1016,7 +1132,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
 	int i;
@@ -1024,10 +1140,13 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
 	/* clear the corresponding DAIs parameters when going to be inactive */
-	if (cpu_dai->active == 1) {
-		cpu_dai->rate = 0;
-		cpu_dai->channels = 0;
-		cpu_dai->sample_bits = 0;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->active == 1) {
+			cpu_dai->rate = 0;
+			cpu_dai->channels = 0;
+			cpu_dai->sample_bits = 0;
+		}
 	}
 
 	for (i = 0; i < rtd->num_codecs; i++) {
@@ -1069,8 +1188,11 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 			codec_dai->driver->ops->hw_free(substream, codec_dai);
 	}
 
-	if (cpu_dai->driver->ops->hw_free)
-		cpu_dai->driver->ops->hw_free(substream, cpu_dai);
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->driver->ops && cpu_dai->driver->ops->hw_free)
+			cpu_dai->driver->ops->hw_free(substream, cpu_dai);
+	}
 
 	mutex_unlock(&rtd->pcm_mutex);
 	return 0;
@@ -1081,7 +1203,7 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	int i, ret;
 
@@ -1107,10 +1229,14 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 			return ret;
 	}
 
-	if (cpu_dai->driver->ops->trigger) {
-		ret = cpu_dai->driver->ops->trigger(substream, cmd, cpu_dai);
-		if (ret < 0)
-			return ret;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->driver->ops->trigger) {
+			ret = cpu_dai->driver->ops->trigger(substream,
+								cmd, cpu_dai);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	if (rtd->dai_link->ops->trigger) {
@@ -1126,7 +1252,7 @@ static int soc_pcm_bespoke_trigger(struct snd_pcm_substream *substream,
 				   int cmd)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	int i, ret;
 
@@ -1140,10 +1266,14 @@ static int soc_pcm_bespoke_trigger(struct snd_pcm_substream *substream,
 		}
 	}
 
-	if (cpu_dai->driver->ops->bespoke_trigger) {
-		ret = cpu_dai->driver->ops->bespoke_trigger(substream, cmd, cpu_dai);
-		if (ret < 0)
-			return ret;
+	for (i = 0; i < rtd->num_cpu_dai; i++) {
+		cpu_dai = rtd->cpu_dais[i];
+		if (cpu_dai->driver->ops->bespoke_trigger) {
+			ret = cpu_dai->driver->ops->bespoke_trigger(substream,
+								cmd, cpu_dai);
+			if (ret < 0)
+				return ret;
+		}
 	}
 	return 0;
 }
@@ -1157,12 +1287,13 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	snd_pcm_uframes_t offset = 0;
 	snd_pcm_sframes_t delay = 0;
 	snd_pcm_sframes_t codec_delay = 0;
+	snd_pcm_sframes_t cpu_delay = 0;
 	int i;
 
 	for_each_rtdcom(rtd, rtdcom) {
@@ -1177,8 +1308,16 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 		break;
 	}
 
+	/*
+	 * Use the delay of cpu_dai[0] as in the case of multi cpu DAIs, the
+	 * CPU DAIs are expected to start together and will have the same
+	 * delay
+	 */
+	cpu_dai = rtd->cpu_dais[0];
 	if (cpu_dai->driver->ops->delay)
-		delay += cpu_dai->driver->ops->delay(substream, cpu_dai);
+		cpu_delay = cpu_dai->driver->ops->delay(substream, cpu_dai);
+
+	delay += cpu_delay;
 
 	for (i = 0; i < rtd->num_codecs; i++) {
 		codec_dai = rtd->codec_dais[i];
@@ -1301,12 +1440,16 @@ static struct snd_soc_pcm_runtime *dpcm_get_be(struct snd_soc_card *card,
 			if (!be->dai_link->no_pcm)
 				continue;
 
-			dev_dbg(card->dev, "ASoC: try BE : %s\n",
-				be->cpu_dai->playback_widget ?
-				be->cpu_dai->playback_widget->name : "(not set)");
+			for (i = 0; i < be->num_cpu_dai; i++) {
+				struct snd_soc_dai *cpu_dai = be->cpu_dais[i];
 
-			if (be->cpu_dai->playback_widget == widget)
-				return be;
+				dev_dbg(card->dev, "ASoC: try BE : %s\n",
+					cpu_dai->playback_widget ?
+					cpu_dai->playback_widget->name : "(not set)");
+
+				if (cpu_dai->playback_widget == widget)
+					return be;
+			}
 
 			for (i = 0; i < be->num_codecs; i++) {
 				struct snd_soc_dai *dai = be->codec_dais[i];
@@ -1321,12 +1464,16 @@ static struct snd_soc_pcm_runtime *dpcm_get_be(struct snd_soc_card *card,
 			if (!be->dai_link->no_pcm)
 				continue;
 
-			dev_dbg(card->dev, "ASoC: try BE %s\n",
-				be->cpu_dai->capture_widget ?
-				be->cpu_dai->capture_widget->name : "(not set)");
+			for (i = 0; i < be->num_cpu_dai; i++) {
+				struct snd_soc_dai *cpu_dai = be->cpu_dais[i];
 
-			if (be->cpu_dai->capture_widget == widget)
-				return be;
+				dev_dbg(card->dev, "ASoC: try BE %s\n",
+					cpu_dai->capture_widget ?
+					cpu_dai->capture_widget->name : "(not set)");
+
+				if (cpu_dai->capture_widget == widget)
+					return be;
+			}
 
 			for (i = 0; i < be->num_codecs; i++) {
 				struct snd_soc_dai *dai = be->codec_dais[i];
@@ -1376,8 +1523,12 @@ static bool dpcm_end_walk_at_be(struct snd_soc_dapm_widget *widget,
 			if (!rtd->dai_link->no_pcm)
 				continue;
 
-			if (rtd->cpu_dai->playback_widget == widget)
-				return true;
+			for (i = 0; i < rtd->num_cpu_dai; ++i) {
+				struct snd_soc_dai *cpu_dai = rtd->cpu_dais[i];
+
+				if (cpu_dai->playback_widget == widget)
+					return true;
+			}
 
 			for (i = 0; i < rtd->num_codecs; ++i) {
 				struct snd_soc_dai *dai = rtd->codec_dais[i];
@@ -1390,8 +1541,12 @@ static bool dpcm_end_walk_at_be(struct snd_soc_dapm_widget *widget,
 			if (!rtd->dai_link->no_pcm)
 				continue;
 
-			if (rtd->cpu_dai->capture_widget == widget)
-				return true;
+			for (i = 0; i < rtd->num_cpu_dai; ++i) {
+				struct snd_soc_dai *cpu_dai = rtd->cpu_dais[i];
+
+				if (cpu_dai->capture_widget == widget)
+					return true;
+			}
 
 			for (i = 0; i < rtd->num_codecs; ++i) {
 				struct snd_soc_dai *dai = rtd->codec_dais[i];
@@ -1431,12 +1586,20 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream,
 	/* Destroy any old FE <--> BE connections */
 	list_for_each_entry(dpcm, &fe->dpcm[stream].be_clients, list_be) {
 		unsigned int i;
+		int widget_found = 0;
 
 		/* is there a valid CPU DAI widget for this BE */
-		widget = dai_get_widget(dpcm->be->cpu_dai, stream);
+		for (i = 0; i < dpcm->be->num_cpu_dai; i++) {
+			struct snd_soc_dai *dai = dpcm->be->cpu_dais[i];
+
+			widget = dai_get_widget(dai, stream);
+
+			/* prune the BE if it's no longer in our active list */
+			if (widget && widget_in_list(list, widget))
+				widget_found++;
+		}
 
-		/* prune the BE if it's no longer in our active list */
-		if (widget && widget_in_list(list, widget))
+		if (widget_found > 0)
 			continue;
 
 		/* is there a valid CODEC DAI widget for this BE */
@@ -1778,11 +1941,13 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
 			be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
 
 		/* Symmetry only applies if we've got an active stream. */
-		if (rtd->cpu_dai->active) {
-			err = soc_pcm_apply_symmetry(fe_substream,
-						     rtd->cpu_dai);
-			if (err < 0)
-				return err;
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			if (rtd->cpu_dais[i]->active) {
+				err = soc_pcm_apply_symmetry(be_substream,
+							rtd->cpu_dais[i]);
+				if (err < 0)
+					return err;
+			}
 		}
 
 		for (i = 0; i < rtd->num_codecs; i++) {
@@ -2884,13 +3049,13 @@ static int soc_rtdcom_mmap(struct snd_pcm_substream *substream,
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
 	struct snd_soc_dai *codec_dai;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_pcm *pcm;
 	char new_name[64];
 	int ret = 0, playback = 0, capture = 0;
-	int i;
+	int i, cpu_capture = 0, cpu_playback = 0;
 
 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
 		playback = rtd->dai_link->dpcm_playback;
@@ -2904,8 +3069,16 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 				capture = 1;
 		}
 
-		capture = capture && cpu_dai->driver->capture.channels_min;
-		playback = playback && cpu_dai->driver->playback.channels_min;
+		for (i = 0; i < rtd->num_cpu_dai; i++) {
+			cpu_dai = rtd->cpu_dais[i];
+			if (cpu_dai->driver->playback.channels_min)
+				cpu_playback = 1;
+			if (cpu_dai->driver->capture.channels_min)
+				cpu_capture = 1;
+		}
+
+		playback = playback && cpu_playback;
+		capture = capture && cpu_capture;
 	}
 
 	if (rtd->dai_link->playback_only) {
@@ -3026,7 +3199,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 out:
 	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
 		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
-		 cpu_dai->name);
+		 (rtd->num_cpu_dai > 1) ? "multicpu" : rtd->cpu_dai->name);
 	return ret;
 }
 
-- 
2.7.4

[PATCH v6 3/3] ASoC: Add multiple CPU DAI support in DAPM

[Shreyas NC]

DAPM handles DAIs during soc_dapm_stream_event() and during addition
and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and
dapm_connect_dai_link_widgets().

Extend these functions to handle multiple cpu dai.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 sound/soc/soc-dapm.c | 71 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index a099c3e..865c47f 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -4113,38 +4113,57 @@ 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,
-					  struct snd_soc_pcm_runtime *rtd)
+static void dapm_add_valid_dai_widget(struct snd_soc_card *card,
+					struct snd_soc_pcm_runtime *rtd,
+					struct snd_soc_dai *codec_dai,
+					struct snd_soc_dai *cpu_dai)
 {
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_soc_dapm_widget *sink, *source;
-	int i;
 
-	for (i = 0; i < rtd->num_codecs; i++) {
-		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
+	/* connect BE DAI playback if widgets are valid */
+	if (codec_dai->playback_widget && cpu_dai->playback_widget) {
+		source = cpu_dai->playback_widget;
+		sink = codec_dai->playback_widget;
+		dev_err(rtd->dev, "connected DAI link %s:%s -> %s:%s\n",
+				cpu_dai->component->name,
+				source->name,
+				codec_dai->component->name,
+				sink->name);
+
+		snd_soc_dapm_add_path(&card->dapm, source, sink,
+				NULL, NULL);
+	}
 
-		/* connect BE DAI playback if widgets are valid */
-		if (codec_dai->playback_widget && cpu_dai->playback_widget) {
-			source = cpu_dai->playback_widget;
-			sink = codec_dai->playback_widget;
-			dev_dbg(rtd->dev, "connected DAI link %s:%s -> %s:%s\n",
-				cpu_dai->component->name, source->name,
-				codec_dai->component->name, sink->name);
+	/* connect BE DAI capture if widgets are valid */
+	if (codec_dai->capture_widget && cpu_dai->capture_widget) {
+		source = codec_dai->capture_widget;
+		sink = cpu_dai->capture_widget;
+		dev_err(rtd->dev, "connected DAI link %s:%s -> %s:%s\n",
+				codec_dai->component->name,
+				source->name,
+				cpu_dai->component->name,
+				sink->name);
 
-			snd_soc_dapm_add_path(&card->dapm, source, sink,
+		snd_soc_dapm_add_path(&card->dapm, source, sink,
 				NULL, NULL);
-		}
+	}
+
+}
 
-		/* connect BE DAI capture if widgets are valid */
-		if (codec_dai->capture_widget && cpu_dai->capture_widget) {
-			source = codec_dai->capture_widget;
-			sink = cpu_dai->capture_widget;
-			dev_dbg(rtd->dev, "connected DAI link %s:%s -> %s:%s\n",
-				codec_dai->component->name, source->name,
-				cpu_dai->component->name, sink->name);
+static void dapm_connect_dai_link_widgets(struct snd_soc_card *card,
+					  struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai *cpu_dai;
+	int i, j;
 
-			snd_soc_dapm_add_path(&card->dapm, source, sink,
-				NULL, NULL);
+	for (i = 0; i < rtd->num_codecs; i++) {
+		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
+
+		for (j = 0; j < rtd->num_cpu_dai; j++) {
+			cpu_dai = rtd->cpu_dais[j];
+
+			dapm_add_valid_dai_widget(card, rtd,
+						codec_dai, cpu_dai);
 		}
 	}
 }
@@ -4211,7 +4230,9 @@ static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
 {
 	int i;
 
-	soc_dapm_dai_stream_event(rtd->cpu_dai, stream, event);
+	for (i = 0; i < rtd->num_cpu_dai; i++)
+		soc_dapm_dai_stream_event(rtd->cpu_dais[i], stream, event);
+
 	for (i = 0; i < rtd->num_codecs; i++)
 		soc_dapm_dai_stream_event(rtd->codec_dais[i], stream, event);
 
-- 
2.7.4

Re: [PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Pierre-Louis Bossart]

>   	/* close any waiting streams */
> @@ -552,16 +565,21 @@ int snd_soc_suspend(struct device *dev)
>   	}
>   
>   	list_for_each_entry(rtd, &card->rtd_list, list) {
> -		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +		struct snd_soc_dai *cpu_dai;
>   
>   		if (rtd->dai_link->ignore_suspend)
>   			continue;
>   
> -		if (cpu_dai->driver->suspend && cpu_dai->driver->bus_control)
> -			cpu_dai->driver->suspend(cpu_dai);
> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
> +			cpu_dai = rtd->cpu_dais[i];
>   
> -		/* deactivate pins to sleep state */
> -		pinctrl_pm_select_sleep_state(cpu_dai->dev);
> +			if (cpu_dai->driver->suspend &&
> +					cpu_dai->driver->bus_control)
> +				cpu_dai->driver->suspend(cpu_dai);
> +
> +			/* deactivate pins to sleep state */
> +			pinctrl_pm_select_sleep_state(cpu_dai->dev);
> +		}
I am not sure I get the relationship between cpu_dai and pins. Is this 
always safe/ok to play with the pincrtl before all cpu_dais are suspended?
Or should you implement this with two loops, one to suspend and one to 
deactivate pins?
> @@ -845,35 +882,47 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
>   {
>   	struct snd_soc_pcm_runtime *rtd;
>   	struct snd_soc_dai_link_component *codecs = dai_link->codecs;
> -	struct snd_soc_dai_link_component cpu_dai_component;
> +	struct snd_soc_dai_link_component *cpu_dai_component;
>   	struct snd_soc_component *component;
> -	struct snd_soc_dai **codec_dais;
> +	struct snd_soc_dai **codec_dais, **cpu_dais;
>   	struct device_node *platform_of_node;
>   	const char *platform_name;
>   	int i;
>   
>   	dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name);
>   
> +	cpu_dai_component = dai_link->cpu_dai;
> +
>   	if (soc_is_dai_link_bound(card, dai_link)) {
>   		dev_dbg(card->dev, "ASoC: dai link %s already bound\n",
>   			dai_link->name);
>   		return 0;
>   	}
>   
> +	if (dai_link->dynamic && dai_link->num_cpu_dai > 1) {
> +		dev_err(card->dev, "ASoC: Multi CPU DAI not supported for FE");
> +		return -EINVAL;
> +	}
> +
>   	rtd = soc_new_pcm_runtime(card, dai_link);
>   	if (!rtd)
>   		return -ENOMEM;
>   
> -	cpu_dai_component.name = dai_link->cpu_name;
> -	cpu_dai_component.of_node = dai_link->cpu_of_node;
> -	cpu_dai_component.dai_name = dai_link->cpu_dai_name;

Have you checked if there are any side effects of using
cpu_dai_component = dai_link->cpu_dai;
instead of a local variable?

if you are not sure, it may be worth implementing as a separate patch 
first before introducing the multi-cpu part, at least to allow for git 
bisect to identify issues?
>   
> +static int snd_soc_init_single_cpu_dai(struct snd_soc_card *card,
> +				   struct snd_soc_dai_link *dai_link)
> +{
> +	if (dai_link->cpu_name || dai_link->cpu_of_node ||
> +					dai_link->cpu_dai_name) {
> +		dai_link->num_cpu_dai = 1;
> +		dai_link->cpu_dai = devm_kzalloc(card->dev,
> +				sizeof(struct snd_soc_dai_link_component),
> +				GFP_KERNEL);
> +
> +		if (!dai_link->cpu_dai)
> +			return -ENOMEM;
> +
> +		dai_link->cpu_dai[0].name = dai_link->cpu_name;
> +		dai_link->cpu_dai[0].of_node = dai_link->cpu_of_node;
> +		dai_link->cpu_dai[0].dai_name = dai_link->cpu_dai_name;
Question: is cpu_dai[i].of_node defined for i>0 in the multi cpu_dai case?

> @@ -1644,7 +1751,7 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>   	unsigned int dai_fmt)
>   {
>   	struct snd_soc_dai **codec_dais = rtd->codec_dais;
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
>   	unsigned int i;
>   	int ret;
>   
> @@ -1659,35 +1766,44 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>   		}
>   	}
>   
> -	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
why was this comment removed?

>   	/* the component which has non_legacy_dai_naming is Codec */
> -	if (cpu_dai->component->driver->non_legacy_dai_naming) {
Not sure if the code refactoring below makes sense in a codec-codec 
link, you probably wouldn't have multiple cpu_dais then, would you?
> -		unsigned int inv_dai_fmt;
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		struct snd_soc_dai *cpu_dai = cpu_dais[i];
> +		unsigned int inv_dai_fmt, temp_dai_fmt;
>   
> -		inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
> -		switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> -		case SND_SOC_DAIFMT_CBM_CFM:
> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
> -			break;
> -		case SND_SOC_DAIFMT_CBM_CFS:
> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
> -			break;
> -		case SND_SOC_DAIFMT_CBS_CFM:
> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
> -			break;
> -		case SND_SOC_DAIFMT_CBS_CFS:
> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> -			break;
> -		}
> +		temp_dai_fmt = dai_fmt;
> +		if (cpu_dai->component->driver->non_legacy_dai_naming) {
>   
> -		dai_fmt = inv_dai_fmt;
> -	}
> +			inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
>   
> -	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
> -	if (ret != 0 && ret != -ENOTSUPP) {
> -		dev_warn(cpu_dai->dev,
> -			 "ASoC: Failed to set DAI format: %d\n", ret);
> -		return ret;
> +			switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +			case SND_SOC_DAIFMT_CBM_CFM:
> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +				break;
> +
> +			case SND_SOC_DAIFMT_CBM_CFS:
> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +				break;
> +
> +			case SND_SOC_DAIFMT_CBS_CFM:
> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +				break;
> +
> +			case SND_SOC_DAIFMT_CBS_CFS:
> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +				break;
> +
> +			}
> +
> +			temp_dai_fmt = inv_dai_fmt;
> +		}
> +
> +		ret = snd_soc_dai_set_fmt(cpu_dai, temp_dai_fmt);
> +		if (ret != 0 && ret != -ENOTSUPP) {
> +			dev_warn(cpu_dai->dev,
> +				"ASoC: Failed to set DAI format: %d\n", ret);
> +			return ret;
> +		}
>   	}
>   
>   	

Re: [PATCH v6 2/3] ASoC: Add multiple CPU DAI support for PCM ops

[Pierre-Louis Bossart]

On 06/20/2018 05:54 AM, Shreyas NC wrote:
> Add support in PCM operations to invoke multiple cpu dais as we do
> for multiple codec dais. Also the symmetry calculations are updated to
> reflect multiple cpu dais.
This doesn't apply on Mark's tree?
Looks like you need to rebase on top of 244e293690a6 ("ASoC: pcm: Tidy 
up open/hw_params handling")
contributed by Charles on June 19.
>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> ---
>   sound/soc/soc-pcm.c | 491 +++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 332 insertions(+), 159 deletions(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 5e7ae47..cdcbc4f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
>    */
>   void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
>   {
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>   	int i;
>   
>   	lockdep_assert_held(&rtd->pcm_mutex);
>   
>   	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -		cpu_dai->playback_active++;
> +		for (i = 0; i < rtd->num_cpu_dai; i++)
> +			rtd->cpu_dais[i]->playback_active++;
>   		for (i = 0; i < rtd->num_codecs; i++)
>   			rtd->codec_dais[i]->playback_active++;
>   	} else {
> -		cpu_dai->capture_active++;
> +		for (i = 0; i < rtd->num_cpu_dai; i++)
> +			rtd->cpu_dais[i]->capture_active++;
>   		for (i = 0; i < rtd->num_codecs; i++)
>   			rtd->codec_dais[i]->capture_active++;
>   	}
>   
> -	cpu_dai->active++;
> -	cpu_dai->component->active++;
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		rtd->cpu_dais[i]->component->active++;
> +		rtd->cpu_dais[i]->active++;
> +	}

This is becoming complicated, how many times do we need to ref-count?
> +
>   	for (i = 0; i < rtd->num_codecs; i++) {
>   		rtd->codec_dais[i]->active++;
>   		rtd->codec_dais[i]->component->active++;
> @@ -99,23 +103,27 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
>    */
>   void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
>   {
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>   	int i;
>   
>   	lockdep_assert_held(&rtd->pcm_mutex);
>   
>   	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -		cpu_dai->playback_active--;
> +		for (i = 0; i < rtd->num_cpu_dai; i++)
> +			rtd->cpu_dais[i]->playback_active--;
>   		for (i = 0; i < rtd->num_codecs; i++)
>   			rtd->codec_dais[i]->playback_active--;
>   	} else {
> -		cpu_dai->capture_active--;
> +		for (i = 0; i < rtd->num_cpu_dai; i++)
> +			rtd->cpu_dais[i]->capture_active--;
>   		for (i = 0; i < rtd->num_codecs; i++)
>   			rtd->codec_dais[i]->capture_active--;
>   	}
>   
> -	cpu_dai->active--;
> -	cpu_dai->component->active--;
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		rtd->cpu_dais[i]->component->active--;
> +		rtd->cpu_dais[i]->active--;
> +	}
> +
>   	for (i = 0; i < rtd->num_codecs; i++) {
>   		rtd->codec_dais[i]->component->active--;
>   		rtd->codec_dais[i]->active--;
> @@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
>   				struct snd_pcm_hw_params *params)
>   {
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>   	unsigned int rate, channels, sample_bits, symmetry, i;
>   
>   	rate = params_rate(params);
> @@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
>   	sample_bits = snd_pcm_format_physical_width(params_format(params));
>   
>   	/* reject unmatched parameters when applying symmetry */
> -	symmetry = cpu_dai->driver->symmetric_rates ||
> -		rtd->dai_link->symmetric_rates;
This was a logical OR, but...

> +	symmetry = rtd->dai_link->symmetric_rates;
> +
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
this is a bitwise OR. is this ok?
>   
>   	for (i = 0; i < rtd->num_codecs; i++)
>   		symmetry |= rtd->codec_dais[i]->driver->symmetric_rates;
>   
> -	if (symmetry && cpu_dai->rate && cpu_dai->rate != rate) {
> -		dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
> -				cpu_dai->rate, rate);
> -		return -EINVAL;
> -	}
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		if (symmetry && rtd->cpu_dais[i]->rate &&
you could move the if (symmetry) out of the for loop to simplify
> +					rtd->cpu_dais[i]->rate != rate) {
> +			dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
> +					rtd->cpu_dais[i]->rate, rate);
> +			return -EINVAL;
> +		}
>   
> -	symmetry = cpu_dai->driver->symmetric_channels ||
> -		rtd->dai_link->symmetric_channels;
> +	symmetry = rtd->dai_link->symmetric_channels;
> +
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		symmetry |= rtd->cpu_dais[i]->driver->symmetric_channels;
>   
>   	for (i = 0; i < rtd->num_codecs; i++)
>   		symmetry |= rtd->codec_dais[i]->driver->symmetric_channels;
>   
> -	if (symmetry && cpu_dai->channels && cpu_dai->channels != channels) {
> -		dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
> -				cpu_dai->channels, channels);
> -		return -EINVAL;
> -	}
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		if (symmetry && rtd->cpu_dais[i]->channels &&
here as well
> +				rtd->cpu_dais[i]->channels != channels) {
> +			dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
> +					rtd->cpu_dais[i]->channels, channels);
> +			return -EINVAL;
> +		}
>   
> -	symmetry = cpu_dai->driver->symmetric_samplebits ||
> -		rtd->dai_link->symmetric_samplebits;
> +	symmetry = rtd->dai_link->symmetric_samplebits;
> +
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		symmetry |= rtd->cpu_dais[i]->driver->symmetric_samplebits;
>   
>   	for (i = 0; i < rtd->num_codecs; i++)
>   		symmetry |= rtd->codec_dais[i]->driver->symmetric_samplebits;
>   
> -	if (symmetry && cpu_dai->sample_bits && cpu_dai->sample_bits != sample_bits) {
> -		dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
> -				cpu_dai->sample_bits, sample_bits);
> -		return -EINVAL;
> -	}
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		if (symmetry && rtd->cpu_dais[i]->sample_bits &&
> +				rtd->cpu_dais[i]->sample_bits != sample_bits) {
> +			dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
> +						rtd->cpu_dais[i]->sample_bits,
> +								sample_bits);
> +			return -EINVAL;
> +		}
>   
>   	return 0;
>   }
> @@ -308,13 +328,18 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
>   static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
>   {
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> -	struct snd_soc_dai_driver *cpu_driver = rtd->cpu_dai->driver;
>   	struct snd_soc_dai_link *link = rtd->dai_link;
>   	unsigned int symmetry, i;
>   
> -	symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
> -		cpu_driver->symmetric_channels || link->symmetric_channels ||
> -		cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
> +	symmetry = link->symmetric_rates || link->symmetric_channels ||
> +			link->symmetric_samplebits;
> +
> +	/* Apply symmetery for multiple cpu dais */
You haven't fixed this comment, is this patch the correct one?
>   
> @@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
>   		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
>   	}
>   
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		cpu_dai_drv = rtd->cpu_dais[i]->driver;
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			cpu_stream = &cpu_dai_drv->playback;
> +		else
> +			cpu_stream = &cpu_dai_drv->capture;
> +
> +		cpu_chan_min = max(cpu_chan_min,
> +					cpu_stream->channels_min);
> +		cpu_chan_max = min(cpu_chan_max,
> +					cpu_stream->channels_max);
> +
> +		if (hw->formats)
> +			hw->formats &= cpu_stream->formats;
> +		else
> +			hw->formats = cpu_stream->formats;
Can you double-check the 'else' case? This doesn't seem right, you will 
always use the format used for the last cpu_dai. If the formats are 
identical for all cpu_dais, then this can be moved outside of the loop.
> @@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>   	if (ret < 0)
>   		goto component_err;
>   
> -	/* store the parameters for each DAIs */
> -	cpu_dai->rate = params_rate(params);
> -	cpu_dai->channels = params_channels(params);
> -	cpu_dai->sample_bits =
> -		snd_pcm_format_physical_width(params_format(params));
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		/* store the parameters for each DAIs */
> +		cpu_dai = rtd->cpu_dais[i];
> +		cpu_dai->rate = params_rate(params);
> +		cpu_dai->channels = params_channels(params);
> +		cpu_dai->sample_bits =
> +			snd_pcm_format_physical_width(params_format(params));
> +	}
I think I've asked this before but can't recall the answer: does this 
mean we assume the same number of channels for each codec_dai[j] and 
cpu_dai[i]?

> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		cpu_dai = rtd->cpu_dais[i];
> +		if (cpu_dai->driver->ops->trigger) {
> +			ret = cpu_dai->driver->ops->trigger(substream,
> +								cmd, cpu_dai);
> +			if (ret < 0)
> +				return ret;
> +		}

Maybe add a comment that in the multi_cpu case the triggers are supposed 
to be aligned, i.e. the first trigger starts the others - that would be 
consistent with the other comments on delay below.
> @@ -1778,11 +1941,13 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
>   			be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
>   
>   		/* Symmetry only applies if we've got an active stream. */
> -		if (rtd->cpu_dai->active) {
> -			err = soc_pcm_apply_symmetry(fe_substream,
> -						     rtd->cpu_dai);
> -			if (err < 0)
> -				return err;
> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
> +			if (rtd->cpu_dais[i]->active) {
> +				err = soc_pcm_apply_symmetry(be_substream,
> +							rtd->cpu_dais[i]);
> +				if (err < 0)
> +					return err;
> +			}
>   		}
Can you recheck this block? In the original code the symmetry is with 
the fe_substream, it's now with a be_substream. Looks to me like a major 
typo having significant impact of the result...

>   
>   		for (i = 0; i < rtd->num_codecs; i++) {
> @@ -2884,13 +3049,13 @@ static int soc_rtdcom_mmap(struct snd_pcm_substream *substream,
>   int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   {
>   	struct snd_soc_dai *codec_dai;
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	struct snd_soc_dai *cpu_dai;
>   	struct snd_soc_component *component;
>   	struct snd_soc_rtdcom_list *rtdcom;
>   	struct snd_pcm *pcm;
>   	char new_name[64];
>   	int ret = 0, playback = 0, capture = 0;
> -	int i;
> +	int i, cpu_capture = 0, cpu_playback = 0;
>   
>   	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
>   		playback = rtd->dai_link->dpcm_playback;
> @@ -2904,8 +3069,16 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   				capture = 1;
>   		}
>   
> -		capture = capture && cpu_dai->driver->capture.channels_min;
> -		playback = playback && cpu_dai->driver->playback.channels_min;
> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
> +			cpu_dai = rtd->cpu_dais[i];
> +			if (cpu_dai->driver->playback.channels_min)
> +				cpu_playback = 1;
> +			if (cpu_dai->driver->capture.channels_min)
> +				cpu_capture = 1;
> +		}
> +
> +		playback = playback && cpu_playback;
> +		capture = capture && cpu_capture;
>   	}
>   
>   	if (rtd->dai_link->playback_only) {
> @@ -3026,7 +3199,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   out:
>   	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
>   		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
> -		 cpu_dai->name);
> +		 (rtd->num_cpu_dai > 1) ? "multicpu" : rtd->cpu_dai->name);
>   	return ret;
>   }
>   

Took me a couple of hours to reach the end of this patch ... I had to 
use a visual diff to figure it out, the diff format is just too hard to 
look at.

Re: [PATCH v6 3/3] ASoC: Add multiple CPU DAI support in DAPM

[Pierre-Louis Bossart]

On 06/20/2018 05:54 AM, Shreyas NC wrote:
> DAPM handles DAIs during soc_dapm_stream_event() and during addition
> and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and
> dapm_connect_dai_link_widgets().
can you split this patch in two, one where you add 
dapm_add_valid_dai_widget() and the second one where you add the 
multi-cpu stuff? the current diff format is really hard to read with the 
two changes lumped together.

> +	for (i = 0; i < rtd->num_codecs; i++) {
> +		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
> +
> +		for (j = 0; j < rtd->num_cpu_dai; j++) {
> +			cpu_dai = rtd->cpu_dais[j];
> +
> +			dapm_add_valid_dai_widget(card, rtd,
> +						codec_dai, cpu_dai);

I didn't click on this earlier, but what makes you think all codec_dais 
are connected to all cpu_dais?
That doesn't seem quite right.
For the multi-codec case, all the codec_dais hang from a single cpu_dai. 
it's a stretch for me to have a full M:N connectivity. And that's 
clearly not the case for SoundWire stream in the multi-link case.
Can't we use the dai_link information here to only connect cpu_ and 
codec_dais that are related?

>   		}
>   	}
>   }
> @@ -4211,7 +4230,9 @@ static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
>   {
>   	int i;
>   
> -	soc_dapm_dai_stream_event(rtd->cpu_dai, stream, event);
> +	for (i = 0; i < rtd->num_cpu_dai; i++)
> +		soc_dapm_dai_stream_event(rtd->cpu_dais[i], stream, event);
> +
>   	for (i = 0; i < rtd->num_codecs; i++)
>   		soc_dapm_dai_stream_event(rtd->codec_dais[i], stream, event);
>   

Re: [PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Shreyas NC]

On Thu, Jun 21, 2018 at 07:35:57PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> >  	/* close any waiting streams */
> >@@ -552,16 +565,21 @@ int snd_soc_suspend(struct device *dev)
> >  	}
> >  	list_for_each_entry(rtd, &card->rtd_list, list) {
> >-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >+		struct snd_soc_dai *cpu_dai;
> >  		if (rtd->dai_link->ignore_suspend)
> >  			continue;
> >-		if (cpu_dai->driver->suspend && cpu_dai->driver->bus_control)
> >-			cpu_dai->driver->suspend(cpu_dai);
> >+		for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+			cpu_dai = rtd->cpu_dais[i];
> >-		/* deactivate pins to sleep state */
> >-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
> >+			if (cpu_dai->driver->suspend &&
> >+					cpu_dai->driver->bus_control)
> >+				cpu_dai->driver->suspend(cpu_dai);
> >+
> >+			/* deactivate pins to sleep state */
> >+			pinctrl_pm_select_sleep_state(cpu_dai->dev);
> >+		}
> I am not sure I get the relationship between cpu_dai and pins. Is this
> always safe/ok to play with the pincrtl before all cpu_dais are suspended?
> Or should you implement this with two loops, one to suspend and one to
> deactivate pins?

Hmmm, you have a valid point. Two loops is a right approach IMO..

> >@@ -845,35 +882,47 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
> >  {
> >  	struct snd_soc_pcm_runtime *rtd;
> >  	struct snd_soc_dai_link_component *codecs = dai_link->codecs;
> >-	struct snd_soc_dai_link_component cpu_dai_component;
> >+	struct snd_soc_dai_link_component *cpu_dai_component;
> >  	struct snd_soc_component *component;
> >-	struct snd_soc_dai **codec_dais;
> >+	struct snd_soc_dai **codec_dais, **cpu_dais;
> >  	struct device_node *platform_of_node;
> >  	const char *platform_name;
> >  	int i;
> >  	dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name);
> >+	cpu_dai_component = dai_link->cpu_dai;
> >+
> >  	if (soc_is_dai_link_bound(card, dai_link)) {
> >  		dev_dbg(card->dev, "ASoC: dai link %s already bound\n",
> >  			dai_link->name);
> >  		return 0;
> >  	}
> >+	if (dai_link->dynamic && dai_link->num_cpu_dai > 1) {
> >+		dev_err(card->dev, "ASoC: Multi CPU DAI not supported for FE");
> >+		return -EINVAL;
> >+	}
> >+
> >  	rtd = soc_new_pcm_runtime(card, dai_link);
> >  	if (!rtd)
> >  		return -ENOMEM;
> >-	cpu_dai_component.name = dai_link->cpu_name;
> >-	cpu_dai_component.of_node = dai_link->cpu_of_node;
> >-	cpu_dai_component.dai_name = dai_link->cpu_dai_name;
> 
> Have you checked if there are any side effects of using
> cpu_dai_component = dai_link->cpu_dai;
> instead of a local variable?

Yes, I have checked this. Also, snd_soc_init_single_cpu_dai() ensures that
when there is a single DAI these are populated and in case of multi cpu DAIs
the DAI Link specifies these ..

> 
> if you are not sure, it may be worth implementing as a separate patch first
> before introducing the multi-cpu part, at least to allow for git bisect to
> identify issues?

I have found it very hard to split these patches as the changes are
monolithic. Infact, my worry is the patch split itself might break git
bisect :(

> >+static int snd_soc_init_single_cpu_dai(struct snd_soc_card *card,
> >+				   struct snd_soc_dai_link *dai_link)
> >+{
> >+	if (dai_link->cpu_name || dai_link->cpu_of_node ||
> >+					dai_link->cpu_dai_name) {
> >+		dai_link->num_cpu_dai = 1;
> >+		dai_link->cpu_dai = devm_kzalloc(card->dev,
> >+				sizeof(struct snd_soc_dai_link_component),
> >+				GFP_KERNEL);
> >+
> >+		if (!dai_link->cpu_dai)
> >+			return -ENOMEM;
> >+
> >+		dai_link->cpu_dai[0].name = dai_link->cpu_name;
> >+		dai_link->cpu_dai[0].of_node = dai_link->cpu_of_node;
> >+		dai_link->cpu_dai[0].dai_name = dai_link->cpu_dai_name;
> Question: is cpu_dai[i].of_node defined for i>0 in the multi cpu_dai case?
> 

Yes, it should be defined.

> >@@ -1644,7 +1751,7 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
> >  	unsigned int dai_fmt)
> >  {
> >  	struct snd_soc_dai **codec_dais = rtd->codec_dais;
> >-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >+	struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
> >  	unsigned int i;
> >  	int ret;
> >@@ -1659,35 +1766,44 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
> >  		}
> >  	}
> >-	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
> why was this comment removed?
> 

Looks like I messed up resolving ocnflicts while rebase

> >  	/* the component which has non_legacy_dai_naming is Codec */
> >-	if (cpu_dai->component->driver->non_legacy_dai_naming) {
> Not sure if the code refactoring below makes sense in a codec-codec link,
> you probably wouldn't have multiple cpu_dais then, would you?

Yes, a valid point. You suggest to leave this piece of code as is ?
 
> >-		unsigned int inv_dai_fmt;
> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		struct snd_soc_dai *cpu_dai = cpu_dais[i];
> >+		unsigned int inv_dai_fmt, temp_dai_fmt;
> >-		inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
> >-		switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >-		case SND_SOC_DAIFMT_CBM_CFM:
> >-			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
> >-			break;
> >-		case SND_SOC_DAIFMT_CBM_CFS:
> >-			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
> >-			break;
> >-		case SND_SOC_DAIFMT_CBS_CFM:
> >-			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
> >-			break;
> >-		case SND_SOC_DAIFMT_CBS_CFS:
> >-			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> >-			break;
> >-		}
> >+		temp_dai_fmt = dai_fmt;
> >+		if (cpu_dai->component->driver->non_legacy_dai_naming) {
> >-		dai_fmt = inv_dai_fmt;
> >-	}
> >+			inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
> >-	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
> >-	if (ret != 0 && ret != -ENOTSUPP) {
> >-		dev_warn(cpu_dai->dev,
> >-			 "ASoC: Failed to set DAI format: %d\n", ret);
> >-		return ret;
> >+			switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >+			case SND_SOC_DAIFMT_CBM_CFM:
> >+				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
> >+				break;
> >+
> >+			case SND_SOC_DAIFMT_CBM_CFS:
> >+				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
> >+				break;
> >+
> >+			case SND_SOC_DAIFMT_CBS_CFM:
> >+				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
> >+				break;
> >+
> >+			case SND_SOC_DAIFMT_CBS_CFS:
> >+				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> >+				break;
> >+
> >+			}
> >+
> >+			temp_dai_fmt = inv_dai_fmt;
> >+		}
> >+
> >+		ret = snd_soc_dai_set_fmt(cpu_dai, temp_dai_fmt);
> >+		if (ret != 0 && ret != -ENOTSUPP) {
> >+			dev_warn(cpu_dai->dev,
> >+				"ASoC: Failed to set DAI format: %d\n", ret);
> >+			return ret;
> >+		}
> >  	}
> >  	
> 

-- 

Re: [PATCH v6 2/3] ASoC: Add multiple CPU DAI support for PCM ops

[Shreyas NC]

On Thu, Jun 21, 2018 at 09:43:50PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 06/20/2018 05:54 AM, Shreyas NC wrote:
> >Add support in PCM operations to invoke multiple cpu dais as we do
> >for multiple codec dais. Also the symmetry calculations are updated to
> >reflect multiple cpu dais.
> This doesn't apply on Mark's tree?
> Looks like you need to rebase on top of 244e293690a6 ("ASoC: pcm: Tidy up
> open/hw_params handling")
> contributed by Charles on June 19.

I did rebase the morning before I sent these patches, may be I should do it
just before sending them out. Will take care of that next time :)

> >
> >Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> >Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> >---
> >  sound/soc/soc-pcm.c | 491 +++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 332 insertions(+), 159 deletions(-)
> >
> >diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> >index 5e7ae47..cdcbc4f 100644
> >--- a/sound/soc/soc-pcm.c
> >+++ b/sound/soc/soc-pcm.c
> >@@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
> >   */
> >  void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> >  {
> >-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >  	int i;
> >  	lockdep_assert_held(&rtd->pcm_mutex);
> >  	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >-		cpu_dai->playback_active++;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >+			rtd->cpu_dais[i]->playback_active++;
> >  		for (i = 0; i < rtd->num_codecs; i++)
> >  			rtd->codec_dais[i]->playback_active++;
> >  	} else {
> >-		cpu_dai->capture_active++;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >+			rtd->cpu_dais[i]->capture_active++;
> >  		for (i = 0; i < rtd->num_codecs; i++)
> >  			rtd->codec_dais[i]->capture_active++;
> >  	}
> >-	cpu_dai->active++;
> >-	cpu_dai->component->active++;
> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		rtd->cpu_dais[i]->component->active++;
> >+		rtd->cpu_dais[i]->active++;
> >+	}
> 
> This is becoming complicated, how many times do we need to ref-count?

Earlier we incremented cpu_dai->playback_active++ and here it is
cpu_dais->component->active++

> >+
> >  	for (i = 0; i < rtd->num_codecs; i++) {
> >  		rtd->codec_dais[i]->active++;
> >  		rtd->codec_dais[i]->component->active++;
> >@@ -99,23 +103,27 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> >   */
> >  void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
> >  {
> >-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >  	int i;
> >  	lockdep_assert_held(&rtd->pcm_mutex);
> >  	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >-		cpu_dai->playback_active--;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >+			rtd->cpu_dais[i]->playback_active--;
> >  		for (i = 0; i < rtd->num_codecs; i++)
> >  			rtd->codec_dais[i]->playback_active--;
> >  	} else {
> >-		cpu_dai->capture_active--;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >+			rtd->cpu_dais[i]->capture_active--;
> >  		for (i = 0; i < rtd->num_codecs; i++)
> >  			rtd->codec_dais[i]->capture_active--;
> >  	}
> >-	cpu_dai->active--;
> >-	cpu_dai->component->active--;
> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		rtd->cpu_dais[i]->component->active--;
> >+		rtd->cpu_dais[i]->active--;
> >+	}
> >+
> >  	for (i = 0; i < rtd->num_codecs; i++) {
> >  		rtd->codec_dais[i]->component->active--;
> >  		rtd->codec_dais[i]->active--;
> >@@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >  				struct snd_pcm_hw_params *params)
> >  {
> >  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >  	unsigned int rate, channels, sample_bits, symmetry, i;
> >  	rate = params_rate(params);
> >@@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >  	sample_bits = snd_pcm_format_physical_width(params_format(params));
> >  	/* reject unmatched parameters when applying symmetry */
> >-	symmetry = cpu_dai->driver->symmetric_rates ||
> >-		rtd->dai_link->symmetric_rates;
> This was a logical OR, but...
> 
> >+	symmetry = rtd->dai_link->symmetric_rates;
> >+
> >+	for (i = 0; i < rtd->num_cpu_dai; i++)
> >+		symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
> this is a bitwise OR. is this ok?

I made this change on purpose and I am struggling to remember the reason :(
If I dont figure that out, I will go back to logical OR..

> >  	for (i = 0; i < rtd->num_codecs; i++)
> >  		symmetry |= rtd->codec_dais[i]->driver->symmetric_rates;
> >-	if (symmetry && cpu_dai->rate && cpu_dai->rate != rate) {
> >-		dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
> >-				cpu_dai->rate, rate);
> >-		return -EINVAL;
> >-	}
> >+	for (i = 0; i < rtd->num_cpu_dai; i++)
> >+		if (symmetry && rtd->cpu_dais[i]->rate &&
> you could move the if (symmetry) out of the for loop to simplify

Yes, that makes sense :)

> >@@ -308,13 +328,18 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >  static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
> >  {
> >  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >-	struct snd_soc_dai_driver *cpu_driver = rtd->cpu_dai->driver;
> >  	struct snd_soc_dai_link *link = rtd->dai_link;
> >  	unsigned int symmetry, i;
> >-	symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
> >-		cpu_driver->symmetric_channels || link->symmetric_channels ||
> >-		cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
> >+	symmetry = link->symmetric_rates || link->symmetric_channels ||
> >+			link->symmetric_samplebits;
> >+
> >+	/* Apply symmetery for multiple cpu dais */
> You haven't fixed this comment, is this patch the correct one?

I am surprised why this crept in :(
This was the first comment I fixed ..

> >@@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
> >  		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
> >  	}
> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		cpu_dai_drv = rtd->cpu_dais[i]->driver;
> >+
> >+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >+			cpu_stream = &cpu_dai_drv->playback;
> >+		else
> >+			cpu_stream = &cpu_dai_drv->capture;
> >+
> >+		cpu_chan_min = max(cpu_chan_min,
> >+					cpu_stream->channels_min);
> >+		cpu_chan_max = min(cpu_chan_max,
> >+					cpu_stream->channels_max);
> >+
> >+		if (hw->formats)
> >+			hw->formats &= cpu_stream->formats;
> >+		else
> >+			hw->formats = cpu_stream->formats;
> Can you double-check the 'else' case? This doesn't seem right, you will
> always use the format used for the last cpu_dai. If the formats are
> identical for all cpu_dais, then this can be moved outside of the loop.

In the second iteration, we would always go into the if (hw->formats) case.

> >@@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> >  	if (ret < 0)
> >  		goto component_err;
> >-	/* store the parameters for each DAIs */
> >-	cpu_dai->rate = params_rate(params);
> >-	cpu_dai->channels = params_channels(params);
> >-	cpu_dai->sample_bits =
> >-		snd_pcm_format_physical_width(params_format(params));
> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		/* store the parameters for each DAIs */
> >+		cpu_dai = rtd->cpu_dais[i];
> >+		cpu_dai->rate = params_rate(params);
> >+		cpu_dai->channels = params_channels(params);
> >+		cpu_dai->sample_bits =
> >+			snd_pcm_format_physical_width(params_format(params));
> >+	}
> I think I've asked this before but can't recall the answer: does this mean
> we assume the same number of channels for each codec_dai[j] and cpu_dai[i]?
> 

Yes, in hw_params we set the same number channels on all the DAIs as that of the
stream. But, as I had mentioned in my previous replies, the
machine driver would set the channel masks on the individual DAIs(like we do
for SoundWire Multi link case)

> >+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+		cpu_dai = rtd->cpu_dais[i];
> >+		if (cpu_dai->driver->ops->trigger) {
> >+			ret = cpu_dai->driver->ops->trigger(substream,
> >+								cmd, cpu_dai);
> >+			if (ret < 0)
> >+				return ret;
> >+		}
> 
> Maybe add a comment that in the multi_cpu case the triggers are supposed to
> be aligned, i.e. the first trigger starts the others - that would be
> consistent with the other comments on delay below.

Would this be the right place to add that comment?
I am not sure if I got a reply to my question in the previous review cycle:
"Just wondering if there can be a case of multiple CPU DAIs but you would
not want them to be triggered at the same time."

Based on the answer to my question , we can add a comment here.

> >@@ -1778,11 +1941,13 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
> >  			be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
> >  		/* Symmetry only applies if we've got an active stream. */
> >-		if (rtd->cpu_dai->active) {
> >-			err = soc_pcm_apply_symmetry(fe_substream,
> >-						     rtd->cpu_dai);
> >-			if (err < 0)
> >-				return err;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+			if (rtd->cpu_dais[i]->active) {
> >+				err = soc_pcm_apply_symmetry(be_substream,
> >+							rtd->cpu_dais[i]);
> >+				if (err < 0)
> >+					return err;
> >+			}
> >  		}
> Can you recheck this block? In the original code the symmetry is with the
> fe_substream, it's now with a be_substream. Looks to me like a major typo
> having significant impact of the result...
> 

This was recently fixed and changed from be_substream to fe_substream :(
commit 99bcedbdebc57fe5d02fb470b7265f2208c2cf96

    ASoC: dpcm: symmetry constraint on FE substream

So, I need to fix my patch as well. Nice catch :)

> >  		for (i = 0; i < rtd->num_codecs; i++) {
> >@@ -2884,13 +3049,13 @@ static int soc_rtdcom_mmap(struct snd_pcm_substream *substream,
> >  int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >  {
> >  	struct snd_soc_dai *codec_dai;
> >-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >+	struct snd_soc_dai *cpu_dai;
> >  	struct snd_soc_component *component;
> >  	struct snd_soc_rtdcom_list *rtdcom;
> >  	struct snd_pcm *pcm;
> >  	char new_name[64];
> >  	int ret = 0, playback = 0, capture = 0;
> >-	int i;
> >+	int i, cpu_capture = 0, cpu_playback = 0;
> >  	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> >  		playback = rtd->dai_link->dpcm_playback;
> >@@ -2904,8 +3069,16 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >  				capture = 1;
> >  		}
> >-		capture = capture && cpu_dai->driver->capture.channels_min;
> >-		playback = playback && cpu_dai->driver->playback.channels_min;
> >+		for (i = 0; i < rtd->num_cpu_dai; i++) {
> >+			cpu_dai = rtd->cpu_dais[i];
> >+			if (cpu_dai->driver->playback.channels_min)
> >+				cpu_playback = 1;
> >+			if (cpu_dai->driver->capture.channels_min)
> >+				cpu_capture = 1;
> >+		}
> >+
> >+		playback = playback && cpu_playback;
> >+		capture = capture && cpu_capture;
> >  	}
> >  	if (rtd->dai_link->playback_only) {
> >@@ -3026,7 +3199,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >  out:
> >  	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
> >  		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
> >-		 cpu_dai->name);
> >+		 (rtd->num_cpu_dai > 1) ? "multicpu" : rtd->cpu_dai->name);
> >  	return ret;
> >  }
> 
> Took me a couple of hours to reach the end of this patch ... I had to use a
> visual diff to figure it out, the diff format is just too hard to look at.
> 

Unfortunately yes :(

-- 

Re: [PATCH v6 3/3] ASoC: Add multiple CPU DAI support in DAPM

[Shreyas NC]

On Thu, Jun 21, 2018 at 09:55:54PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 06/20/2018 05:54 AM, Shreyas NC wrote:
> >DAPM handles DAIs during soc_dapm_stream_event() and during addition
> >and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and
> >dapm_connect_dai_link_widgets().
> can you split this patch in two, one where you add
> dapm_add_valid_dai_widget() and the second one where you add the multi-cpu
> stuff? the current diff format is really hard to read with the two changes
> lumped together.

As I had replied earlier, the change is really moving the same code from one
function to a new one. So, a patch split would mean we would have the same
duplicated code in one patch which surely is not desirable.

I just realized that in my earlier reply I had excluded the list and replied
only to you :)

> 
> >+	for (i = 0; i < rtd->num_codecs; i++) {
> >+		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
> >+
> >+		for (j = 0; j < rtd->num_cpu_dai; j++) {
> >+			cpu_dai = rtd->cpu_dais[j];
> >+
> >+			dapm_add_valid_dai_widget(card, rtd,
> >+						codec_dai, cpu_dai);
> 
> I didn't click on this earlier, but what makes you think all codec_dais are
> connected to all cpu_dais?

Yes, there need not be a M:N connectivity. But, how do you find that out ?

> That doesn't seem quite right.
> For the multi-codec case, all the codec_dais hang from a single cpu_dai.
> it's a stretch for me to have a full M:N connectivity. And that's clearly
> not the case for SoundWire stream in the multi-link case.

I mostly do not disagree with you here..

> Can't we use the dai_link information here to only connect cpu_ and
> codec_dais that are related?

Which DAI Link information are you referring to here ?
Other than the machine driver which sets the audio route, I am unable to
figure out how we will find out the connected cpu_dai and codec_dais at
ASoC core level.

May be I am missing something :(

--Shreyas

-- 

Re: [PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Pierre-Louis Bossart]

>>> +static int snd_soc_init_single_cpu_dai(struct snd_soc_card *card,
>>> +				   struct snd_soc_dai_link *dai_link)
>>> +{
>>> +	if (dai_link->cpu_name || dai_link->cpu_of_node ||
>>> +					dai_link->cpu_dai_name) {
>>> +		dai_link->num_cpu_dai = 1;
>>> +		dai_link->cpu_dai = devm_kzalloc(card->dev,
>>> +				sizeof(struct snd_soc_dai_link_component),
>>> +				GFP_KERNEL);
>>> +
>>> +		if (!dai_link->cpu_dai)
>>> +			return -ENOMEM;
>>> +
>>> +		dai_link->cpu_dai[0].name = dai_link->cpu_name;
>>> +		dai_link->cpu_dai[0].of_node = dai_link->cpu_of_node;
>>> +		dai_link->cpu_dai[0].dai_name = dai_link->cpu_dai_name;
>> Question: is cpu_dai[i].of_node defined for i>0 in the multi cpu_dai case?
>>
> 
> Yes, it should be defined.

I have limited understanding of how cpu_of_node would be handled and if 
there is any guidance for DT folks on how to deal with multiple cpu_dais.

> 
>>> @@ -1644,7 +1751,7 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>>   	unsigned int dai_fmt)
>>>   {
>>>   	struct snd_soc_dai **codec_dais = rtd->codec_dais;
>>> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>> +	struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
>>>   	unsigned int i;
>>>   	int ret;
>>> @@ -1659,35 +1766,44 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>>   		}
>>>   	}
>>> -	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
>> why was this comment removed?
>>
> 
> Looks like I messed up resolving ocnflicts while rebase
> 
>>>   	/* the component which has non_legacy_dai_naming is Codec */
>>> -	if (cpu_dai->component->driver->non_legacy_dai_naming) {
>> Not sure if the code refactoring below makes sense in a codec-codec link,
>> you probably wouldn't have multiple cpu_dais then, would you?
> 
> Yes, a valid point. You suggest to leave this piece of code as is ?

Not necessarily. I don't understand how the codec-codec and multi 
cpu_dais intersect, all I am asking for is a check if this change is 
needed or not.

>   
>>> -		unsigned int inv_dai_fmt;
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +		struct snd_soc_dai *cpu_dai = cpu_dais[i];
>>> +		unsigned int inv_dai_fmt, temp_dai_fmt;
>>> -		inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
>>> -		switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>>> -		case SND_SOC_DAIFMT_CBM_CFM:
>>> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
>>> -			break;
>>> -		case SND_SOC_DAIFMT_CBM_CFS:
>>> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
>>> -			break;
>>> -		case SND_SOC_DAIFMT_CBS_CFM:
>>> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
>>> -			break;
>>> -		case SND_SOC_DAIFMT_CBS_CFS:
>>> -			inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
>>> -			break;
>>> -		}
>>> +		temp_dai_fmt = dai_fmt;
>>> +		if (cpu_dai->component->driver->non_legacy_dai_naming) {
>>> -		dai_fmt = inv_dai_fmt;
>>> -	}
>>> +			inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK;
>>> -	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
>>> -	if (ret != 0 && ret != -ENOTSUPP) {
>>> -		dev_warn(cpu_dai->dev,
>>> -			 "ASoC: Failed to set DAI format: %d\n", ret);
>>> -		return ret;
>>> +			switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>>> +			case SND_SOC_DAIFMT_CBM_CFM:
>>> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
>>> +				break;
>>> +
>>> +			case SND_SOC_DAIFMT_CBM_CFS:
>>> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
>>> +				break;
>>> +
>>> +			case SND_SOC_DAIFMT_CBS_CFM:
>>> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
>>> +				break;
>>> +
>>> +			case SND_SOC_DAIFMT_CBS_CFS:
>>> +				inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
>>> +				break;
>>> +
>>> +			}
>>> +
>>> +			temp_dai_fmt = inv_dai_fmt;
>>> +		}
>>> +
>>> +		ret = snd_soc_dai_set_fmt(cpu_dai, temp_dai_fmt);
>>> +		if (ret != 0 && ret != -ENOTSUPP) {
>>> +			dev_warn(cpu_dai->dev,
>>> +				"ASoC: Failed to set DAI format: %d\n", ret);
>>> +			return ret;
>>> +		}
>>>   	}
>>>   	
>>
> 

Re: [PATCH v6 2/3] ASoC: Add multiple CPU DAI support for PCM ops

[Pierre-Louis Bossart]

>>> @@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
>>>    */
>>>   void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
>>>   {
>>> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>>   	int i;
>>>   	lockdep_assert_held(&rtd->pcm_mutex);
>>>   	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>> -		cpu_dai->playback_active++;
>>> +		for (i = 0; i < rtd->num_cpu_dai; i++)
>>> +			rtd->cpu_dais[i]->playback_active++;
>>>   		for (i = 0; i < rtd->num_codecs; i++)
>>>   			rtd->codec_dais[i]->playback_active++;
>>>   	} else {
>>> -		cpu_dai->capture_active++;
>>> +		for (i = 0; i < rtd->num_cpu_dai; i++)
>>> +			rtd->cpu_dais[i]->capture_active++;
>>>   		for (i = 0; i < rtd->num_codecs; i++)
>>>   			rtd->codec_dais[i]->capture_active++;
>>>   	}
>>> -	cpu_dai->active++;
>>> -	cpu_dai->component->active++;
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +		rtd->cpu_dais[i]->component->active++;
>>> +		rtd->cpu_dais[i]->active++;
>>> +	}
>>
>> This is becoming complicated, how many times do we need to ref-count?
> 
> Earlier we incremented cpu_dai->playback_active++ and here it is
> cpu_dais->component->active++

my point was whether this can be simplified to use a single counter, 
even before we do the change to multi-cpu?

>>> @@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
>>>   				struct snd_pcm_hw_params *params)
>>>   {
>>>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>>   	unsigned int rate, channels, sample_bits, symmetry, i;
>>>   	rate = params_rate(params);
>>> @@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
>>>   	sample_bits = snd_pcm_format_physical_width(params_format(params));
>>>   	/* reject unmatched parameters when applying symmetry */
>>> -	symmetry = cpu_dai->driver->symmetric_rates ||
>>> -		rtd->dai_link->symmetric_rates;
>> This was a logical OR, but...
>>
>>> +	symmetry = rtd->dai_link->symmetric_rates;
>>> +
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++)
>>> +		symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
>> this is a bitwise OR. is this ok?
> 
> I made this change on purpose and I am struggling to remember the reason :(
> If I dont figure that out, I will go back to logical OR..

the same change was made for multiple codecs, but it's worth rechecking.

>>> @@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
>>>   		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
>>>   	}
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +		cpu_dai_drv = rtd->cpu_dais[i]->driver;
>>> +
>>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>> +			cpu_stream = &cpu_dai_drv->playback;
>>> +		else
>>> +			cpu_stream = &cpu_dai_drv->capture;
>>> +
>>> +		cpu_chan_min = max(cpu_chan_min,
>>> +					cpu_stream->channels_min);
>>> +		cpu_chan_max = min(cpu_chan_max,
>>> +					cpu_stream->channels_max);
>>> +
>>> +		if (hw->formats)
>>> +			hw->formats &= cpu_stream->formats;
>>> +		else
>>> +			hw->formats = cpu_stream->formats;
>> Can you double-check the 'else' case? This doesn't seem right, you will
>> always use the format used for the last cpu_dai. If the formats are
>> identical for all cpu_dais, then this can be moved outside of the loop.
> 
> In the second iteration, we would always go into the if (hw->formats) case.

Sorry I don't get your comment. This test is now placed inside of a for 
loop, so why would the hw->formats change between iterations?

> 
>>> @@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>>>   	if (ret < 0)
>>>   		goto component_err;
>>> -	/* store the parameters for each DAIs */
>>> -	cpu_dai->rate = params_rate(params);
>>> -	cpu_dai->channels = params_channels(params);
>>> -	cpu_dai->sample_bits =
>>> -		snd_pcm_format_physical_width(params_format(params));
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +		/* store the parameters for each DAIs */
>>> +		cpu_dai = rtd->cpu_dais[i];
>>> +		cpu_dai->rate = params_rate(params);
>>> +		cpu_dai->channels = params_channels(params);
>>> +		cpu_dai->sample_bits =
>>> +			snd_pcm_format_physical_width(params_format(params));
>>> +	}
>> I think I've asked this before but can't recall the answer: does this mean
>> we assume the same number of channels for each codec_dai[j] and cpu_dai[i]?
>>
> 
> Yes, in hw_params we set the same number channels on all the DAIs as that of the
> stream. But, as I had mentioned in my previous replies, the
> machine driver would set the channel masks on the individual DAIs(like we do
> for SoundWire Multi link case)

so the number of channels is really defined by the stream, and for each 
cpu_dai and codec_dai there is a channel mask defining what the dais 
should produce/consume, right?

> 
>>> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +		cpu_dai = rtd->cpu_dais[i];
>>> +		if (cpu_dai->driver->ops->trigger) {
>>> +			ret = cpu_dai->driver->ops->trigger(substream,
>>> +								cmd, cpu_dai);
>>> +			if (ret < 0)
>>> +				return ret;
>>> +		}
>>
>> Maybe add a comment that in the multi_cpu case the triggers are supposed to
>> be aligned, i.e. the first trigger starts the others - that would be
>> consistent with the other comments on delay below.
> 
> Would this be the right place to add that comment?
> I am not sure if I got a reply to my question in the previous review cycle:
> "Just wondering if there can be a case of multiple CPU DAIs but you would
> not want them to be triggered at the same time."
> 
> Based on the answer to my question , we can add a comment here.

If you use the stream/multi cpu_dai concept you want the data to remain 
phase locked and triggered at the same time. If you don't care about 
phase, then you are handling different streams that can be handled with 
the existing model with a set of runtimes that deal with individual 
cpu_dais.

> 
>>> @@ -1778,11 +1941,13 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
>>>   			be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
>>>   		/* Symmetry only applies if we've got an active stream. */
>>> -		if (rtd->cpu_dai->active) {
>>> -			err = soc_pcm_apply_symmetry(fe_substream,
>>> -						     rtd->cpu_dai);
>>> -			if (err < 0)
>>> -				return err;
>>> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +			if (rtd->cpu_dais[i]->active) {
>>> +				err = soc_pcm_apply_symmetry(be_substream,
>>> +							rtd->cpu_dais[i]);
>>> +				if (err < 0)
>>> +					return err;
>>> +			}
>>>   		}
>> Can you recheck this block? In the original code the symmetry is with the
>> fe_substream, it's now with a be_substream. Looks to me like a major typo
>> having significant impact of the result...
>>
> 
> This was recently fixed and changed from be_substream to fe_substream :(
> commit 99bcedbdebc57fe5d02fb470b7265f2208c2cf96
> 
>      ASoC: dpcm: symmetry constraint on FE substream
> 
> So, I need to fix my patch as well. Nice catch :)

I see. We want to double-check all the fixes since this work started 
(and yes unfortunately playing with patches lets this sort of things go 
through, I had the same issue yesterday with my SOF renaming patch).

> 
>>>   		for (i = 0; i < rtd->num_codecs; i++) {
>>> @@ -2884,13 +3049,13 @@ static int soc_rtdcom_mmap(struct snd_pcm_substream *substream,
>>>   int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>>>   {
>>>   	struct snd_soc_dai *codec_dai;
>>> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>> +	struct snd_soc_dai *cpu_dai;
>>>   	struct snd_soc_component *component;
>>>   	struct snd_soc_rtdcom_list *rtdcom;
>>>   	struct snd_pcm *pcm;
>>>   	char new_name[64];
>>>   	int ret = 0, playback = 0, capture = 0;
>>> -	int i;
>>> +	int i, cpu_capture = 0, cpu_playback = 0;
>>>   	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
>>>   		playback = rtd->dai_link->dpcm_playback;
>>> @@ -2904,8 +3069,16 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>>>   				capture = 1;
>>>   		}
>>> -		capture = capture && cpu_dai->driver->capture.channels_min;
>>> -		playback = playback && cpu_dai->driver->playback.channels_min;
>>> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +			cpu_dai = rtd->cpu_dais[i];
>>> +			if (cpu_dai->driver->playback.channels_min)
>>> +				cpu_playback = 1;
>>> +			if (cpu_dai->driver->capture.channels_min)
>>> +				cpu_capture = 1;
>>> +		}
>>> +
>>> +		playback = playback && cpu_playback;
>>> +		capture = capture && cpu_capture;
>>>   	}
>>>   	if (rtd->dai_link->playback_only) {
>>> @@ -3026,7 +3199,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>>>   out:
>>>   	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
>>>   		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
>>> -		 cpu_dai->name);
>>> +		 (rtd->num_cpu_dai > 1) ? "multicpu" : rtd->cpu_dai->name);
>>>   	return ret;
>>>   }
>>
>> Took me a couple of hours to reach the end of this patch ... I had to use a
>> visual diff to figure it out, the diff format is just too hard to look at.
>>
> 
> Unfortunately yes :(
> 

Re: [PATCH v6 3/3] ASoC: Add multiple CPU DAI support in DAPM

[Pierre-Louis Bossart]

On 6/22/18 12:53 AM, Shreyas NC wrote:
> On Thu, Jun 21, 2018 at 09:55:54PM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> On 06/20/2018 05:54 AM, Shreyas NC wrote:
>>> DAPM handles DAIs during soc_dapm_stream_event() and during addition
>>> and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and
>>> dapm_connect_dai_link_widgets().
>> can you split this patch in two, one where you add
>> dapm_add_valid_dai_widget() and the second one where you add the multi-cpu
>> stuff? the current diff format is really hard to read with the two changes
>> lumped together.
> 
> As I had replied earlier, the change is really moving the same code from one
> function to a new one. So, a patch split would mean we would have the same
> duplicated code in one patch which surely is not desirable.

I don't understand your answer. It's fine to have a small preparation 
patch that just moves one piece of code to another function, and they 
change how that function is called.

> I just realized that in my earlier reply I had excluded the list and replied
> only to you :)
> 
>>
>>> +	for (i = 0; i < rtd->num_codecs; i++) {
>>> +		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
>>> +
>>> +		for (j = 0; j < rtd->num_cpu_dai; j++) {
>>> +			cpu_dai = rtd->cpu_dais[j];
>>> +
>>> +			dapm_add_valid_dai_widget(card, rtd,
>>> +						codec_dai, cpu_dai);
>>
>> I didn't click on this earlier, but what makes you think all codec_dais are
>> connected to all cpu_dais?
> 
> Yes, there need not be a M:N connectivity. But, how do you find that out ?
> 
>> That doesn't seem quite right.
>> For the multi-codec case, all the codec_dais hang from a single cpu_dai.
>> it's a stretch for me to have a full M:N connectivity. And that's clearly
>> not the case for SoundWire stream in the multi-link case.
> 
> I mostly do not disagree with you here..
> 
>> Can't we use the dai_link information here to only connect cpu_ and
>> codec_dais that are related?
> 
> Which DAI Link information are you referring to here ?
> Other than the machine driver which sets the audio route, I am unable to
> figure out how we will find out the connected cpu_dai and codec_dais at
> ASoC core level.
> 
> May be I am missing something :(

How is it different from the multi-codec support? You must have a 
description somewhere that tells you how the cpu_dai is connected to 
various codec_dais.

Maybe we should start with the examples you provided for Soundwire and 
describe how the dailinks would be represented.

With the M:N connectivity you'd end-up having spurious events with 
non-existent connections, it's not necessarily fatal but certainly not 
elegant and may or may not work depending on state management in codec 
drivers.

Re: [PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Shreyas NC]

On Fri, Jun 22, 2018 at 10:13:28AM -0500, Pierre-Louis Bossart wrote:
> 
> >>>+static int snd_soc_init_single_cpu_dai(struct snd_soc_card *card,
> >>>+				   struct snd_soc_dai_link *dai_link)
> >>>+{
> >>>+	if (dai_link->cpu_name || dai_link->cpu_of_node ||
> >>>+					dai_link->cpu_dai_name) {
> >>>+		dai_link->num_cpu_dai = 1;
> >>>+		dai_link->cpu_dai = devm_kzalloc(card->dev,
> >>>+				sizeof(struct snd_soc_dai_link_component),
> >>>+				GFP_KERNEL);
> >>>+
> >>>+		if (!dai_link->cpu_dai)
> >>>+			return -ENOMEM;
> >>>+
> >>>+		dai_link->cpu_dai[0].name = dai_link->cpu_name;
> >>>+		dai_link->cpu_dai[0].of_node = dai_link->cpu_of_node;
> >>>+		dai_link->cpu_dai[0].dai_name = dai_link->cpu_dai_name;
> >>Question: is cpu_dai[i].of_node defined for i>0 in the multi cpu_dai case?
> >>
> >
> >Yes, it should be defined.
> 
> I have limited understanding of how cpu_of_node would be handled and if
> there is any guidance for DT folks on how to deal with multiple cpu_dais.
> 

>From what I could gather, it looks like we need not add additional code.
But, my understanding of DT is pretty limited as well. Vinod, Liam,  can you help us
here?

> >
> >>>@@ -1644,7 +1751,7 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
> >>>  	unsigned int dai_fmt)
> >>>  {
> >>>  	struct snd_soc_dai **codec_dais = rtd->codec_dais;
> >>>-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >>>+	struct snd_soc_dai **cpu_dais = rtd->cpu_dais;
> >>>  	unsigned int i;
> >>>  	int ret;
> >>>@@ -1659,35 +1766,44 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
> >>>  		}
> >>>  	}
> >>>-	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
> >>why was this comment removed?
> >>
> >
> >Looks like I messed up resolving ocnflicts while rebase
> >
> >>>  	/* the component which has non_legacy_dai_naming is Codec */
> >>>-	if (cpu_dai->component->driver->non_legacy_dai_naming) {
> >>Not sure if the code refactoring below makes sense in a codec-codec link,
> >>you probably wouldn't have multiple cpu_dais then, would you?
> >
> >Yes, a valid point. You suggest to leave this piece of code as is ?
> 
> Not necessarily. I don't understand how the codec-codec and multi cpu_dais
> intersect, all I am asking for is a check if this change is needed or not.
> 

In soc_link_dai_widgets() which is called from probe_dai_links(),
we call out that we do not support multi cpu/codec for CODEC - CODEC link.
All the operation there are done only on single CPU DAI/Codec DAI.

But, in this function snd_soc_dai_set_fmt() is set on all Codec DAIs. So,
for sake of consistency we can do it for CPU DAIs too.

--Shreyas

-- 

Re: [PATCH v6 2/3] ASoC: Add multiple CPU DAI support for PCM ops

[Shreyas NC]

On Fri, Jun 22, 2018 at 11:05:00AM -0500, Pierre-Louis Bossart wrote:
> 
> >>>@@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
> >>>   */
> >>>  void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> >>>  {
> >>>-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >>>  	int i;
> >>>  	lockdep_assert_held(&rtd->pcm_mutex);
> >>>  	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >>>-		cpu_dai->playback_active++;
> >>>+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >>>+			rtd->cpu_dais[i]->playback_active++;
> >>>  		for (i = 0; i < rtd->num_codecs; i++)
> >>>  			rtd->codec_dais[i]->playback_active++;
> >>>  	} else {
> >>>-		cpu_dai->capture_active++;
> >>>+		for (i = 0; i < rtd->num_cpu_dai; i++)
> >>>+			rtd->cpu_dais[i]->capture_active++;
> >>>  		for (i = 0; i < rtd->num_codecs; i++)
> >>>  			rtd->codec_dais[i]->capture_active++;
> >>>  	}
> >>>-	cpu_dai->active++;
> >>>-	cpu_dai->component->active++;
> >>>+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >>>+		rtd->cpu_dais[i]->component->active++;
> >>>+		rtd->cpu_dais[i]->active++;
> >>>+	}
> >>
> >>This is becoming complicated, how many times do we need to ref-count?
> >
> >Earlier we incremented cpu_dai->playback_active++ and here it is
> >cpu_dais->component->active++
> 
> my point was whether this can be simplified to use a single counter, even
> before we do the change to multi-cpu?
> 

Ok

> >>>@@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >>>  				struct snd_pcm_hw_params *params)
> >>>  {
> >>>  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >>>-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >>>  	unsigned int rate, channels, sample_bits, symmetry, i;
> >>>  	rate = params_rate(params);
> >>>@@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >>>  	sample_bits = snd_pcm_format_physical_width(params_format(params));
> >>>  	/* reject unmatched parameters when applying symmetry */
> >>>-	symmetry = cpu_dai->driver->symmetric_rates ||
> >>>-		rtd->dai_link->symmetric_rates;
> >>This was a logical OR, but...
> >>
> >>>+	symmetry = rtd->dai_link->symmetric_rates;
> >>>+
> >>>+	for (i = 0; i < rtd->num_cpu_dai; i++)
> >>>+		symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
> >>this is a bitwise OR. is this ok?
> >
> >I made this change on purpose and I am struggling to remember the reason :(
> >If I dont figure that out, I will go back to logical OR..
> 
> the same change was made for multiple codecs, but it's worth rechecking.
> 

Yes, I will check this :)

> >>>@@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
> >>>  		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
> >>>  	}
> >>>+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >>>+		cpu_dai_drv = rtd->cpu_dais[i]->driver;
> >>>+
> >>>+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >>>+			cpu_stream = &cpu_dai_drv->playback;
> >>>+		else
> >>>+			cpu_stream = &cpu_dai_drv->capture;
> >>>+
> >>>+		cpu_chan_min = max(cpu_chan_min,
> >>>+					cpu_stream->channels_min);
> >>>+		cpu_chan_max = min(cpu_chan_max,
> >>>+					cpu_stream->channels_max);
> >>>+
> >>>+		if (hw->formats)
> >>>+			hw->formats &= cpu_stream->formats;
> >>>+		else
> >>>+			hw->formats = cpu_stream->formats;
> >>Can you double-check the 'else' case? This doesn't seem right, you will
> >>always use the format used for the last cpu_dai. If the formats are
> >>identical for all cpu_dais, then this can be moved outside of the loop.
> >
> >In the second iteration, we would always go into the if (hw->formats) case.
> 
> Sorry I don't get your comment. This test is now placed inside of a for
> loop, so why would the hw->formats change between iterations?
> 

Ok, the else case can be hit only for the first time(if hw->formats == 0)
when  we enter the loop.

I get your point, will fix this :)
 
> >
> >>>@@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> >>>  	if (ret < 0)
> >>>  		goto component_err;
> >>>-	/* store the parameters for each DAIs */
> >>>-	cpu_dai->rate = params_rate(params);
> >>>-	cpu_dai->channels = params_channels(params);
> >>>-	cpu_dai->sample_bits =
> >>>-		snd_pcm_format_physical_width(params_format(params));
> >>>+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >>>+		/* store the parameters for each DAIs */
> >>>+		cpu_dai = rtd->cpu_dais[i];
> >>>+		cpu_dai->rate = params_rate(params);
> >>>+		cpu_dai->channels = params_channels(params);
> >>>+		cpu_dai->sample_bits =
> >>>+			snd_pcm_format_physical_width(params_format(params));
> >>>+	}
> >>I think I've asked this before but can't recall the answer: does this mean
> >>we assume the same number of channels for each codec_dai[j] and cpu_dai[i]?
> >>
> >
> >Yes, in hw_params we set the same number channels on all the DAIs as that of the
> >stream. But, as I had mentioned in my previous replies, the
> >machine driver would set the channel masks on the individual DAIs(like we do
> >for SoundWire Multi link case)
> 
> so the number of channels is really defined by the stream, and for each
> cpu_dai and codec_dai there is a channel mask defining what the dais should
> produce/consume, right?
> 

Yes

> >
> >>>+	for (i = 0; i < rtd->num_cpu_dai; i++) {
> >>>+		cpu_dai = rtd->cpu_dais[i];
> >>>+		if (cpu_dai->driver->ops->trigger) {
> >>>+			ret = cpu_dai->driver->ops->trigger(substream,
> >>>+								cmd, cpu_dai);
> >>>+			if (ret < 0)
> >>>+				return ret;
> >>>+		}
> >>
> >>Maybe add a comment that in the multi_cpu case the triggers are supposed to
> >>be aligned, i.e. the first trigger starts the others - that would be
> >>consistent with the other comments on delay below.
> >
> >Would this be the right place to add that comment?
> >I am not sure if I got a reply to my question in the previous review cycle:
> >"Just wondering if there can be a case of multiple CPU DAIs but you would
> >not want them to be triggered at the same time."
> >
> >Based on the answer to my question , we can add a comment here.
> 
> If you use the stream/multi cpu_dai concept you want the data to remain
> phase locked and triggered at the same time. If you don't care about phase,
> then you are handling different streams that can be handled with the
> existing model with a set of runtimes that deal with individual cpu_dais.
> 

Ok, will add a comment here :)

--Shreyas

-- 

Re: [PATCH v6 1/3] ASoC: Add initial support for multiple CPU DAIs

[Charles Keepax]

On Fri, Jun 22, 2018 at 10:13:28AM -0500, Pierre-Louis Bossart wrote:
> >>>  	/* the component which has non_legacy_dai_naming is Codec */
> >>>-	if (cpu_dai->component->driver->non_legacy_dai_naming) {
> >>Not sure if the code refactoring below makes sense in a codec-codec link,
> >>you probably wouldn't have multiple cpu_dais then, would you?
> >
> >Yes, a valid point. You suggest to leave this piece of code as is ?
> 
> Not necessarily. I don't understand how the codec-codec and multi
> cpu_dais intersect, all I am asking for is a check if this change is
> needed or not.
> 

Seems plausible to me you could have multiple cpu_dais on CODEC
to CODEC links. Really for CODEC to CODEC links the whole
CPU/CODEC distinction doesn't really exist. It doesn't really
matter which end you make which, so I guess it makes sense to
support multiple CPU DAIs on such a link.

Thanks,
Charles

Re: [PATCH v6 3/3] ASoC: Add multiple CPU DAI support in DAPM

[Shreyas NC]

> >>>DAPM handles DAIs during soc_dapm_stream_event() and during addition
> >>>and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and
> >>>dapm_connect_dai_link_widgets().
> >>can you split this patch in two, one where you add
> >>dapm_add_valid_dai_widget() and the second one where you add the multi-cpu
> >>stuff? the current diff format is really hard to read with the two changes
> >>lumped together.
> >
> >As I had replied earlier, the change is really moving the same code from one
> >function to a new one. So, a patch split would mean we would have the same
> >duplicated code in one patch which surely is not desirable.
> 
> I don't understand your answer. It's fine to have a small preparation patch
> that just moves one piece of code to another function, and they change how
> that function is called.
> 

Ok, I will give it a try :)

> >I just realized that in my earlier reply I had excluded the list and replied
> >only to you :)
> >
> >>
> >>>+	for (i = 0; i < rtd->num_codecs; i++) {
> >>>+		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
> >>>+
> >>>+		for (j = 0; j < rtd->num_cpu_dai; j++) {
> >>>+			cpu_dai = rtd->cpu_dais[j];
> >>>+
> >>>+			dapm_add_valid_dai_widget(card, rtd,
> >>>+						codec_dai, cpu_dai);
> >>
> >>I didn't click on this earlier, but what makes you think all codec_dais are
> >>connected to all cpu_dais?
> >
> >Yes, there need not be a M:N connectivity. But, how do you find that out ?
> >
> >>That doesn't seem quite right.
> >>For the multi-codec case, all the codec_dais hang from a single cpu_dai.
> >>it's a stretch for me to have a full M:N connectivity. And that's clearly
> >>not the case for SoundWire stream in the multi-link case.
> >
> >I mostly do not disagree with you here..
> >
> >>Can't we use the dai_link information here to only connect cpu_ and
> >>codec_dais that are related?
> >
> >Which DAI Link information are you referring to here ?
> >Other than the machine driver which sets the audio route, I am unable to
> >figure out how we will find out the connected cpu_dai and codec_dais at
> >ASoC core level.
> >
> >May be I am missing something :(
> 
> How is it different from the multi-codec support? You must have a
> description somewhere that tells you how the cpu_dai is connected to various
> codec_dais.
> 

No, there is no such description that I could find.

My reference was skl_nau88l25_ssm4567.c machine driver:

/* Back End DAI links */
        {
                /* SSP0 - Codec */
                .name = "SSP0-Codec",
                .id = 0,
                .cpu_dai_name = "SSP0 Pin",
                .platform_name = "0000:00:1f.3",
                .no_pcm = 1,
                .codecs = ssm4567_codec_components,
                .num_codecs = ARRAY_SIZE(ssm4567_codec_components),
                ..
        },

The only way I could infer the connection was from DAPM route:
{
        /* CODEC BE connections */
        { "Left Playback", NULL, "ssp0 Tx"},
        { "Right Playback", NULL, "ssp0 Tx"},
        { "ssp0 Tx", NULL, "codec0_out"},
}

> Maybe we should start with the examples you provided for Soundwire and
> describe how the dailinks would be represented.
> 

Ok , sure. Let me describe a case where we would want to play a 2 ch stream
and we have two Masters "int-sdw.1" and "int-sdw.2"

So, DAI Link would look this way:

Here I specify the multi CPU DAIs
static struct snd_soc_dai_link_component sdw_multi_cpu_comp[] = {
        { /* Left */
                .name = "int-sdw.1",
                .dai_name = "SDW1 Pin0",
        },
        { /* Right */
                .name = "int-sdw.2",
                .dai_name = "SDW2 Pin0",
        },
};

static struct snd_soc_codec_conf rt700_codec_conf[] = {
        {
                .dev_name = "sdw:1:25d:700:0:0",
                .name_prefix = "Left",
        },
        {
                .dev_name = "sdw:2:25d:701:0:0",
                .name_prefix = "Right",
        },
};

And the DAI Link as:
struct snd_soc_dai_link cnl_rt700_msic_dailink[] = {
{
        ...
        {
                .name = "SDW0-Codec",
                .cpu_dai = sdw_multi_cpu_comp,
                .num_cpu_dai = ARRAY_SIZE(sdw_multi_cpu_comp),
                .platform_name = "0000:00:1f.3",
                .codecs = sdw_multi_codec_comp,
                .num_codecs = ARRAY_SIZE(sdw_multi_codec_comp),
	..
        },
}

And it is in the DAPM route that I would specify :
{
         ...
        { "Left DP1 Playback", NULL, "SDW1 Tx0"},
        { "SDW1 Tx0", NULL, "codec0_out"},

        { "Right DP1 Playback", NULL, "SDW2 Tx0"},
        { "SDW2 Tx0", NULL, "codec0_out"},
        ...
}

> With the M:N connectivity you'd end-up having spurious events with
> non-existent connections, it's not necessarily fatal but certainly not
> elegant and may or may not work depending on state management in codec
> drivers.
>

What are the events that you are referring to here?
The reason I ask that is because my understanding is that we will get these
events only for those widgets marked connected by DAPM after parsing the DAPM
route map.
I will check further if you can let me know of the events you are suspecting
:)

Sorry for the longish mail. I thought it would be better to put in the
details rather than go back and forth over them :)

--Shreyas
>

--