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

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

On Thu, May 24, 2018 at 03:45:40PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 05/24/2018 12:34 AM, Shreyas NC wrote:
> >>>@@ -1120,6 +1123,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;
> >>>+
> >>This structure is becoming difficult to interpret:
> >>
> >>     struct snd_soc_dai *codec_dai;
> >>     struct snd_soc_dai *cpu_dai;
> >>
> >>     struct snd_soc_dai **codec_dais;
> >>     unsigned int num_codecs;
> >>
> >>     struct snd_soc_dai **cpu_dais;
> >>     unsigned int num_cpu_dai;
> >>
> >>What is the intended usage of codec_dai vs. codec_dais and cpu_dai vs.
> >>cpu_dais?
> >rtd->cpu_dai is used in many drivers to get cpu_dai from soc_pcm_runtime.
> >Similarly cpu_dais is addded to get multiple cpu_dais from runtime.
> >
> >TBH, I have shadowed the codec_dais implementation for sake of
> >convenience and being conformal :)
> >
> >>If this is only to handle the single codec_dai/single cpu_dai as an
> >>exception, should we port the code to support multiple cpu_dais/multiple
> >>codec_dais?
> >>
> >As mentioned in [1] (in cover letter), there are discussions to unify cpu and
> >codec dais and this is the first step towards that. So, yes, eventually we
> >should port the code as you have suggested :)
> ok, maybe add a comment in the commit message then?

Ok

> >
> >-	/*
> >-	 * 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)) {
> >>This doesn't seem to be the logical translation of the initial condition
> >>based on link->cpu_name and link->cpu_of_node?
> >>
> >pasting the code added from the function above to show the mapping b/w name,
> >of_node and dai_name:
> >
> >		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;
> >
> >and the original condition was:
> >		if (!link->cpu_dai_name &&
> >			!(link->cpu_name || link->cpu_of_node)) {
> >
> >So, it does look correct to me, unless I am missing something obvious :(
> The original code I was referring to is this:
> 
>     /*
>      * CPU device may be specified by either name or OF node, but
>      * 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;
>     }
> 
>     return 0;
> 
> The new code is this:
> 
>     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;
>         }
>     }
> 
> Yes, it's equivalent for i==0, but it's not clear to me for non-zero
> indices. I don't get how/where
> link->cpu_dai[i].name and link->cpu_dai[i].of_node are initialized for i>0.
> 

Aaah ok, I understand your question now :)

That is added in the dai_link description in the machine driver.

Like this:

static struct snd_soc_dai_link_component test_multi_cpu_component[] = {
        { /* Left */
                .name = "int-sdw.1",
                .dai_name = "SDW1 Pin0",
        },
        { /* Right */
                .name = "int-sdw.2",
                .dai_name = "SDW2 Pin0",
        },
};

struct snd_soc_dai_link test_dailink[] = {
        	{
                	.name = "SDW0-Codec",
	                .cpu_dai = test_multi_cpu_component,
        	        .num_cpu_dai = ARRAY_SIZE(sdw_multi_cpu_comp),
			...
		}
}

> >>>-	if (cpu_dai->driver->compress_new) {
> >>>+	if (rtd->cpu_dais[0]->driver->compress_new) {
> >>>+		if (rtd->num_cpu_dai > 1)
> >>>+			dev_warn(card->dev,
> >>>+				"ASoC: multi-cpu compress dais not supported");
> >>Not sure this is right, you could have a case where the compress dai is not
> >>on the cpu_dai[0]?
> >I am not able to comprehend that we can have multi CPU DAIS in a DAI Link
> >with one of the DAI being compress and the other being a PCM DAI.
> >
> >Any such systems that you can think of ?
> Yes I agree, my point was that you test only for the first cpu_dai (index
> 0), but if rtd->cpu_dais[1]->drivers->compress_new is non NULL then it's
> also wrong. You should test for all cpu_dais to make sure they are all PCM.
> 

Ok

--Shreyas
--