Re: [PATCH v6 1/3] dt-bindings: clock: cs2600: Add support for the CS2600

[Richard Fitzgerald]

On 01/07/2026 7:48 am, Krzysztof Kozlowski wrote:
> On Tue, Jun 30, 2026 at 04:55:47PM +0100, Richard Fitzgerald wrote:
> > From: Paul Handrigan <paulha@xxxxxxxxxxxxxxxxxxxxx>
> >
> > Add device tree schema for the Cirrus Logic CS2600 clock generator.
> >
> > The majority of the schema is typical clock, power and I2C
> > properties.
> >
> > Passes dt_binding_check:
> > make dt_binding_check DT_SCHEMA_FILES=clock/cirrus,cs2600.yaml
> >    SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >    CHKDT   ./Documentation/devicetree/bindings
> >    LINT    ./Documentation/devicetree/bindings
> >    STYLE   ./Documentation/devicetree/bindings
> >    DTEX    Documentation/devicetree/bindings/clock/cirrus,cs2600.example.dts
> >    DTC [C] Documentation/devicetree/bindings/clock/cirrus,cs2600.example.dtb
>
> Why is this in the commit msg? What is its purpose? Do you see any of
> this in any commits?

Purpose is to inform people that I ran the check and it passes.
If you don't want that information I'm fine with that. I'll leave it out
in V7.

> > Signed-off-by: Paul Handrigan <paulha@xxxxxxxxxxxxxxxxxxxxx>
> > Co-developed-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Changes in V6:
> > - Fixed clock naming in descriptions to match datasheet.
> > - Fixed clock-names property values to match datasheet names.
> >
> > - Added cirrus,internal-oscillator boolean to flag that the internal
> >    oscillator is the clock source. Previously the driver inferred this
> >    if clock-names did not contain "ref_clk_in", but this made it difficult
> >    to enforce dts correctness because there was no way to tell whether
> >    ref_clk_in was intentionally or accidentally missing.
> >
> > - Changed the cirrus,clock-mode enum property into two booleans, since
> >    effectively it was two separate features masquerading as an enum:
> >    - cirrus,smart-mode present to enable smart mode.
> >    - cirrus,smart-mode-clkin-only to enable a feature where the output will
> >      be suppressed until both input clocks are present.
> >
> > - Changes to cirrus,aux-output-source property:
> >      - Renamed to cirrus,aux1-output-source because it's for the AUX1 pin.
> >      - Added more options.
> >      - Renamed the "no_clkin" option to "clkin_missing".
> >      - Reformatted the description as a list instead of one long sentence.
> >
> > - Changed clock-names from an enum to an ordered list of const.
> >    This implicitly ensures ref_clk_in is always required.
> >
> > - Added properties to invert bclk and fsync outputs.
> > - Added property cirrus,fsync-duty-cycles.
> > - Added #clock-cells and vdd-supply to the list of required properies.
> > - Rewritten description description section.
> > - Reordered the property list to put common properties before custom
> >    cirrus properties.
> > - Added more examples.
> > - Added header file to define the clock indexes for DT consumers of the
> >    CS2600 clocks.
> >
> >   Note:
> >   V5 was Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >   but I haven't carried this forward because the changes in V6 are large.
>
> Dropping my tag is right thing to do, but making significant changes to
> hardware at v6 is very odd. Bindings represent here the hardware, so how
> is that v5 had one view of hardware and the next revision rewrites it
> completely.
>
> Probably answer - v5 was heavily incomplete - but we do ask to make it
> complete in the first place (see writing bindings), so all my previous
> review was waste of time.

Unfortunately I can't do anything about previous versions. I apologize
on behalf of Cirrus that they could have been better. I've made what I
think are improvements in V6.

V5 was sent in Dec 2024. That's over a year to (a) find issues with V5
and (b) incorporate learnings from non-Linux products about how the
CS2600 will be used by manufacturers.

> This will wait for review.
>
> > +examples:
> > +  - |
> > +    /* Smart mode */
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      clock-controller@2c {
> > +        compatible = "cirrus,cs2600";
> > +        reg = <0x2c>;
> > +        #clock-cells = <1>;
> > +        clocks = <&xtl_clk>, <&sync_clock>;
> > +        clock-names = "ref_clk_in", "clk_in";
> > +        vdd-supply = <&vreg>;
> > +        cirrus,smart-mode;
> > +      };
> > +    };
> > +
>
> Two examples max.

That's an unfortunate rule. But if that's the rule I'll trim it
to 2.

> Best regards,
> Krzysztof