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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

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
the pin to have the appropriate function. Admittedly one could drop
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.

Thanks,
Charles