RE: [PATCH v2 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> Subject: Re: [PATCH v2 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 09/03/2023 17:55, 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.
> >
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +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: |
> 
> | is not correct here

OK will remove.

> 
> > +
> > + https://www.renesas.com/us/en/products/clocks-timing/clock-generatio
> > + n/programmable-clocks/5p35023-versaclock-3s-programmable-clock-gener
> > + ator
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,5p35023
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  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
> > +    maxItems: 37
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Stray blank line, drop.

Agreed. Will send next version based on feedback for the driver patch and reviews
from others.

Cheers,
Biju
> 
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> 
> With both above:
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> 
> Best regards,
> Krzysztof