Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/02/2023 14:13, Biju Das wrote:
> Document Renesas versa3 clock generator(5P35023) bindings.
> 
> The 5P35023 is a VersaClock programmable clock generator and
> is designed for low-power, consumer, and high-performance PCI
> Express applications. The 5P35023 device is a three PLL
> architecture design, and each PLL is individually programmable
> and allowing for up to 6 unique frequency outputs.
> 
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
>  .../bindings/clock/renesas,versaclock3.yaml   | 135 ++++++++++++++++++
>  1 file changed, 135 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> new file mode 100644
> index 000000000000..f45b8da73ec3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml

Filename usually is based on the compatible. Why these two are so different?


> @@ -0,0 +1,135 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,versaclock3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas VersaClock 3 programmable I2C clock generators
> +
> +description: |
> +  The 5P35023 is a VersaClock programmable clock generator and
> +  is designed for low-power, consumer, and high-performance PCI
> +  express applications. The 5P35023 device is a three PLL
> +  architecture design, and each PLL is individually programmable
> +  and allowing for up to 6 unique frequency outputs.
> +
> +  An internal OTP memory allows the user to store the configuration
> +  in the device. After power up, the user can change the device register
> +  settings through the I2C interface when I2C mode is selected.
> +
> +  The driver can read a full register map from the DT, and will use that
> +  register map to initialize the attached part (via I2C) when the system
> +  boots. Any configuration not supported by the common clock framework
> +  must be done via the full register map, including optimized settings.
> +
> +  Link to datasheet: https://www.renesas.com/us/en/products/clocks-timing/
> +                     clock-generation/programmable-clocks/
> +                     5p35023-versaclock-3s-programmable-clock-generator

I think link should not be wrapped. Start in next line and just make it
long.

While touching this, please keep the same order of entries as in
example-schema, so maintainers go after title.

> +
> +maintainers:
> +  - Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,5p35023
> +
> +  reg:
> +    description: I2C device address

Drop description, it's obvious.

> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clock-names:
> +    oneOf:
> +      - items:
> +          - const: x1
> +      - items:
> +          - const: clkin

This should be specific, not one or another. Why do you have two
entirely different clock inputs?

(and if this stays, then just items: - enum: [])


> +
> +  clocks:
> +    maxItems: 1
> +
> +  renesas,settings:
> +    description: Optional, complete register map of the device.
> +      Optimized settings for the device must be provided in full
> +      and are written during initialization.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    minItems: 37

maxItems instead... but I am not sure that we allow register settings in
DT in general.

> +
> +  assigned-clocks:
> +    minItems: 6

Drop.

> +
> +  assigned-clock-rates:
> +    minItems: 6

Drop.

> +
> +  renesas,clock-divider-read-only:
> +    description: Flag for setting divider in read only mode.

Flag? Then type: boolean.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5

This is broken...

> +
> +  renesas,clock-flags:
> +    description: Flags used in common clock frame work for configuring
> +      clk outputs. See include/linux/clk-provider.h

These are not bindings, so why do you non-bindings constants as
bindings? They can change anytime. Choose one:
1. Drop entire property,
2. Make it a proper binding property, so an ABI and explain why this is
DT specific. None of clock providers have to do it, so you need here
good explanation.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 6

maxItems instead

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* 24MHz crystal */
> +    x1_x2: xtal {
> +      compatible = "fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency = <24000000>;
> +    };

Drop this part, obvious.

> +
> +    i2c@0 {
> +        reg = <0x0 0x100>;

Just i2c { and drop reg

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        versa3: clock-generator@68 {
> +            compatible = "renesas,5p35023";
> +            reg = <0x68>;
> +            #clock-cells = <1>;
> +
> +            clocks = <&x1_x2>;
> +            clock-names = "x1";
> +
> +            renesas,settings = [
> +                80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> +                00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
> +                80 b0 45 c4 95
> +            ];
> +
> +            assigned-clocks = <&versa3 0>,
> +                              <&versa3 1>,
> +                              <&versa3 2>,
> +                              <&versa3 3>,
> +                              <&versa3 4>,
> +                              <&versa3 5>;
> +            assigned-clock-rates = <12288000>, <25000000>,
> +                                   <12000000>, <11289600>,
> +                                   <11289600>, <24000000>;
> +            renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
> +            renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
> +                                  <2176>, <2048>;
> +        };
> +    };
> +
> +    /* Consumer referencing the versa 3 */
> +    consumer {
> +        /* ... */
> +        clocks = <&versa3 3>;
> +        /* ... */

Drop consumer. Do you see it anywhere else?

Best regards,
Krzysztof




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux