| Message ID | 20240515135411.343333-10-elinor.montmasson@savoirfairelinux.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv4,1/9] ASoC: fsl-asoc-card: add support for dai links with multiple codecs | expand |
On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote: > Add documentation about new dts bindings following new support > for compatible "fsl,imx-audio-generic". > audio-codec: > - $ref: /schemas/types.yaml#/definitions/phandle > - description: The phandle of an audio codec > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: | > + The phandle of an audio codec. > + If using the "fsl,imx-audio-generic" compatible, give instead a pair of > + phandles with the spdif_transmitter first (driver SPDIF DIT) and the > + spdif_receiver second (driver SPDIF DIR). > + items: > + maxItems: 1 This description (and the code) don't feel like they're actually generic - they're clearly specific to the bidrectional S/PDIF case. I'd expect something called -generic to cope with single CODECs as well as double, and not to have any constraints on what those are.
From: "Mark Brown" <broonie@kernel.org> Sent: Thursday, 16 May, 2024 14:11:11 > On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote: > >> Add documentation about new dts bindings following new support >> for compatible "fsl,imx-audio-generic". > >> audio-codec: >> - $ref: /schemas/types.yaml#/definitions/phandle >> - description: The phandle of an audio codec >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: | >> + The phandle of an audio codec. >> + If using the "fsl,imx-audio-generic" compatible, give instead a pair of >> + phandles with the spdif_transmitter first (driver SPDIF DIT) and the >> + spdif_receiver second (driver SPDIF DIR). >> + items: >> + maxItems: 1 > > This description (and the code) don't feel like they're actually generic > - they're clearly specific to the bidrectional S/PDIF case. I'd expect > something called -generic to cope with single CODECs as well as double, > and not to have any constraints on what those are. I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski, the compatible "fsl,imx-audio-no-codec" instead of "generic". Krzysztof thought it was too generic, but it would convey more clearly that it is for cases without codec driver. Would this other compatible string be more appropriate ? Best regards, Elinor Montmasson
On Fri, May 17, 2024 at 05:05:41AM -0400, Elinor Montmasson wrote: > From: "Mark Brown" <broonie@kernel.org> > > This description (and the code) don't feel like they're actually generic > > - they're clearly specific to the bidrectional S/PDIF case. I'd expect > > something called -generic to cope with single CODECs as well as double, > > and not to have any constraints on what those are. > I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski, > the compatible "fsl,imx-audio-no-codec" instead of "generic". > Krzysztof thought it was too generic, but it would convey more clearly > that it is for cases without codec driver. > Would this other compatible string be more appropriate ? No. There is very clearly a CODEC here, it physically exists, we can point at it on the board and it has a software representation. Your code is also very specific to the two CODEC case.
On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote: > Add documentation about new dts bindings following new support > for compatible "fsl,imx-audio-generic". > > Some CPU DAI don't require a real audio codec. The new compatible > "fsl,imx-audio-generic" allows using the driver with codec drivers > SPDIF DIT and SPDIF DIR as dummy codecs. > It also allows using not pre-configured audio codecs which > don't require specific control through a codec driver. > > The new dts properties give the possibility to set some parameters > about the CPU DAI usually set through the codec configuration. > > Signed-off-by: Elinor Montmasson <elinor.montmasson@savoirfairelinux.com> > --- > .../bindings/sound/fsl-asoc-card.yaml | 96 ++++++++++++++++++- > 1 file changed, 92 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml > index 9922664d5ccc..332d8bf96e06 100644 > --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml > +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml > @@ -23,6 +23,16 @@ description: > and PCM DAI formats. However, it'll be also possible to support those non > AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as > long as the driver has been properly upgraded. > + To use CPU DAIs that do not require a codec such as an S/PDIF controller, > + or to use a DAI to output or capture raw I2S/TDM data, you can > + use the compatible "fsl,imx-audio-generic". > + > +definitions: > + imx-audio-generic-dependency: > + properties: > + compatible: > + contains: > + const: fsl,imx-audio-generic > > maintainers: > - Shengjiu Wang <shengjiu.wang@nxp.com> > @@ -81,6 +91,7 @@ properties: > - fsl,imx-audio-wm8960 > - fsl,imx-audio-wm8962 > - fsl,imx-audio-wm8958 > + - fsl,imx-audio-generic > > model: > $ref: /schemas/types.yaml#/definitions/string > @@ -93,8 +104,14 @@ properties: > need to add ASRC support via DPCM. > > audio-codec: > - $ref: /schemas/types.yaml#/definitions/phandle > - description: The phandle of an audio codec > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: | > + The phandle of an audio codec. > + If using the "fsl,imx-audio-generic" compatible, give instead a pair of > + phandles with the spdif_transmitter first (driver SPDIF DIT) and the > + spdif_receiver second (driver SPDIF DIR). minItems: 1 maxItems: 2 > + items: > + maxItems: 1 > > audio-cpu: > $ref: /schemas/types.yaml#/definitions/phandle > @@ -150,8 +167,8 @@ properties: > description: dai-link uses bit clock inversion. > > mclk-id: > - $ref: /schemas/types.yaml#/definitions/uint32 > - description: main clock id, specific for each card configuration. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Main clock id for each codec, specific for each card configuration. minItems: 1 maxItems: 2 > > mux-int-port: > $ref: /schemas/types.yaml#/definitions/uint32 > @@ -167,10 +184,68 @@ properties: > $ref: /schemas/types.yaml#/definitions/phandle > description: The phandle of an CPU DAI controller > > + # Properties relevant only with "fsl,imx-audio-generic" compatible > + dai-tdm-slot-width: > + description: See tdm-slot.txt. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + dai-tdm-slot-num: > + description: See tdm-slot.txt. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + clocks: > + description: | > + The CPU DAI system clock, used to retrieve > + the CPU DAI system clock frequency with the generic codec. > + maxItems: 1 > + > + clock-names: > + items: > + - const: cpu_sysclk > + > + cpu-system-clock-direction-out: > + description: | > + Specifies cpu system clock direction as 'out' on initialization. > + If not set, direction is 'in'. > + $ref: /schemas/types.yaml#/definitions/flag > + > +dependencies: > + dai-tdm-slot-width: > + $ref: "#/definitions/imx-audio-generic-dependency" > + dai-tdm-slot-num: > + $ref: "#/definitions/imx-audio-generic-dependency" > + clocks: > + $ref: "#/definitions/imx-audio-generic-dependency" > + cpu-system-clock-direction-out: > + $ref: "#/definitions/imx-audio-generic-dependency" This works, but is an unusual pattern... > + > required: > - compatible > - model > > +allOf: > + - if: > + $ref: "#/definitions/imx-audio-generic-dependency" > + then: > + properties: > + audio-codec: > + items: > + - description: SPDIF DIT phandle > + - description: SPDIF DIR phandle > + mclk-id: > + maxItems: 1 > + items: > + minItems: 1 > + maxItems: 2 > + else: > + properties: > + audio-codec: > + maxItems: 1 > + mclk-id: > + maxItems: 1 > + items: > + maxItems: 1 You can handle the dependency like this instead: dai-tdm-slot-width: false dai-tdm-slot-num: false And then you don't need the definitions. > + > unevaluatedProperties: false > > examples: > @@ -195,3 +270,16 @@ examples: > "AIN2L", "Line In Jack", > "AIN2R", "Line In Jack"; > }; > + > + - | > + #include <dt-bindings/clock/imx8mn-clock.h> > + sound-spdif-asrc { > + compatible = "fsl,imx-audio-generic"; > + model = "spdif-asrc-audio"; > + audio-cpu = <&spdif>; > + audio-asrc = <&easrc>; > + audio-codec = <&spdifdit>, <&spdifdir>; > + clocks = <&clk IMX8MN_CLK_SAI5_ROOT>; > + clock-names = "cpu_sysclk"; > + cpu-system-clock-direction-out; > + }; > -- > 2.34.1 >
From: "Mark Brown" <broonie@kernel.org> Sent: Friday, 17 May, 2024 13:11:43 > On Fri, May 17, 2024 at 05:05:41AM -0400, Elinor Montmasson wrote: >> From: "Mark Brown" <broonie@kernel.org> > >> > This description (and the code) don't feel like they're actually generic >> > - they're clearly specific to the bidrectional S/PDIF case. I'd expect >> > something called -generic to cope with single CODECs as well as double, >> > and not to have any constraints on what those are. > >> I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski, >> the compatible "fsl,imx-audio-no-codec" instead of "generic". >> Krzysztof thought it was too generic, but it would convey more clearly >> that it is for cases without codec driver. >> Would this other compatible string be more appropriate ? > > No. There is very clearly a CODEC here, it physically exists, we can > point at it on the board and it has a software representation. Your > code is also very specific to the two CODEC case. Then maybe it's not be a good idea to make this compatible generic for this contribution. The original intention is to bring support for the S/PDIF, so maybe the contribution should focus on this use case? In that case, would changing the compatible for "fsl,imx-audio-spdif-card" be acceptable? "fsl,imx-audio-spdif" is already used for the `imx-spdif.c` which does not use the ASRC.
From: "Rob Herring" <robh@kernel.org> Sent: Monday, 20 May, 2024 20:31:48 > On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote: >> Add documentation about new dts bindings following new support >> for compatible "fsl,imx-audio-generic". >> >> Some CPU DAI don't require a real audio codec. The new compatible >> "fsl,imx-audio-generic" allows using the driver with codec drivers >> SPDIF DIT and SPDIF DIR as dummy codecs. >> It also allows using not pre-configured audio codecs which >> don't require specific control through a codec driver. >> >> The new dts properties give the possibility to set some parameters >> about the CPU DAI usually set through the codec configuration. >> >> Signed-off-by: Elinor Montmasson <elinor.montmasson@savoirfairelinux.com> >> --- >> .../bindings/sound/fsl-asoc-card.yaml | 96 ++++++++++++++++++- >> 1 file changed, 92 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml >> b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml >> index 9922664d5ccc..332d8bf96e06 100644 >> --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml >> +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml >> @@ -23,6 +23,16 @@ description: >> and PCM DAI formats. However, it'll be also possible to support those non >> AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as >> long as the driver has been properly upgraded. >> + To use CPU DAIs that do not require a codec such as an S/PDIF controller, >> + or to use a DAI to output or capture raw I2S/TDM data, you can >> + use the compatible "fsl,imx-audio-generic". >> + >> +definitions: >> + imx-audio-generic-dependency: >> + properties: >> + compatible: >> + contains: >> + const: fsl,imx-audio-generic >> >> maintainers: >> - Shengjiu Wang <shengjiu.wang@nxp.com> >> @@ -81,6 +91,7 @@ properties: >> - fsl,imx-audio-wm8960 >> - fsl,imx-audio-wm8962 >> - fsl,imx-audio-wm8958 >> + - fsl,imx-audio-generic >> >> model: >> $ref: /schemas/types.yaml#/definitions/string >> @@ -93,8 +104,14 @@ properties: >> need to add ASRC support via DPCM. >> >> audio-codec: >> - $ref: /schemas/types.yaml#/definitions/phandle >> - description: The phandle of an audio codec >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: | >> + The phandle of an audio codec. >> + If using the "fsl,imx-audio-generic" compatible, give instead a pair of >> + phandles with the spdif_transmitter first (driver SPDIF DIT) and the >> + spdif_receiver second (driver SPDIF DIR). > > minItems: 1 > maxItems: 2 > >> + items: >> + maxItems: 1 >> >> audio-cpu: >> $ref: /schemas/types.yaml#/definitions/phandle >> @@ -150,8 +167,8 @@ properties: >> description: dai-link uses bit clock inversion. >> >> mclk-id: >> - $ref: /schemas/types.yaml#/definitions/uint32 >> - description: main clock id, specific for each card configuration. >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + description: Main clock id for each codec, specific for each card >> configuration. > > minItems: 1 > maxItems: 2 >> >> mux-int-port: >> $ref: /schemas/types.yaml#/definitions/uint32 >> @@ -167,10 +184,68 @@ properties: >> $ref: /schemas/types.yaml#/definitions/phandle >> description: The phandle of an CPU DAI controller >> >> + # Properties relevant only with "fsl,imx-audio-generic" compatible >> + dai-tdm-slot-width: >> + description: See tdm-slot.txt. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + dai-tdm-slot-num: >> + description: See tdm-slot.txt. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + clocks: >> + description: | >> + The CPU DAI system clock, used to retrieve >> + the CPU DAI system clock frequency with the generic codec. >> + maxItems: 1 >> + >> + clock-names: >> + items: >> + - const: cpu_sysclk >> + >> + cpu-system-clock-direction-out: >> + description: | >> + Specifies cpu system clock direction as 'out' on initialization. >> + If not set, direction is 'in'. >> + $ref: /schemas/types.yaml#/definitions/flag >> + >> +dependencies: >> + dai-tdm-slot-width: >> + $ref: "#/definitions/imx-audio-generic-dependency" >> + dai-tdm-slot-num: >> + $ref: "#/definitions/imx-audio-generic-dependency" >> + clocks: >> + $ref: "#/definitions/imx-audio-generic-dependency" >> + cpu-system-clock-direction-out: >> + $ref: "#/definitions/imx-audio-generic-dependency" > > This works, but is an unusual pattern... > >> + >> required: >> - compatible >> - model >> >> +allOf: >> + - if: >> + $ref: "#/definitions/imx-audio-generic-dependency" >> + then: >> + properties: >> + audio-codec: >> + items: >> + - description: SPDIF DIT phandle >> + - description: SPDIF DIR phandle >> + mclk-id: >> + maxItems: 1 >> + items: >> + minItems: 1 >> + maxItems: 2 >> + else: >> + properties: >> + audio-codec: >> + maxItems: 1 >> + mclk-id: >> + maxItems: 1 >> + items: >> + maxItems: 1 > > > You can handle the dependency like this instead: > > dai-tdm-slot-width: false > dai-tdm-slot-num: false > > > And then you don't need the definitions. > >> + >> unevaluatedProperties: false >> >> examples: >> @@ -195,3 +270,16 @@ examples: >> "AIN2L", "Line In Jack", >> "AIN2R", "Line In Jack"; >> }; >> + >> + - | >> + #include <dt-bindings/clock/imx8mn-clock.h> >> + sound-spdif-asrc { >> + compatible = "fsl,imx-audio-generic"; >> + model = "spdif-asrc-audio"; >> + audio-cpu = <&spdif>; >> + audio-asrc = <&easrc>; >> + audio-codec = <&spdifdit>, <&spdifdir>; >> + clocks = <&clk IMX8MN_CLK_SAI5_ROOT>; >> + clock-names = "cpu_sysclk"; >> + cpu-system-clock-direction-out; >> + }; >> -- >> 2.34.1 Ok, thank you for the review, I'll make these modifications in v5.
On Fri, May 31, 2024 at 08:48:04AM -0400, Elinor Montmasson wrote: > Then maybe it's not be a good idea to make this compatible generic > for this contribution. > The original intention is to bring support for the S/PDIF, > so maybe the contribution should focus on this use case? > In that case, would changing the compatible for "fsl,imx-audio-spdif-card" > be acceptable? > "fsl,imx-audio-spdif" is already used for the `imx-spdif.c` > which does not use the ASRC. Why not just use the existing compatible - why would someone not want to be able to use the ASRC if it's available in their system?
From: "Mark Brown" <broonie@kernel.org> Sent: Friday, 31 May, 2024 15:14:13 > On Fri, May 31, 2024 at 08:48:04AM -0400, Elinor Montmasson wrote: > >> Then maybe it's not be a good idea to make this compatible generic >> for this contribution. >> The original intention is to bring support for the S/PDIF, >> so maybe the contribution should focus on this use case? >> In that case, would changing the compatible for "fsl,imx-audio-spdif-card" >> be acceptable? >> "fsl,imx-audio-spdif" is already used for the `imx-spdif.c` >> which does not use the ASRC. > > Why not just use the existing compatible - why would someone not want to > be able to use the ASRC if it's available in their system? That's true but it will be a problem if both `fsl-asoc-card.c` and `imx-spdif.c` drivers have the same compatible, and they don't have the same DT properties.
On Fri, May 31, 2024 at 10:48:14AM -0400, Elinor Montmasson wrote: > From: "Mark Brown" <broonie@kernel.org> > > Why not just use the existing compatible - why would someone not want to > > be able to use the ASRC if it's available in their system? > That's true but it will be a problem if both `fsl-asoc-card.c` and > `imx-spdif.c` drivers have the same compatible, and they don't > have the same DT properties. So merge the two then?
> From: "Mark Brown" <broonie@kernel.org> > Sent: Friday, 31 May, 2024 18:06:30 > On Fri, May 31, 2024 at 10:48:14AM -0400, Elinor Montmasson wrote: >> From: "Mark Brown" <broonie@kernel.org> > >> > Why not just use the existing compatible - why would someone not want to >> > be able to use the ASRC if it's available in their system? > >> That's true but it will be a problem if both `fsl-asoc-card.c` and >> `imx-spdif.c` drivers have the same compatible, and they don't >> have the same DT properties. > > So merge the two then? It would avoid having duplicate drivers yes, I will do this for the v5 of this contribution. Thank you for the review.
diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml index 9922664d5ccc..332d8bf96e06 100644 --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml @@ -23,6 +23,16 @@ description: and PCM DAI formats. However, it'll be also possible to support those non AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as long as the driver has been properly upgraded. + To use CPU DAIs that do not require a codec such as an S/PDIF controller, + or to use a DAI to output or capture raw I2S/TDM data, you can + use the compatible "fsl,imx-audio-generic". + +definitions: + imx-audio-generic-dependency: + properties: + compatible: + contains: + const: fsl,imx-audio-generic maintainers: - Shengjiu Wang <shengjiu.wang@nxp.com> @@ -81,6 +91,7 @@ properties: - fsl,imx-audio-wm8960 - fsl,imx-audio-wm8962 - fsl,imx-audio-wm8958 + - fsl,imx-audio-generic model: $ref: /schemas/types.yaml#/definitions/string @@ -93,8 +104,14 @@ properties: need to add ASRC support via DPCM. audio-codec: - $ref: /schemas/types.yaml#/definitions/phandle - description: The phandle of an audio codec + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + The phandle of an audio codec. + If using the "fsl,imx-audio-generic" compatible, give instead a pair of + phandles with the spdif_transmitter first (driver SPDIF DIT) and the + spdif_receiver second (driver SPDIF DIR). + items: + maxItems: 1 audio-cpu: $ref: /schemas/types.yaml#/definitions/phandle @@ -150,8 +167,8 @@ properties: description: dai-link uses bit clock inversion. mclk-id: - $ref: /schemas/types.yaml#/definitions/uint32 - description: main clock id, specific for each card configuration. + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Main clock id for each codec, specific for each card configuration. mux-int-port: $ref: /schemas/types.yaml#/definitions/uint32 @@ -167,10 +184,68 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: The phandle of an CPU DAI controller + # Properties relevant only with "fsl,imx-audio-generic" compatible + dai-tdm-slot-width: + description: See tdm-slot.txt. + $ref: /schemas/types.yaml#/definitions/uint32 + + dai-tdm-slot-num: + description: See tdm-slot.txt. + $ref: /schemas/types.yaml#/definitions/uint32 + + clocks: + description: | + The CPU DAI system clock, used to retrieve + the CPU DAI system clock frequency with the generic codec. + maxItems: 1 + + clock-names: + items: + - const: cpu_sysclk + + cpu-system-clock-direction-out: + description: | + Specifies cpu system clock direction as 'out' on initialization. + If not set, direction is 'in'. + $ref: /schemas/types.yaml#/definitions/flag + +dependencies: + dai-tdm-slot-width: + $ref: "#/definitions/imx-audio-generic-dependency" + dai-tdm-slot-num: + $ref: "#/definitions/imx-audio-generic-dependency" + clocks: + $ref: "#/definitions/imx-audio-generic-dependency" + cpu-system-clock-direction-out: + $ref: "#/definitions/imx-audio-generic-dependency" + required: - compatible - model +allOf: + - if: + $ref: "#/definitions/imx-audio-generic-dependency" + then: + properties: + audio-codec: + items: + - description: SPDIF DIT phandle + - description: SPDIF DIR phandle + mclk-id: + maxItems: 1 + items: + minItems: 1 + maxItems: 2 + else: + properties: + audio-codec: + maxItems: 1 + mclk-id: + maxItems: 1 + items: + maxItems: 1 + unevaluatedProperties: false examples: @@ -195,3 +270,16 @@ examples: "AIN2L", "Line In Jack", "AIN2R", "Line In Jack"; }; + + - | + #include <dt-bindings/clock/imx8mn-clock.h> + sound-spdif-asrc { + compatible = "fsl,imx-audio-generic"; + model = "spdif-asrc-audio"; + audio-cpu = <&spdif>; + audio-asrc = <&easrc>; + audio-codec = <&spdifdit>, <&spdifdir>; + clocks = <&clk IMX8MN_CLK_SAI5_ROOT>; + clock-names = "cpu_sysclk"; + cpu-system-clock-direction-out; + };
Add documentation about new dts bindings following new support for compatible "fsl,imx-audio-generic". Some CPU DAI don't require a real audio codec. The new compatible "fsl,imx-audio-generic" allows using the driver with codec drivers SPDIF DIT and SPDIF DIR as dummy codecs. It also allows using not pre-configured audio codecs which don't require specific control through a codec driver. The new dts properties give the possibility to set some parameters about the CPU DAI usually set through the codec configuration. Signed-off-by: Elinor Montmasson <elinor.montmasson@savoirfairelinux.com> --- .../bindings/sound/fsl-asoc-card.yaml | 96 ++++++++++++++++++- 1 file changed, 92 insertions(+), 4 deletions(-)