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

[Shreyas NC <shreyas.nc@intel.com>]

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;
> >+		}
> >  	}
> >  	
> 

--