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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]

On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote:
> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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?

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

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]