Message-ID: <CAPJMGm4=sRQGPmVi8NjAVvOVrr8s2By6PO8kKRKZt3W0FR9j-Q@mail.gmail.com>
Date:   Wed, 31 May 2023 08:59:53 +0200
From:   Fabrizio Lamarque <fl.scratchpad@...il.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     jic23@...nel.org, Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Alexandru Tachici <alexandru.tachici@...log.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 5/5] dt-bindings: iio: ad7192: Allow selection of clock modes

On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@...nel.org> wrote:
>
> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@...il.com wrote:
> > From: Fabrizio Lamarque <fl.scratchpad@...il.com>
> >
> > AD7192 supports external clock sources, generated by a digital clock
> > source or a crystal oscillator, or internally generated clock option
> > without external components.
> >
> > Describe choice between internal and external clock, crystal or external
> > oscillator, and internal clock output enable.
> >
> > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@...il.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > index 16def2985ab4..f7ecfd65ad80 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > @@ -32,7 +32,8 @@ properties:
> >
> >    clocks:
> >      maxItems: 1
> > -    description: phandle to the master clock (mclk)
> > +    description: |
> > +      Master clock (mclk). If not set, internal clock is used.
> >
> >    clock-names:
> >      items:
> > @@ -50,6 +51,17 @@ properties:
> >    vref-supply:
> >      description: VRef voltage supply
> >
> > +  adi,clock-xtal:
> > +    description: |
> > +      Select whether an external crystal oscillator or an external
> > +      clock is applied as master (mclk) clock.
> > +    type: boolean
>
> Am I being daft, or are these the same thing? If they are not, and use
> different input pins, I think it should be explained as it not clear.
> Could you explain why we actually care that the source is a xtal versus
> it being mclk, and why just having master clock is not sufficient?

I may revise the description as follows. Feel free to add your suggestions
in case it is still not clear enough.

"Select whether an external crystal oscillator between MCLK1 and MCLK2 or
an external CMOS-compatible clock on MCLK2 is used as master clock".

This is used to properly set CLK0 and CLK1 bits in the MODE register.
I guess most applications would use an external crystal or internal clock.
The external digital clock would allow synchronization of multiple ADCs,

>
> > +  adi,int-clock-output-enable:
> > +    description: |
> > +      When internal clock is selected, this bit enables clock out pin.
> > +    type: boolean
>
> And this one makes you a clock provider, so the devices advocate
> position would be that you know that this bit should be set if
> "clocks" is not present and a consumer requests a clock.
> I don't seem to have got the driver patches (at least not in this
> mailbox), so I have got no information on how you've actually implemented
> this.

I see... When this bit is set, the AD7192 node should also be a clock provider.
The clock is output on MCLK2 pin, hence it can be used with internally
generated clock only.
I tend to dislike the idea of a "conditional clock provider". Also, I'd guess
there is a very limited usage of a low precision clock output for
synchronization purposes between multiple ADCs. In the remote case,
I would rather use a precise, dedicated external digital clock.
Would you agree if I remove the related lines from the change set?
If not, I kindly ask for your suggestions.

The existing implementation from AD already includes all these
configurations (there are no driver patches, the proposed changes are
just related to documentation).

Thank you!
Fabrizio

>
> Cheers,
> Conor.
>
> > +
> >    adi,rejection-60-Hz-enable:
> >      description: |
> >        This bit enables a notch at 60 Hz when the first notch of the sinc
> > @@ -84,11 +96,12 @@ properties:
> >      description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
> >      type: boolean
> >
> > +dependencies:
> > +  adi,clock-xtal: ['clocks', 'clock-names']
> > +
> >  required:
> >    - compatible
> >    - reg
> > -  - clocks
> > -  - clock-names
> >    - interrupts
> >    - dvdd-supply
> >    - avdd-supply
> > @@ -98,6 +111,13 @@ required:
> >
> >  allOf:
> >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +  - if:
> > +      required:
> > +        - clocks
> > +        - clock-names
> > +    then:
> > +      properties:
> > +        adi,int-clock-output-enable: false
> >
> >  unevaluatedProperties: false
> >
> > @@ -115,6 +135,7 @@ examples:
> >              spi-cpha;
> >              clocks = <&ad7192_mclk>;
> >              clock-names = "mclk";
> > +            adi,clock-xtal;
> >              interrupts = <25 0x2>;
> >              interrupt-parent = <&gpio>;
> >              dvdd-supply = <&dvdd>;
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists