diff mbox series

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

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

Commit Message

Alexander Sverdlin Feb. 12, 2023, 11:21 p.m. UTC
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

Comments

Krzysztof Kozlowski Feb. 14, 2023, 10:58 a.m. UTC | #1
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
Alexander Sverdlin Feb. 14, 2023, 1:02 p.m. UTC | #2
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.
Krzysztof Kozlowski Feb. 14, 2023, 2:11 p.m. UTC | #3
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
Alexander Sverdlin Feb. 14, 2023, 2:26 p.m. UTC | #4
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?
Mark Brown Feb. 14, 2023, 3:02 p.m. UTC | #5
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.
Rob Herring (Arm) Feb. 15, 2023, 7:16 p.m. UTC | #6
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
Krzysztof Kozlowski Feb. 15, 2023, 7:19 p.m. UTC | #7
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 mbox series

Patch

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