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

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

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

Thank you!

v5 -> v6
  - protect ad7192_update_filter_freq_avail with lock
  - better bindings description for AINCOM
  - the pseudo-differential channels are no longer configured as differential
    when aincom supply is not present in devicetree, in this case the offset for
    the channels is set to 0
  - because of the above change, there is no longer a need for multiple channel
    options
  - correct channels regex in bindings
  - no need to move chip_info anymore
  - change names to ad7194_parse_channel/s
  - add else statement to highlight parse_channels effect

v4 -> v5
  - add aincom supply as discussed previously
    https://lore.kernel.org/all/CAMknhBF5mAsN1c-194Qwa5oKmqKzef2khXnqA1cSdKpWHKWp0w@mail.gmail.com/#t
  - ad7194 differential channels are now dynamically configured in the
    devicetree

v3 -> v4
  - drop device properties patch, changes already applied to tree
  - change bindings and driver such that for AD7194 there are 16
    differential channels, by default set to AINx - AINCOM, which can be
    configured in devicetree however the user likes
  - corrected mistake regarding positive and negative channel macros:
    subtract 1 from the number corresponding to AIN input

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 standard attribute
  dt-bindings: iio: adc: ad7192: Add aincom supply
  iio: adc: ad7192: Add aincom supply
  dt-bindings: iio: adc: ad7192: Add AD7194 support
  iio: adc: ad7192: Add AD7194 support

 .../bindings/iio/adc/adi,ad7192.yaml          |  83 +++++++
 drivers/iio/adc/Kconfig                       |  11 +-
 drivers/iio/adc/ad7192.c                      | 226 ++++++++++++++----
 3 files changed, 274 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH v6 1/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>
Reviewed-by: David Lechner <dlechner@baylibre.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 7bcc7e2aa2a2..fe8dbb68a8ba 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -190,6 +190,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;
@@ -473,6 +474,16 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
 	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;
 }
 
@@ -586,48 +597,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);
@@ -637,7 +624,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
 };
@@ -647,7 +633,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
@@ -665,17 +650,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;
@@ -826,6 +809,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);
@@ -845,6 +829,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
 						3, st->mode);
 				break;
 			}
+		ad7192_update_filter_freq_avail(st);
 		mutex_unlock(&st->lock);
 		break;
 	default:
@@ -888,6 +873,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;
@@ -956,7 +947,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 v6 1/5] iio: adc: ad7192: Use standard attribute

[Jonathan Cameron]

On Wed, 17 Apr 2024 20:00:50 +0300
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>
> Reviewed-by: David Lechner <dlechner@baylibre.com>

Locking comment inline.  Note that I'm fairly sure there is an existing
bug because the 3db filter write isn't protecting st->conf.

I only noticed this because I was checking you'd fixed the locking issue
noted by David in v5.

Jonathan

> ---
>  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 7bcc7e2aa2a2..fe8dbb68a8ba 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -190,6 +190,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;
> @@ -473,6 +474,16 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
>  	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;
>  }
>  
> @@ -586,48 +597,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);
> @@ -637,7 +624,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
>  };
> @@ -647,7 +633,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
> @@ -665,17 +650,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;
> @@ -826,6 +809,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);

This now needs to take the mutex.
There are a bunch of writes in this update function and in theory it two
sysfs writes could occur at the same time hitting this path and the one below
resulting in an inconsistent set of values ending up in 
st->filter_freq_avail[x]

Would be a minor effect and is pretty unlikely but might as well close it
down.

Note that I'm fairly sure the update of the 3db filter frequency is already
capable of corrupting st->conf because it doesn't take the mutex.

With that in mind, I'd suggestion moving the mutex outside the switch statement.




>  		break;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000);
> @@ -845,6 +829,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>  						3, st->mode);
>  				break;
>  			}
> +		ad7192_update_filter_freq_avail(st);
>  		mutex_unlock(&st->lock);
>  		break;
>  	default:
> @@ -888,6 +873,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;
> @@ -956,7 +947,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 = { \

[PATCH v6 2/5] dt-bindings: iio: adc: ad7192: Add aincom supply

[Alisa-Dariana Roman]

AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
in pseduo-differential operation mode. AINCOM voltage represets the
offset of corresponding channels.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 16def2985ab4..cf5c568f140a 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -41,6 +41,11 @@ properties:
   interrupts:
     maxItems: 1
 
+  aincom-supply:
+    description: |
+      AINCOM voltage supply. Analog inputs AINx are referenced to this input
+      when configured for pseudo-differential operation.
+
   dvdd-supply:
     description: DVdd voltage supply
 
@@ -117,6 +122,7 @@ examples:
             clock-names = "mclk";
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
+            aincom-supply = <&aincom>;
             dvdd-supply = <&dvdd>;
             avdd-supply = <&avdd>;
             vref-supply = <&vref>;
-- 
2.34.1

Re: [PATCH v6 2/5] dt-bindings: iio: adc: ad7192: Add aincom supply

[Rob Herring]

On Wed, 17 Apr 2024 20:00:51 +0300, Alisa-Dariana Roman wrote:
> AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
> in pseduo-differential operation mode. AINCOM voltage represets the
> offset of corresponding channels.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v6 3/5] iio: adc: ad7192: Add aincom supply

[Alisa-Dariana Roman]

AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
in pseduo-differential operation mode. AINCOM voltage represets the
offset of corresponding channels.

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

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index fe8dbb68a8ba..8d56cf889973 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -186,6 +186,7 @@ struct ad7192_state {
 	struct regulator		*vref;
 	struct clk			*mclk;
 	u16				int_vref_mv;
+	u32				aincom_mv;
 	u32				fclk;
 	u32				mode;
 	u32				conf;
@@ -742,10 +743,19 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
 			*val = -(1 << (chan->scan_type.realbits - 1));
 		else
 			*val = 0;
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (st->aincom_mv && !chan->differential)
+				*val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
+							      st->scale_avail[gain][1]);
+			return IIO_VAL_INT;
 		/* Kelvin to Celsius */
-		if (chan->type == IIO_TEMP)
+		case IIO_TEMP:
 			*val -= 273 * ad7192_get_temp_scale(unipolar);
-		return IIO_VAL_INT;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
 		return IIO_VAL_INT;
@@ -1052,6 +1062,7 @@ static int ad7192_probe(struct spi_device *spi)
 {
 	struct ad7192_state *st;
 	struct iio_dev *indio_dev;
+	struct regulator *aincom;
 	int ret;
 
 	if (!spi->irq) {
@@ -1067,6 +1078,27 @@ static int ad7192_probe(struct spi_device *spi)
 
 	mutex_init(&st->lock);
 
+	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
+	if (!IS_ERR(aincom)) {
+		ret = regulator_enable(aincom);
+		if (ret) {
+			dev_err(&spi->dev, "Failed to enable specified AINCOM supply\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(aincom);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "Device tree error, AINCOM voltage undefined\n");
+		st->aincom_mv = ret / 1000;
+	} else {
+		st->aincom_mv = 0;
+	}
+
 	st->avdd = devm_regulator_get(&spi->dev, "avdd");
 	if (IS_ERR(st->avdd))
 		return PTR_ERR(st->avdd);
@@ -1113,6 +1145,7 @@ static int ad7192_probe(struct spi_device *spi)
 	st->int_vref_mv = ret / 1000;
 
 	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;
-- 
2.34.1

Re: [PATCH v6 3/5] iio: adc: ad7192: Add aincom supply

[Andy Shevchenko]

On Wed, Apr 17, 2024 at 8:01 PM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
> in pseduo-differential operation mode. AINCOM voltage represets the

pseudo

> offset of corresponding channels.

...

> +               case IIO_VOLTAGE:
> +                       if (st->aincom_mv && !chan->differential)
> +                               *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,

It's quite easy to make a mistake in this long constant. Can you use
an appropriate one from units.h?

> +                                                             st->scale_avail[gain][1]);
> +                       return IIO_VAL_INT;

...

> +       aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> +       if (!IS_ERR(aincom)) {

Why not a positive condition?

> +               ret = regulator_enable(aincom);
> +               if (ret) {

> +                       dev_err(&spi->dev, "Failed to enable specified AINCOM supply\n");

return dev_err_probe();

> +                       return ret;
> +               }
> +
> +               ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> +               if (ret)
> +                       return ret;
> +
> +               ret = regulator_get_voltage(aincom);
> +               if (ret < 0)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Device tree error, AINCOM voltage undefined\n");
> +               st->aincom_mv = ret / 1000;
> +       } else {
> +               st->aincom_mv = 0;
> +       }

...

> @@ -1113,6 +1145,7 @@ static int ad7192_probe(struct spi_device *spi)
>         st->int_vref_mv = ret / 1000;
>
>         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;

Stray change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7192: Add aincom supply

[Jonathan Cameron]

On Wed, 17 Apr 2024 20:00:52 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
> in pseduo-differential operation mode. AINCOM voltage represets the
> offset of corresponding channels.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
One request inline to make my life easier :)

Otherwise, I noticed a subset of what Andy picked up on so won't
repeat his comments!

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index fe8dbb68a8ba..8d56cf889973 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -186,6 +186,7 @@ struct ad7192_state {
>  	struct regulator		*vref;
>  	struct clk			*mclk;
>  	u16				int_vref_mv;
> +	u32				aincom_mv;
>  	u32				fclk;
>  	u32				mode;
>  	u32				conf;
> @@ -742,10 +743,19 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>  			*val = -(1 << (chan->scan_type.realbits - 1));
>  		else
>  			*val = 0;
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->aincom_mv && !chan->differential)
> +				*val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
> +							      st->scale_avail[gain][1]);
> +			return IIO_VAL_INT;
>  		/* Kelvin to Celsius */
> -		if (chan->type == IIO_TEMP)
> +		case IIO_TEMP:
>  			*val -= 273 * ad7192_get_temp_scale(unipolar);
> -		return IIO_VAL_INT;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
>  		return IIO_VAL_INT;
> @@ -1052,6 +1062,7 @@ static int ad7192_probe(struct spi_device *spi)
>  {
>  	struct ad7192_state *st;
>  	struct iio_dev *indio_dev;
> +	struct regulator *aincom;
>  	int ret;
>  
>  	if (!spi->irq) {
> @@ -1067,6 +1078,27 @@ static int ad7192_probe(struct spi_device *spi)
>  
>  	mutex_init(&st->lock);
>  
As we are going around again, could you do me a favour and add a comment
here along the lines of.

	/*
	 * aincom is optional to maintain compatibility with older DT.
	 * Newer firmware should provide a zero volt fixed supply if
	 * this is wired to ground.	 
	 */

Aim being to discourage this getting cut and paste into other drivers
that support the common voltage from the start!


> +	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> +	if (!IS_ERR(aincom)) {
> +		ret = regulator_enable(aincom);
> +		if (ret) {
> +			dev_err(&spi->dev, "Failed to enable specified AINCOM supply\n");
> +			return ret;
> +		}
> +
> +		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(aincom);
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Device tree error, AINCOM voltage undefined\n");
> +		st->aincom_mv = ret / 1000;
> +	} else {
> +		st->aincom_mv = 0;
> +	}
> +
>  	st->avdd = devm_regulator_get(&spi->dev, "avdd");
>  	if (IS_ERR(st->avdd))
>  		return PTR_ERR(st->avdd);
> @@ -1113,6 +1145,7 @@ static int ad7192_probe(struct spi_device *spi)
>  	st->int_vref_mv = ret / 1000;
>  
>  	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;

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

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The user can dynamically configure them in 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          | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index cf5c568f140a..7e4e15e4e648 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
 
@@ -89,6 +96,30 @@ properties:
     description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
     type: boolean
 
+patternProperties:
+  "^channel@[0-9a-z]+$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description: The channel index.
+        minimum: 1
+        maximum: 256
+
+      diff-channels:
+        description: |
+          Both inputs can be connected to pins AIN1 to AIN16 by choosing the
+          appropriate value from 1 to 16.
+        items:
+          minimum: 1
+          maximum: 16
+
+    required:
+      - reg
+      - diff-channels
+
 required:
   - compatible
   - reg
@@ -103,6 +134,17 @@ required:
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          enum:
+            - adi,ad7190
+            - adi,ad7192
+            - adi,ad7193
+            - adi,ad7195
+    then:
+      patternProperties:
+        "^channel@[0-9a-z]+$": false
 
 unevaluatedProperties: false
 
@@ -133,3 +175,38 @@ examples:
             adi,burnout-currents-enable;
         };
     };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad7194";
+            reg = <0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+            spi-cpha;
+            clocks = <&ad7192_mclk>;
+            clock-names = "mclk";
+            interrupts = <25 0x2>;
+            interrupt-parent = <&gpio>;
+            aincom-supply = <&aincom>;
+            dvdd-supply = <&dvdd>;
+            avdd-supply = <&avdd>;
+            vref-supply = <&vref>;
+
+            channel@1 {
+                reg = <1>;
+                diff-channels = <1 6>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                diff-channels = <16 5>;
+            };
+        };
+    };
-- 
2.34.1

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

[Rob Herring]

On Wed, Apr 17, 2024 at 08:00:53PM +0300, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The user can dynamically configure them in 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          | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index cf5c568f140a..7e4e15e4e648 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
>  
> @@ -89,6 +96,30 @@ properties:
>      description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>      type: boolean
>  
> +patternProperties:
> +  "^channel@[0-9a-z]+$":

Unit-addresses are hex (typically). So something like:

'^channel@(100|[0-9a-f]{1,2})$'

> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description: The channel index.
> +        minimum: 1
> +        maximum: 256

Why not 0 based?


> +
> +      diff-channels:
> +        description: |

Don't need '|' if no formatting.

> +          Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> +          appropriate value from 1 to 16.
> +        items:
> +          minimum: 1
> +          maximum: 16
> +
> +    required:
> +      - reg
> +      - diff-channels

Single ended modes aren't supported?

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

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

[Jonathan Cameron]

On Thu, 18 Apr 2024 09:40:19 -0500
Rob Herring <robh@kernel.org> wrote:

> On Wed, Apr 17, 2024 at 08:00:53PM +0300, Alisa-Dariana Roman wrote:
> > Unlike the other AD719Xs, AD7194 has configurable differential
> > channels. The user can dynamically configure them in the devicetree.
> > 
> > Also add an example for AD7194 devicetree.
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

More general follow up comment inline.


> > ---
> >  .../bindings/iio/adc/adi,ad7192.yaml          | 77 +++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > index cf5c568f140a..7e4e15e4e648 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
> >  
> > @@ -89,6 +96,30 @@ properties:
> >      description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
> >      type: boolean
> >  
> > +patternProperties:
> > +  "^channel@[0-9a-z]+$":  
> 
> Unit-addresses are hex (typically). So something like:
> 
> '^channel@(100|[0-9a-f]{1,2})$'
> 
> > +    type: object
> > +    $ref: adc.yaml
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel index.
> > +        minimum: 1
> > +        maximum: 256  
> 
> Why not 0 based?
> 
> 
> > +
> > +      diff-channels:
> > +        description: |  
> 
> Don't need '|' if no formatting.
> 
> > +          Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> > +          appropriate value from 1 to 16.
> > +        items:
> > +          minimum: 1
> > +          maximum: 16
> > +
> > +    required:
> > +      - reg
> > +      - diff-channels  
> 
> Single ended modes aren't supported?
To follow up on this. 

For single ended only devices we just let reg reference the the channel.
For differential channels that never worked (because there was a pair).
So for mixed devices it's a bit of a mess.

I'd be happy if we cleaned that up by adding a binding alongside
diff-channels that was simply channel.

For single ended only devices it would be optional but it would make
this device and similar easier to describe.

We can wrap it up in a helper function that queries channel first
and falls back to reg if it isn't there.

Maybe could call it single-channel to make it obvious it's an either
or with diff-channels.

With hindsight we could have just made that take 1 or 2 elements and
given it a generic name from the start.   Ah the wonder if retro designing
interfaces... :(

New element in adc.yaml would looks something like.
  single-channel:
    $ref: /schemas/types.yaml#/definitions/uint32
    description:
      When devices combine single and differential channels, allow
      the channel for a single element to be specified, independent
      of reg (as for differentila channels). If this and
      diff-channels are not present reg shall be used instead.

Jonathan

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

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

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The user can dynamically configure them in the devicetree.

Also modify config AD7192 description for better scaling.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/Kconfig  |  11 +++-
 drivers/iio/adc/ad7192.c | 122 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..74fecc284f1a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -88,12 +88,17 @@ config AD7173
 	  called ad7173.
 
 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 8d56cf889973..dc113405f1bc 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.
  */
@@ -128,10 +128,21 @@
 #define AD7193_CH_AIN8		0x480 /* AIN7 - AINCOM */
 #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
 
+#define AD7194_CH_POS(x)	(((x) - 1) << 4)
+#define AD7194_CH_NEG(x)	((x) - 1)
+#define AD7194_CH_DIFF(pos, neg) \
+		(((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))
+#define AD7194_CH_TEMP		0x100 /* Temp sensor */
+#define AD7194_CH_BASE_NR	18
+#define AD7194_CH_AIN_START	1
+#define AD7194_CH_AIN_NR	16
+#define AD7194_CH_DIFF_NR_MAX	256
+
 /* 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)
 
@@ -169,6 +180,7 @@ enum {
 	ID_AD7190,
 	ID_AD7192,
 	ID_AD7193,
+	ID_AD7194,
 	ID_AD7195,
 };
 
@@ -178,6 +190,7 @@ struct ad7192_chip_info {
 	const struct iio_chan_spec	*channels;
 	u8				num_channels;
 	const struct iio_info		*info;
+	int (*parse_channels)(struct iio_dev *indio_dev);
 };
 
 struct ad7192_state {
@@ -931,6 +944,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,
@@ -1022,6 +1044,84 @@ static const struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static int ad7194_parse_channel(struct fwnode_handle *child,
+				struct iio_chan_spec *ad7194_channel)
+{
+	u32 ain[2];
+	int ret;
+
+	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_channel->channel = ain[0];
+	ad7194_channel->channel2 = ain[1];
+	ad7194_channel->address = AD7194_CH_DIFF(ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7194_parse_channels(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *ad7194_channels;
+	struct fwnode_handle *child;
+	struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
+	struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
+	struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
+	struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
+	unsigned int num_channels, index = 0, ain_chan;
+	int ret;
+
+	num_channels = device_get_child_node_count(dev);
+	if (num_channels > AD7194_CH_DIFF_NR_MAX)
+		return -EINVAL;
+
+	num_channels += AD7194_CH_BASE_NR;
+
+	ad7194_channels = devm_kcalloc(dev, num_channels,
+				       sizeof(*ad7194_channels), GFP_KERNEL);
+	if (!ad7194_channels)
+		return -ENOMEM;
+
+	indio_dev->channels = ad7194_channels;
+	indio_dev->num_channels = num_channels;
+
+	device_for_each_child_node(dev, child) {
+		*ad7194_channels = ad7194_chan_diff;
+		ad7194_channels->scan_index = index++;
+		ret = ad7194_parse_channel(child, ad7194_channels);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+		ad7194_channels++;
+	}
+
+	*ad7194_channels = ad7194_chan_temp;
+	ad7194_channels->scan_index = index++;
+	ad7194_channels->address = AD7194_CH_TEMP;
+	ad7194_channels++;
+
+	for (ain_chan = 1; ain_chan <= 16; ain_chan++) {
+		*ad7194_channels = ad7194_chan;
+		ad7194_channels->scan_index = index++;
+		ad7194_channels->channel = ain_chan;
+		ad7194_channels->address = AD7194_CH_DIFF(ain_chan, 0);
+		ad7194_channels++;
+	}
+
+	*ad7194_channels = ad7194_chan_timestamp;
+	ad7194_channels->scan_index = index;
+
+	return 0;
+}
+
 static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	[ID_AD7190] = {
 		.chip_id = CHIPID_AD7190,
@@ -1044,6 +1144,12 @@ 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",
+		.info = &ad7194_info,
+		.parse_channels = ad7194_parse_channels,
+	},
 	[ID_AD7195] = {
 		.chip_id = CHIPID_AD7195,
 		.name = "ad7195",
@@ -1148,9 +1254,15 @@ static int ad7192_probe(struct spi_device *spi)
 
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = st->chip_info->channels;
-	indio_dev->num_channels = st->chip_info->num_channels;
 	indio_dev->info = st->chip_info->info;
+	if (st->chip_info->parse_channels) {
+		ret = st->chip_info->parse_channels(indio_dev);
+		if (ret)
+			return ret;
+	} else {
+		indio_dev->channels = st->chip_info->channels;
+		indio_dev->num_channels = st->chip_info->num_channels;
+	}
 
 	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
 	if (ret)
@@ -1189,6 +1301,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] },
 	{}
 };
@@ -1198,6 +1311,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] },
 	{}
 };
@@ -1214,6 +1328,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 v6 5/5] iio: adc: ad7192: Add AD7194 support

[Andy Shevchenko]

On Wed, Apr 17, 2024 at 8:01 PM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The user can dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.

...

> +       device_for_each_child_node(dev, child) {

You can use scoped variant AFAIU that's available in Jonathan's tree.

> +               *ad7194_channels = ad7194_chan_diff;
> +               ad7194_channels->scan_index = index++;
> +               ret = ad7194_parse_channel(child, ad7194_channels);
> +               if (ret) {

> +                       fwnode_handle_put(child);

With the above this wouldn't be needed.

> +                       return ret;
> +               }
> +               ad7194_channels++;
> +       }

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Wed, 17 Apr 2024 20:06:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 17, 2024 at 8:01 PM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> >
> > Unlike the other AD719Xs, AD7194 has configurable differential
> > channels. The user can dynamically configure them in the devicetree.
> >
> > Also modify config AD7192 description for better scaling.  
> 
> ...
> 
> > +       device_for_each_child_node(dev, child) {  
> 
> You can use scoped variant AFAIU that's available in Jonathan's tree.
> 
> > +               *ad7194_channels = ad7194_chan_diff;
> > +               ad7194_channels->scan_index = index++;
> > +               ret = ad7194_parse_channel(child, ad7194_channels);
> > +               if (ret) {  
> 
> > +                       fwnode_handle_put(child);  
> 
> With the above this wouldn't be needed.
> 
As it's a minor improvement, and I tend not to like unnecessary
interdependence of series in my tree (until they are in char-misc
and hence no chance of them changing), I'm fine with not using
that new functionality here. 

That will change once it's upstream of course!

I will send a pull request to Greg nice and early this cycle
so that should be in my upstream soon.

I'm also fine with you using this if you want to though!

Jonathan

> > +                       return ret;
> > +               }
> > +               ad7194_channels++;
> > +       }  
> 

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

[Jonathan Cameron]

On Wed, 17 Apr 2024 20:00:54 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The user can dynamically configure them in the devicetree.
> 
> Also modify config AD7192 description for better scaling.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Late feedback (sorry!) but I think we should resolve the single ended
channel description so that it sits at the same level in DT binding
as do differential channels.  Current situation just feels inconsistent.

See DT binding reply.  Should be easy to do now, and wouldn't be possible
to add later.  It would be possible to add support for moving to
per channel DT entries for a driver that previously assumed all should be
available, but we can't easily move from assuming all single ended channels
are but differential are specified by DT child nodes.

Jonathan

> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8d56cf889973..dc113405f1bc 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@


> +static int ad7194_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *ad7194_channels;
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
> +	struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
> +	struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
> +	struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
> +	unsigned int num_channels, index = 0, ain_chan;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD7194_CH_DIFF_NR_MAX)
> +		return -EINVAL;
> +
> +	num_channels += AD7194_CH_BASE_NR;
> +
> +	ad7194_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad7194_channels), GFP_KERNEL);
> +	if (!ad7194_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad7194_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node(dev, child) {
> +		*ad7194_channels = ad7194_chan_diff;
> +		ad7194_channels->scan_index = index++;
> +		ret = ad7194_parse_channel(child, ad7194_channels);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +		ad7194_channels++;
> +	}
> +
> +	*ad7194_channels = ad7194_chan_temp;
> +	ad7194_channels->scan_index = index++;
> +	ad7194_channels->address = AD7194_CH_TEMP;
> +	ad7194_channels++;
> +
> +	for (ain_chan = 1; ain_chan <= 16; ain_chan++) {

I think it's worth making these more similar to the differential channels.
Seems odd to allow DT to provide that list, but not the same for single ended.
See comment on binding.  Should be fairly easy to add now, and if we
leave it until later, there won't be a way to move this forwards because
we won't be able to tell if no single ended channels in DT means none
are relevant, or if it means the DT file predates us adding per single ended
channel description.


> +		*ad7194_channels = ad7194_chan;
> +		ad7194_channels->scan_index = index++;
> +		ad7194_channels->channel = ain_chan;
> +		ad7194_channels->address = AD7194_CH_DIFF(ain_chan, 0);
> +		ad7194_channels++;
> +	}
> +
> +	*ad7194_channels = ad7194_chan_timestamp;
> +	ad7194_channels->scan_index = index;
> +
> +	return 0;
> +}