| Message ID | 20230212232137.299005-1-alexander.sverdlin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions | expand |
On 13/02/2023 00:21, Alexander Sverdlin wrote: > Add device tree bindings for Cirrus Logic EP93xx internal SoCs' I2S and > AC'97 controllers. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > .../bindings/sound/cirrus,ep9301-ac97.yaml | 47 ++++++++++++++ > .../bindings/sound/cirrus,ep9301-i2s.yaml | 63 +++++++++++++++++++ > MAINTAINERS | 1 + > 3 files changed, 111 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml > create mode 100644 Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml > > diff --git a/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml b/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml > new file mode 100644 > index 000000000000..bfa5d392f525 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml > @@ -0,0 +1,47 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/cirrus,ep9301-ac97.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus EP93xx AC'97 Controller > + > +description: | > + The AC’97 Controller includes a 5-pin serial interface to an external audio > + codec. > + > +maintainers: > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + missing ref to dai-common.yaml > +properties: > + compatible: > + const: cirrus,ep9301-ac97 > + (...) > diff --git a/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml b/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml > new file mode 100644 > index 000000000000..b47d28dc8389 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml > @@ -0,0 +1,63 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/cirrus,ep9301-i2s.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus EP93xx I2S Controller > + > +description: | > + The I2S controller is used to stream serial audio data between the external > + I2S CODECs’, ADCs/DACs, and the ARM Core. The controller supports I2S, Left- > + and Right-Justified DSP formats. > + > +maintainers: > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + ref to dai-common > +properties: > + compatible: > + const: cirrus,ep9301-i2s > + > + '#sound-dai-cells': > + const: 0 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + minItems: 3 maxItems instead > + > + clock-names: > + items: > + - const: mclk > + - const: sclk > + - const: lrclk The clk suffixes are quite redundant. Don't these inputs have some meaningful name? > + > +required: > + - compatible > + - '#sound-dai-cells' > + - reg > + - interrupts > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + i2s: i2s@80820000 { > + compatible = "cirrus,ep9301-i2s"; > + #sound-dai-cells = <0>; > + reg = <0x80820000 0x100>; > + interrupt-parent = <&vic1>; > + interrupts = <28>; > + clocks = <&syscon 29 > + &syscon 30 > + &syscon 31>; That's one phandle, but you want three, so <>, <>, <>. Best regards, Krzysztof
Hello Krzysztof, thank you for the quick review! On Tue, 2023-02-14 at 11:58 +0100, Krzysztof Kozlowski wrote: > > +properties: > > + compatible: > > + const: cirrus,ep9301-i2s > > + > > + '#sound-dai-cells': > > + const: 0 > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + minItems: 3 > > maxItems instead reg and clocks are required, I suppose I should include both minItems and maxItems for both of them? > > + > > + clock-names: > > + items: > > + - const: mclk > > + - const: sclk > > + - const: lrclk > > > The clk suffixes are quite redundant. Don't these inputs have some > meaningful name? They are actually meaningful, as they are usually named in I2S, please refer to the EP93xx User's Guide: https://cdn.embeddedts.com/resource-attachments/ts-7000_ep9301-ug.pdf page 71, for instance.
On 14/02/2023 14:02, Alexander Sverdlin wrote: > Hello Krzysztof, > > thank you for the quick review! > > On Tue, 2023-02-14 at 11:58 +0100, Krzysztof Kozlowski wrote: >>> +properties: >>> + compatible: >>> + const: cirrus,ep9301-i2s >>> + >>> + '#sound-dai-cells': >>> + const: 0 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: >>> + minItems: 3 >> >> maxItems instead > > reg and clocks are required, I suppose I should include both minItems > and maxItems for both of them? No. minItems is implied. > >>> + >>> + clock-names: >>> + items: >>> + - const: mclk >>> + - const: sclk >>> + - const: lrclk >> >> >> The clk suffixes are quite redundant. Don't these inputs have some >> meaningful name? > > They are actually meaningful, as they are usually named in I2S, please > refer to the EP93xx User's Guide: > https://cdn.embeddedts.com/resource-attachments/ts-7000_ep9301-ug.pdf > page 71, for instance. OK, but then I like the example - if datasheet would use name "clk_clk_this_is_clk" would you still find it meaningful? Every clock input in clocks is a clock. There is usually no need to say that a clock is a clock... > Best regards, Krzysztof
Hello Krzysztof! On Tue, 2023-02-14 at 15:11 +0100, Krzysztof Kozlowski wrote: > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + minItems: 3 > > > > > > maxItems instead > > > > reg and clocks are required, I suppose I should include both minItems > > and maxItems for both of them? > > No. minItems is implied. I see. But I think minItems "3" should be included for clocks, right? > > > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: mclk > > > > + - const: sclk > > > > + - const: lrclk > > > > > > > > > The clk suffixes are quite redundant. Don't these inputs have some > > > meaningful name? > > > > They are actually meaningful, as they are usually named in I2S, please > > refer to the EP93xx User's Guide: > > https://cdn.embeddedts.com/resource-attachments/ts-7000_ep9301-ug.pdf > > page 71, for instance. > > OK, but then I like the example - if datasheet would use name > "clk_clk_this_is_clk" would you still find it meaningful? > > Every clock input in clocks is a clock. There is usually no need to say > that a clock is a clock... I see you point, but this is legacy code (sound/soc/cirrus/ep93xx-i2s.c) which took these names back in platform data times... I also see that rather majority of *i2s*.yaml use something "*clk", so maybe it could be accepted for legacy code?
On Tue, Feb 14, 2023 at 03:26:14PM +0100, Alexander Sverdlin wrote: > On Tue, 2023-02-14 at 15:11 +0100, Krzysztof Kozlowski wrote: > > OK, but then I like the example - if datasheet would use name > > "clk_clk_this_is_clk" would you still find it meaningful? > > Every clock input in clocks is a clock. There is usually no need to say > > that a clock is a clock... > I see you point, but this is legacy code (sound/soc/cirrus/ep93xx-i2s.c) > which took these names back in platform data times... I also see that > rather majority of *i2s*.yaml use something "*clk", so maybe it could > be accepted for legacy code? Even ignoring the whole legacy thing these are industry standard names for the clocks - they are pretty much universally named and referred to with the clk suffix. I can't see what removing it would accomplish other than reducing clarity.
On Tue, Feb 14, 2023 at 03:02:02PM +0000, Mark Brown wrote: > On Tue, Feb 14, 2023 at 03:26:14PM +0100, Alexander Sverdlin wrote: > > On Tue, 2023-02-14 at 15:11 +0100, Krzysztof Kozlowski wrote: > > > > OK, but then I like the example - if datasheet would use name > > > "clk_clk_this_is_clk" would you still find it meaningful? > > > > Every clock input in clocks is a clock. There is usually no need to say > > > that a clock is a clock... > > > I see you point, but this is legacy code (sound/soc/cirrus/ep93xx-i2s.c) > > which took these names back in platform data times... I also see that > > rather majority of *i2s*.yaml use something "*clk", so maybe it could > > be accepted for legacy code? > > Even ignoring the whole legacy thing these are industry standard > names for the clocks - they are pretty much universally named and > referred to with the clk suffix. I can't see what removing it > would accomplish other than reducing clarity. Agreed. If a clock is called "Nclk" then I think that is fine. If it is foo_bar_clk, then yes, 'foo_bar' is preferred. Rob
On 14/02/2023 15:26, Alexander Sverdlin wrote: > Hello Krzysztof! > > On Tue, 2023-02-14 at 15:11 +0100, Krzysztof Kozlowski wrote: >>>>> + interrupts: >>>>> + maxItems: 1 >>>>> + >>>>> + clocks: >>>>> + minItems: 3 >>>> >>>> maxItems instead >>> >>> reg and clocks are required, I suppose I should include both minItems >>> and maxItems for both of them? >> >> No. minItems is implied. > > I see. But I think minItems "3" should be included for clocks, right? No, why? It is implied when you provide maxItems. Therefore why would you need it to make it explicitly (although you can, but we avoid it for smaller code)? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml b/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml new file mode 100644 index 000000000000..bfa5d392f525 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml @@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/cirrus,ep9301-ac97.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Cirrus EP93xx AC'97 Controller + +description: | + The AC’97 Controller includes a 5-pin serial interface to an external audio + codec. + +maintainers: + - Alexander Sverdlin <alexander.sverdlin@gmail.com> + +properties: + compatible: + const: cirrus,ep9301-ac97 + + '#sound-dai-cells': + const: 0 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - '#sound-dai-cells' + - reg + - interrupts + +additionalProperties: false + +examples: + - | + ac97: ac97@80880000 { + compatible = "cirrus,ep9301-ac97"; + #sound-dai-cells = <0>; + reg = <0x80880000 0xac>; + interrupt-parent = <&vic0>; + interrupts = <6>; + }; + +... diff --git a/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml b/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml new file mode 100644 index 000000000000..b47d28dc8389 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/cirrus,ep9301-i2s.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Cirrus EP93xx I2S Controller + +description: | + The I2S controller is used to stream serial audio data between the external + I2S CODECs’, ADCs/DACs, and the ARM Core. The controller supports I2S, Left- + and Right-Justified DSP formats. + +maintainers: + - Alexander Sverdlin <alexander.sverdlin@gmail.com> + +properties: + compatible: + const: cirrus,ep9301-i2s + + '#sound-dai-cells': + const: 0 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + minItems: 3 + + clock-names: + items: + - const: mclk + - const: sclk + - const: lrclk + +required: + - compatible + - '#sound-dai-cells' + - reg + - interrupts + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + i2s: i2s@80820000 { + compatible = "cirrus,ep9301-i2s"; + #sound-dai-cells = <0>; + reg = <0x80820000 0x100>; + interrupt-parent = <&vic1>; + interrupts = <28>; + clocks = <&syscon 29 + &syscon 30 + &syscon 31>; + clock-names = "mclk", "sclk", "lrclk"; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 254aab631fd4..765902756c37 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2092,6 +2092,7 @@ M: Alexander Sverdlin <alexander.sverdlin@gmail.com> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Maintained F: Documentation/devicetree/bindings/iio/adc/cirrus,ep9301-adc.yaml +F: Documentation/devicetree/bindings/sound/cirrus,ep9301-* F: arch/arm/mach-ep93xx/ F: arch/arm/mach-ep93xx/include/mach/ F: drivers/iio/adc/ep93xx_adc.c
Add device tree bindings for Cirrus Logic EP93xx internal SoCs' I2S and AC'97 controllers. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> --- .../bindings/sound/cirrus,ep9301-ac97.yaml | 47 ++++++++++++++ .../bindings/sound/cirrus,ep9301-i2s.yaml | 63 +++++++++++++++++++ MAINTAINERS | 1 + 3 files changed, 111 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cirrus,ep9301-ac97.yaml create mode 100644 Documentation/devicetree/bindings/sound/cirrus,ep9301-i2s.yaml