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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Tue, Jun 30, 2026 at 04:55:47PM +0100, Richard Fitzgerald wrote:
> From: Paul Handrigan <paulha@opensource.cirrus.com>
> 
> 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?


> 
> Signed-off-by: Paul Handrigan <paulha@opensource.cirrus.com>
> Co-developed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> 
> 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@linaro.org>
>  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.

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.

Best regards,
Krzysztof