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.