diff mbox series

[v5,5/5] dt-bindings: audio-graph-port: add ch-map-idx property

Message ID 87wmvdkiif.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. 23, 2023, 5:36 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>
---
 .../devicetree/bindings/sound/audio-graph-port.yaml        | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Conor Dooley Oct. 23, 2023, 4:50 p.m. UTC | #1
Yo,

On Mon, Oct 23, 2023 at 05:36:09AM +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>
> ---
>  .../devicetree/bindings/sound/audio-graph-port.yaml        | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> index 60b5e3fd1115..47f04cdd6670 100644
> --- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> +++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> @@ -19,7 +19,12 @@ definitions:
>      properties:
>        mclk-fs:
>          $ref: simple-card.yaml#/definitions/mclk-fs
> -

Why have you removed the blank line here?

> +      ch-map-idx:

I would rather this be spelt out as "channel-map-index" - although I
don't know if that is the best name for the property, as it seems very
tied to a single operating systems variable names.
I'll leave it to Mark as to whether there is a less linux implementation
coupled name for this property.

> +        description: It indicates index of ch_maps array for CPU / Codec if number of

From a bindings perspective, "ch_maps array" is meaningless, as it is
(AFAICT) a linux driver variable name, whereas the property description
needs to describe the hardware alone.

> +          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
> +          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and

Again, relying on header files in an operating system to explain the
property is not a runner. You need to explain how to populate this
property in an operating system independent manner.

> +          ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample.

I'd much rather you added an example to this dt-binding, rather than
pointing off to another location. A proper example will also be able to
be validated by dt-binding-check.

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

Blank line here please.

Cheers,
Conor.

>    endpoint-base:
>      allOf:
>        - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> -- 
> 2.25.1
>
Mark Brown Oct. 23, 2023, 6:47 p.m. UTC | #2
On Mon, Oct 23, 2023 at 05:50:42PM +0100, Conor Dooley wrote:
> On Mon, Oct 23, 2023 at 05:36:09AM +0000, Kuninori Morimoto wrote:

> > +      ch-map-idx:

> I would rather this be spelt out as "channel-map-index" - although I
> don't know if that is the best name for the property, as it seems very
> tied to a single operating systems variable names.
> I'll leave it to Mark as to whether there is a less linux implementation
> coupled name for this property.

It's not particularly Linux coupled, this is a fairly general concept.
Kuninori Morimoto Oct. 23, 2023, 10:58 p.m. UTC | #3
Hi Conor


> > +          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
> > +          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and
> 
> Again, relying on header files in an operating system to explain the
> property is not a runner. You need to explain how to populate this
> property in an operating system independent manner.

Sample is not mandatory here, I will remove Linux header pointer from here in v6.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Conor Dooley Oct. 24, 2023, 3:27 p.m. UTC | #4
On Mon, Oct 23, 2023 at 07:47:09PM +0100, Mark Brown wrote:
> On Mon, Oct 23, 2023 at 05:50:42PM +0100, Conor Dooley wrote:
> > On Mon, Oct 23, 2023 at 05:36:09AM +0000, Kuninori Morimoto wrote:
> 
> > > +      ch-map-idx:
> 
> > I would rather this be spelt out as "channel-map-index" - although I
> > don't know if that is the best name for the property, as it seems very
> > tied to a single operating systems variable names.
> > I'll leave it to Mark as to whether there is a less linux implementation
> > coupled name for this property.
> 
> It's not particularly Linux coupled, this is a fairly general concept.

You'd know better than I, it just seemed like a rip from the variable
name :)
Conor Dooley Oct. 24, 2023, 3:28 p.m. UTC | #5
On Mon, Oct 23, 2023 at 10:58:28PM +0000, Kuninori Morimoto wrote:
> 
> Hi Conor
> 
> 
> > > +          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
> > > +          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and
> > 
> > Again, relying on header files in an operating system to explain the
> > property is not a runner. You need to explain how to populate this
> > property in an operating system independent manner.
> 
> Sample is not mandatory here, I will remove Linux header pointer from here in v6.

Please don't just remove the reference to the header file, and actually
explain the property instead.
Rob Herring (Arm) Oct. 24, 2023, 8:24 p.m. UTC | #6
On Mon, Oct 23, 2023 at 05:36:09AM +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>
> ---
>  .../devicetree/bindings/sound/audio-graph-port.yaml        | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> index 60b5e3fd1115..47f04cdd6670 100644
> --- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> +++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
> @@ -19,7 +19,12 @@ definitions:
>      properties:
>        mclk-fs:
>          $ref: simple-card.yaml#/definitions/mclk-fs
> -
> +      ch-map-idx:
> +        description: It indicates index of ch_maps array for CPU / Codec if number of
> +          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
> +          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and
> +          ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample.

Why do we have a dtsi file hidden away here?

We have examples and actual users. Do we really need a 3rd way?

Rob
Kuninori Morimoto Oct. 24, 2023, 10:56 p.m. UTC | #7
Hi Conor

> > > Again, relying on header files in an operating system to explain the
> > > property is not a runner. You need to explain how to populate this
> > > property in an operating system independent manner.
> > 
> > Sample is not mandatory here, I will remove Linux header pointer from here in v6.
> 
> Please don't just remove the reference to the header file, and actually
> explain the property instead.

Yes, I will try my best.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Oct. 24, 2023, 11:06 p.m. UTC | #8
Hi Rob

> > +      ch-map-idx:
> > +        description: It indicates index of ch_maps array for CPU / Codec if number of
> > +          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
> > +          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and
> > +          ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample.
> 
> Why do we have a dtsi file hidden away here?
> 
> We have examples and actual users. Do we really need a 3rd way?

ASoC is supporting many type of (complex) connections, and Audio Graph Card2 is
supporting all of them. There is no actual user who is using all type of
connections. Thus there is no good sample for it.

Above is using all type of connections. And I'm using it for Audio Graph
Card2 test purpose.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Oct. 25, 2023, 1:14 a.m. UTC | #9
Hi Mark, Conor

Thank you for your feedbacks.

> > > > +      ch-map-idx:
> > 
> > > I would rather this be spelt out as "channel-map-index" - although I
> > > don't know if that is the best name for the property, as it seems very
> > > tied to a single operating systems variable names.
> > > I'll leave it to Mark as to whether there is a less linux implementation
> > > coupled name for this property.
> > 
> > It's not particularly Linux coupled, this is a fairly general concept.
> 
> You'd know better than I, it just seemed like a rip from the variable
> name :)

I have no special opinion about this, but let's use more generic naming.
v6 will use "channel-map-index" for it.

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..47f04cdd6670 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
+++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
@@ -19,7 +19,12 @@  definitions:
     properties:
       mclk-fs:
         $ref: simple-card.yaml#/definitions/mclk-fs
-
+      ch-map-idx:
+        description: It indicates index of ch_maps array for CPU / Codec if number of
+          CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the
+          numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and
+          ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample.
+        $ref: /schemas/types.yaml#/definitions/uint32-array
   endpoint-base:
     allOf:
       - $ref: /schemas/graph.yaml#/$defs/endpoint-base