| Message ID | 20240614163500.386747-2-piotr.wojtaszczyk@timesys.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add audio support for LPC32XX CPUs | expand |
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
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
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
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.
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
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 --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>; + }; + +...
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