| Message ID | 87wmvdkiif.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ASoC: makes CPU/Codec channel connection map more generic | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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 >
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.
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
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 :)
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.
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
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
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
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 --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
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(-)