diff mbox series

[1/2] dt-binding: clock: cs2600: Add support for the CS2600

Message ID 20241211003236.2523604-2-paulha@opensource.cirrus.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Cirrus Logic CS2600 clock device | expand

Commit Message

Paul Handrigan Dec. 11, 2024, 12:32 a.m. UTC
Add device tree support for the Cirrus Logic CS2600 clock
device.

Signed-off-by: Paul Handrigan <paulha@opensource.cirrus.com>
---
 .../bindings/clock/cirrus,cs2600.yaml         | 78 +++++++++++++++++++
 include/dt-bindings/clock/cirrus,cs2600.h     | 23 ++++++
 2 files changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
 create mode 100644 include/dt-bindings/clock/cirrus,cs2600.h

Comments

Krzysztof Kozlowski Dec. 13, 2024, 10:55 a.m. UTC | #1
On Tue, Dec 10, 2024 at 06:32:35PM -0600, Paul Handrigan wrote:
> Add device tree support for the Cirrus Logic CS2600 clock
> device.
> 
> Signed-off-by: Paul Handrigan <paulha@opensource.cirrus.com>
> ---
>  .../bindings/clock/cirrus,cs2600.yaml         | 78 +++++++++++++++++++
>  include/dt-bindings/clock/cirrus,cs2600.h     | 23 ++++++
>  2 files changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
>  create mode 100644 include/dt-bindings/clock/cirrus,cs2600.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml b/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
> new file mode 100644
> index 000000000000..ed23f347f382
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/cirrus,cs2600.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic Fractional-N Clock Synthesizer & Clock Multiplier
> +
> +maintainers:
> +  - Paul Handrigan <paulha@opensource.cirrus.com>
> +  - patches@opensource.cirrus.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The CS2600 is a system-clocking device that enables frequency synthesis and
> +  clock generation from a stable timing reference clock. The device can generate
> +  low-jitter clocks from a noisy clock reference at frequencies as low as 50 Hz.

Be sure these are wrapped at 80 (looks like 81).

> +
> +properties:
> +  compatible:
> +    enum:
> +      - cirrus,cs2600
> +
> +  clocks:
> +    description:
> +      Common clock binding for XTI/REF_CLK, CLK_IN

Just drop description, kind of obvious from the clock-names. If you want
to keep it, then rather list the items with description per each item.

> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: ref_clk

s/ref_clk/ref/
(or xti)

"clk" is just obvious.

> +      - const: clk_in
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Power Supply
> +
> +  cirrus,aux-output-source:
> +    description:
> +      Specifies the function of the auxiliary clock output pin
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 0 # CS2600_AUX_OUTPUT_FREQ_UNLOCK: Frequency unlock notification
> +      - 1 # CS2600_AUX_OUTPUT_PHASE_UNLOCK: Phase unlock notification
> +      - 2 # CS2600_AUX_OUTPUT_NO_CLKIN: CLK_IN is not available

I don't get the meanings - how clock output could be clock is not
available? clk_in is a required property, so it cannot be unavailable.


> +    default: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/cirrus,cs2600.h>
> +
> +    i2c@0 {

i2c {

> +      reg = <0x0 0x100>;

Drop reg

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      clock-controller@2c {
> +        #clock-cells = <1>;
> +        compatible = "cirrus,cs2600";
> +        reg = <0x2c>;
> +        clocks = <&xtl_clk>, <&sync_clock>;
> +        clock-names = "ref_clk", "clk_in";
> +        vdd-supply = <&vreg>;
> +      };
> +    };
> diff --git a/include/dt-bindings/clock/cirrus,cs2600.h b/include/dt-bindings/clock/cirrus,cs2600.h
> new file mode 100644
> index 000000000000..673b4b4cb1f4
> --- /dev/null
> +++ b/include/dt-bindings/clock/cirrus,cs2600.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * Copyright (c) 2024 Cirrus Logic, Inc. and
> + *		      Cirrus Logic International Simiconductor Ltd.
> + *
> + * Author: Paul Handrigan <paulha@opensource.cirrus.com>
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_CIRRUS_CS2600_H
> +#define _DT_BINDINGS_CLK_CIRRUS_CS2600_H
> +
> +/* CS2600 Clock Outputs  */
> +#define CS2600_CLK_OUTPUT		0

Drop "OUTPUT". These are names of the clocks.

> +#define CS2600_BCLK_OUTPUT		1
> +#define CS2600_FSYNC_OUTPUT		2
> +
> +/* CS2600 Auxiliary Output */
> +#define CS2600_AUX_OUTPUT_FREQ_UNLOCK	0
> +#define CS2600_AUX_OUTPUT_PHASE_UNLOCK	1
> +#define CS2600_AUX_OUTPUT_NO_CLKIN	2

Not sure what are these, but feel like some register values. Otherwise,
why these are exactly bindings? What part of driver do they bind?

Best regards,
Krzysztof
Paul Handrigan Dec. 16, 2024, 2:21 a.m. UTC | #2
On Fri, Dec 13, 2024 at 11:55:22AM +0100, Krzysztof Kozlowski wrote:
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
Ack

> 
> > +  The CS2600 is a system-clocking device that enables frequency synthesis and
> > +  clock generation from a stable timing reference clock. The device can generate
> > +  low-jitter clocks from a noisy clock reference at frequencies as low as 50 Hz.
> 
> Be sure these are wrapped at 80 (looks like 81).
Ack

> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - cirrus,cs2600
> > +
> > +  clocks:
> > +    description:
> > +      Common clock binding for XTI/REF_CLK, CLK_IN
> 
> Just drop description, kind of obvious from the clock-names. If you want
> to keep it, then rather list the items with description per each item.
Ack

> 
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ref_clk
> 
> s/ref_clk/ref/
> (or xti)
> 
> "clk" is just obvious.
Ack
> 
> > +  cirrus,aux-output-source:
> > +    description:
> > +      Specifies the function of the auxiliary clock output pin
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 0 # CS2600_AUX_OUTPUT_FREQ_UNLOCK: Frequency unlock notification
> > +      - 1 # CS2600_AUX_OUTPUT_PHASE_UNLOCK: Phase unlock notification
> > +      - 2 # CS2600_AUX_OUTPUT_NO_CLKIN: CLK_IN is not available
> 
> I don't get the meanings - how clock output could be clock is not
> available? clk_in is a required property, so it cannot be unavailable.
These are the values for the enum.  I see how this is confusing. I will
change it.

> > +      reg = <0x0 0x100>;
> 
> Drop reg
Ack

> 
> > +#define CS2600_CLK_OUTPUT		0
> 
> Drop "OUTPUT". These are names of the clocks.
Ack
> 
> > +#define CS2600_BCLK_OUTPUT		1
> > +#define CS2600_FSYNC_OUTPUT		2
> > +
> > +/* CS2600 Auxiliary Output */
> > +#define CS2600_AUX_OUTPUT_FREQ_UNLOCK	0
> > +#define CS2600_AUX_OUTPUT_PHASE_UNLOCK	1
> > +#define CS2600_AUX_OUTPUT_NO_CLKIN	2
> 
> Not sure what are these, but feel like some register values. Otherwise,
> why these are exactly bindings? What part of driver do they bind?
They are different outputs for the aux_out_source pin. I think it may be
best just to remove these defines.

Regards,
Paul
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml b/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
new file mode 100644
index 000000000000..ed23f347f382
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/cirrus,cs2600.yaml
@@ -0,0 +1,78 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/cirrus,cs2600.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus Logic Fractional-N Clock Synthesizer & Clock Multiplier
+
+maintainers:
+  - Paul Handrigan <paulha@opensource.cirrus.com>
+  - patches@opensource.cirrus.com>
+
+description: |
+  The CS2600 is a system-clocking device that enables frequency synthesis and
+  clock generation from a stable timing reference clock. The device can generate
+  low-jitter clocks from a noisy clock reference at frequencies as low as 50 Hz.
+
+properties:
+  compatible:
+    enum:
+      - cirrus,cs2600
+
+  clocks:
+    description:
+      Common clock binding for XTI/REF_CLK, CLK_IN
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: ref_clk
+      - const: clk_in
+
+  '#clock-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: Power Supply
+
+  cirrus,aux-output-source:
+    description:
+      Specifies the function of the auxiliary clock output pin
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 0 # CS2600_AUX_OUTPUT_FREQ_UNLOCK: Frequency unlock notification
+      - 1 # CS2600_AUX_OUTPUT_PHASE_UNLOCK: Phase unlock notification
+      - 2 # CS2600_AUX_OUTPUT_NO_CLKIN: CLK_IN is not available
+    default: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/cirrus,cs2600.h>
+
+    i2c@0 {
+      reg = <0x0 0x100>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      clock-controller@2c {
+        #clock-cells = <1>;
+        compatible = "cirrus,cs2600";
+        reg = <0x2c>;
+        clocks = <&xtl_clk>, <&sync_clock>;
+        clock-names = "ref_clk", "clk_in";
+        vdd-supply = <&vreg>;
+      };
+    };
diff --git a/include/dt-bindings/clock/cirrus,cs2600.h b/include/dt-bindings/clock/cirrus,cs2600.h
new file mode 100644
index 000000000000..673b4b4cb1f4
--- /dev/null
+++ b/include/dt-bindings/clock/cirrus,cs2600.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2024 Cirrus Logic, Inc. and
+ *		      Cirrus Logic International Simiconductor Ltd.
+ *
+ * Author: Paul Handrigan <paulha@opensource.cirrus.com>
+ *
+ */
+
+#ifndef _DT_BINDINGS_CLK_CIRRUS_CS2600_H
+#define _DT_BINDINGS_CLK_CIRRUS_CS2600_H
+
+/* CS2600 Clock Outputs  */
+#define CS2600_CLK_OUTPUT		0
+#define CS2600_BCLK_OUTPUT		1
+#define CS2600_FSYNC_OUTPUT		2
+
+/* CS2600 Auxiliary Output */
+#define CS2600_AUX_OUTPUT_FREQ_UNLOCK	0
+#define CS2600_AUX_OUTPUT_PHASE_UNLOCK	1
+#define CS2600_AUX_OUTPUT_NO_CLKIN	2
+
+#endif