The Cadence Multi-channel I2S (I2S-MC) Controller implements a function of
the multi-channel (up to 8-channel) bus. Each stereo channel combines
functions of a transmitter and a receiver, and can switch freely between
them. Each channel has independent gating, clock and interruption control.
It also support some of these channels are used as playback and others can
also be used as record in the same time.
Four I2S controllers are used on the StarFive JH8100 SoC. Two of the I2S
controllers use two stereo channels, one of them use four channels, and
one use eight. It had tested on the fpga with WM8960 and PDM.
Changes since v1:
- Added new compatible for StarFive JH8100 SoC and a special property to
be got as the max channels number in the bindings.
- Dropped the useless '|' in the bindings.
- Moved the drivers to a new folder named 'cdns' and modified the name
of functions.
v1: https://lore.kernel.org/all/[email protected]/
Xingyu Wu (2):
ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller
ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller
.../bindings/sound/cdns,i2s-mc.yaml | 110 +++
MAINTAINERS | 6 +
sound/soc/Kconfig | 1 +
sound/soc/Makefile | 1 +
sound/soc/cdns/Kconfig | 18 +
sound/soc/cdns/Makefile | 3 +
sound/soc/cdns/cdns-i2s-mc-pcm.c | 262 +++++++
sound/soc/cdns/cdns-i2s-mc.c | 724 ++++++++++++++++++
sound/soc/cdns/cdns-i2s-mc.h | 157 ++++
9 files changed, 1282 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
create mode 100644 sound/soc/cdns/Kconfig
create mode 100644 sound/soc/cdns/Makefile
create mode 100644 sound/soc/cdns/cdns-i2s-mc-pcm.c
create mode 100644 sound/soc/cdns/cdns-i2s-mc.c
create mode 100644 sound/soc/cdns/cdns-i2s-mc.h
--
2.25.1
Add bindings for the Multi-Channel I2S controller of Cadence.
The Multi-Channel I2S (I2S-MC) implements a function of the
8-channel I2S bus interfasce. Each channel can become receiver
or transmitter. Four I2S instances are used on the StarFive
JH8100 SoC. One instance of them is limited to 2 channels, two
instance are limited to 4 channels, and the other one can use
most 8 channels. Add a unique property about
'starfive,i2s-max-channels' to distinguish each instance.
Signed-off-by: Xingyu Wu <[email protected]>
---
.../bindings/sound/cdns,i2s-mc.yaml | 110 ++++++++++++++++++
1 file changed, 110 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
new file mode 100644
index 000000000000..0a1b0122a246
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence multi-channel I2S controller
+
+description:
+ The Cadence I2S Controller implements a function of the multi-channel
+ (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
+
+maintainers:
+ - Xingyu Wu <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - cdns,i2s-mc
+ - starfive,jh8100-i2s
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ The interrupt line number for the I2S controller. Add this
+ parameter if the I2S controller that you are using does not
+ using DMA.
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: Bit clock
+ - description: Main ICG clock
+ - description: Inner master clock
+
+ clock-names:
+ items:
+ - const: bclk
+ - const: icg
+ - const: mclk_inner
+
+ resets:
+ maxItems: 1
+
+ dmas:
+ items:
+ - description: TX DMA Channel
+ - description: RX DMA Channel
+ minItems: 1
+
+ dma-names:
+ items:
+ - const: tx
+ - const: rx
+ minItems: 1
+
+ "#sound-dai-cells":
+ const: 0
+
+allOf:
+ - $ref: dai-common.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh8100-i2s
+ then:
+ properties:
+ starfive,i2s-max-channels:
+ description:
+ Number of I2S max stereo channels supported on the StarFive
+ JH8100 SoC.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [2, 4, 8]
+ required:
+ - starfive,i2s-max-channels
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - resets
+
+oneOf:
+ - required:
+ - dmas
+ - dma-names
+ - required:
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ i2s@122b0000 {
+ compatible = "cdns,i2s-mc";
+ reg = <0x122b0000 0x1000>;
+ clocks = <&syscrg_ne 133>,
+ <&syscrg_ne 170>,
+ <&syscrg 50>;
+ clock-names = "bclk", "icg",
+ "mclk_inner";
+ resets = <&syscrg_ne 43>;
+ dmas = <&dma 7>, <&dma 6>;
+ dma-names = "tx", "rx";
+ #sound-dai-cells = <0>;
+ };
--
2.25.1
On 20/03/2024 10:02, Xingyu Wu wrote:
> Add bindings for the Multi-Channel I2S controller of Cadence.
>
> The Multi-Channel I2S (I2S-MC) implements a function of the
> 8-channel I2S bus interfasce. Each channel can become receiver
> or transmitter. Four I2S instances are used on the StarFive
> JH8100 SoC. One instance of them is limited to 2 channels, two
> instance are limited to 4 channels, and the other one can use
> most 8 channels. Add a unique property about
> 'starfive,i2s-max-channels' to distinguish each instance.
>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> .../bindings/sound/cdns,i2s-mc.yaml | 110 ++++++++++++++++++
> 1 file changed, 110 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
>
> diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> new file mode 100644
> index 000000000000..0a1b0122a246
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence multi-channel I2S controller
> +
> +description:
> + The Cadence I2S Controller implements a function of the multi-channel
> + (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> +
> +maintainers:
> + - Xingyu Wu <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - cdns,i2s-mc
Why did this appear? Who asked for this? Usually these blocks are not
usable on their own.
> + - starfive,jh8100-i2s
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + The interrupt line number for the I2S controller. Add this
> + parameter if the I2S controller that you are using does not
> + using DMA.
That's still wrong. You already got comment on this. Either you have
interrupt or not. You do not add interrupts, based on your choice or not
of having DMA. Drop the comment.
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: Bit clock
> + - description: Main ICG clock
> + - description: Inner master clock
> +
> + clock-names:
> + items:
> + - const: bclk
> + - const: icg
> + - const: mclk_inner
> +
> + resets:
> + maxItems: 1
> +
> + dmas:
> + items:
> + - description: TX DMA Channel
> + - description: RX DMA Channel
> + minItems: 1
> +
> + dma-names:
> + items:
> + - const: tx
> + - const: rx
> + minItems: 1
> +
> + "#sound-dai-cells":
> + const: 0
> +
> +allOf:
> + - $ref: dai-common.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: starfive,jh8100-i2s
> + then:
> + properties:
> + starfive,i2s-max-channels:
> + description:
> + Number of I2S max stereo channels supported on the StarFive
> + JH8100 SoC.
> + $ref: /schemas/types.yaml#/definitions/uint32
Properties must be defined in top-level. You can disallow the property
for other variants, but I claim you won't have here other variants.
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
> + enum: [2, 4, 8]
> + required:
> + - starfive,i2s-max-channels
> +
> +required:
required goes after properties.
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - resets
> +
> +oneOf:
> + - required:
> + - dmas
> + - dma-names
> + - required:
> + - interrupts
This won't work. Provide both interrupts and dmas, and then test your DTS.
> +
> +unevaluatedProperties: false
> +
Best regards,
Krzysztof
>
> On 20/03/2024 10:02, Xingyu Wu wrote:
> > Add bindings for the Multi-Channel I2S controller of Cadence.
> >
> > The Multi-Channel I2S (I2S-MC) implements a function of the 8-channel
> > I2S bus interfasce. Each channel can become receiver or transmitter.
> > Four I2S instances are used on the StarFive
> > JH8100 SoC. One instance of them is limited to 2 channels, two
> > instance are limited to 4 channels, and the other one can use most 8
> > channels. Add a unique property about 'starfive,i2s-max-channels' to
> > distinguish each instance.
> >
> > Signed-off-by: Xingyu Wu <[email protected]>
> > ---
> > .../bindings/sound/cdns,i2s-mc.yaml | 110 ++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > new file mode 100644
> > index 000000000000..0a1b0122a246
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cadence multi-channel I2S controller
> > +
> > +description:
> > + The Cadence I2S Controller implements a function of the
> > +multi-channel
> > + (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> > +
> > +maintainers:
> > + - Xingyu Wu <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - cdns,i2s-mc
>
> Why did this appear? Who asked for this? Usually these blocks are not usable on their
> own.
I wonder if I should keep the original IP compatible. Do I not need it?
>
> > + - starfive,jh8100-i2s
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description:
> > + The interrupt line number for the I2S controller. Add this
> > + parameter if the I2S controller that you are using does not
> > + using DMA.
>
> That's still wrong. You already got comment on this. Either you have interrupt or not.
> You do not add interrupts, based on your choice or not of having DMA. Drop the
> comment.
Do I keep this property and drop this description?
>
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Bit clock
> > + - description: Main ICG clock
> > + - description: Inner master clock
> > +
> > + clock-names:
> > + items:
> > + - const: bclk
> > + - const: icg
> > + - const: mclk_inner
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + dmas:
> > + items:
> > + - description: TX DMA Channel
> > + - description: RX DMA Channel
> > + minItems: 1
> > +
> > + dma-names:
> > + items:
> > + - const: tx
> > + - const: rx
> > + minItems: 1
> > +
> > + "#sound-dai-cells":
> > + const: 0
> > +
> > +allOf:
> > + - $ref: dai-common.yaml#
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: starfive,jh8100-i2s
> > + then:
> > + properties:
> > + starfive,i2s-max-channels:
> > + description:
> > + Number of I2S max stereo channels supported on the StarFive
> > + JH8100 SoC.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Properties must be defined in top-level. You can disallow the property for other
> variants, but I claim you won't have here other variants.
>
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/e
> xample-schema.yaml#L212
>
Will fix.
>
> > + enum: [2, 4, 8]
> > + required:
> > + - starfive,i2s-max-channels
> > +
> > +required:
>
> required goes after properties.
Will fix.
>
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - resets
> > +
> > +oneOf:
> > + - required:
> > + - dmas
> > + - dma-names
> > + - required:
> > + - interrupts
>
> This won't work. Provide both interrupts and dmas, and then test your DTS.
I provided both properties in the DTS and test by dtbs_check. Then it printed that:
'More than one condition true in one of shema: ...'
>
> > +
> > +unevaluatedProperties: false
> > +
>
> Best regards,
> Krzysztof
Best regards,
Xingyu Wu
On 26/03/2024 07:29, Xingyu Wu wrote:
>>
>> On 20/03/2024 10:02, Xingyu Wu wrote:
>>> Add bindings for the Multi-Channel I2S controller of Cadence.
Your email app responds very weird. You bypassed all possible filters
and it is simply not visible that you answer to me. You Reply to
everyone, not to me with Cc to others. There is no standard header whom
do you quote, e.g. "On 26/03/2024 07:29, Xingyu Wu wrote:"
Please use some decent email system, otherwise I will miss all replies
from you.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - cdns,i2s-mc
>>
>> Why did this appear? Who asked for this? Usually these blocks are not usable on their
>> own.
>
> I wonder if I should keep the original IP compatible. Do I not need it?
As I said, it is not usable on its own, so unless you have other
arguments then no. But my point was that no one asked for this.
>
>>
>>> + - starfive,jh8100-i2s
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + description:
>>> + The interrupt line number for the I2S controller. Add this
>>> + parameter if the I2S controller that you are using does not
>>> + using DMA.
>>
>> That's still wrong. You already got comment on this. Either you have interrupt or not.
>> You do not add interrupts, based on your choice or not of having DMA. Drop the
>> comment.
>
> Do I keep this property and drop this description?
Drop description. Keep property, if your hardware has interrupts.
..
>>
>>> + - compatible
>>> + - reg
>>> + - clocks
>>> + - clock-names
>>> + - resets
>>> +
>>> +oneOf:
>>> + - required:
>>> + - dmas
>>> + - dma-names
>>> + - required:
>>> + - interrupts
>>
>> This won't work. Provide both interrupts and dmas, and then test your DTS.
>
> I provided both properties in the DTS and test by dtbs_check. Then it printed that:
> 'More than one condition true in one of shema: ...'
Exactly. Having both properties is a correct DTS. Interrupts do not
disappear just because you decide to describe DMA. It is OS choice what
to use if both are provided.
Best regards,
Krzysztof
On 26/03/2024 14:46, Krzysztof Kozlowski wrote:
>
> On 26/03/2024 07:29, Xingyu Wu wrote:
> >>
> >> On 20/03/2024 10:02, Xingyu Wu wrote:
> >>> Add bindings for the Multi-Channel I2S controller of Cadence.
>
> Your email app responds very weird. You bypassed all possible filters and it is
> simply not visible that you answer to me. You Reply to everyone, not to me with
> Cc to others. There is no standard header whom do you quote, e.g. "On
> 26/03/2024 07:29, Xingyu Wu wrote:"
>
> Please use some decent email system, otherwise I will miss all replies from you.
Thank you for reminding me. I will correct it.
>
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + enum:
> >>> + - cdns,i2s-mc
> >>
> >> Why did this appear? Who asked for this? Usually these blocks are not
> >> usable on their own.
> >
> > I wonder if I should keep the original IP compatible. Do I not need it?
>
> As I said, it is not usable on its own, so unless you have other arguments then no.
> But my point was that no one asked for this.
I want to keep the original IP compatible which can distinguish from the JH8100 SoC.
Can I write it like this:
compatible:
enum:
- starfive,jh8100-i2s
const: cdns,i2s-mc
and I write this in the DTS:
compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
>
> >
> >>
> >>> + - starfive,jh8100-i2s
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + interrupts:
> >>> + description:
> >>> + The interrupt line number for the I2S controller. Add this
> >>> + parameter if the I2S controller that you are using does not
> >>> + using DMA.
> >>
> >> That's still wrong. You already got comment on this. Either you have interrupt
> or not.
> >> You do not add interrupts, based on your choice or not of having DMA.
> >> Drop the comment.
> >
> > Do I keep this property and drop this description?
>
> Drop description. Keep property, if your hardware has interrupts.
>
Will drop.
> ...
>
> >>
> >>> + - compatible
> >>> + - reg
> >>> + - clocks
> >>> + - clock-names
> >>> + - resets
> >>> +
> >>> +oneOf:
> >>> + - required:
> >>> + - dmas
> >>> + - dma-names
> >>> + - required:
> >>> + - interrupts
> >>
> >> This won't work. Provide both interrupts and dmas, and then test your DTS.
> >
> > I provided both properties in the DTS and test by dtbs_check. Then it printed
> that:
> > 'More than one condition true in one of shema: ...'
>
> Exactly. Having both properties is a correct DTS. Interrupts do not disappear just
> because you decide to describe DMA. It is OS choice what to use if both are
> provided.
>
But this I2S can only use either DMA or interrupts.
Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM) to choose DMA or
interrupt if having both them in DTS?
Best regards,
Xingyu Wu
On 26/03/2024 14:43, Xingyu Wu wrote:
>>>>> +
>>>>> +properties:
>>>>> + compatible:
>>>>> + enum:
>>>>> + - cdns,i2s-mc
>>>>
>>>> Why did this appear? Who asked for this? Usually these blocks are not
>>>> usable on their own.
>>>
>>> I wonder if I should keep the original IP compatible. Do I not need it?
>>
>> As I said, it is not usable on its own, so unless you have other arguments then no.
>> But my point was that no one asked for this.
>
> I want to keep the original IP compatible which can distinguish from the JH8100 SoC.
> Can I write it like this:
> compatible:
> enum:
> - starfive,jh8100-i2s
> const: cdns,i2s-mc
>
> and I write this in the DTS:
> compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
Can you provide any rationale for this? I asked "unless you have other
arguments", so where are the arguments?
Nothing was explained in patch changelog. Nothing was provided in this
email thread.
>
>>
>>>
>>>>
>>>>> + - starfive,jh8100-i2s
>>>>> +
>>>>> + reg:
>>>>> + maxItems: 1
>>>>> +
>>>>> + interrupts:
>>>>> + description:
>>>>> + The interrupt line number for the I2S controller. Add this
>>>>> + parameter if the I2S controller that you are using does not
>>>>> + using DMA.
>>>>
>>>> That's still wrong. You already got comment on this. Either you have interrupt
>> or not.
>>>> You do not add interrupts, based on your choice or not of having DMA.
>>>> Drop the comment.
>>>
>>> Do I keep this property and drop this description?
>>
>> Drop description. Keep property, if your hardware has interrupts.
>>
>
> Will drop.
>
>> ...
>>
>>>>
>>>>> + - compatible
>>>>> + - reg
>>>>> + - clocks
>>>>> + - clock-names
>>>>> + - resets
>>>>> +
>>>>> +oneOf:
>>>>> + - required:
>>>>> + - dmas
>>>>> + - dma-names
>>>>> + - required:
>>>>> + - interrupts
>>>>
>>>> This won't work. Provide both interrupts and dmas, and then test your DTS.
>>>
>>> I provided both properties in the DTS and test by dtbs_check. Then it printed
>> that:
>>> 'More than one condition true in one of shema: ...'
>>
>> Exactly. Having both properties is a correct DTS. Interrupts do not disappear just
>> because you decide to describe DMA. It is OS choice what to use if both are
>> provided.
>>
>
> But this I2S can only use either DMA or interrupts.
Just like many other components. DTS should reflect hardware. Hardware
has interrupts and DMA, right?
>
> Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM) to choose DMA or
> interrupt if having both them in DTS?
Don't know, I tend to focus here on bindings.
Best regards,
Krzysztof
On 27/03/2024 13:12, Krzysztof Kozlowski wrote:
> On 26/03/2024 14:43, Xingyu Wu wrote:
> >>>>> +
> >>>>> +properties:
> >>>>> + compatible:
> >>>>> + enum:
> >>>>> + - cdns,i2s-mc
> >>>>
> >>>> Why did this appear? Who asked for this? Usually these blocks are
> >>>> not usable on their own.
> >>>
> >>> I wonder if I should keep the original IP compatible. Do I not need it?
> >>
> >> As I said, it is not usable on its own, so unless you have other arguments then
> no.
> >> But my point was that no one asked for this.
> >
> > I want to keep the original IP compatible which can distinguish from the JH8100
> SoC.
> > Can I write it like this:
> > compatible:
> > enum:
> > - starfive,jh8100-i2s
> > const: cdns,i2s-mc
> >
> > and I write this in the DTS:
> > compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
>
> Can you provide any rationale for this? I asked "unless you have other
> arguments", so where are the arguments?
>
> Nothing was explained in patch changelog. Nothing was provided in this email
> thread.
I don't know if I understood what mark said[1]. Is it to keep the original IP and
add other compatible?
[1] https://lore.kernel.org/all/[email protected]/
Or should I only keep the compatible 'starfive,jh8110-i2s' and change the
bindings name to same to this compatible?
>
> >
> >>
> >>>
> >>>>
> >>>>> + - starfive,jh8100-i2s
> >>>>> +
> >>>>> + reg:
> >>>>> + maxItems: 1
> >>>>> +
> >>>>> + interrupts:
> >>>>> + description:
> >>>>> + The interrupt line number for the I2S controller. Add this
> >>>>> + parameter if the I2S controller that you are using does not
> >>>>> + using DMA.
> >>>>
> >>>> That's still wrong. You already got comment on this. Either you
> >>>> have interrupt
> >> or not.
> >>>> You do not add interrupts, based on your choice or not of having DMA.
> >>>> Drop the comment.
> >>>
> >>> Do I keep this property and drop this description?
> >>
> >> Drop description. Keep property, if your hardware has interrupts.
> >>
> >
> > Will drop.
> >
> >> ...
> >>
> >>>>
> >>>>> + - compatible
> >>>>> + - reg
> >>>>> + - clocks
> >>>>> + - clock-names
> >>>>> + - resets
> >>>>> +
> >>>>> +oneOf:
> >>>>> + - required:
> >>>>> + - dmas
> >>>>> + - dma-names
> >>>>> + - required:
> >>>>> + - interrupts
> >>>>
> >>>> This won't work. Provide both interrupts and dmas, and then test your DTS.
> >>>
> >>> I provided both properties in the DTS and test by dtbs_check. Then
> >>> it printed
> >> that:
> >>> 'More than one condition true in one of shema: ...'
> >>
> >> Exactly. Having both properties is a correct DTS. Interrupts do not
> >> disappear just because you decide to describe DMA. It is OS choice
> >> what to use if both are provided.
> >>
> >
> > But this I2S can only use either DMA or interrupts.
>
> Just like many other components. DTS should reflect hardware. Hardware has
> interrupts and DMA, right?
Yes. The hardware can use interrupts and provide the handshake interface of
DMA to DMA controller. In software, we can choose only one between them.
Do I keep them both in the bindings and provide the selection in the driver?
>
> >
> > Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM) to choose DMA
> > or interrupt if having both them in DTS?
>
> Don't know, I tend to focus here on bindings.
>
Best regards,
Xingyu Wu
On 29/03/2024 04:56, Xingyu Wu wrote:
>>> I want to keep the original IP compatible which can distinguish from the JH8100
>> SoC.
>>> Can I write it like this:
>>> compatible:
>>> enum:
>>> - starfive,jh8100-i2s
>>> const: cdns,i2s-mc
>>>
>>> and I write this in the DTS:
>>> compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
>>
>> Can you provide any rationale for this? I asked "unless you have other
>> arguments", so where are the arguments?
>>
>> Nothing was explained in patch changelog. Nothing was provided in this email
>> thread.
>
> I don't know if I understood what mark said[1]. Is it to keep the original IP and
> add other compatible?
>
> [1] https://lore.kernel.org/all/[email protected]/
>
I stated and I keep my statement that such block is usually not usable
on its own and always needs some sort of quirks or SoC-specific
implementation. At least this is what I saw in other similar cases, but
not exactly I2S.
Therefore I think fallback is not usable here, thus please use only
starfive compatible. Drop the fallback. It could be added in the future
if I am proven wrong. If you think that fallback is usable alone, please
bring some real life case.
> Or should I only keep the compatible 'starfive,jh8110-i2s' and change the
> bindings name to same to this compatible?
Filename could be cdns,i2s-mc.yaml, assuming that's the name of original
IP block.
..
>>>>
>>>
>>> But this I2S can only use either DMA or interrupts.
>>
>> Just like many other components. DTS should reflect hardware. Hardware has
>> interrupts and DMA, right?
>
> Yes. The hardware can use interrupts and provide the handshake interface of
> DMA to DMA controller. In software, we can choose only one between them.
> Do I keep them both in the bindings and provide the selection in the driver?
Yes.
Best regards,
Krzysztof
On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:
> I stated and I keep my statement that such block is usually not usable
> on its own and always needs some sort of quirks or SoC-specific
> implementation. At least this is what I saw in other similar cases, but
> not exactly I2S.
I wouldn't be so pessimistic, especially not for I2S - a good portion of
quirks there are extra features rather than things needed for basic
operation, a lot of things that might in the past have been quirks for
basic operation are these days hidden behind the DT bindings.
On 29/03/2024 14:36, Mark Brown wrote:
> On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:
>
>> I stated and I keep my statement that such block is usually not usable
>> on its own and always needs some sort of quirks or SoC-specific
>> implementation. At least this is what I saw in other similar cases, but
>> not exactly I2S.
>
> I wouldn't be so pessimistic, especially not for I2S - a good portion of
> quirks there are extra features rather than things needed for basic
> operation, a lot of things that might in the past have been quirks for
> basic operation are these days hidden behind the DT bindings.
OK, I trust your judgement, so cdns as fallback seems okay, but I don't
think it warrants cdns as used alone. Not particularly because cdns is
different, but because we expect specific SoC compatible always.
Thus if cdns,i2s-mc stays, then:
items:
- enum:
- starfive,jh8100-i2s
- cdns,i2s-mc
Best regards,
Krzysztof
On 30/03/2024 0:02, Krzysztof Kozlowski wrote:
>
> On 29/03/2024 14:36, Mark Brown wrote:
> > On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:
> >
> >> I stated and I keep my statement that such block is usually not
> >> usable on its own and always needs some sort of quirks or
> >> SoC-specific implementation. At least this is what I saw in other
> >> similar cases, but not exactly I2S.
> >
> > I wouldn't be so pessimistic, especially not for I2S - a good portion
> > of quirks there are extra features rather than things needed for basic
> > operation, a lot of things that might in the past have been quirks for
> > basic operation are these days hidden behind the DT bindings.
>
> OK, I trust your judgement, so cdns as fallback seems okay, but I don't think it
> warrants cdns as used alone. Not particularly because cdns is different, but
> because we expect specific SoC compatible always.
>
> Thus if cdns,i2s-mc stays, then:
>
> items:
> - enum:
> - starfive,jh8100-i2s
> - cdns,i2s-mc
>
OK, thanks Krzysztof and Mark. I will modify it in next patch.
Best regards,
Xingyu Wu