diff mbox series

[v3,1/4] ASoC: dt-bindings: lpc32xx: Add lpc32xx i2s DT binding

Message ID 20240614163500.386747-2-piotr.wojtaszczyk@timesys.com (mailing list archive)
State Superseded
Headers show
Series Add audio support for LPC32XX CPUs | expand

Commit Message

Piotr Wojtaszczyk June 14, 2024, 4:34 p.m. UTC
Add nxp,lpc3220-i2s DT binding documentation.

Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
---
Changes for v3:
- Added '$ref: dai-common.yaml#' and '#sound-dai-cells'
- Dropped all clock-names, references
- Dropped status property from the example
- Added interrupts property
- 'make dt_binding_check' pass

Changes for v2:
- Added maintainers field
- Dropped clock-names
- Dropped unused unneded interrupts field

 .../bindings/sound/nxp,lpc3220-i2s.yaml       | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml

Comments

Krzysztof Kozlowski June 15, 2024, 10:01 a.m. UTC | #1
On 14/06/2024 18:34, Piotr Wojtaszczyk wrote:
> Add nxp,lpc3220-i2s DT binding documentation.
> 
> Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.


b4 diff '<20240614163500.386747-2-piotr.wojtaszczyk@timesys.com>'
Grabbing thread from
lore.kernel.org/all/20240614163500.386747-2-piotr.wojtaszczyk@timesys.com/t.mbox.gz
Checking for older revisions
Grabbing search results from lore.kernel.org
Nothing matching that query.
---
Analyzing 24 messages in the thread
Preparing fake-am for v2: ASoC: fsl: Add i2s and pcm drivers for LPC32xx
CPUs
  range: dda6bddbafe9..33a2a5c8fb4c
Preparing fake-am for v3: ASoC: dt-bindings: lpc32xx: Add lpc32xx i2s DT
binding
ERROR: Could not fake-am version v3
---
Could not create fake-am range for upper series v3


You are making review more difficult.

> ---
> Changes for v3:
> - Added '$ref: dai-common.yaml#' and '#sound-dai-cells'
> - Dropped all clock-names, references
> - Dropped status property from the example
> - Added interrupts property
> - 'make dt_binding_check' pass
> 
> Changes for v2:
> - Added maintainers field
> - Dropped clock-names
> - Dropped unused unneded interrupts field
> 
>  .../bindings/sound/nxp,lpc3220-i2s.yaml       | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
> new file mode 100644
> index 000000000000..04a1090f70cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/nxp,lpc3220-i2s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP LPC32XX I2S Controller
> +
> +description:
> +  The I2S controller in LPC32XX SoCs, ASoC DAI.
> +
> +maintainers:
> +  - J.M.B. Downing <jonathan.downing@nautel.com>
> +  - Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,lpc3220-i2s
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: input clock of the peripheral.
> +
> +  dma-vc-names:

Missing vendor prefix... but I don't really get what's the point of this
property.

> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: |
> +      names of virtual pl08x dma channels for tx and rx
> +      directions in this order.
> +    minItems: 2
> +    maxItems: 2

What part of hardware or board configuration this represents?

It wasn't here and nothing in changelog explained it.

Drop.


> +
> +  "#sound-dai-cells":
> +    const: 0
> +

Best regards,
Krzysztof
Piotr Wojtaszczyk June 17, 2024, 9:33 a.m. UTC | #2
On Sat, Jun 15, 2024 at 12:01 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Do not attach (thread) your patchsets to some other threads (unrelated
> or older versions). This buries them deep in the mailbox and might
> interfere with applying entire sets.

I'm sorry about that, it won't happen again.

> > +  dma-vc-names:
>
> Missing vendor prefix... but I don't really get what's the point of this
> property.

Is "nxp,lpc3xxx-dma-vc-names" acceptable?

>
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +    description: |
> > +      names of virtual pl08x dma channels for tx and rx
> > +      directions in this order.
> > +    minItems: 2
> > +    maxItems: 2
>
> What part of hardware or board configuration this represents?
>
> It wasn't here and nothing in changelog explained it.

That's information which DMA signal and mux setting an I2S interface uses.
It's a name (bus_id field) of platform data entry from phy3250.c in
[PATCH v3 3/4].
It's used by snd_soc_dai_init_dma_data() in [PATCH v3 4/4] to give the
dmaengine a
hint which dma config to use. The LPC32xx doesn't have yet a dmamux driver like
lpc18xx-dmamux.c therefore it still uses platform data entries for
pl08x dma channels
and 'SND_DMAENGINE_PCM_FLAG_NO_DT | SND_DMAENGINE_PCM_FLAG_COMPAT'
flags in the devm_snd_dmaengine_pcm_register().
Typically instead of this platform data you would use regular 'dma'
and 'dma-names' if it had
proper dmamux driver like lpc18xx-dmamux.c

>
> Drop.
>
>
> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +

The "dai-common.yam" doesn't declare a default value for this so
isn't it required? It's declared in others yaml files like:
Documentation/devicetree/bindings/sound/qcom,q6apm.yaml


--
Piotr Wojtaszczyk
Timesys
Krzysztof Kozlowski June 17, 2024, 12:14 p.m. UTC | #3
On 17/06/2024 11:33, Piotr Wojtaszczyk wrote:
> On Sat, Jun 15, 2024 at 12:01 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> Do not attach (thread) your patchsets to some other threads (unrelated
>> or older versions). This buries them deep in the mailbox and might
>> interfere with applying entire sets.
> 
> I'm sorry about that, it won't happen again.
> 
>>> +  dma-vc-names:
>>
>> Missing vendor prefix... but I don't really get what's the point of this
>> property.
> 
> Is "nxp,lpc3xxx-dma-vc-names" acceptable?

No, because it does not help me to understand:
" what's the point of this property."

> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +    description: |
>>> +      names of virtual pl08x dma channels for tx and rx
>>> +      directions in this order.
>>> +    minItems: 2
>>> +    maxItems: 2
>>
>> What part of hardware or board configuration this represents?
>>
>> It wasn't here and nothing in changelog explained it.
> 
> That's information which DMA signal and mux setting an I2S interface uses.
> It's a name (bus_id field) of platform data entry from phy3250.c in
> [PATCH v3 3/4].

platform entries from driver do not seem related at all to hardware
description. You know encode driver model into bindings, so obviously no-go.

> It's used by snd_soc_dai_init_dma_data() in [PATCH v3 4/4] to give the
> dmaengine a
> hint which dma config to use. The LPC32xx doesn't have yet a dmamux driver like

and if I change driver platform data to foo and bar, does the DTS work? No.

> lpc18xx-dmamux.c therefore it still uses platform data entries for
> pl08x dma channels
> and 'SND_DMAENGINE_PCM_FLAG_NO_DT | SND_DMAENGINE_PCM_FLAG_COMPAT'
> flags in the devm_snd_dmaengine_pcm_register().
> Typically instead of this platform data you would use regular 'dma'
> and 'dma-names' if it had
> proper dmamux driver like lpc18xx-dmamux.c

Exactly. Use these.

> 
>>
>> Drop.
>>
>>
>>> +
>>> +  "#sound-dai-cells":
>>> +    const: 0
>>> +
> 
> The "dai-common.yam" doesn't declare a default value for this so

Where is my comment to which you refer to? Please do not drop context
from replies. I have no clue what you want to discuss here.

> isn't it required? It's declared in others yaml files like:
> Documentation/devicetree/bindings/sound/qcom,q6apm.yaml


Best regards,
Krzysztof
Piotr Wojtaszczyk June 17, 2024, 2:04 p.m. UTC | #4
On Mon, Jun 17, 2024 at 2:14 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/06/2024 11:33, Piotr Wojtaszczyk wrote:
> > On Sat, Jun 15, 2024 at 12:01 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> Do not attach (thread) your patchsets to some other threads (unrelated
> >> or older versions). This buries them deep in the mailbox and might
> >> interfere with applying entire sets.
> >
> > I'm sorry about that, it won't happen again.
> >
> >>> +  dma-vc-names:
> >>
> >> Missing vendor prefix... but I don't really get what's the point of this
> >> property.
> >
> > Is "nxp,lpc3xxx-dma-vc-names" acceptable?
>
> No, because it does not help me to understand:
> " what's the point of this property."
>
> >
> >>
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +    description: |
> >>> +      names of virtual pl08x dma channels for tx and rx
> >>> +      directions in this order.
> >>> +    minItems: 2
> >>> +    maxItems: 2
> >>
> >> What part of hardware or board configuration this represents?
> >>
> >> It wasn't here and nothing in changelog explained it.
> >
> > That's information which DMA signal and mux setting an I2S interface uses.
> > It's a name (bus_id field) of platform data entry from phy3250.c in
> > [PATCH v3 3/4].
>
> platform entries from driver do not seem related at all to hardware
> description. You know encode driver model into bindings, so obviously no-go.

In this case platform entries do exactly that, they define which dma
signal number is
routed to peripherals in LPC32xx. They describe hardware capabilities
of the pl08x dma
and which AHB bus the dma is connected to. This was carried over from
linux versions
before DT was introduced.

>
> > It's used by snd_soc_dai_init_dma_data() in [PATCH v3 4/4] to give the
> > dmaengine a
> > hint which dma config to use. The LPC32xx doesn't have yet a dmamux driver like
>
> and if I change driver platform data to foo and bar, does the DTS work? No.

They shouldn't change the same way as expected dma-names shouldn't change.
Lots of drivers expect the dma-names to be "rx", "tx"

>
> > lpc18xx-dmamux.c therefore it still uses platform data entries for
> > pl08x dma channels
> > and 'SND_DMAENGINE_PCM_FLAG_NO_DT | SND_DMAENGINE_PCM_FLAG_COMPAT'
> > flags in the devm_snd_dmaengine_pcm_register().
> > Typically instead of this platform data you would use regular 'dma'
> > and 'dma-names' if it had
> > proper dmamux driver like lpc18xx-dmamux.c
>
> Exactly. Use these.

Then I need to write a lpc32xx dma mux driver, device tree binding for
it and adjust the
LPC32xx I2S driver for it. Is this a hard requirement to accept this
patch set for the
legacy LPC32xx SoC?

>
> >
> >>
> >> Drop.
> >>
> >>
> >>> +
> >>> +  "#sound-dai-cells":
> >>> +    const: 0
> >>> +
> >
> > The "dai-common.yam" doesn't declare a default value for this so
>
> Where is my comment to which you refer to? Please do not drop context
> from replies. I have no clue what you want to discuss here.
Well I didn't remove the context, you said:
"
Drop.
(...)
+  "#sound-dai-cells":
+    const: 0
"
So I'm confused whether the "#sound-dai-cells" should be in the dt
binding or not.
Krzysztof Kozlowski June 17, 2024, 3:48 p.m. UTC | #5
On 17/06/2024 16:04, Piotr Wojtaszczyk wrote:
>>
>>> It's used by snd_soc_dai_init_dma_data() in [PATCH v3 4/4] to give the
>>> dmaengine a
>>> hint which dma config to use. The LPC32xx doesn't have yet a dmamux driver like
>>
>> and if I change driver platform data to foo and bar, does the DTS work? No.
> 
> They shouldn't change the same way as expected dma-names shouldn't change.
> Lots of drivers expect the dma-names to be "rx", "tx"
> 
>>
>>> lpc18xx-dmamux.c therefore it still uses platform data entries for
>>> pl08x dma channels
>>> and 'SND_DMAENGINE_PCM_FLAG_NO_DT | SND_DMAENGINE_PCM_FLAG_COMPAT'
>>> flags in the devm_snd_dmaengine_pcm_register().
>>> Typically instead of this platform data you would use regular 'dma'
>>> and 'dma-names' if it had
>>> proper dmamux driver like lpc18xx-dmamux.c
>>
>> Exactly. Use these.
> 
> Then I need to write a lpc32xx dma mux driver, device tree binding for
> it and adjust the
> LPC32xx I2S driver for it. Is this a hard requirement to accept this
> patch set for the
> legacy LPC32xx SoC?

I do not see at all analogy with dma-names. dma-names are used ONLY by
the consumer to pick up proper property "dmas" from DT. They are not
passed to DMA code. They are not used to configure DMA provider at all.

You parse string from DT and pass it further as DMA filtering code. This
is abuse of hardware description for programming your driver and their
dependencies.

Why you cannot hard-code them?

Sorry, to be clear: NAK

> 
>>
>>>
>>>>
>>>> Drop.
>>>>
>>>>
>>>>> +
>>>>> +  "#sound-dai-cells":
>>>>> +    const: 0
>>>>> +
>>>
>>> The "dai-common.yam" doesn't declare a default value for this so
>>
>> Where is my comment to which you refer to? Please do not drop context
>> from replies. I have no clue what you want to discuss here.
> Well I didn't remove the context, you said:
> "
> Drop.
> (...)
> +  "#sound-dai-cells":
> +    const: 0
> "
> So I'm confused whether the "#sound-dai-cells" should be in the dt
> binding or not.

??? Drop is above the text so why do you refer to dai cells? We use here
text-based mailing list style of discussions, not corporate MS Office.

Best regards,
Krzysztof
Piotr Wojtaszczyk June 17, 2024, 4:30 p.m. UTC | #6
On Mon, Jun 17, 2024 at 5:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/06/2024 16:04, Piotr Wojtaszczyk wrote:
> >>
> >>> It's used by snd_soc_dai_init_dma_data() in [PATCH v3 4/4] to give the
> >>> dmaengine a
> >>> hint which dma config to use. The LPC32xx doesn't have yet a dmamux driver like
> >>
> >> and if I change driver platform data to foo and bar, does the DTS work? No.
> >
> > They shouldn't change the same way as expected dma-names shouldn't change.
> > Lots of drivers expect the dma-names to be "rx", "tx"
> >
> >>
> >>> lpc18xx-dmamux.c therefore it still uses platform data entries for
> >>> pl08x dma channels
> >>> and 'SND_DMAENGINE_PCM_FLAG_NO_DT | SND_DMAENGINE_PCM_FLAG_COMPAT'
> >>> flags in the devm_snd_dmaengine_pcm_register().
> >>> Typically instead of this platform data you would use regular 'dma'
> >>> and 'dma-names' if it had
> >>> proper dmamux driver like lpc18xx-dmamux.c
> >>
> >> Exactly. Use these.
> >
> > Then I need to write a lpc32xx dma mux driver, device tree binding for
> > it and adjust the
> > LPC32xx I2S driver for it. Is this a hard requirement to accept this
> > patch set for the
> > legacy LPC32xx SoC?
>
> I do not see at all analogy with dma-names. dma-names are used ONLY by
> the consumer to pick up proper property "dmas" from DT. They are not
> passed to DMA code. They are not used to configure DMA provider at all.
>
> You parse string from DT and pass it further as DMA filtering code. This
> is abuse of hardware description for programming your driver and their
> dependencies.
>
> Why you cannot hard-code them?
>
> Sorry, to be clear: NAK

That's fine, clear answers are always good.
I considered to hardcode this as it was in the first version of the patch set
but LPC32XX has two I2S interfaces which use different DMA signals
and mux settings and I really didn't want to pick the virtual DMA channel
name based on hardcoded I2S node name therefore I thought having a DT
property to select proper dma channel is a better solution.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
new file mode 100644
index 000000000000..04a1090f70cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
@@ -0,0 +1,69 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/nxp,lpc3220-i2s.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP LPC32XX I2S Controller
+
+description:
+  The I2S controller in LPC32XX SoCs, ASoC DAI.
+
+maintainers:
+  - J.M.B. Downing <jonathan.downing@nautel.com>
+  - Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nxp,lpc3220-i2s
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: input clock of the peripheral.
+
+  dma-vc-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: |
+      names of virtual pl08x dma channels for tx and rx
+      directions in this order.
+    minItems: 2
+    maxItems: 2
+
+  "#sound-dai-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - dma-vc-names
+  - '#sound-dai-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/lpc32xx-clock.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2s@20094000 {
+      compatible = "nxp,lpc3220-i2s";
+      reg = <0x20094000 0x1000>;
+      interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&clk LPC32XX_CLK_I2S0>;
+      dma-vc-names = "i2s0-tx", "i2s0-rx";
+      #sound-dai-cells = <0>;
+    };
+
+...