Re: [PATCH v2 1/2] dt-binding: clock: cs2600: Add support for the CS2600

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/12/2024 12:02, Charles Keepax wrote:
> On Thu, Dec 19, 2024 at 09:51:00AM +0100, Krzysztof Kozlowski wrote:
>> On Wed, Dec 18, 2024 at 08:46:30PM -0600, Paul Handrigan wrote:
>>> +  clock-output-names:
>>> +    maxItems: 3
>>> +    description: Names for CLK_OUT, BCLK_OUT and FSYNC_OUT clocks.
>>> +
>>> +  cirrus,aux-output-source:
>>> +    description:
>>> +      Specifies the function of the auxiliary output pin
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum:
>>> +      - 0 # CS2600_AUX_OUTPUT_FREQ_UNLOCK: Frequency unlock notification
>>> +      - 1 # CS2600_AUX_OUTPUT_PHASE_UNLOCK: Phase unlock notification
>>> +      - 2 # CS2600_AUX_OUTPUT_NO_CLKIN: CLK_IN is not available
>>
>> I still do not understand how "clk_in", which is required, could be not
>> available. To me it contradicts itself, but maybe just description is a
>> bit incomplete.
> 
> I think yeah the description perhaps needs to make this more
> clear this is setting an error output pin, ie. the pin goes high
> for the linked error condition.
> 
>>> +/* CS2600 Auxiliary Output */
>>> +#define CS2600_AUX_OUTPUT_FREQ_UNLOCK	0
>>> +#define CS2600_AUX_OUTPUT_PHASE_UNLOCK	1
>>> +#define CS2600_AUX_OUTPUT_NO_CLKIN	2
>>
>> I still don't see why these three are supposed to be bindings. Drop
>> them.
> 
> In a binding one would presumably do:
> 
> cirrus,aux-output-source = <CS2600_AUX_OUTPUT_FREQ_UNLOCK>;
> 
> Apologies but I don't quite understand what you mean by the values
> are not used in the binding? The driver reads the property and sets

There is no user of these defines, so not a binding.

> the pin to have the appropriate function. Admittedly one could drop

It's not a proof that this is a binding.

> the defines and then DTS would just have to do:
> 
> cirrus,aux-output-source = <0>;
> 
> But that feels a bit less helpful when reading the binding.

Binding and being helpful are two different things. This to be the
binding, it has to be used as a binding, so some translation layer
between driver and DTS. It must have an user in DTS. I keep repeating
this over and over...

Best regards,
Krzysztof