Fix ad7192 driver issues

From: Fabrizio Lamarque <[email protected]>

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

Reviewed-by Nuno Sa <[email protected]>

---
 drivers/iio/adc/ad7192.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/adc/ad7192.c +3 -3

index 55a6ab591016..94a9cf34a255 100644
@@ -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;
 

From: Fabrizio Lamarque <[email protected]>

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

Reviewed-by Nuno Sa <[email protected]>

---
 drivers/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/ad7192.c +1 -1

index 94a9cf34a255..5a9c8898f8af 100644
@@ -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 {

From: Fabrizio Lamarque <[email protected]>

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.

drivers/iio/adc/ad7192.c +4 -0

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

From: Fabrizio Lamarque <[email protected]>

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

Quoted 4 lines · On Tue, May 30, 2023 at 09:53:10AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

Add required reference voltage (VRef) supply regulator.

Quoted 5 lines

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

Acked-by Conor Dooley <[email protected]>

Thanks,
Conor.

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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:

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +12 -2

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

Signature · 1 line

2.34.1

Quoted 32 lines · On Tue, May 30, 2023 at 09:53:11AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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
 

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 58

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +5 -0

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

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 63

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +4 -0

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

Quoted 44 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +12 -3

+
   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

Quoted 34 lines · Conor Dooley

Quoted 32 lines · On Tue, May 30, 2023 at 09:53:11AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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
 

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 58

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +5 -0

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

Quoted 7 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 63

Quoted 6 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +4 -0

+  adi,int-clock-output-enable:
+    description: |
+      When internal clock is selected, this bit enables clock out pin.
+    type: boolean

Quoted 6 lines

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

Quoted 52 lines · Fabrizio Lamarque

Quoted 34 lines · Conor Dooley

Quoted 32 lines · On Tue, May 30, 2023 at 09:53:11AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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
 

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 58

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +5 -0

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

Quoted 3 lines

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?

Quoted 15 lines

Quoted 14 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 63

Quoted 7 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +4 -0

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

Quoted 4 lines

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.

Quoted 54 lines · Krzysztof Kozlowski

Quoted 52 lines · Fabrizio Lamarque

Quoted 34 lines · Conor Dooley

Quoted 32 lines · On Tue, May 30, 2023 at 09:53:11AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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
 

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 58

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +5 -0

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

Quoted 8 lines

Quoted 3 lines

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.

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

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 63

Quoted 4 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +4 -0

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

Quoted 8 lines

Quoted 4 lines

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

Quoted 97 lines · Fabrizio Lamarque

Quoted 54 lines · Krzysztof Kozlowski

Quoted 52 lines · Fabrizio Lamarque

Quoted 34 lines · Conor Dooley

Quoted 32 lines · On Tue, May 30, 2023 at 09:53:11AM +0200, [email protected] wrote:

From: Fabrizio Lamarque <[email protected]>

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

---
 .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +2 -1

index 16def2985ab4..f7ecfd65ad80 100644
@@ -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
 

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml properties: · line 58

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +5 -0

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 2 lines

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").