Re: [PATCH 1/2] dt-bindings: clk: si521xx: Add Skyworks Si521xx I2C PCIe clock generators

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 16/12/2022 22:21, Marek Vasut wrote:
> Add binding for Skyworks Si521xx PCIe clock generators. This binding
> is designed to support Si52144/Si52146/Si52147 series I2C PCIe clock
> generators, tested model is Si52144. It should be possible to add
> Si5213x series as well.
> 
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> ---
>  .../bindings/clock/skyworks,si521xx.yaml      | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/skyworks,si521xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/skyworks,si521xx.yaml b/Documentation/devicetree/bindings/clock/skyworks,si521xx.yaml
> new file mode 100644
> index 0000000000000..c30114e3a7631
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/skyworks,si521xx.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/skyworks,si521xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for Skyworks Si521xx I2C PCIe clock generators

Drop "Binding for"


> +
> +description: |
> +  The Skyworks Si521xx are I2C PCIe clock generators providing
> +  from 4 to 9 output clocks.
> +
> +maintainers:
> +  - Marek Vasut <marex@xxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - skyworks,si52144
> +      - skyworks,si52146
> +      - skyworks,si52147
> +
> +  reg:
> +    description: I2C device address

Drop description. It seems you copied some old binding. Better to start
from something recent or example-schema.

> +    const: 0x6b
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clocks:
> +    items:
> +      - description: XTal input clock
> +
> +  skyworks,out-amplitude-microvolt:
> +    enum: [ 300000, 400000, 500000, 600000, 700000, 800000, 900000, 1000000 ]
> +    description: Output clock signal amplitude
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* 25MHz reference crystal */
> +    ref25: ref25m {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-frequency = <25000000>;
> +    };

Drop this part, it's obvious. Example should show how to use this
device, not fixed clocks. It is a common pattern appearing in many DTSes.

> +
> +    i2c@0 {
> +        reg = <0x0 0x100>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        clock-generator@6b {
> +            compatible = "skyworks,si52144";
> +            reg = <0x6b>;
> +            #clock-cells = <1>;
> +
> +            clocks = <&ref25m>;
> +        };
> +    };
> +
> +...

Best regards,
Krzysztof