diff mbox series

[RFC] ASoC: simple-card: Use dai_id from node description

Message ID 20231117163900.766996-1-daniel.baluta@oss.nxp.com
State New
Headers show
Series [RFC] ASoC: simple-card: Use dai_id from node description | expand

Commit Message

Daniel Baluta Nov. 17, 2023, 4:39 p.m. UTC 
From: Daniel Baluta <daniel.baluta@nxp.com>

We can specify DAI id using reg property. When dts
node has only 1 DAI simple-card always assumes that DAI id is 0.

But this is not correct in the case of SOF for example which adds DAIs
staticaly (See definition of snd_soc_dai_driver in sound/soc/sof/imx/imx8m.c)

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/generic/simple-card-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kuninori Morimoto Nov. 19, 2023, 11:28 p.m. UTC | #1 
Hi Daniel

Thank you for your patch.

> We can specify DAI id using reg property. When dts
> node has only 1 DAI simple-card always assumes that DAI id is 0.
> 
> But this is not correct in the case of SOF for example which adds DAIs
> staticaly (See definition of snd_soc_dai_driver in sound/soc/sof/imx/imx8m.c)
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
(snip)
> -	args.args_count	= (of_graph_get_endpoint_count(node) > 1);
> +	args.args_count	= (of_graph_get_endpoint_count(node) >= 1);

Hmm... I'm not sure how it works for existing drivers.
I'm busy this week, so I will check it next week.

Thanks
Kuninori Morimoto Nov. 20, 2023, 4:36 a.m. UTC | #2 
Hi Daniel, Mark

> We can specify DAI id using reg property. When dts
> node has only 1 DAI simple-card always assumes that DAI id is 0.
> 
> But this is not correct in the case of SOF for example which adds DAIs
> staticaly (See definition of snd_soc_dai_driver in sound/soc/sof/imx/imx8m.c)
(snip)
> -	args.args_count	= (of_graph_get_endpoint_count(node) > 1);
> +	args.args_count	= (of_graph_get_endpoint_count(node) >= 1);

If my understanding was correct, for example you want to use 2nd DAI
but your DT has only 1 port (thus, it is using reg property) ?

Current simple utils is assuming (1) DT has all DAI settings, (2) having
reg property is option.

But current DT requests reg property.
So maybe it is good time to remove non-reg-property support ?


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Daniel Baluta Nov. 20, 2023, 9:57 a.m. UTC | #3 
Hello Morimoto-san,

On Mon, Nov 20, 2023 at 6:36 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Daniel, Mark
>
> > We can specify DAI id using reg property. When dts
> > node has only 1 DAI simple-card always assumes that DAI id is 0.
> >
> > But this is not correct in the case of SOF for example which adds DAIs
> > staticaly (See definition of snd_soc_dai_driver in sound/soc/sof/imx/imx8m.c)
> (snip)
> > -     args.args_count = (of_graph_get_endpoint_count(node) > 1);
> > +     args.args_count = (of_graph_get_endpoint_count(node) >= 1);
>
> If my understanding was correct, for example you want to use 2nd DAI
> but your DT has only 1 port (thus, it is using reg property) ?

Yes.

>
> Current simple utils is assuming (1) DT has all DAI settings, (2) having
> reg property is option.
>
> But current DT requests reg property.
> So maybe it is good time to remove non-reg-property support ?

I have no problem removing non-reg-property support. This will work
for me. Will later send a patch.

I want to understand how current non-reg-property support works.

I'm looking at commit 73b17f1a65c881fc ("SoC: simple-card-utils:
support snd_soc_get_dai_id()").

So, the reg property was introduced for cases where we can have ports
of different types? E.g
In the case of HDMI we can have Audio ports and Video ports? And we
need reg property in order
to get the correct DAI id?

I don't understand how DAI id is currently computed if we don't  have
the reg property and also
we have Non HDMI sound case:

Here is the code:

»       /*
»        * Non HDMI sound case, counting port/endpoint on its DT
»        * is enough. Let's count it.
»        */
»       i = 0;
»       id = -1;
»       for_each_endpoint_of_node(node, endpoint) {
»       »       if (endpoint == ep)
»       »       »       id = i;
»       »       i++;
»       }

»       of_node_put(node);


So, this code assumes that the DAI id is exactly the number of the port, right?
But this is wrong if we have a component (port) with multiple DAIs attached.

Daniel.
Kuninori Morimoto Nov. 20, 2023, 11:04 p.m. UTC | #4 
Hi Daniel, Mark

> > > -     args.args_count = (of_graph_get_endpoint_count(node) > 1);
> > > +     args.args_count = (of_graph_get_endpoint_count(node) >= 1);
> >
> > If my understanding was correct, for example you want to use 2nd DAI
> > but your DT has only 1 port (thus, it is using reg property) ?
> 
> Yes.

But hmm... in your case, you need to setup 2ports, and use 2nd port
is assumed approach.
Why you don't setup full port ? Do you have some reason ??


I think almost all DTS are already using "reg" property, thus,
there is no problem if we remove non-reg-support,
but we don't know details.

Removing non-reg-support needs to update many codes. 
But your original patch is enough for 1st approach, and it is easy
to rewind the code if there was some issue.
I can create more detail cleanup code if there was no problem.

But then, I want to know it is necessary.
If there is good enough reasons why you don't setup full-port,
we can/need to remove non-reg-support. But if there is no good reason,
the things we need is not update code but you add full-port setup, I think.
Daniel Baluta Nov. 21, 2023, 2:02 p.m. UTC | #5 
On Tue, Nov 21, 2023 at 1:04 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
> Hi Daniel, Mark
>
> > > > -     args.args_count = (of_graph_get_endpoint_count(node) > 1);
> > > > +     args.args_count = (of_graph_get_endpoint_count(node) >= 1);
> > >
> > > If my understanding was correct, for example you want to use 2nd DAI
> > > but your DT has only 1 port (thus, it is using reg property) ?
> >
> > Yes.
>
> But hmm... in your case, you need to setup 2ports, and use 2nd port
> is assumed approach.
> Why you don't setup full port ? Do you have some reason ??

I'm not sure I understand what is a full port setup. But let me
describe my scenario so that we have a common ground.

I want to use audio-graph-card2 machine driver to setup Sound Open
Firrmware cards.

Here we start with a normal link with the following components:

Component 0 (DAI) : 3b6e8000.dsp (See sound/soc/sof/core.c: 280)
                                  -> for imx8m this has 3 statically
defined DAIs
See sound/soc/sof/imx/imx8m.c:

static struct snd_soc_dai_driver imx8m_dai[] = {
{     // DAI with index 0
»       .name = "sai1",
},
{      // DAI with index 1
»       .name = "sai3",
},
{ /   // DAI with index 2
»       .name = "micfil",
},
};

Component 1 (Codec): wm8960-hifi
                                    -> with 1 DAI

static struct snd_soc_dai_driver wm8960_dai = {
»       .name = "wm8960-hifi",
};

Now, I want to write a DTS description where my DAI link uses
Component 0 (CPU) (with its DAI index 1) connected with Component 1
(codec) (with its DAI index 0).

So, for this I use the following dts snippet:

sof-sound-wm8960 {
»       »       compatible = "audio-graph-card2";
»       »       links = <&cpu>;
}

dsp: dsp@3b6e8000 {
    cpu: port@1 {
»       »       reg = <1>;
»       »       cpu_ep: endpoint { remote-endpoint = <&codec_ep>; };
»       };
}

wm8960 {

»       port {
»       »       codec_ep: endpoint { remote-endpoint = <&cpu_ep>; };
»       };
}

So, property reg = <1> refferes to DAI with index 1 associated with
component DSP.
Kuninori Morimoto Nov. 22, 2023, 3:39 a.m. UTC | #6 
Hi Daniel

> > But hmm... in your case, you need to setup 2ports, and use 2nd port
> > is assumed approach.
> > Why you don't setup full port ? Do you have some reason ??
(snip)
> Now, I want to write a DTS description where my DAI link uses
> Component 0 (CPU) (with its DAI index 1) connected with Component 1
> (codec) (with its DAI index 0).

Thank you for indicating your DTS.

So in imx8m_dai case, it has total 3 DAIs, and you want to use reg = 1.
In such case, your DTS need to have like below, if my understanding was
correct.

	dsp: dsp@3b6e8000 {
		ports {
			     port@0 { reg = <0>;         endpoint { /* not used */ };  };
			cpu: port@1 { reg = <1>; cpu_ep: endpoint { remote-endpoint = <xxx>; }; };
			     port@2 { reg = <2>;         endpoint { /* not used */ };  };
		};
	};


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Daniel Baluta Nov. 22, 2023, 10:16 a.m. UTC | #7 
On Wed, Nov 22, 2023 at 5:39 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Daniel
>
> > > But hmm... in your case, you need to setup 2ports, and use 2nd port
> > > is assumed approach.
> > > Why you don't setup full port ? Do you have some reason ??
> (snip)
> > Now, I want to write a DTS description where my DAI link uses
> > Component 0 (CPU) (with its DAI index 1) connected with Component 1
> > (codec) (with its DAI index 0).
>
> Thank you for indicating your DTS.
>
> So in imx8m_dai case, it has total 3 DAIs, and you want to use reg = 1.
> In such case, your DTS need to have like below, if my understanding was
> correct.
>
>         dsp: dsp@3b6e8000 {
>                 ports {
>                              port@0 { reg = <0>;         endpoint { /* not used */ };  };
>                         cpu: port@1 { reg = <1>; cpu_ep: endpoint { remote-endpoint = <xxx>; }; };
>                              port@2 { reg = <2>;         endpoint { /* not used */ };  };
>                 };
>         };
>

This works! Thanks. I didn't know that you can leave an endpoint unused :).

So please ignore my initial patch then.
diff mbox series

Patch

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 8ec5d413e8e60..739cb71593a88 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -1121,7 +1121,7 @@  int asoc_graph_parse_dai(struct device *dev, struct device_node *ep,
 	/* Get dai->name */
 	args.np		= node;
 	args.args[0]	= graph_get_dai_id(ep);
-	args.args_count	= (of_graph_get_endpoint_count(node) > 1);
+	args.args_count	= (of_graph_get_endpoint_count(node) >= 1);
 
 	/*
 	 * FIXME