Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

From: Dragan Simic
Date: Mon Jul 15 2024 - 11:15:58 EST


Hello Heiko,

Please see some comments below.

On 2024-07-15 13:02, Heiko Stuebner wrote:
In contrast to fixed clocks that are described as ungateable, boards
sometimes use additional oscillators for things like PCIe reference
clocks, that need actual supplies to get enabled and enable-gpios to be
toggled for them to work.

This adds a binding for such oscillators that are not configurable
themself, but need to handle supplies for them to work.

In schematics they often can be seen as

----------------
Enable - | 100MHz,3.3V, | - VDD
| 3225 |
GND - | | - OUT
----------------

or similar. The enable pin might be separate but can also just be tied
to the vdd supply, hence it is optional in the binding.

Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
---
.../bindings/clock/voltage-oscillator.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644
Documentation/devicetree/bindings/clock/voltage-oscillator.yaml

diff --git
a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
new file mode 100644
index 0000000000000..8bff6b0fd582e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Voltage controlled oscillator

Frankly, I find the "voltage-oscillator" and "voltage controlled
oscillator" names awkward. In general, "clock" is used throughout
the entire kernel, when it comes to naming files and defining
"compatible" strings. Thus, I'd suggest that "clock" is used here
instead of "oscillator", because it's consistent and shorter.

How about using "gated-clock" for the "compatible" string, and
"Simple gated clock generator" instead of "voltage controlled
oscillator"? Besides sounding awkward, "voltage controlled
oscillator" may suggest that the clock generator can be adjusted
or programmed somehow by applying the voltage, while it can only
be enabled or disabled that way, which is by definition clock
gating. Thus, "gated-clock" and "Simple gated clock generator"
would fit very well.

+maintainers:
+ - Heiko Stuebner <heiko@xxxxxxxxx>
+
+properties:
+ compatible:
+ const: voltage-oscillator
+
+ "#clock-cells":
+ const: 0
+
+ clock-frequency: true
+
+ clock-output-names:
+ maxItems: 1
+
+ enable-gpios:
+ description:
+ Contains a single GPIO specifier for the GPIO that enables and disables
+ the oscillator.
+ maxItems: 1
+
+ vdd-supply:
+ description: handle of the regulator that provides the supply voltage
+
+required:
+ - compatible
+ - "#clock-cells"
+ - clock-frequency
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ voltage-oscillator {
+ compatible = "voltage-oscillator";
+ #clock-cells = <0>;
+ clock-frequency = <1000000000>;
+ vdd-supply = <&reg_vdd>;
+ };
+...