[PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions
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 + +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 the new cirrus,ep9301-ac97 and cirrus,ep9301-i2s compatibles.
Signed-off-by: Alexander Sverdlin alexander.sverdlin@gmail.com --- sound/soc/cirrus/ep93xx-ac97.c | 8 ++++++++ sound/soc/cirrus/ep93xx-i2s.c | 8 ++++++++ 2 files changed, 16 insertions(+)
diff --git a/sound/soc/cirrus/ep93xx-ac97.c b/sound/soc/cirrus/ep93xx-ac97.c index 37593abe6053..fd7f6b3a5317 100644 --- a/sound/soc/cirrus/ep93xx-ac97.c +++ b/sound/soc/cirrus/ep93xx-ac97.c @@ -12,6 +12,7 @@ #include <linux/io.h> #include <linux/init.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/slab.h>
@@ -430,11 +431,18 @@ static int ep93xx_ac97_remove(struct platform_device *pdev) return 0; }
+static const struct of_device_id ep93xx_ac97_of_ids[] = { + { .compatible = "cirrus,ep9301-ac97" }, + {} +}; +MODULE_DEVICE_TABLE(of, ep93xx_ac97_of_ids); + static struct platform_driver ep93xx_ac97_driver = { .probe = ep93xx_ac97_probe, .remove = ep93xx_ac97_remove, .driver = { .name = "ep93xx-ac97", + .of_match_table = ep93xx_ac97_of_ids, }, };
diff --git a/sound/soc/cirrus/ep93xx-i2s.c b/sound/soc/cirrus/ep93xx-i2s.c index f41712df7994..3c6fa8e23c52 100644 --- a/sound/soc/cirrus/ep93xx-i2s.c +++ b/sound/soc/cirrus/ep93xx-i2s.c @@ -15,6 +15,7 @@ #include <linux/slab.h> #include <linux/clk.h> #include <linux/io.h> +#include <linux/of.h>
#include <sound/core.h> #include <sound/dmaengine_pcm.h> @@ -505,11 +506,18 @@ static int ep93xx_i2s_remove(struct platform_device *pdev) return 0; }
+static const struct of_device_id ep93xx_i2s_of_ids[] = { + { .compatible = "cirrus,ep9301-i2s" }, + {} +}; +MODULE_DEVICE_TABLE(of, ep93xx_i2s_of_ids); + static struct platform_driver ep93xx_i2s_driver = { .probe = ep93xx_i2s_probe, .remove = ep93xx_i2s_remove, .driver = { .name = "ep93xx-i2s", + .of_match_table = ep93xx_i2s_of_ids, }, };
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
participants (4)
-
Alexander Sverdlin -
Krzysztof Kozlowski -
Mark Brown -
Rob Herring