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
--
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
