Re: Re: [PATCH 1/1] dt-bindings: clock: ti,cdce925: Convert to DT schema

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Krzysztof,

thanks for your review.

Am Dienstag, 18. Oktober 2022, 15:51:35 CEST schrieb Krzysztof Kozlowski:
> On 18/10/2022 03:21, Alexander Stein wrote:
> > Convert the TI CDCE925 clock binding to DT schema format.
> > Including a small fix: Add the missing 'ti' prefix in the example
> > compatible.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > diff --git a/Documentation/devicetree/bindings/clock/ti,cdce925.yaml
> > b/Documentation/devicetree/bindings/clock/ti,cdce925.yaml new file mode
> > 100644
> > index 000000000000..1e68ee68e458
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/ti,cdce925.yaml
> > @@ -0,0 +1,104 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ti,cdce925.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI CDCE913/925/937/949 programmable I2C clock synthesizers node
> > bindings
> Drop "node bindings"

Thanks, will do so.

> > +
> > +maintainers:
> > +  - Mike Looijmans <mike.looijmans@topic.nl>
> > +
> > +description: |
> > +  Flexible Low Power LVCMOS Clock Generator with SSC Support for EMI
> > Reduction +
> > +  - CDCE(L)913: 1-PLL, 3 Outputs https://www.ti.com/product/cdce913
> > +  - CDCE(L)925: 2-PLL, 5 Outputs https://www.ti.com/product/cdce925
> > +  - CDCE(L)937: 3-PLL, 7 Outputs https://www.ti.com/product/cdce937
> > +  - CDCE(L)949: 4-PLL, 9 Outputs https://www.ti.com/product/cdce949
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^clock-controller$"
> 
> Drop this requirement. It is in general expected, but there is no need
> for each binding to specify it.

Should this be put in a common binding then?

> Other problem is that you did not actually test these bindings before
> sending...
> 
> > +
> > +  compatible:
> > +    enum:
> > +      - ti,cdce913
> > +      - ti,cdce925
> > +      - ti,cdce937
> > +      - ti,cdce949
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: fixed parent clock
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  vdd-supply:
> > +    description: Regulator that provides 1.8V Vdd power supply
> > +
> > +  vddout-supply:
> > +    description: |
> > +      Regulator that provides Vddout power supply.
> > +      non-L variant: 2.5V or 3.3V for
> > +      L variant: 1.8V for
> > +
> > +  xtal-load-pf:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Crystal load-capacitor value to fine-tune performance on a
> > +      board, or to compensate for external influences.
> > +
> > +patternProperties:
> > +  "^PLL[1-4]$":
> > +    type: object
> > +    description: |
> > +      optional child node can be used to specify spread
> > +      spectrum clocking parameters for a board
> > +
> 
>     additionalProperties: false

Will do.

Thanks and best regards,
Alexander

> > +    properties:
> > +      spread-spectrum:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: SSC mode as defined in the data sheet
> > +
> > +      spread-spectrum-center:
> > +        type: boolean
> > +        description: |
> > +          Use "centered" mode instead of "max" mode. When
> > +          present, the clock runs at the requested frequency on average.
> > +          Otherwise the requested frequency is the maximum value of the
> > +          SCC range.
> > +
> 
> Best regards,
> Krzysztof