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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_U6fUGbOV2SdO_C@pengutronix.de>
Date: Tue, 8 Apr 2025 17:02:21 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	kernel@...gutronix.de,
	Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH 3/3] dt-bindings: clock: add TI CDCE6214 binding

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?


> > +  "^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?

> 
> > +
> > +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?

> 
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        clock-generator@67 {
> > +            compatible = "ti,cdce6214";
> > +            reg = <0x67>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            #clock-cells = <1>;
> > +            clocks = <&clock_ref25m>;
> > +            clock-names = "priref";
> > +
> > +            clk@...E6214_CLK_SECREF {
> 
> That's not a valid unit address. Use simple numbers, see DT spec and DTS
> coding style.

CDCE6214_CLK_SECREF is a macro added in dt-bindings/clock/ti,cdce6214.h
and it expands to a simple number (1 in this case). While I haven't
found any examples of someone using macros for the unit address / reg
property I thought I'd give it a try as it nicely shows how it is used.

I can switch to plain numbers if you prefer that though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |