Re: [PATCH v3 5/5] dt-bindings: iio: ad7192: Allow selection of clock modes

[Fabrizio Lamarque]

On Wed, May 31, 2023 at 9:24 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 31/05/2023 11:40, Fabrizio Lamarque wrote:
> > On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> >>> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> >>>>
> >>>> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@xxxxxxxxx wrote:
> >>>>> From: Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxx>
> >>>>> ---
> >>>>>  .../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,
> >>
> >> Description confuses me. Why would it matter what type of clock you have
> >> as input - external crystal oscillator or external CMOS-compatible
> >> clock? Later you refer to "internal", so maybe you meant here also
> >> internal for one of the options?
> >
> > The AD7192 needs to be configured according to the type of external
> > clock that is
> > applied on MCLK1/MCLK2 pins in order to activate the correct circuitry.
> >
> > Here are some citations from the datasheet:
> >
> > MCLK2 pin description:
> > "The AD7192 has an internal 4.92 MHz clock. This internal clock can be
> > made available
> > on the MCLK2 pin. The clock for the AD7192 can be provided externally
> > also in the form
> > of a crystal or external clock. A crystal can be tied across the MCLK1
> > and MCLK2 pins.
> > Alternatively, the MCLK2 pin can be driven with a CMOS-compatible clock and the
> > MCLK1 pin left unconnected."
> >
> > Each of these clock modes have to be configured via AD7192 mode register.
> > (Clock source configuration bits, mode register, CLK0 and CLK1).
> > Here is their description from datasheet:
> >
> > "Either the on-chip 4.92 MHz clock or an external clock can be used.
> > The ability to
> > use an external clock allows several AD7192 devices to be synchronized. Also,
> > 50 Hz/60 Hz rejection is improved when an accurate external clock
> > drives the AD7192."
> >
> > The choice between internal clock, external crystal oscillator or
> > external CMOS digital
> > clock is a decision of the HW designer driven by noise rejection,
> > synchronization, and
> > cost requirements.
> >
> > If possible, I kindly ask you suggestions on how to adjust the description
> > so that it would be cleaner.
>
> It's fine. I missed that part that first option requires feeding the
> clock through two pins ("and").

Thank you once again.
I've reworded the commit message, removed the
adi,int-clock-output-enable option and clarified the adi,clock-xtal
entry.

This would be the new commit message:

AD7192 can be clocked from either:
 - Internal clock
 - CMOS-compatible clock on MCLK2
 - Crystal oscillator on MCLK1 and MCLK2

Each of these modes have to be configured via AD7192 mode register.
Describe choice between these clock modes.


And this is the new description for adi,clock-xtal:

This bit sets external clock mode. When set, master clock is sourced from
a crystal connected from MCLK1 to MCLK2.
When cleared, a CMOS-compatible clock source is expected on MCLK2.


Is this description cleaner? If possible, I kindly ask you for feedback before
posting a v4 (with only patch 3/5 and 5/5 since the others have
already been applied).

Best regards,
Fabrizio Lamarque