diff mbox series

[v3,4/4] dt-bindings: audio-graph-port: add ch-maps property

Message ID 871qe0y6aq.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State Superseded
Headers show
Series ASoC: makes CPU/Codec channel connection map more generic | expand

Commit Message

Kuninori Morimoto Oct. 12, 2023, 1:32 a.m. UTC
This patch adds ch-maps property to enable handling CPU:Codec = N:M
connection.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 Documentation/devicetree/bindings/sound/audio-graph-port.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Conor Dooley Oct. 12, 2023, 7:53 a.m. UTC | #1
Hey,

On Thu, Oct 12, 2023 at 01:32:13AM +0000, Kuninori Morimoto wrote:
> This patch adds ch-maps property to enable handling CPU:Codec = N:M
> connection.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  Documentation/devicetree/bindings/sound/audio-graph-port.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> index 60b5e3fd1115..3c4b331e8498 100644
> --- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> +++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> @@ -19,6 +19,8 @@ definitions:
>      properties:
>        mclk-fs:
>          $ref: simple-card.yaml#/definitions/mclk-fs
> +      ch-maps:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array

Most of what I said on the last version applies here too. Only the
s/_/-/ was done. Is there a reason you ignored those comments?

Thanks,
Conor.

>  
>    endpoint-base:
>      allOf:
> -- 
> 2.25.1
>
Kuninori Morimoto Oct. 13, 2023, 12:33 a.m. UTC | #2
Hi Conor

Thank you for your feedback

> > This patch adds ch-maps property to enable handling CPU:Codec = N:M
> > connection.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
(snip)
> > +      ch-maps:
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> Most of what I said on the last version applies here too. Only the
> s/_/-/ was done. Is there a reason you ignored those comments?

Ah sorry. I thought you wanted was add your address on To/Cc for
all patch-set.

> I only got this one patch, so I have no context at all for this change.
> Given that, and since I know almost nothing about sound stuff...
(snip)
> ...I have absolutely no idea how I would populate "ch_maps" correctly.
> Please describe (in the binding) what this property actually does
> & how to use it. Also, properties use -s not _s.

Some Sound want to use multiple connections between CPUs (N) and Codecs (M).
Current audio-graph-card2 driver is already supporting 1:N / N:1 / N:N
connections, this patch expand it.

These are implemented by using Of-Graph.
For example N:N connection case, it is expressed like below.
One note here is that cpu0/cpu1 and codec0/codec1 are *not* independent.
We need to consider cpu0/cpu1 pair and codec0/codec1 pair.

ep (endpoint)

	(A)						(B)
	<- port ->   <- port ->       <- port ->      <- port ->
		          ax(ep) <--> (ep)bx
	cpu0(ep) <--> (ep)a0		  b0(ep) <--> (ep)codec0
	cpu1(ep) <--> (ep)a1		  b1(ep) <--> (ep)codec1


In N:N case, it is assuming cpu0/codec0, and cpu1/codec1 has related.
This patch expand (A)/(B) part to N:M (N != M). Then, ch-maps indicates
how these are related.

	(A)						(B)
	<- port ->   <- port ->       <- port ->      <- port ->
		          ax(ep) <--> (ep)bx
	cpu0(ep) <--> (ep)a0		  b0(ep) <--> (ep)codec0
	cpu1(ep) <--> (ep)a1		  b1(ep) <--> (ep)codec1
=>	cpu2(ep) <--> (ep)a2

	ch-maps = <0 0 1>

ch-maps = <0 0 1> means, 
	cpu0 <-> codec0
	cpu1 <-> codec0
	cpu2 <-> codec1

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Mark Brown Oct. 13, 2023, 3:41 p.m. UTC | #3
On Fri, Oct 13, 2023 at 12:33:34AM +0000, Kuninori Morimoto wrote:

> > > +      ch-maps:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-array

> > I only got this one patch, so I have no context at all for this change.
> > Given that, and since I know almost nothing about sound stuff...
> (snip)
> > ...I have absolutely no idea how I would populate "ch_maps" correctly.
> > Please describe (in the binding) what this property actually does
> > & how to use it. Also, properties use -s not _s.

> Some Sound want to use multiple connections between CPUs (N) and Codecs (M).
> Current audio-graph-card2 driver is already supporting 1:N / N:1 / N:N
> connections, this patch expand it.

Some of this explanation needs to go into the binding - someone reading
the binding should really be able to figure out what numbers to put in
there without looking at the code.

> ch-maps = <0 0 1> means, 
> 	cpu0 <-> codec0
> 	cpu1 <-> codec0
> 	cpu2 <-> codec1

> Thank you for your help !!

So probably somthing along the lines of saying "there should be one
element in the array for each CPU DAI, this should be the CODEC number
to route to" (that's probably still a bit unclear but roughly that).
Conor Dooley Oct. 13, 2023, 4:12 p.m. UTC | #4
On Fri, Oct 13, 2023 at 04:41:42PM +0100, Mark Brown wrote:
> On Fri, Oct 13, 2023 at 12:33:34AM +0000, Kuninori Morimoto wrote:
> 
> > > > +      ch-maps:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> > > I only got this one patch, so I have no context at all for this change.
> > > Given that, and since I know almost nothing about sound stuff...
> > (snip)
> > > ...I have absolutely no idea how I would populate "ch_maps" correctly.
> > > Please describe (in the binding) what this property actually does
> > > & how to use it. Also, properties use -s not _s.
> 
> > Some Sound want to use multiple connections between CPUs (N) and Codecs (M).
> > Current audio-graph-card2 driver is already supporting 1:N / N:1 / N:N
> > connections, this patch expand it.
> 
> Some of this explanation needs to go into the binding - someone reading
> the binding should really be able to figure out what numbers to put in
> there without looking at the code.

Absolutely :)

> > ch-maps = <0 0 1> means, 
> > 	cpu0 <-> codec0
> > 	cpu1 <-> codec0
> > 	cpu2 <-> codec1

What happens when you want to convey that codec0 & codec1 are both
connected to cpu0 & codec2 is connected to cpu1?
How would that be described in a DT?
Or is that not something anyone would even want to do?

> > Thank you for your help !!
> 
> So probably somthing along the lines of saying "there should be one
> element in the array for each CPU DAI, this should be the CODEC number
> to route to" (that's probably still a bit unclear but roughly that).
Kuninori Morimoto Oct. 16, 2023, 12:46 a.m. UTC | #5
Hi Conor

> > Some of this explanation needs to go into the binding - someone reading
> > the binding should really be able to figure out what numbers to put in
> > there without looking at the code.
> 
> Absolutely :)

Indeed :) will do in v4

> > > ch-maps = <0 0 1> means, 
> > > 	cpu0 <-> codec0
> > > 	cpu1 <-> codec0
> > > 	cpu2 <-> codec1
> 
> What happens when you want to convey that codec0 & codec1 are both
> connected to cpu0 & codec2 is connected to cpu1?
> How would that be described in a DT?
> Or is that not something anyone would even want to do?

In such case, ch-maps is from codec. it will be like below.
It is judged by number of cpu vs codec. [PATCH 3/4] has both case sample.

	cpu >= codec : CPU   base
	cpu <  codec : Codec base

	ch-maps = <0 0 1>
	codec0 <-> cpu0
	codec1 <-> cpu0
	codec2 <-> cpu1

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Conor Dooley Oct. 17, 2023, 10:04 a.m. UTC | #6
On Mon, Oct 16, 2023 at 12:46:29AM +0000, Kuninori Morimoto wrote:
> 
> Hi Conor
> 
> > > Some of this explanation needs to go into the binding - someone reading
> > > the binding should really be able to figure out what numbers to put in
> > > there without looking at the code.
> > 
> > Absolutely :)
> 
> Indeed :) will do in v4
> 
> > > > ch-maps = <0 0 1> means, 
> > > > 	cpu0 <-> codec0
> > > > 	cpu1 <-> codec0
> > > > 	cpu2 <-> codec1
> > 
> > What happens when you want to convey that codec0 & codec1 are both
> > connected to cpu0 & codec2 is connected to cpu1?
> > How would that be described in a DT?
> > Or is that not something anyone would even want to do?
> 
> In such case, ch-maps is from codec. it will be like below.
> It is judged by number of cpu vs codec. [PATCH 3/4] has both case sample.
> 
> 	cpu >= codec : CPU   base
> 	cpu <  codec : Codec base
> 
> 	ch-maps = <0 0 1>
> 	codec0 <-> cpu0
> 	codec1 <-> cpu0
> 	codec2 <-> cpu1

That seems like a very unintuitive interface. If there were 32 CPUs and
30 codecs, then it'd be very inconvenient for a human reader to grok the
configuration. CPUs were to be disabled in the DT, could the meaning of
the property invert?

I am not really the best when it comes to audio (or media) bindings, but
I am wondering if a phandle based approach would be better, where the
codecs have phandles for their associated CPUs. Maybe Mark, Rob etc could
comment if doing that sort of thing is not feasible.

Cheers,
Conor.
Kuninori Morimoto Oct. 18, 2023, 12:34 a.m. UTC | #7
Hi Conor

> That seems like a very unintuitive interface. If there were 32 CPUs and
> 30 codecs, then it'd be very inconvenient for a human reader to grok the
> configuration.

I don't think such huge number connection will be used...

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Conor Dooley Oct. 18, 2023, 7:57 a.m. UTC | #8
On Wed, Oct 18, 2023 at 12:34:01AM +0000, Kuninori Morimoto wrote:
> 
> Hi Conor
> 
> > That seems like a very unintuitive interface. If there were 32 CPUs and
> > 30 codecs, then it'd be very inconvenient for a human reader to grok the
> > configuration.
> 
> I don't think such huge number connection will be used...

Your binding allows for that though!
I really don't like the number of elements inverting the meaning of the
property.

Also, this was not the only item in my mail. Why did you not respond to
the other comments?

Cheers,
Conor.

> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto
Mark Brown Oct. 18, 2023, 6:25 p.m. UTC | #9
On Wed, Oct 18, 2023 at 08:57:17AM +0100, Conor Dooley wrote:
> On Wed, Oct 18, 2023 at 12:34:01AM +0000, Kuninori Morimoto wrote:

> > > That seems like a very unintuitive interface. If there were 32 CPUs and
> > > 30 codecs, then it'd be very inconvenient for a human reader to grok the
> > > configuration.

> > I don't think such huge number connection will be used...

I suspect that if we run into systems with very large numbers of devices
on a single bus they'll either want to do something more dynamic and use
it as a switch or be very regular which makes writing things a lot more
manageable.

> Your binding allows for that though!
> I really don't like the number of elements inverting the meaning of the
> property.

I have to say I'm not a big fan of that either.  It might be easier to
map each channel to a slot number on the link, each DAI could then have
an independent map and the kernel could compare DAI slots to join things
up.
Kuninori Morimoto Oct. 20, 2023, 1:13 a.m. UTC | #10
Hi Mark

> > I really don't like the number of elements inverting the meaning of the
> > property.
> 
> I have to say I'm not a big fan of that either.  It might be easier to
> map each channel to a slot number on the link, each DAI could then have
> an independent map and the kernel could compare DAI slots to join things
> up.  

Do you mean something like this ?
almost pseudo code...

Image
	CPU0 <---> Codec0
	CPU1 <-+-> Codec1
	CPU2 <-/

code
	ch_maps = {
		.cpu;
		.codec;
		...
	};

	.ch_maps[0].cpu = CPU0
	.ch_maps[1].cpu = CPU1
	.ch_maps[2].cpu = CPU2

	.ch_maps[0].codec = Codec0
	.ch_maps[1].codec = Codec1
	.ch_maps[2].codec = Codec1

DT
		(A)						(B)
		<- port ->   <- port ->       <- port ->      <- port ->
			          ax(ep) <--> (ep)bx
	map=<0>	cpu0(ep) <--> (ep)a0		  b0(ep) <--> (ep)codec0  map=<0>
	map=<1>	cpu1(ep) <--> (ep)a1		  b1(ep) <--> (ep)codec1  map=<1 2>
	map=<2>	cpu2(ep) <--> (ep)a2					  ~~~~~~~~~
	~~~~~~~	

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Mark Brown Oct. 20, 2023, 11:58 a.m. UTC | #11
On Fri, Oct 20, 2023 at 01:13:31AM +0000, Kuninori Morimoto wrote:

> DT
> 		(A)						(B)
> 		<- port ->   <- port ->       <- port ->      <- port ->
> 			          ax(ep) <--> (ep)bx
> 	map=<0>	cpu0(ep) <--> (ep)a0		  b0(ep) <--> (ep)codec0  map=<0>
> 	map=<1>	cpu1(ep) <--> (ep)a1		  b1(ep) <--> (ep)codec1  map=<1 2>
> 	map=<2>	cpu2(ep) <--> (ep)a2					  ~~~~~~~~~

I think that looks like what I was thinking of, yes.  Might be
misreading it though!
Kuninori Morimoto Oct. 23, 2023, 12:08 a.m. UTC | #12
Hi Mark

> > DT
> > 		(A)						(B)
> > 		<- port ->   <- port ->       <- port ->      <- port ->
> > 			          ax(ep) <--> (ep)bx
> > 	map=<0>	cpu0(ep) <--> (ep)a0		  b0(ep) <--> (ep)codec0  map=<0>
> > 	map=<1>	cpu1(ep) <--> (ep)a1		  b1(ep) <--> (ep)codec1  map=<1 2>
> > 	map=<2>	cpu2(ep) <--> (ep)a2					  ~~~~~~~~~
> 
> I think that looks like what I was thinking of, yes.  Might be
> misreading it though!

OK, I see.
Indeed this is better idea than mine. It is easy to understand
and code can be more simple. Will use it on v5.


Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
index 60b5e3fd1115..3c4b331e8498 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
+++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
@@ -19,6 +19,8 @@  definitions:
     properties:
       mclk-fs:
         $ref: simple-card.yaml#/definitions/mclk-fs
+      ch-maps:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
 
   endpoint-base:
     allOf: