[PATCH v3 0/5] iio: adc: ad7192: Add support for AD7194

[PATCH v3 0/5] iio: adc: ad7192: Add support for AD7194

[Alisa-Dariana Roman]

Dear maintainers,

Thank you all for the feedback!

I am submitting the upgraded series of patches for the ad7192 driver.

Please consider applying in order.

Note that I dropped the patch related to the clock bindings. I will be
back with another series of patches related to the clock.

Thank you!

v2 -> v3
	- add precursor patch to simply functions to only pass
	  ad7192_state
	- add patch to replace custom attribute
	- bindings patch: correct use of allOf and some minor changes to
	  the ad7194 example
	- add ad7194 patch:
		- use "ad7192 and similar"
		- ad7194 no longer needs attribute group
		- use callback function in chip_info to parse channels
		- move struct ad7192_chip_info
		- change position of parse functions
	- drop clock bindings patch

v1 -> v2
	- new commit with missing documentation for properties
	- add constraint for channels in binding
	- correct pattern for channels
	- correct commit message by adding "()" to functions
	- use in_range
	- use preferred structure in Kconfig

Kind regards,

Alisa-Dariana Roman (5):
  iio: adc: ad7192: Use device api
  iio: adc: ad7192: Pass state directly
  iio: adc: ad7192: Use standard attribute
  dt-bindings: iio: adc: ad7192: Add AD7194 support
  iio: adc: ad7192: Add AD7194 support

 .../bindings/iio/adc/adi,ad7192.yaml          |  75 ++++++
 drivers/iio/adc/Kconfig                       |  11 +-
 drivers/iio/adc/ad7192.c                      | 252 +++++++++++++-----
 3 files changed, 269 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] iio: adc: ad7192: Use device api

[Alisa-Dariana Roman]

Replace of.h and corresponding functions with preferred device specific
functions.

Also replace of_device_get_match_data() with
spi_get_device_match_data().

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad7192.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index adc3cbe92d6e..48e0357564af 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -17,7 +17,6 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
-#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -364,19 +363,19 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
 		freq <= AD7192_EXT_FREQ_MHZ_MAX);
 }
 
-static int ad7192_of_clock_select(struct ad7192_state *st)
+static int ad7192_device_clock_select(struct ad7192_state *st)
 {
-	struct device_node *np = st->sd.spi->dev.of_node;
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int clock_sel;
 
 	clock_sel = AD7192_CLK_INT;
 
 	/* use internal clock */
 	if (!st->mclk) {
-		if (of_property_read_bool(np, "adi,int-clock-output-enable"))
+		if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
 			clock_sel = AD7192_CLK_INT_CO;
 	} else {
-		if (of_property_read_bool(np, "adi,clock-xtal"))
+		if (device_property_read_bool(dev, "adi,clock-xtal"))
 			clock_sel = AD7192_CLK_EXT_MCLK1_2;
 		else
 			clock_sel = AD7192_CLK_EXT_MCLK2;
@@ -385,9 +384,10 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
 	return clock_sel;
 }
 
-static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
+static int ad7192_setup(struct iio_dev *indio_dev)
 {
 	struct ad7192_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
 	bool rej60_en, refin2_en;
 	bool buf_en, bipolar, burnout_curr_en;
 	unsigned long long scale_uv;
@@ -416,26 +416,26 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
 
 	st->conf = FIELD_PREP(AD7192_CONF_GAIN_MASK, 0);
 
-	rej60_en = of_property_read_bool(np, "adi,rejection-60-Hz-enable");
+	rej60_en = device_property_read_bool(dev, "adi,rejection-60-Hz-enable");
 	if (rej60_en)
 		st->mode |= AD7192_MODE_REJ60;
 
-	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
+	refin2_en = device_property_read_bool(dev, "adi,refin2-pins-enable");
 	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
 		st->conf |= AD7192_CONF_REFSEL;
 
 	st->conf &= ~AD7192_CONF_CHOP;
 
-	buf_en = of_property_read_bool(np, "adi,buffer-enable");
+	buf_en = device_property_read_bool(dev, "adi,buffer-enable");
 	if (buf_en)
 		st->conf |= AD7192_CONF_BUF;
 
-	bipolar = of_property_read_bool(np, "bipolar");
+	bipolar = device_property_read_bool(dev, "bipolar");
 	if (!bipolar)
 		st->conf |= AD7192_CONF_UNIPOLAR;
 
-	burnout_curr_en = of_property_read_bool(np,
-						"adi,burnout-currents-enable");
+	burnout_curr_en =
+		device_property_read_bool(dev, "adi,burnout-currents-enable");
 	if (burnout_curr_en && buf_en) {
 		st->conf |= AD7192_CONF_BURN;
 	} else if (burnout_curr_en) {
@@ -1117,9 +1117,7 @@ static int ad7192_probe(struct spi_device *spi)
 	}
 	st->int_vref_mv = ret / 1000;
 
-	st->chip_info = of_device_get_match_data(&spi->dev);
-	if (!st->chip_info)
-		st->chip_info = (void *)spi_get_device_id(spi)->driver_data;
+	st->chip_info = spi_get_device_match_data(spi);
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = st->chip_info->channels;
@@ -1140,7 +1138,7 @@ static int ad7192_probe(struct spi_device *spi)
 	if (IS_ERR(st->mclk))
 		return PTR_ERR(st->mclk);
 
-	st->clock_sel = ad7192_of_clock_select(st);
+	st->clock_sel = ad7192_device_clock_select(st);
 
 	if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
 	    st->clock_sel == AD7192_CLK_EXT_MCLK2) {
@@ -1152,7 +1150,7 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
-	ret = ad7192_setup(indio_dev, spi->dev.of_node);
+	ret = ad7192_setup(indio_dev);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v3 1/5] iio: adc: ad7192: Use device api

[Andy Shevchenko]

Subject should be:

  iio: adc: ad7192: Use device property APIs

On Thu, Feb 08, 2024 at 07:24:55PM +0200, Alisa-Dariana Roman wrote:
> Replace of.h and corresponding functions with preferred device specific
> functions.
> 
> Also replace of_device_get_match_data() with
> spi_get_device_match_data().

...

>  #include <linux/err.h>
>  #include <linux/sched.h>
>  #include <linux/delay.h>
> -#include <linux/of.h>

Actually this has to be replaced by property.h (placed somewhere before
slab.h).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/5] iio: adc: ad7192: Pass state directly

[Alisa-Dariana Roman]

Pass only the ad7192_state structure. There is no need to pass the
iio_dev structure.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 48e0357564af..5e8043865233 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -384,9 +384,8 @@ static int ad7192_device_clock_select(struct ad7192_state *st)
 	return clock_sel;
 }
 
-static int ad7192_setup(struct iio_dev *indio_dev)
+static int ad7192_setup(struct ad7192_state *st)
 {
-	struct ad7192_state *st = iio_priv(indio_dev);
 	struct device *dev = &st->sd.spi->dev;
 	bool rej60_en, refin2_en;
 	bool buf_en, bipolar, burnout_curr_en;
@@ -458,7 +457,7 @@ static int ad7192_setup(struct iio_dev *indio_dev)
 	/* Populate available ADC input ranges */
 	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
 		scale_uv = ((u64)st->int_vref_mv * 100000000)
-			>> (indio_dev->channels[0].scan_type.realbits -
+			>> (st->chip_info->channels[0].scan_type.realbits -
 			!FIELD_GET(AD7192_CONF_UNIPOLAR, st->conf));
 		scale_uv >>= i;
 
@@ -1150,7 +1149,7 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
-	ret = ad7192_setup(indio_dev);
+	ret = ad7192_setup(st);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v3 3/5] iio: adc: ad7192: Use standard attribute

[Alisa-Dariana Roman]

Replace custom attribute filter_low_pass_3db_frequency_available with
standard attribute.

Store the available values in ad7192_state struct.

The function that used to compute those values replaced by
ad7192_update_filter_freq_avail().

Function ad7192_show_filter_avail() is no longer needed.

Note that the initial available values are hardcoded.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 67 ++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 5e8043865233..d8393ac048e7 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -187,6 +187,7 @@ struct ad7192_state {
 	u32				mode;
 	u32				conf;
 	u32				scale_avail[8][2];
+	u32				filter_freq_avail[4][2];
 	u32				oversampling_ratio_avail[4];
 	u8				gpocon;
 	u8				clock_sel;
@@ -470,6 +471,16 @@ static int ad7192_setup(struct ad7192_state *st)
 	st->oversampling_ratio_avail[2] = 8;
 	st->oversampling_ratio_avail[3] = 16;
 
+	st->filter_freq_avail[0][0] = 600;
+	st->filter_freq_avail[1][0] = 800;
+	st->filter_freq_avail[2][0] = 2300;
+	st->filter_freq_avail[3][0] = 2720;
+
+	st->filter_freq_avail[0][1] = 1000;
+	st->filter_freq_avail[1][1] = 1000;
+	st->filter_freq_avail[2][1] = 1000;
+	st->filter_freq_avail[3][1] = 1000;
+
 	return 0;
 }
 
@@ -583,48 +594,24 @@ static int ad7192_get_f_adc(struct ad7192_state *st)
 				 f_order * FIELD_GET(AD7192_MODE_RATE_MASK, st->mode));
 }
 
-static void ad7192_get_available_filter_freq(struct ad7192_state *st,
-						    int *freq)
+static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
 {
 	unsigned int fadc;
 
 	/* Formulas for filter at page 25 of the datasheet */
 	fadc = ad7192_compute_f_adc(st, false, true);
-	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+	st->filter_freq_avail[0][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
 	fadc = ad7192_compute_f_adc(st, true, true);
-	freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+	st->filter_freq_avail[1][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
 	fadc = ad7192_compute_f_adc(st, false, false);
-	freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
+	st->filter_freq_avail[2][0] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
 
 	fadc = ad7192_compute_f_adc(st, true, false);
-	freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
+	st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
 }
 
-static ssize_t ad7192_show_filter_avail(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7192_state *st = iio_priv(indio_dev);
-	unsigned int freq_avail[4], i;
-	size_t len = 0;
-
-	ad7192_get_available_filter_freq(st, freq_avail);
-
-	for (i = 0; i < ARRAY_SIZE(freq_avail); i++)
-		len += sysfs_emit_at(buf, len, "%d.%03d ", freq_avail[i] / 1000,
-				     freq_avail[i] % 1000);
-
-	buf[len - 1] = '\n';
-
-	return len;
-}
-
-static IIO_DEVICE_ATTR(filter_low_pass_3db_frequency_available,
-		       0444, ad7192_show_filter_avail, NULL, 0);
-
 static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
 		       ad7192_show_bridge_switch, ad7192_set,
 		       AD7192_REG_GPOCON);
@@ -634,7 +621,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
 		       AD7192_REG_CONF);
 
 static struct attribute *ad7192_attributes[] = {
-	&iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
 	NULL
 };
@@ -644,7 +630,6 @@ static const struct attribute_group ad7192_attribute_group = {
 };
 
 static struct attribute *ad7195_attributes[] = {
-	&iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
 	&iio_dev_attr_ac_excitation_en.dev_attr.attr,
 	NULL
@@ -662,17 +647,15 @@ static unsigned int ad7192_get_temp_scale(bool unipolar)
 static int ad7192_set_3db_filter_freq(struct ad7192_state *st,
 				      int val, int val2)
 {
-	int freq_avail[4], i, ret, freq;
+	int i, ret, freq;
 	unsigned int diff_new, diff_old;
 	int idx = 0;
 
 	diff_old = U32_MAX;
 	freq = val * 1000 + val2;
 
-	ad7192_get_available_filter_freq(st, freq_avail);
-
-	for (i = 0; i < ARRAY_SIZE(freq_avail); i++) {
-		diff_new = abs(freq - freq_avail[i]);
+	for (i = 0; i < ARRAY_SIZE(st->filter_freq_avail); i++) {
+		diff_new = abs(freq - st->filter_freq_avail[i][0]);
 		if (diff_new < diff_old) {
 			diff_old = diff_new;
 			idx = i;
@@ -823,6 +806,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
 		st->mode &= ~AD7192_MODE_RATE_MASK;
 		st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
 		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+		ad7192_update_filter_freq_avail(st);
 		break;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000);
@@ -843,6 +827,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
 				break;
 			}
 		mutex_unlock(&st->lock);
+		ad7192_update_filter_freq_avail(st);
 		break;
 	default:
 		ret = -EINVAL;
@@ -885,6 +870,12 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
 		/* Values are stored in a 2D matrix  */
 		*length = ARRAY_SIZE(st->scale_avail) * 2;
 
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = (int *)st->filter_freq_avail;
+		*type = IIO_VAL_FRACTIONAL;
+		*length = ARRAY_SIZE(st->filter_freq_avail) * 2;
+
 		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*vals = (int *)st->oversampling_ratio_avail;
@@ -953,7 +944,9 @@ static const struct iio_info ad7195_info = {
 			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
 			(_mask_all), \
 		.info_mask_shared_by_type_available = (_mask_type_av), \
-		.info_mask_shared_by_all_available = (_mask_all_av), \
+		.info_mask_shared_by_all_available = \
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+			(_mask_all_av), \
 		.ext_info = (_ext_info), \
 		.scan_index = (_si), \
 		.scan_type = { \
-- 
2.34.1

Re: [PATCH v3 3/5] iio: adc: ad7192: Use standard attribute

[Jonathan Cameron]

On Thu,  8 Feb 2024 19:24:57 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Replace custom attribute filter_low_pass_3db_frequency_available with
> standard attribute.
> 
> Store the available values in ad7192_state struct.
> 
> The function that used to compute those values replaced by
> ad7192_update_filter_freq_avail().
> 
> Function ad7192_show_filter_avail() is no longer needed.
> 
> Note that the initial available values are hardcoded.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Looks good. Thanks for tidying this up.

I've nothing to add to other reviews that have already come in for v3.

Thanks,

Jonathan

[PATCH v3 4/5] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The default configuration for these channels can be changed
from the devicetree.

Also add an example for AD7194 devicetree.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../bindings/iio/adc/adi,ad7192.yaml          | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 16def2985ab4..169bdd1dd0e1 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -21,8 +21,15 @@ properties:
       - adi,ad7190
       - adi,ad7192
       - adi,ad7193
+      - adi,ad7194
       - adi,ad7195
 
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
   reg:
     maxItems: 1
 
@@ -96,8 +103,44 @@ required:
   - spi-cpol
   - spi-cpha
 
+patternProperties:
+  "^channel@([0-7a-f])$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description: The channel index.
+        minimum: 0
+        maximum: 7
+
+      diff-channels:
+        description: |
+          The differential channel pair for Ad7194 configurable channels. The
+          first channel is the positive input, the second channel is the
+          negative input.
+        items:
+          minimum: 1
+          maximum: 16
+
+    required:
+      - reg
+      - diff-channels
+
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          enum:
+            - adi,ad7190
+            - adi,ad7192
+            - adi,ad7193
+            - adi,ad7195
+    then:
+      patternProperties:
+        "^channel@([0-7a-f])$": false
 
 unevaluatedProperties: false
 
@@ -127,3 +170,35 @@ examples:
             adi,burnout-currents-enable;
         };
     };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "adi,ad7194";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+            spi-cpha;
+            clocks = <&ad7192_mclk>;
+            clock-names = "mclk";
+            interrupts = <25 0x2>;
+            interrupt-parent = <&gpio>;
+            dvdd-supply = <&dvdd>;
+            avdd-supply = <&avdd>;
+            vref-supply = <&vref>;
+
+            channel@0 {
+                reg = <0>;
+                diff-channels = <1 6>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                diff-channels = <2 5>;
+            };
+        };
+    };
-- 
2.34.1

Re: [PATCH v3 4/5] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Conor Dooley]

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

Hey,

On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote:

> +patternProperties:
> +  "^channel@([0-7a-f])$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description: The channel index.
> +        minimum: 0
> +        maximum: 7

There are only 8 possible channels, at indices 0 to 7, so why is the
pattern property more permissive than that? Shouldn't "^channel@[0-7]$"
suffice?

> +
> +       diff-channels:

> +        description: |
> +          The differential channel pair for Ad7194 configurable channels. The
> +          first channel is the positive input, the second channel is the
> +          negative input.

This duplicates the description in adc.yaml

> +        items:
> +          minimum: 1
> +          maximum: 16

Hmm, this makes me wonder: why doesn't this match the number of channels
available and why is 0 not a valid channel for differential measurements?

Thanks,
Conor.

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

Re: [PATCH v3 4/5] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

On 08.02.2024 20:03, Conor Dooley wrote:
> Hey,
> 
> On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote:
> 
>> +patternProperties:
>> +  "^channel@([0-7a-f])$":
>> +    type: object
>> +    $ref: adc.yaml
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        description: The channel index.
>> +        minimum: 0
>> +        maximum: 7
> 
> There are only 8 possible channels, at indices 0 to 7, so why is the
> pattern property more permissive than that? Shouldn't "^channel@[0-7]$"
> suffice?
> 
>> +
>> +       diff-channels:
> 
>> +        description: |
>> +          The differential channel pair for Ad7194 configurable channels. The
>> +          first channel is the positive input, the second channel is the
>> +          negative input.
> 
> This duplicates the description in adc.yaml
> 
>> +        items:
>> +          minimum: 1
>> +          maximum: 16
> 
> Hmm, this makes me wonder: why doesn't this match the number of channels
> available and why is 0 not a valid channel for differential measurements?
> 
> Thanks,
> Conor.

Hello and thank you for the feedback!

I will change the pattern property and the description.

Regarding the channels, I followed the existing style of the driver for 
the AD7194 channels: one iio channel for each pseudo-differential input 
channel(AINx - AINCOM), summing up to 16 channels; and one iio channel 
for each differential channel (AINx - AINy), summing up to 8 channels. 
For the diff-channels, I thought the possible values should be 1->16 
corresponding to AIN1->AIN16 (I will add this to the description as 
suggested by David).

Kind regards,
Alisa-Dariana Roman

Re: [PATCH v3 4/5] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Conor Dooley]

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

On Thu, Feb 15, 2024 at 02:13:38PM +0200, Alisa-Dariana Roman wrote:
> On 08.02.2024 20:03, Conor Dooley wrote:
> > Hey,
> > 
> > On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote:
> > 
> > > +patternProperties:
> > > +  "^channel@([0-7a-f])$":
> > > +    type: object
> > > +    $ref: adc.yaml
> > > +    unevaluatedProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: The channel index.
> > > +        minimum: 0
> > > +        maximum: 7
> > 
> > There are only 8 possible channels, at indices 0 to 7, so why is the
> > pattern property more permissive than that? Shouldn't "^channel@[0-7]$"
> > suffice?
> > 
> > > +
> > > +       diff-channels:
> > 
> > > +        description: |
> > > +          The differential channel pair for Ad7194 configurable channels. The
> > > +          first channel is the positive input, the second channel is the
> > > +          negative input.
> > 
> > This duplicates the description in adc.yaml
> > 
> > > +        items:
> > > +          minimum: 1
> > > +          maximum: 16
> > 
> > Hmm, this makes me wonder: why doesn't this match the number of channels
> > available and why is 0 not a valid channel for differential measurements?
> > 
> > Thanks,
> > Conor.
> 
> Hello and thank you for the feedback!
> 
> I will change the pattern property and the description.
> 
> Regarding the channels, I followed the existing style of the driver for the
> AD7194 channels: one iio channel for each pseudo-differential input
> channel(AINx - AINCOM), summing up to 16 channels; and one iio channel for
> each differential channel (AINx - AINy), summing up to 8 channels.

I don't know what question this is answering. Everything here is about
channels so it is hard for me to relate it back.
Please reply inline & not at the end of the message to everything :)
Was it meant to answer the following?

> > > +    properties:
> > > +      reg:
> > > +        description: The channel index.
> > > +        minimum: 0
> > > +        maximum: 7
> > 
> > There are only 8 possible channels, at indices 0 to 7, so why is the
> > pattern property more permissive than that? Shouldn't "^channel@[0-7]$"
> > suffice?

If it was a response to this, the reg property only allows 8 channels so
the regex should only allow 8 too. The number after @ must match the number
in reg. If using each of the 16 "pseudo-differential" inputs in
isolation is thing you want to be able to do, your reg property does not
allow it.

> For the
> diff-channels, I thought the possible values should be 1->16 corresponding
> to AIN1->AIN16 (I will add this to the description as suggested by David).

With a description, this should be fine.

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

Re: [PATCH v3 4/5] dt-bindings: iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
>
> Also add an example for AD7194 devicetree.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7192.yaml          | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 16def2985ab4..169bdd1dd0e1 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -21,8 +21,15 @@ properties:
>        - adi,ad7190
>        - adi,ad7192
>        - adi,ad7193
> +      - adi,ad7194
>        - adi,ad7195
>
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
>    reg:
>      maxItems: 1
>
> @@ -96,8 +103,44 @@ required:
>    - spi-cpol
>    - spi-cpha
>
> +patternProperties:
> +  "^channel@([0-7a-f])$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description: The channel index.
> +        minimum: 0
> +        maximum: 7

Should the max be 16 since in pseudo-differential mode there can be up
to 16 inputs?

> +
> +      diff-channels:
> +        description: |
> +          The differential channel pair for Ad7194 configurable channels. The

all caps: AD7194

> +          first channel is the positive input, the second channel is the
> +          negative input.
> +        items:
> +          minimum: 1
> +          maximum: 16

As someone who would need to write a .dts based on these bindings, the
information I would find helpful in the description here is that this
is specifying how the logical channels are mapped to the 16 possible
input pins. It seems safe to assume that the values 1 to 16 correspond
to the pins AIN1 to AIN16, but it would be nice to say this
explicitly. And what do we do when using pseudo-differential inputs
where AINCOM is used as the negative input? Use 0?

> +
> +    required:
> +      - reg
> +      - diff-channels
> +
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - adi,ad7190
> +            - adi,ad7192
> +            - adi,ad7193
> +            - adi,ad7195
> +    then:
> +      patternProperties:
> +        "^channel@([0-7a-f])$": false
>
>  unevaluatedProperties: false
>
> @@ -127,3 +170,35 @@ examples:
>              adi,burnout-currents-enable;
>          };
>      };
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "adi,ad7194";
> +            reg = <0>;
> +            spi-max-frequency = <1000000>;
> +            spi-cpol;
> +            spi-cpha;
> +            clocks = <&ad7192_mclk>;
> +            clock-names = "mclk";
> +            interrupts = <25 0x2>;
> +            interrupt-parent = <&gpio>;
> +            dvdd-supply = <&dvdd>;
> +            avdd-supply = <&avdd>;
> +            vref-supply = <&vref>;
> +
> +            channel@0 {
> +                reg = <0>;
> +                diff-channels = <1 6>;
> +            };
> +
> +            channel@1 {
> +                reg = <1>;
> +                diff-channels = <2 5>;
> +            };
> +        };
> +    };
> --
> 2.34.1
>

[PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The default configuration for these channels can be changed
from the devicetree.

The default configuration is hardcoded in order to have a stable number
of channels.

Also modify config AD7192 description for better scaling.

Moved ad7192_chip_info struct definition to allow use of callback
function parse_channels().

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/Kconfig  |  11 ++-
 drivers/iio/adc/ad7192.c | 150 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 148 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 59ae1d17b50d..8062a4d1cbe7 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -71,12 +71,17 @@ config AD7124
 	  called ad7124.
 
 config AD7192
-	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+	tristate "Analog Devices AD7192 and similar ADC driver"
 	depends on SPI
 	select AD_SIGMA_DELTA
 	help
-	  Say yes here to build support for Analog Devices AD7190,
-	  AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+	  Say yes here to build support for Analog Devices SPI analog to digital
+	  converters (ADC):
+	  - AD7190
+	  - AD7192
+	  - AD7193
+	  - AD7194
+	  - AD7195
 	  If unsure, say N (but it's safe to say "Y").
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index d8393ac048e7..a3ff60ed6f63 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7192 and similar SPI ADC driver
  *
  * Copyright 2011-2015 Analog Devices Inc.
  */
@@ -125,10 +125,39 @@
 #define AD7193_CH_AIN8		0x480 /* AIN7 - AINCOM */
 #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
 
+#define AD7194_CH_TEMP		0x100 /* Temp sensor */
+#define AD7194_CH_AIN1		0x400 /* AIN1 - AINCOM */
+#define AD7194_CH_AIN2		0x410 /* AIN2 - AINCOM */
+#define AD7194_CH_AIN3		0x420 /* AIN3 - AINCOM */
+#define AD7194_CH_AIN4		0x430 /* AIN4 - AINCOM */
+#define AD7194_CH_AIN5		0x440 /* AIN5 - AINCOM */
+#define AD7194_CH_AIN6		0x450 /* AIN6 - AINCOM */
+#define AD7194_CH_AIN7		0x460 /* AIN7 - AINCOM */
+#define AD7194_CH_AIN8		0x470 /* AIN8 - AINCOM */
+#define AD7194_CH_AIN9		0x480 /* AIN9 - AINCOM */
+#define AD7194_CH_AIN10		0x490 /* AIN10 - AINCOM */
+#define AD7194_CH_AIN11		0x4A0 /* AIN11 - AINCOM */
+#define AD7194_CH_AIN12		0x4B0 /* AIN12 - AINCOM */
+#define AD7194_CH_AIN13		0x4C0 /* AIN13 - AINCOM */
+#define AD7194_CH_AIN14		0x4D0 /* AIN14 - AINCOM */
+#define AD7194_CH_AIN15		0x4E0 /* AIN15 - AINCOM */
+#define AD7194_CH_AIN16		0x4F0 /* AIN16 - AINCOM */
+#define AD7194_CH_POS_MASK	GENMASK(7, 4)
+#define AD7194_CH_POS(x)	FIELD_PREP(AD7194_CH_POS_MASK, (x))
+#define AD7194_CH_NEG_MASK	GENMASK(3, 0)
+#define AD7194_CH_NEG(x)	FIELD_PREP(AD7194_CH_NEG_MASK, (x))
+#define AD7194_CH_DIFF(pos, neg) \
+		(AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))
+#define AD7194_CH_DIFF_START	0
+#define AD7194_CH_DIFF_NR	8
+#define AD7194_CH_AIN_START	1
+#define AD7194_CH_AIN_NR	16
+
 /* ID Register Bit Designations (AD7192_REG_ID) */
 #define CHIPID_AD7190		0x4
 #define CHIPID_AD7192		0x0
 #define CHIPID_AD7193		0x2
+#define CHIPID_AD7194		0x3
 #define CHIPID_AD7195		0x6
 #define AD7192_ID_MASK		GENMASK(3, 0)
 
@@ -166,17 +195,10 @@ enum {
 	ID_AD7190,
 	ID_AD7192,
 	ID_AD7193,
+	ID_AD7194,
 	ID_AD7195,
 };
 
-struct ad7192_chip_info {
-	unsigned int			chip_id;
-	const char			*name;
-	const struct iio_chan_spec	*channels;
-	u8				num_channels;
-	const struct iio_info		*info;
-};
-
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
 	struct regulator		*avdd;
@@ -197,6 +219,15 @@ struct ad7192_state {
 	struct ad_sigma_delta		sd;
 };
 
+struct ad7192_chip_info {
+	unsigned int			chip_id;
+	const char			*name;
+	const struct iio_chan_spec	*channels;
+	u8				num_channels;
+	const struct iio_info		*info;
+	int (*parse_channels)(struct ad7192_state *st);
+};
+
 static const char * const ad7192_syscalib_modes[] = {
 	[AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
 	[AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
@@ -918,6 +949,15 @@ static const struct iio_info ad7192_info = {
 	.update_scan_mode = ad7192_update_scan_mode,
 };
 
+static const struct iio_info ad7194_info = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
+};
+
 static const struct iio_info ad7195_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static struct iio_chan_spec ad7194_channels[] = {
+	AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
+	AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
+	AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
+	AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
+	AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
+	AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
+	AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
+	AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
+	AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
+	AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
+	AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
+	AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
+	AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
+	AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
+	AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
+	AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
+	AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
+	AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
+	AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
+	AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
+	AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
+	AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
+	AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
+	AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
+	AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
+	IIO_CHAN_SOFT_TIMESTAMP(25),
+};
+
+static int ad7192_parse_channel(struct fwnode_handle *child)
+{
+	u32 reg, ain[2];
+	int ret;
+
+	ret = fwnode_property_read_u32(child, "reg", &reg);
+	if (ret)
+		return ret;
+
+	if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR))
+		return -EINVAL;
+
+	ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
+					     ARRAY_SIZE(ain));
+	if (ret)
+		return ret;
+
+	if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
+	    !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
+		return -EINVAL;
+
+	ad7194_channels[reg].channel = ain[0];
+	ad7194_channels[reg].channel2 = ain[1];
+	ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7192_parse_channels(struct ad7192_state *st)
+{
+	struct device *dev = &st->sd.spi->dev;
+	struct fwnode_handle *child;
+	int ret;
+
+	device_for_each_child_node(dev, child) {
+		ret = ad7192_parse_channel(child);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	[ID_AD7190] = {
 		.chip_id = CHIPID_AD7190,
@@ -1031,6 +1145,14 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7193_channels),
 		.info = &ad7192_info,
 	},
+	[ID_AD7194] = {
+		.chip_id = CHIPID_AD7194,
+		.name = "ad7194",
+		.channels = ad7194_channels,
+		.num_channels = ARRAY_SIZE(ad7194_channels),
+		.info = &ad7194_info,
+		.parse_channels = ad7192_parse_channels,
+	},
 	[ID_AD7195] = {
 		.chip_id = CHIPID_AD7195,
 		.name = "ad7195",
@@ -1142,6 +1264,12 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
+	if (st->chip_info->parse_channels) {
+		ret = st->chip_info->parse_channels(st);
+		if (ret)
+			return ret;
+	}
+
 	ret = ad7192_setup(st);
 	if (ret)
 		return ret;
@@ -1153,6 +1281,7 @@ static const struct of_device_id ad7192_of_match[] = {
 	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
 	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
 	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+	{ .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
 	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1162,6 +1291,7 @@ static const struct spi_device_id ad7192_ids[] = {
 	{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
 	{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
 	{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
+	{ "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
 	{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1178,6 +1308,6 @@ static struct spi_driver ad7192_driver = {
 module_spi_driver(ad7192_driver);
 
 MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
+MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
-- 
2.34.1

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
>
> The default configuration is hardcoded in order to have a stable number
> of channels.
>
> Also modify config AD7192 description for better scaling.
>
> Moved ad7192_chip_info struct definition to allow use of callback
> function parse_channels().
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |  11 ++-
>  drivers/iio/adc/ad7192.c | 150 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 148 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 59ae1d17b50d..8062a4d1cbe7 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -71,12 +71,17 @@ config AD7124
>           called ad7124.
>
>  config AD7192
> -       tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> +       tristate "Analog Devices AD7192 and similar ADC driver"
>         depends on SPI
>         select AD_SIGMA_DELTA
>         help
> -         Say yes here to build support for Analog Devices AD7190,
> -         AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> +         Say yes here to build support for Analog Devices SPI analog to digital
> +         converters (ADC):
> +         - AD7190
> +         - AD7192
> +         - AD7193
> +         - AD7194
> +         - AD7195
>           If unsure, say N (but it's safe to say "Y").
>
>           To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index d8393ac048e7..a3ff60ed6f63 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7192 and similar SPI ADC driver
>   *
>   * Copyright 2011-2015 Analog Devices Inc.
>   */
> @@ -125,10 +125,39 @@
>  #define AD7193_CH_AIN8         0x480 /* AIN7 - AINCOM */
>  #define AD7193_CH_AINCOM       0x600 /* AINCOM - AINCOM */
>
> +#define AD7194_CH_TEMP         0x100 /* Temp sensor */
> +#define AD7194_CH_AIN1         0x400 /* AIN1 - AINCOM */
> +#define AD7194_CH_AIN2         0x410 /* AIN2 - AINCOM */
> +#define AD7194_CH_AIN3         0x420 /* AIN3 - AINCOM */
> +#define AD7194_CH_AIN4         0x430 /* AIN4 - AINCOM */
> +#define AD7194_CH_AIN5         0x440 /* AIN5 - AINCOM */
> +#define AD7194_CH_AIN6         0x450 /* AIN6 - AINCOM */
> +#define AD7194_CH_AIN7         0x460 /* AIN7 - AINCOM */
> +#define AD7194_CH_AIN8         0x470 /* AIN8 - AINCOM */
> +#define AD7194_CH_AIN9         0x480 /* AIN9 - AINCOM */
> +#define AD7194_CH_AIN10                0x490 /* AIN10 - AINCOM */
> +#define AD7194_CH_AIN11                0x4A0 /* AIN11 - AINCOM */
> +#define AD7194_CH_AIN12                0x4B0 /* AIN12 - AINCOM */
> +#define AD7194_CH_AIN13                0x4C0 /* AIN13 - AINCOM */
> +#define AD7194_CH_AIN14                0x4D0 /* AIN14 - AINCOM */
> +#define AD7194_CH_AIN15                0x4E0 /* AIN15 - AINCOM */
> +#define AD7194_CH_AIN16                0x4F0 /* AIN16 - AINCOM */
> +#define AD7194_CH_POS_MASK     GENMASK(7, 4)
> +#define AD7194_CH_POS(x)       FIELD_PREP(AD7194_CH_POS_MASK, (x))

AD7194_CH_POS_MASK isn't used elsewhere, so maybe just this?

#define AD7194_CH_POS(x)       FIELD_PREP(GENMASK(7, 4), (x))

> +#define AD7194_CH_NEG_MASK     GENMASK(3, 0)
> +#define AD7194_CH_NEG(x)       FIELD_PREP(AD7194_CH_NEG_MASK, (x))
> +#define AD7194_CH_DIFF(pos, neg) \
> +               (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))

You could also add `((neg) == 0 ? BIT(10) : 0) | ...` and then use
this macro to define AD7194_CH_AINx.

#define AD7194_CH_AIN1    AD7194_CH_DIFF(1, 0)

> +#define AD7194_CH_DIFF_START   0
> +#define AD7194_CH_DIFF_NR      8
> +#define AD7194_CH_AIN_START    1
> +#define AD7194_CH_AIN_NR       16
> +
>  /* ID Register Bit Designations (AD7192_REG_ID) */
>  #define CHIPID_AD7190          0x4
>  #define CHIPID_AD7192          0x0
>  #define CHIPID_AD7193          0x2
> +#define CHIPID_AD7194          0x3
>  #define CHIPID_AD7195          0x6
>  #define AD7192_ID_MASK         GENMASK(3, 0)
>
> @@ -166,17 +195,10 @@ enum {
>         ID_AD7190,
>         ID_AD7192,
>         ID_AD7193,
> +       ID_AD7194,
>         ID_AD7195,
>  };
>
> -struct ad7192_chip_info {
> -       unsigned int                    chip_id;
> -       const char                      *name;
> -       const struct iio_chan_spec      *channels;
> -       u8                              num_channels;
> -       const struct iio_info           *info;
> -};
> -
>  struct ad7192_state {
>         const struct ad7192_chip_info   *chip_info;
>         struct regulator                *avdd;
> @@ -197,6 +219,15 @@ struct ad7192_state {
>         struct ad_sigma_delta           sd;
>  };
>
> +struct ad7192_chip_info {
> +       unsigned int                    chip_id;
> +       const char                      *name;
> +       const struct iio_chan_spec      *channels;
> +       u8                              num_channels;
> +       const struct iio_info           *info;
> +       int (*parse_channels)(struct ad7192_state *st);
> +};
> +
>  static const char * const ad7192_syscalib_modes[] = {
>         [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
>         [AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
> @@ -918,6 +949,15 @@ static const struct iio_info ad7192_info = {
>         .update_scan_mode = ad7192_update_scan_mode,
>  };
>
> +static const struct iio_info ad7194_info = {
> +       .read_raw = ad7192_read_raw,
> +       .write_raw = ad7192_write_raw,
> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> +       .read_avail = ad7192_read_avail,
> +       .validate_trigger = ad_sd_validate_trigger,
> +       .update_scan_mode = ad7192_update_scan_mode,
> +};

Isn't this identical to ad7192_info and ad7195_info now that .attrs is
removed? It seems like we could consolidate here.

> +
>  static const struct iio_info ad7195_info = {
>         .read_raw = ad7192_read_raw,
>         .write_raw = ad7192_write_raw,
> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
>         IIO_CHAN_SOFT_TIMESTAMP(14),
>  };
>
> +static struct iio_chan_spec ad7194_channels[] = {
> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),

Shouldn't these be differential channels since they are
pseudo-differential inputs measuring the difference between AINx and
AINCOM?

> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> +};

i.e. like this (where AINCOM is voltage0 AINx is voltagex)

static struct iio_chan_spec ad7194_channels[] = {
       AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
       AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
       AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
       AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
       AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
       AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
       AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
       AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
       AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
       AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
       AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
       AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
       AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
       AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
       AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
       AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
       AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
       IIO_CHAN_SOFT_TIMESTAMP(17),
};

> +
> +static int ad7192_parse_channel(struct fwnode_handle *child)
> +{
> +       u32 reg, ain[2];
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(child, "reg", &reg);
> +       if (ret)
> +               return ret;
> +
> +       if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR))
> +               return -EINVAL;
> +
> +       ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
> +                                            ARRAY_SIZE(ain));
> +       if (ret)
> +               return ret;
> +
> +       if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
> +           !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> +               return -EINVAL;
> +
> +       ad7194_channels[reg].channel = ain[0];
> +       ad7194_channels[reg].channel2 = ain[1];
> +       ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]);

... then the suggested change to AD7194_CH_DIFF above also make this
work when ain[1] is zero which should be allowed in the range check
immediately before this.

> +
> +       return 0;
> +}
> +
> +static int ad7192_parse_channels(struct ad7192_state *st)
> +{
> +       struct device *dev = &st->sd.spi->dev;
> +       struct fwnode_handle *child;
> +       int ret;
> +
> +       device_for_each_child_node(dev, child) {
> +               ret = ad7192_parse_channel(child);
> +               if (ret) {
> +                       fwnode_handle_put(child);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
>         [ID_AD7190] = {
>                 .chip_id = CHIPID_AD7190,
> @@ -1031,6 +1145,14 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
>                 .num_channels = ARRAY_SIZE(ad7193_channels),
>                 .info = &ad7192_info,
>         },
> +       [ID_AD7194] = {
> +               .chip_id = CHIPID_AD7194,
> +               .name = "ad7194",
> +               .channels = ad7194_channels,
> +               .num_channels = ARRAY_SIZE(ad7194_channels),
> +               .info = &ad7194_info,
> +               .parse_channels = ad7192_parse_channels,
> +       },
>         [ID_AD7195] = {
>                 .chip_id = CHIPID_AD7195,
>                 .name = "ad7195",
> @@ -1142,6 +1264,12 @@ static int ad7192_probe(struct spi_device *spi)
>                 }
>         }
>
> +       if (st->chip_info->parse_channels) {
> +               ret = st->chip_info->parse_channels(st);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         ret = ad7192_setup(st);
>         if (ret)
>                 return ret;
> @@ -1153,6 +1281,7 @@ static const struct of_device_id ad7192_of_match[] = {
>         { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
>         { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
>         { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +       { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
>         { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
>         {}
>  };
> @@ -1162,6 +1291,7 @@ static const struct spi_device_id ad7192_ids[] = {
>         { "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
>         { "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
>         { "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
> +       { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
>         { "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
>         {}
>  };
> @@ -1178,6 +1308,6 @@ static struct spi_driver ad7192_driver = {
>  module_spi_driver(ad7192_driver);
>
>  MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
> +MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
> --
> 2.34.1
>

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

Hello and thank you for the feedback!

On 09.02.2024 00:27, David Lechner wrote:
> On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
>>
>> Unlike the other AD719Xs, AD7194 has configurable differential
>> channels. The default configuration for these channels can be changed
>> from the devicetree.

...

>>
>> +static const struct iio_info ad7194_info = {
>> +       .read_raw = ad7192_read_raw,
>> +       .write_raw = ad7192_write_raw,
>> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
>> +       .read_avail = ad7192_read_avail,
>> +       .validate_trigger = ad_sd_validate_trigger,
>> +       .update_scan_mode = ad7192_update_scan_mode,
>> +};
> 
> Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> removed? It seems like we could consolidate here.

Those are not exactly identical since: 92 has bridge switch attribute, 
95 has bridge switch and ac excitation attributes and 94 has no custom 
attributes. I used a different info structure for 94 in order to avoid 
showing extra attributes.

> 
>> +
>>   static const struct iio_info ad7195_info = {
>>          .read_raw = ad7192_read_raw,
>>          .write_raw = ad7192_write_raw,
>> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
>>          IIO_CHAN_SOFT_TIMESTAMP(14),
>>   };
>>
>> +static struct iio_chan_spec ad7194_channels[] = {
>> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
>> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
>> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
>> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
>> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
>> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
>> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
>> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
>> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
>> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
>> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
>> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
>> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
>> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
>> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
>> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
>> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
>> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
>> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
>> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
>> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
>> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
>> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
>> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
>> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
> 
> Shouldn't these be differential channels since they are
> pseudo-differential inputs measuring the difference between AINx and
> AINCOM?
> 
>> +       IIO_CHAN_SOFT_TIMESTAMP(25),
>> +};
> 
> i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> 
> static struct iio_chan_spec ad7194_channels[] = {
>         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
>         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
>         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
>         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
>         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
>         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
>         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
>         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
>         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
>         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
>         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
>         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
>         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
>         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
>         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
>         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
>         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
>         IIO_CHAN_SOFT_TIMESTAMP(17),
> };
> 

I tried to follow the existing style of the driver: for each 
pseudo-differential channel(AINx - AINCOM) there is an iio channel like 
this in_voltagex_raw; and for each differential channel(AINx - AINy) 
there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194 
has 16 pseudo-differential channels/8 fully differential channels so I 
thought the (AINx - AINCOM) channels should be static and only the 8 
differential ones could be configured by the user from the devicetree by 
choosing the input pins.

The existing style of the driver, AD7192 has 4 pseudo differential 
channels and 2 (non configurable) differential channels:
static const struct iio_chan_spec ad7192_channels[] = {
	AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
	AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
	AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
	AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
	AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
	AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
	AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
	AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
	IIO_CHAN_SOFT_TIMESTAMP(8),
};

Would it be better to respect the existing style or to do like you 
suggested and have a total of 16 differential channels that are 
configurable from the device tree?

Kind regards,
Alisa-Dariana Roman

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Hello and thank you for the feedback!
>
> On 09.02.2024 00:27, David Lechner wrote:
> > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> > <alisadariana@gmail.com> wrote:
> >>
> >> Unlike the other AD719Xs, AD7194 has configurable differential
> >> channels. The default configuration for these channels can be changed
> >> from the devicetree.
>
> ...
>
> >>
> >> +static const struct iio_info ad7194_info = {
> >> +       .read_raw = ad7192_read_raw,
> >> +       .write_raw = ad7192_write_raw,
> >> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> >> +       .read_avail = ad7192_read_avail,
> >> +       .validate_trigger = ad_sd_validate_trigger,
> >> +       .update_scan_mode = ad7192_update_scan_mode,
> >> +};
> >
> > Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> > removed? It seems like we could consolidate here.
>
> Those are not exactly identical since: 92 has bridge switch attribute,
> 95 has bridge switch and ac excitation attributes and 94 has no custom
> attributes. I used a different info structure for 94 in order to avoid
> showing extra attributes.
>

Ah, I see what you mean. I didn't look close enough at the other patch
removing one attribute to see that were still other attributes.

> >
> >> +
> >>   static const struct iio_info ad7195_info = {
> >>          .read_raw = ad7192_read_raw,
> >>          .write_raw = ad7192_write_raw,
> >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> >>          IIO_CHAN_SOFT_TIMESTAMP(14),
> >>   };
> >>
> >> +static struct iio_chan_spec ad7194_channels[] = {
> >> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> >> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> >> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> >> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> >> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> >> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> >> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> >> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> >> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> >> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> >> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> >> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> >> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> >> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> >> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> >> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> >> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> >> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> >> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> >> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> >> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> >> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> >> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> >> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> >> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
> >
> > Shouldn't these be differential channels since they are
> > pseudo-differential inputs measuring the difference between AINx and
> > AINCOM?
> >
> >> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> >> +};
> >
> > i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> >
> > static struct iio_chan_spec ad7194_channels[] = {
> >         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> >         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> >         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> >         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> >         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> >         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> >         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> >         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> >         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> >         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> >         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> >         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> >         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> >         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> >         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> >         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> >         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> >         IIO_CHAN_SOFT_TIMESTAMP(17),
> > };
> >
>
> I tried to follow the existing style of the driver: for each
> pseudo-differential channel(AINx - AINCOM) there is an iio channel like
> this in_voltagex_raw; and for each differential channel(AINx - AINy)
> there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
> has 16 pseudo-differential channels/8 fully differential channels so I
> thought the (AINx - AINCOM) channels should be static and only the 8
> differential ones could be configured by the user from the devicetree by
> choosing the input pins.
>
> The existing style of the driver, AD7192 has 4 pseudo differential
> channels and 2 (non configurable) differential channels:
> static const struct iio_chan_spec ad7192_channels[] = {
>         AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
>         AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
>         AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
>         AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
>         AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
>         AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
>         AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
>         AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
>         IIO_CHAN_SOFT_TIMESTAMP(8),
> };
>
> Would it be better to respect the existing style or to do like you
> suggested and have a total of 16 differential channels that are
> configurable from the device tree?

Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was
done this way since only certain combinations of inputs can be used
together. (Although I think indexes 4 to 7 should really be
differential because they are the difference between the input and
AINCOM which may not be GND, but it is probably too late to fix that.)

Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
much more configurable than AD7192 when it comes to assigning
channels. There are basically no restrictions on which inputs can be
used together. So I am still confident that my suggestion is the way
to go for AD7194. (Although I didn't actually try it on hardware, so
can't be 100% confident. But at least 90% confident :-p)

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Thu, 15 Feb 2024 11:13:19 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> >
> > Hello and thank you for the feedback!
> >
> > On 09.02.2024 00:27, David Lechner wrote:  
> > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> > > <alisadariana@gmail.com> wrote:  
> > >>
> > >> Unlike the other AD719Xs, AD7194 has configurable differential
> > >> channels. The default configuration for these channels can be changed
> > >> from the devicetree.  
> >
> > ...
> >  
> > >>
> > >> +static const struct iio_info ad7194_info = {
> > >> +       .read_raw = ad7192_read_raw,
> > >> +       .write_raw = ad7192_write_raw,
> > >> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> > >> +       .read_avail = ad7192_read_avail,
> > >> +       .validate_trigger = ad_sd_validate_trigger,
> > >> +       .update_scan_mode = ad7192_update_scan_mode,
> > >> +};  
> > >
> > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> > > removed? It seems like we could consolidate here.  
> >
> > Those are not exactly identical since: 92 has bridge switch attribute,
> > 95 has bridge switch and ac excitation attributes and 94 has no custom
> > attributes. I used a different info structure for 94 in order to avoid
> > showing extra attributes.
> >  
> 
> Ah, I see what you mean. I didn't look close enough at the other patch
> removing one attribute to see that were still other attributes.
> 
> > >  
> > >> +
> > >>   static const struct iio_info ad7195_info = {
> > >>          .read_raw = ad7192_read_raw,
> > >>          .write_raw = ad7192_write_raw,
> > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> > >>          IIO_CHAN_SOFT_TIMESTAMP(14),
> > >>   };
> > >>
> > >> +static struct iio_chan_spec ad7194_channels[] = {
> > >> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> > >> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> > >> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> > >> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> > >> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> > >> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> > >> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> > >> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> > >> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> > >> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> > >> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> > >> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> > >> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> > >> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> > >> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> > >> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> > >> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> > >> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> > >> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> > >> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> > >> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> > >> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> > >> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> > >> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> > >> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),  
> > >
> > > Shouldn't these be differential channels since they are
> > > pseudo-differential inputs measuring the difference between AINx and
> > > AINCOM?
> > >  
> > >> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> > >> +};  
> > >
> > > i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> > >
> > > static struct iio_chan_spec ad7194_channels[] = {
> > >         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> > >         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> > >         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> > >         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> > >         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> > >         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> > >         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> > >         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> > >         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> > >         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> > >         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> > >         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> > >         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> > >         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> > >         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> > >         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> > >         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> > >         IIO_CHAN_SOFT_TIMESTAMP(17),
> > > };
> > >  
> >
> > I tried to follow the existing style of the driver: for each
> > pseudo-differential channel(AINx - AINCOM) there is an iio channel like
> > this in_voltagex_raw; and for each differential channel(AINx - AINy)
> > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
> > has 16 pseudo-differential channels/8 fully differential channels so I
> > thought the (AINx - AINCOM) channels should be static and only the 8
> > differential ones could be configured by the user from the devicetree by
> > choosing the input pins.
> >
> > The existing style of the driver, AD7192 has 4 pseudo differential
> > channels and 2 (non configurable) differential channels:
> > static const struct iio_chan_spec ad7192_channels[] = {
> >         AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
> >         AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
> >         AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
> >         AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
> >         AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
> >         AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
> >         AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
> >         AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
> >         IIO_CHAN_SOFT_TIMESTAMP(8),
> > };
> >
> > Would it be better to respect the existing style or to do like you
> > suggested and have a total of 16 differential channels that are
> > configurable from the device tree?  
> 
> Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was
> done this way since only certain combinations of inputs can be used
> together. (Although I think indexes 4 to 7 should really be
> differential because they are the difference between the input and
> AINCOM which may not be GND, but it is probably too late to fix that.)
Ground is never absolute anyway, but we could indeed be relative to something
that changes. Ah well - no one has asked for it on that part I guess
so not important.

> 
> Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> much more configurable than AD7192 when it comes to assigning
> channels. There are basically no restrictions on which inputs can be
> used together. So I am still confident that my suggestion is the way
> to go for AD7194. (Although I didn't actually try it on hardware, so
> can't be 100% confident. But at least 90% confident :-p)
You would have to define a channel number for aincom.  There is an explicit
example in the datasheet of it being at 2.5V using a reference supply.

I wonder what expectation here is.  Allways a reference regulator on that pin, or
an actually varying input?  Maybe in long term we want to support both
options - so if aincom-supply is provided these are single ended with
an offset, but if not they are differential channels between channel X and
channel AINCOM.

Note though that this mode is described a pseudo differential which normally
means a fixed voltage on the negative.

So gut feeling from me is treat them as single ended and add an
aincom-supply + the offsets that result if that is provided in DT and
voltage from it is non 0.

What fun.

Jonathan

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 15 Feb 2024 11:13:19 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>

...

> >
> > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> > much more configurable than AD7192 when it comes to assigning
> > channels. There are basically no restrictions on which inputs can be
> > used together. So I am still confident that my suggestion is the way
> > to go for AD7194. (Although I didn't actually try it on hardware, so
> > can't be 100% confident. But at least 90% confident :-p)
>
> You would have to define a channel number for aincom.  There is an explicit
> example in the datasheet of it being at 2.5V using a reference supply.
>
> I wonder what expectation here is.  Allways a reference regulator on that pin, or
> an actually varying input? Maybe in long term we want to support both
> options - so if aincom-supply is provided these are single ended with
> an offset, but if not they are differential channels between channel X and
> channel AINCOM.
>
> Note though that this mode is described a pseudo differential which normally
> means a fixed voltage on the negative.
>
> So gut feeling from me is treat them as single ended and add an
> aincom-supply + the offsets that result if that is provided in DT and
> voltage from it is non 0.

Calling AINCOM a supply doesn't sound right to me since usually this
signal is coming somewhere external, i.e. you have a twisted pair
connected to AIN1 and AINCOM going to some signal source that may be
hot-pluggable and not known at compile time. As an example, if AINCOM
was modeled as a supply, then we would have to change the device tree
every time we changed the voltage offset on the signal generator while
we are testing using an evaluation board.

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Fri, 16 Feb 2024 10:57:33 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 15 Feb 2024 11:13:19 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> 
> ...
> 
> > >
> > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> > > much more configurable than AD7192 when it comes to assigning
> > > channels. There are basically no restrictions on which inputs can be
> > > used together. So I am still confident that my suggestion is the way
> > > to go for AD7194. (Although I didn't actually try it on hardware, so
> > > can't be 100% confident. But at least 90% confident :-p)  
> >
> > You would have to define a channel number for aincom.  There is an explicit
> > example in the datasheet of it being at 2.5V using a reference supply.
> >
> > I wonder what expectation here is.  Allways a reference regulator on that pin, or
> > an actually varying input? Maybe in long term we want to support both
> > options - so if aincom-supply is provided these are single ended with
> > an offset, but if not they are differential channels between channel X and
> > channel AINCOM.
> >
> > Note though that this mode is described a pseudo differential which normally
> > means a fixed voltage on the negative.
> >
> > So gut feeling from me is treat them as single ended and add an
> > aincom-supply + the offsets that result if that is provided in DT and
> > voltage from it is non 0.  
> 
> Calling AINCOM a supply doesn't sound right to me since usually this
> signal is coming somewhere external, i.e. you have a twisted pair
> connected to AIN1 and AINCOM going to some signal source that may be
> hot-pluggable and not known at compile time. As an example, if AINCOM
> was modeled as a supply, then we would have to change the device tree
> every time we changed the voltage offset on the signal generator while
> we are testing using an evaluation board.

We tend to stick away from designing features to support testing with
devboards where external wiring is involved because anything could be
wired up there. (Examples are things like shunt resistors - normally
they are DT only) So sometimes it's a bit painful to work with such boards.
The main focus has to be production devices or at least stable set ups
where a fixed DT is sufficient.

So I'm more interested in focusing on production device use cases.
Do we have an information on how this is this used in those environments?

Jonathan

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Sat, Feb 17, 2024 at 10:25 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 16 Feb 2024 10:57:33 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Thu, 15 Feb 2024 11:13:19 -0600
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> >
> > ...
> >
> > > >
> > > > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> > > > much more configurable than AD7192 when it comes to assigning
> > > > channels. There are basically no restrictions on which inputs can be
> > > > used together. So I am still confident that my suggestion is the way
> > > > to go for AD7194. (Although I didn't actually try it on hardware, so
> > > > can't be 100% confident. But at least 90% confident :-p)
> > >
> > > You would have to define a channel number for aincom.  There is an explicit
> > > example in the datasheet of it being at 2.5V using a reference supply.
> > >
> > > I wonder what expectation here is.  Allways a reference regulator on that pin, or
> > > an actually varying input? Maybe in long term we want to support both
> > > options - so if aincom-supply is provided these are single ended with
> > > an offset, but if not they are differential channels between channel X and
> > > channel AINCOM.
> > >
> > > Note though that this mode is described a pseudo differential which normally
> > > means a fixed voltage on the negative.
> > >
> > > So gut feeling from me is treat them as single ended and add an
> > > aincom-supply + the offsets that result if that is provided in DT and
> > > voltage from it is non 0.
> >
> > Calling AINCOM a supply doesn't sound right to me since usually this
> > signal is coming somewhere external, i.e. you have a twisted pair
> > connected to AIN1 and AINCOM going to some signal source that may be
> > hot-pluggable and not known at compile time. As an example, if AINCOM
> > was modeled as a supply, then we would have to change the device tree
> > every time we changed the voltage offset on the signal generator while
> > we are testing using an evaluation board.
>
> We tend to stick away from designing features to support testing with
> devboards where external wiring is involved because anything could be
> wired up there. (Examples are things like shunt resistors - normally
> they are DT only) So sometimes it's a bit painful to work with such boards.
> The main focus has to be production devices or at least stable set ups
> where a fixed DT is sufficient.
>
> So I'm more interested in focusing on production device use cases.
> Do we have an information on how this is this used in those environments?
>

Point taken. I also checked with an apps engineer at ADI and it does
sound like AINCOM should be a supply.

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Thu, Feb 15, 2024 at 11:13 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> >
> > Hello and thank you for the feedback!
> >
> > On 09.02.2024 00:27, David Lechner wrote:
> > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> > > <alisadariana@gmail.com> wrote:
> > >>
> > >> Unlike the other AD719Xs, AD7194 has configurable differential
> > >> channels. The default configuration for these channels can be changed
> > >> from the devicetree.
> >
> > ...
> >
> > >>
> > >> +static const struct iio_info ad7194_info = {
> > >> +       .read_raw = ad7192_read_raw,
> > >> +       .write_raw = ad7192_write_raw,
> > >> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> > >> +       .read_avail = ad7192_read_avail,
> > >> +       .validate_trigger = ad_sd_validate_trigger,
> > >> +       .update_scan_mode = ad7192_update_scan_mode,
> > >> +};
> > >
> > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> > > removed? It seems like we could consolidate here.
> >
> > Those are not exactly identical since: 92 has bridge switch attribute,
> > 95 has bridge switch and ac excitation attributes and 94 has no custom
> > attributes. I used a different info structure for 94 in order to avoid
> > showing extra attributes.
> >
>
> Ah, I see what you mean. I didn't look close enough at the other patch
> removing one attribute to see that were still other attributes.
>
> > >
> > >> +
> > >>   static const struct iio_info ad7195_info = {
> > >>          .read_raw = ad7192_read_raw,
> > >>          .write_raw = ad7192_write_raw,
> > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> > >>          IIO_CHAN_SOFT_TIMESTAMP(14),
> > >>   };
> > >>
> > >> +static struct iio_chan_spec ad7194_channels[] = {
> > >> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> > >> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> > >> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> > >> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> > >> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> > >> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> > >> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> > >> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> > >> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> > >> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> > >> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> > >> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> > >> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> > >> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> > >> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> > >> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> > >> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> > >> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> > >> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> > >> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> > >> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> > >> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> > >> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> > >> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> > >> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
> > >
> > > Shouldn't these be differential channels since they are
> > > pseudo-differential inputs measuring the difference between AINx and
> > > AINCOM?
> > >
> > >> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> > >> +};
> > >
> > > i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> > >
> > > static struct iio_chan_spec ad7194_channels[] = {
> > >         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> > >         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> > >         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> > >         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> > >         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> > >         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> > >         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> > >         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> > >         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> > >         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> > >         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> > >         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> > >         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> > >         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> > >         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> > >         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> > >         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> > >         IIO_CHAN_SOFT_TIMESTAMP(17),
> > > };
> > >
> >
> > I tried to follow the existing style of the driver: for each
> > pseudo-differential channel(AINx - AINCOM) there is an iio channel like
> > this in_voltagex_raw; and for each differential channel(AINx - AINy)
> > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
> > has 16 pseudo-differential channels/8 fully differential channels so I
> > thought the (AINx - AINCOM) channels should be static and only the 8
> > differential ones could be configured by the user from the devicetree by
> > choosing the input pins.
> >
> > The existing style of the driver, AD7192 has 4 pseudo differential
> > channels and 2 (non configurable) differential channels:
> > static const struct iio_chan_spec ad7192_channels[] = {
> >         AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
> >         AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
> >         AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
> >         AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
> >         AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
> >         AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
> >         AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
> >         AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
> >         IIO_CHAN_SOFT_TIMESTAMP(8),
> > };
> >
> > Would it be better to respect the existing style or to do like you
> > suggested and have a total of 16 differential channels that are
> > configurable from the device tree?


Now that we have it sorted that AINCOM should be a supply, it does
sound like we should more closely follow the pattern from AD7192. But
to cover every possible programmable combination of AINx to AINy, we
would need 256 differential channels (16 * 16) in addition to the
other channels. Realistically, we probably don't need that many
though. Since I see that AD7192 has a differential channel where uses
AIN2 for both + and - I guess having 16 differential channels that are
configured via device tree would be enough to allow all 16 AINx inputs
to be used this way. Is there a use case where the same AINx would be
assigned to multiple channels at the same time?

>
> Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was
> done this way since only certain combinations of inputs can be used
> together. (Although I think indexes 4 to 7 should really be
> differential because they are the difference between the input and
> AINCOM which may not be GND, but it is probably too late to fix that.)
>
> Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> much more configurable than AD7192 when it comes to assigning
> channels. There are basically no restrictions on which inputs can be
> used together. So I am still confident that my suggestion is the way
> to go for AD7194. (Although I didn't actually try it on hardware, so
> can't be 100% confident. But at least 90% confident :-p)

Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Mon, 19 Feb 2024 10:33:45 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Thu, Feb 15, 2024 at 11:13 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> > <alisadariana@gmail.com> wrote:  
> > >
> > > Hello and thank you for the feedback!
> > >
> > > On 09.02.2024 00:27, David Lechner wrote:  
> > > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> > > > <alisadariana@gmail.com> wrote:  
> > > >>
> > > >> Unlike the other AD719Xs, AD7194 has configurable differential
> > > >> channels. The default configuration for these channels can be changed
> > > >> from the devicetree.  
> > >
> > > ...
> > >  
> > > >>
> > > >> +static const struct iio_info ad7194_info = {
> > > >> +       .read_raw = ad7192_read_raw,
> > > >> +       .write_raw = ad7192_write_raw,
> > > >> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> > > >> +       .read_avail = ad7192_read_avail,
> > > >> +       .validate_trigger = ad_sd_validate_trigger,
> > > >> +       .update_scan_mode = ad7192_update_scan_mode,
> > > >> +};  
> > > >
> > > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> > > > removed? It seems like we could consolidate here.  
> > >
> > > Those are not exactly identical since: 92 has bridge switch attribute,
> > > 95 has bridge switch and ac excitation attributes and 94 has no custom
> > > attributes. I used a different info structure for 94 in order to avoid
> > > showing extra attributes.
> > >  
> >
> > Ah, I see what you mean. I didn't look close enough at the other patch
> > removing one attribute to see that were still other attributes.
> >  
> > > >  
> > > >> +
> > > >>   static const struct iio_info ad7195_info = {
> > > >>          .read_raw = ad7192_read_raw,
> > > >>          .write_raw = ad7192_write_raw,
> > > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> > > >>          IIO_CHAN_SOFT_TIMESTAMP(14),
> > > >>   };
> > > >>
> > > >> +static struct iio_chan_spec ad7194_channels[] = {
> > > >> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> > > >> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> > > >> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> > > >> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> > > >> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> > > >> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> > > >> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> > > >> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> > > >> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> > > >> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> > > >> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> > > >> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> > > >> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> > > >> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> > > >> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> > > >> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> > > >> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> > > >> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> > > >> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> > > >> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> > > >> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> > > >> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> > > >> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> > > >> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> > > >> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),  
> > > >
> > > > Shouldn't these be differential channels since they are
> > > > pseudo-differential inputs measuring the difference between AINx and
> > > > AINCOM?
> > > >  
> > > >> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> > > >> +};  
> > > >
> > > > i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> > > >
> > > > static struct iio_chan_spec ad7194_channels[] = {
> > > >         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> > > >         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> > > >         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> > > >         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> > > >         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> > > >         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> > > >         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> > > >         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> > > >         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> > > >         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> > > >         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> > > >         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> > > >         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> > > >         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> > > >         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> > > >         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> > > >         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> > > >         IIO_CHAN_SOFT_TIMESTAMP(17),
> > > > };
> > > >  
> > >
> > > I tried to follow the existing style of the driver: for each
> > > pseudo-differential channel(AINx - AINCOM) there is an iio channel like
> > > this in_voltagex_raw; and for each differential channel(AINx - AINy)
> > > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
> > > has 16 pseudo-differential channels/8 fully differential channels so I
> > > thought the (AINx - AINCOM) channels should be static and only the 8
> > > differential ones could be configured by the user from the devicetree by
> > > choosing the input pins.
> > >
> > > The existing style of the driver, AD7192 has 4 pseudo differential
> > > channels and 2 (non configurable) differential channels:
> > > static const struct iio_chan_spec ad7192_channels[] = {
> > >         AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
> > >         AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
> > >         AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
> > >         AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
> > >         AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
> > >         AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
> > >         AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
> > >         AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
> > >         IIO_CHAN_SOFT_TIMESTAMP(8),
> > > };
> > >
> > > Would it be better to respect the existing style or to do like you
> > > suggested and have a total of 16 differential channels that are
> > > configurable from the device tree?  
> 
> 
> Now that we have it sorted that AINCOM should be a supply, it does
> sound like we should more closely follow the pattern from AD7192. But
> to cover every possible programmable combination of AINx to AINy, we
> would need 256 differential channels (16 * 16) in addition to the
> other channels. Realistically, we probably don't need that many
> though. Since I see that AD7192 has a differential channel where uses
> AIN2 for both + and - I guess having 16 differential channels that are
> configured via device tree would be enough to allow all 16 AINx inputs
> to be used this way. Is there a use case where the same AINx would be
> assigned to multiple channels at the same time?

If there are very large numbers of options, common solution is to
move to dynamic assignment and channel nodes so DT specifies what is
wired.  In theory we then allow all combinations at the same time but
rely on common sense to restrict it.  I don't suggest channel nodes
for most drivers because it adds complexity and a few unwired channels
being exposed is rarely a problem (mostly people buy the right sized ADC).
For cases like this though it can reduce things to a manageable level.

Also little purpose in supporting 1-2 and 2-1 which can simplify things
somewhat. However that can also be left unconstrained on assumption
common sense will be exercised in the DT.


> 
> >
> > Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was
> > done this way since only certain combinations of inputs can be used
> > together. (Although I think indexes 4 to 7 should really be
> > differential because they are the difference between the input and
> > AINCOM which may not be GND, but it is probably too late to fix that.)
> >
> > Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> > much more configurable than AD7192 when it comes to assigning
> > channels. There are basically no restrictions on which inputs can be
> > used together. So I am still confident that my suggestion is the way
> > to go for AD7194. (Although I didn't actually try it on hardware, so
> > can't be 100% confident. But at least 90% confident :-p)