Re: [PATCH 3/3] dt-bindings: clock: add TI CDCE6214 binding

[Rob Herring <robh@kernel.org>]

On Tue, Apr 08, 2025 at 05:02:21PM +0200, Sascha Hauer wrote:
> On Tue, Apr 08, 2025 at 04:27:23PM +0200, Krzysztof Kozlowski wrote:
> > On 08/04/2025 14:00, Sascha Hauer wrote:
> > > +
> > 
> > A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> > prefix is already stating that these are bindings.
> > See also:
> > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> > 
> > 
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - ti,cdce6214
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    minItems: 1
> > > +    items:
> > > +      - const: priref
> > > +      - const: secref
> > 
> > So one input is optional?
> 
> The chip has two clock inputs and to be operational it needs at least
> one clock, could be priref or secref or both.
> 
> Is there a proper way to express this situation?

If I understand correctly that only 'secref' is possible then you want:

items:
  - enum: [ priref, secref ]
  - const: secref

(By default, entries have to be unique, so that eliminates 'secref' in 
both)

> 
> 
> > > +  "^clk@[2-9]$":
> > > +    type: object
> > > +    description: |
> > > +      optional child node that can be used to specify output pin parameters.  The reg
> > > +      properties match the CDCE6214_CLK_* defines.
> > > +
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description:
> > > +          clock output identifier.
> > > +        minimum: 2
> > > +        maximum: 9
> > > +
> > > +      ti,lphcsl:
> > > +        type: boolean
> > > +        description: |
> > > +          If true enable LP-HCSL output mode for this clock
> > > +
> > > +      ti,lvds:
> > > +        type: boolean
> > > +        description: |
> > > +          If true enable LVDS output mode for this clock
> > > +
> > > +      ti,cmosp:
> > > +        type: boolean
> > > +        description: |
> > > +          If true enable CMOSP output for this clock
> > > +
> > > +      ti,cmosn:
> > > +        type: boolean
> > > +        description: |
> > > +          If true enable CMOSN output for this clock
> > 
> > Looks the same here. Anyway having these as subnodes is too much. You
> > have fixed number of clocks, so you need one or two array properties in
> > top-level.
> 
> There are several properties I haven't yet modeled, like
> 
> - 1.8V / 2.5V output
> - sync_delay
> - LVDS common-mode trim increment/decrement
> - differential buffer BIAS trim
> - slew rate
> - BIAS current setting for XTAL mode
> - load capacity for XTAL mode
> 
> I don't know which of them will ever be supported, but I thought having a
> node per pin would add a natural place to add these properties. Do you
> still think arrays would be more appropriate?

Assuming they are connected to something in DT (if not, why care), you 
could add a flags cell so the consumer side can define what they need.

> 
> > 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - "#clock-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/ti,cdce6214.h>
> > 
> > This file does not exist. Something is odd in this example.
> 
> It is added in the driver patch. Should it come with the binding patch
> instead?

Yes.

Rob