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

[Richard Fitzgerald <rf@opensource.cirrus.com>]

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

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@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.

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
>