[PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Alexander Sverdlin]

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
-- 
2.39.1

[PATCH 2/2] ASoC: ep93xx: Add OF support

[Alexander Sverdlin]

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,
 	},
 };
 
-- 
2.39.1

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Alexander Sverdlin]

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.

-- 
Alexander Sverdlin.

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Alexander Sverdlin]

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?
 
-- 
Alexander Sverdlin.

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Rob Herring]

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

Re: [PATCH 1/2] dt-bindings: sound: ep93xx: Add I2S and AC'97 descriptions

[Krzysztof Kozlowski]

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