[PATCH v3 0/5] Fix ad7192 driver issues

[PATCH v3 0/5] Fix ad7192 driver issues

[fl.scratchpad]

From: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Here is a patch set to overcome a number of issues in current ad7192
adc driver implementation prevending the driver to be loaded and
behave correctly:

 - null pointer dereference causing kernel panic on driver probe;
 - wrong internal clock selection;
 - use of "avdd" regulator name in place of vref (reference voltage);
 - missing clock options in bindings docs only (already implemented).

The first two issues are regressions.

Backported patches have been tested on a platform with an ARM Cortex-A7
CPU from NXP with kernel 5.15.

As a side note, on the tested platform there is still an issue with a
pending interrupt that I worked around by setting the DRDY IRQ
to LEVEL in place of EDGE sensing. Also, setting IRQ_DISABLE_UNLAZY
flag does not help in my case.

You may find further information here:
https://lore.kernel.org/all/CAPJMGm4GaSjD6bdqMwCr2EVZGenWzT-nCCf3BMRaD1TSfAabpA@mail.gmail.com/

Version log:


v2->v3
 - Reworded commit messages
 - Split binding fixes

v1->v2
 - Obtained ad7192_state from iio_dev pointer
 - Added patch on bindings documentation


Fabrizio Lamarque (5):
  iio: adc: ad7192: Fix null ad7192_state pointer access
  iio: adc: ad7192: Fix internal/external clock selection
  iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source
  dt-bindings: iio: ad7192: Add mandatory reference voltage source
  dt-bindings: iio: ad7192: Allow selection of clock modes

 .../bindings/iio/adc/adi,ad7192.yaml          | 32 +++++++++++++++++--
 drivers/iio/adc/ad7192.c                      | 31 +++++++++++++++---
 2 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] iio: adc: ad7192: Fix null ad7192_state pointer access

[fl.scratchpad]

From: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Pointer to indio_dev structure is obtained via spi_get_drvdata() at
the beginning of function ad7192_setup(), but the spi->dev->driver_data
member is not initialized, hence a NULL pointer is returned.

Fix by changing ad7192_setup() signature to take pointer to struct
iio_dev, and get ad7192_state pointer via st = iio_priv(indio_dev);

Fixes: bd5dcdeb3fd0 ("iio: adc: ad7192: convert to device-managed functions")
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad7192.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 55a6ab591016..94a9cf34a255 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
 	return clock_sel;
 }
 
-static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
+static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
 {
-	struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
+	struct ad7192_state *st = iio_priv(indio_dev);
 	bool rej60_en, refin2_en;
 	bool buf_en, bipolar, burnout_curr_en;
 	unsigned long long scale_uv;
@@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
-	ret = ad7192_setup(st, spi->dev.of_node);
+	ret = ad7192_setup(indio_dev, spi->dev.of_node);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v3 1/5] iio: adc: ad7192: Fix null ad7192_state pointer access

[Jonathan Cameron]

On Tue, 30 May 2023 09:53:07 +0200
fl.scratchpad@gmail.com wrote:

> From: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> 
> Pointer to indio_dev structure is obtained via spi_get_drvdata() at
> the beginning of function ad7192_setup(), but the spi->dev->driver_data
> member is not initialized, hence a NULL pointer is returned.
> 
> Fix by changing ad7192_setup() signature to take pointer to struct
> iio_dev, and get ad7192_state pointer via st = iio_priv(indio_dev);
> 
> Fixes: bd5dcdeb3fd0 ("iio: adc: ad7192: convert to device-managed functions")
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
I'll pick those of this series up that everyone seems happy with.

Applied to the fixes-togreg branch of iio.git
Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 55a6ab591016..94a9cf34a255 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>  	return clock_sel;
>  }
>  
> -static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> +static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
>  {
> -	struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
> +	struct ad7192_state *st = iio_priv(indio_dev);
>  	bool rej60_en, refin2_en;
>  	bool buf_en, bipolar, burnout_curr_en;
>  	unsigned long long scale_uv;
> @@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
>  		}
>  	}
>  
> -	ret = ad7192_setup(st, spi->dev.of_node);
> +	ret = ad7192_setup(indio_dev, spi->dev.of_node);
>  	if (ret)
>  		return ret;
>  

[PATCH v3 2/5] iio: adc: ad7192: Fix internal/external clock selection

[fl.scratchpad]

From: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Fix wrong selection of internal clock when mclk is defined.

Resolve a logical inversion introduced in c9ec2cb328e3.

Fixes: c9ec2cb328e3 ("iio: adc: ad7192: use devm_clk_get_optional() for mclk")
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 94a9cf34a255..5a9c8898f8af 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
 	clock_sel = AD7192_CLK_INT;
 
 	/* use internal clock */
-	if (st->mclk) {
+	if (!st->mclk) {
 		if (of_property_read_bool(np, "adi,int-clock-output-enable"))
 			clock_sel = AD7192_CLK_INT_CO;
 	} else {
-- 
2.34.1

Re: [PATCH v3 2/5] iio: adc: ad7192: Fix internal/external clock selection

[Jonathan Cameron]

On Tue, 30 May 2023 09:53:08 +0200
fl.scratchpad@gmail.com wrote:

> From: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> 
> Fix wrong selection of internal clock when mclk is defined.
> 
> Resolve a logical inversion introduced in c9ec2cb328e3.
> 
> Fixes: c9ec2cb328e3 ("iio: adc: ad7192: use devm_clk_get_optional() for mclk")
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 94a9cf34a255..5a9c8898f8af 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>  	clock_sel = AD7192_CLK_INT;
>  
>  	/* use internal clock */
> -	if (st->mclk) {
> +	if (!st->mclk) {
>  		if (of_property_read_bool(np, "adi,int-clock-output-enable"))
>  			clock_sel = AD7192_CLK_INT_CO;
>  	} else {

[PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source

[fl.scratchpad]

From: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Add missing vref-supply and fix avdd-supply used as if it were vref.

AD7192 requires three independent voltage sources, digital supply (on
pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
alternate pin pair REFIN1 or REFIN2).

Emit a warning message when AVdd is used in place of VRef for backwards
compatibility.

Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
---
 drivers/iio/adc/ad7192.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 5a9c8898f8af..bc41323ea810 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -177,6 +177,7 @@ struct ad7192_chip_info {
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
 	struct regulator		*avdd;
+	struct regulator		*vref;
 	struct clk			*mclk;
 	u16				int_vref_mv;
 	u32				fclk;
@@ -1014,11 +1015,31 @@ static int ad7192_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+	if (!IS_ERR(st->vref)) {
+		ret = regulator_enable(st->vref);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to enable specified VRef supply\n");
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
+		if (ret)
+			return ret;
+	} else if (PTR_ERR(st->vref) != -ENODEV) {
+		return PTR_ERR(st->vref);
+	}
+
 	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
 	if (ret)
 		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
 
-	ret = regulator_get_voltage(st->avdd);
+
+	if (!IS_ERR(st->vref)) {
+		ret = regulator_get_voltage(st->vref);
+	} else {
+		dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an old DTS\n");
+		ret = regulator_get_voltage(st->avdd);
+	}
 	if (ret < 0) {
 		dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
 		return ret;
-- 
2.34.1

Re: [PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source

[andy.shevchenko]

Tue, May 30, 2023 at 09:53:09AM +0200, fl.scratchpad@gmail.com kirjoitti:
> From: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> 
> Add missing vref-supply and fix avdd-supply used as if it were vref.
> 
> AD7192 requires three independent voltage sources, digital supply (on
> pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
> alternate pin pair REFIN1 or REFIN2).
> 
> Emit a warning message when AVdd is used in place of VRef for backwards
> compatibility.

...

> +	st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> +	if (!IS_ERR(st->vref)) {
> +		ret = regulator_enable(st->vref);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable specified VRef supply\n");
> +
> +		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
> +		if (ret)
> +			return ret;
> +	} else if (PTR_ERR(st->vref) != -ENODEV) {
> +		return PTR_ERR(st->vref);
> +	}

Wouldn't this be better?

	if (IS_ERR(st->vref)) {
		if (PTR_ERR(st->vref) != -ENODEV)
			return PTR_ERR(st->vref);
	} else {

...

>  	if (ret)
>  		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
>  
> -	ret = regulator_get_voltage(st->avdd);
> +

One blank line is enough.

> +	if (!IS_ERR(st->vref)) {
> +		ret = regulator_get_voltage(st->vref);

Why negative conditional? Usual pattern is to check for errors first, so

	if (IS_ERR(st->vref)) {
		dev_warn(...);
		...
	} else {
		ret = regulator_get_voltage(st->vref);
	}

> +	} else {
> +		dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an old DTS\n");
> +		ret = regulator_get_voltage(st->avdd);
> +	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/5] dt-bindings: iio: ad7192: Add mandatory reference voltage source

[fl.scratchpad]

From: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Add required reference voltage (VRef) supply regulator.

AD7192 requires three independent voltage sources: DVdd, AVdd and VRef
(on REFINx pin pairs).

Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index d521d516088b..16def2985ab4 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -47,6 +47,9 @@ properties:
   avdd-supply:
     description: AVdd voltage supply
 
+  vref-supply:
+    description: VRef voltage supply
+
   adi,rejection-60-Hz-enable:
     description: |
       This bit enables a notch at 60 Hz when the first notch of the sinc
@@ -89,6 +92,7 @@ required:
   - interrupts
   - dvdd-supply
   - avdd-supply
+  - vref-supply
   - spi-cpol
   - spi-cpha
 
@@ -115,6 +119,7 @@ examples:
             interrupt-parent = <&gpio>;
             dvdd-supply = <&dvdd>;
             avdd-supply = <&avdd>;
+            vref-supply = <&vref>;
 
             adi,refin2-pins-enable;
             adi,rejection-60-Hz-enable;
-- 
2.34.1

Re: [PATCH v3 4/5] dt-bindings: iio: ad7192: Add mandatory reference voltage source

[Conor Dooley]

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

On Tue, May 30, 2023 at 09:53:10AM +0200, fl.scratchpad@gmail.com wrote:
> From: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> 
> Add required reference voltage (VRef) supply regulator.
> 
> AD7192 requires three independent voltage sources: DVdd, AVdd and VRef
> (on REFINx pin pairs).
> 
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

Re: [PATCH v3 4/5] dt-bindings: iio: ad7192: Add mandatory reference voltage source

[Jonathan Cameron]

On Tue, 30 May 2023 18:22:45 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Tue, May 30, 2023 at 09:53:10AM +0200, fl.scratchpad@gmail.com wrote:
> > From: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> > 
> > Add required reference voltage (VRef) supply regulator.
> > 
> > AD7192 requires three independent voltage sources: DVdd, AVdd and VRef
> > (on REFINx pin pairs).
> > 
> > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>  
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
Applied.

I'll leave 3 and 5 for a v4 posting.
> 
> Thanks,
> Conor.

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

[fl.scratchpad]

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
+
+  adi,int-clock-output-enable:
+    description: |
+      When internal clock is selected, this bit enables clock out pin.
+    type: boolean
+
   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

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

[Conor Dooley]

[-- 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 --]

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

[Fabrizio Lamarque]

On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote:
>
> 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?

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

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

[Krzysztof Kozlowski]

On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote:
>>
>> 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?
> 
> 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?

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

Either this is a clock provider via common clock framework or is not.
Don't re-implement clock provider via other properties but just skip
such feature.

Best regards,
Krzysztof

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

[Fabrizio Lamarque]

On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote:
> >>
> >> 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?
> >
> > 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.

>
> >
> >>
> >>> +  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
>
> Either this is a clock provider via common clock framework or is not.
> Don't re-implement clock provider via other properties but just skip
> such feature.

Ok, I understand. I will remove the bit from the patch in V4. Thank you.

The bit was already existing upstream in the driver, but I would just drop
the change in documentation without any additional patch that removes it
from the driver.

Best regards,
Fabrizio Lamarque

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

[Conor Dooley]

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

On Wed, May 31, 2023 at 11:40:08AM +0200, Fabrizio Lamarque wrote:
> On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> > > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote:
> > >>
> > >> 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?
> > >
> > > 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.

For me at least, I partially wanted it explained so that intimate
knowledge of the part was not required to review the binding! To me, the
original description is perfectly clear about how the hardware is
configured, but nothing says why software needs to actually know about
it.
I'd be happy if you worked
> Each of these clock modes have to be configured via AD7192 mode register.
into the description, but perhaps Krzysztof disagrees.

Cheers,
Conor.

> > >>> +  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
> >
> > Either this is a clock provider via common clock framework or is not.
> > Don't re-implement clock provider via other properties but just skip
> > such feature.
> 
> Ok, I understand. I will remove the bit from the patch in V4. Thank you.
> 
> The bit was already existing upstream in the driver, but I would just drop
> the change in documentation without any additional patch that removes it
> from the driver.
> 
> Best regards,
> Fabrizio Lamarque

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

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@linaro.org> wrote:
>
> On 31/05/2023 11:40, Fabrizio Lamarque wrote:
> > On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> >>> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote:
> >>>>
> >>>> 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?
> >>>
> >>> 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