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

[Pierre-Louis Bossart]

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?
>
>>>   	/* 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;
>>> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
>>> +			cpu_dai = rtd->cpu_dais[i];
>>> +
>>> +			if (rtd->dai_link->ignore_suspend)
>>> +				continue;
>> this test needs to be outside of the for loop? the 'continue' should result
>> in moving to the next element of the list, not to the next cpu_dai.
>> see below, this statement is indeed outside for the resume part.
> Yes, will fix this. Thanks :)
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.

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