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

[PATCH v5 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!

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                      | 258 ++++++++++++++----
 3 files changed, 299 insertions(+), 53 deletions(-)

-- 
2.34.1

[PATCH v5 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>
---
 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..ac737221beae 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);
@@ -846,6 +830,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;
@@ -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 v5 1/5] iio: adc: ad7192: Use standard attribute

[David Lechner]

On Sat, Apr 13, 2024 at 10:12 AM 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>
> ---

With the question below addressed:

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..ac737221beae 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);
> @@ -846,6 +830,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>                                 break;
>                         }
>                 mutex_unlock(&st->lock);
> +               ad7192_update_filter_freq_avail(st);

Does this need to go inside of the mutex guard to avoid potential race
conditions?

>                 break;
>         default:
>                 ret = -EINVAL;
> @@ -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
>

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

[Alisa-Dariana Roman]

AINCOM should actually be a supply. If present and it has a non-zero
voltage, the pseudo-differential channels are configured as single-ended
with an offset. Otherwise, they are configured as differential channels
between AINx and AINCOM pins.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 16def2985ab4..ba506af3b73e 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -41,6 +41,14 @@ properties:
   interrupts:
     maxItems: 1
 
+  aincom-supply:
+    description: |
+      Optional AINCOM voltage supply. If present and it has a non-zero voltage,
+      the pseudo-differential channels are configured as single-ended channels
+      with the AINCOM voltage as offset. Otherwise, the pseudo-differential
+      channels are configured as differential channels: voltageX-voltage0, with
+      AINCOM as the negative input.
+
   dvdd-supply:
     description: DVdd voltage supply
 
@@ -117,6 +125,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 v5 2/5] dt-bindings: iio: adc: ad7192: Add aincom supply

[David Lechner]

On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> AINCOM should actually be a supply. If present and it has a non-zero
> voltage, the pseudo-differential channels are configured as single-ended
> with an offset. Otherwise, they are configured as differential channels
> between AINx and AINCOM pins.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml          | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 16def2985ab4..ba506af3b73e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -41,6 +41,14 @@ properties:
>    interrupts:
>      maxItems: 1
>
> +  aincom-supply:
> +    description: |
> +      Optional AINCOM voltage supply. If present and it has a non-zero voltage,
> +      the pseudo-differential channels are configured as single-ended channels
> +      with the AINCOM voltage as offset. Otherwise, the pseudo-differential
> +      channels are configured as differential channels: voltageX-voltage0, with
> +      AINCOM as the negative input.

This description doesn't sound quite right to me. The datasheet has no
mention of single-ended inputs. And how each AINx input is used is
independent of whether this property is present or not. And
voltageX-voltage0 is a driver implementation detail that doesn't
really belong in the bindings.

Also, just FYI, in a similar case, Jonathan recently mentioned that he
would prefer that these sorts of supplies would be required rather
than optional [1]. But in this case, I think it needs to be optional
for backwards compatibility since we are modifying existing DT
bindings. But the point still stands that this property being present
or not doesn't mean anything special (other than we might assume the
AINCOM pin is connected to GND if the property is omitted).

[1]: https://lore.kernel.org/linux-iio/20240413181025.39d1a62e@jic23-huawei/

In any case, a better description would be one more like what the
datasheet says for the AINCOM pin:

"Analog inputs AIN1 to AIN4 are referenced to this input when
configured for pseudodifferential operation."

> +
>    dvdd-supply:
>      description: DVdd voltage supply
>
> @@ -117,6 +125,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
>

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

[Alisa-Dariana Roman]

AINCOM should actually be a supply. If present and it has a non-zero
voltage, the pseudo-differential channels are configured as single-ended
with an offset. Otherwise, they are configured as differential channels
between AINx and AINCOM pins.

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

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index ac737221beae..a9eb4fab39ca 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -175,7 +175,7 @@ enum {
 struct ad7192_chip_info {
 	unsigned int			chip_id;
 	const char			*name;
-	const struct iio_chan_spec	*channels;
+	struct iio_chan_spec		*channels;
 	u8				num_channels;
 	const struct iio_info		*info;
 };
@@ -186,6 +186,7 @@ struct ad7192_state {
 	struct regulator		*vref;
 	struct clk			*mclk;
 	u16				int_vref_mv;
+	u16				aincom_mv;
 	u32				fclk;
 	u32				mode;
 	u32				conf;
@@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
 		/* Kelvin to Celsius */
 		if (chan->type == IIO_TEMP)
 			*val -= 273 * ad7192_get_temp_scale(unipolar);
+		else if (st->aincom_mv && chan->channel2 == -1)
+			*val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
+						      st->scale_avail[gain][1]);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
@@ -982,7 +986,7 @@ static const struct iio_info ad7195_info = {
 #define AD7193_CHANNEL(_si, _channel1, _address) \
 	AD7193_DIFF_CHANNEL(_si, _channel1, -1, _address)
 
-static const struct iio_chan_spec ad7192_channels[] = {
+static 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),
@@ -994,7 +998,7 @@ static const struct iio_chan_spec ad7192_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(8),
 };
 
-static const struct iio_chan_spec ad7193_channels[] = {
+static struct iio_chan_spec ad7193_channels[] = {
 	AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M),
 	AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M),
 	AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M),
@@ -1048,11 +1052,27 @@ static void ad7192_reg_disable(void *reg)
 	regulator_disable(reg);
 }
 
+static int ad7192_config_channels(struct ad7192_state *st)
+{
+	struct iio_chan_spec *channels = st->chip_info->channels;
+	int i;
+
+	if (!st->aincom_mv)
+		for (i = 0; i < st->chip_info->num_channels; i++)
+			if (channels[i].channel2 == -1) {
+				channels[i].differential = 1;
+				channels[i].channel2 = 0;
+			}
+
+	return 0;
+}
+
 static int ad7192_probe(struct spi_device *spi)
 {
 	struct ad7192_state *st;
 	struct iio_dev *indio_dev;
-	int ret;
+	struct regulator *aincom;
+	int ret = 0;
 
 	if (!spi->irq) {
 		dev_err(&spi->dev, "no IRQ?\n");
@@ -1067,6 +1087,26 @@ 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;
+
 	st->avdd = devm_regulator_get(&spi->dev, "avdd");
 	if (IS_ERR(st->avdd))
 		return PTR_ERR(st->avdd);
@@ -1113,6 +1153,11 @@ static int ad7192_probe(struct spi_device *spi)
 	st->int_vref_mv = ret / 1000;
 
 	st->chip_info = spi_get_device_match_data(spi);
+
+	ret = ad7192_config_channels(st);
+	if (ret)
+		return ret;
+
 	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 v5 3/5] iio: adc: ad7192: Add aincom supply

[Jonathan Cameron]

On Sat, 13 Apr 2024 18:11:50 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> AINCOM should actually be a supply. If present and it has a non-zero
> voltage, the pseudo-differential channels are configured as single-ended
> with an offset. Otherwise, they are configured as differential channels
> between AINx and AINCOM pins.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Hi Alisa,

Main comment here is that you can't modify the channel arrays in place
because there may be more that one instance of the hardware.
My preference would be to pick between static const iio_chan_spec
arrays depending on whether the aincom is provided or not.

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index ac737221beae..a9eb4fab39ca 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -175,7 +175,7 @@ enum {
>  struct ad7192_chip_info {
>  	unsigned int			chip_id;
>  	const char			*name;
> -	const struct iio_chan_spec	*channels;
> +	struct iio_chan_spec		*channels;
>  	u8				num_channels;
>  	const struct iio_info		*info;
>  };
> @@ -186,6 +186,7 @@ struct ad7192_state {
>  	struct regulator		*vref;
>  	struct clk			*mclk;
>  	u16				int_vref_mv;
> +	u16				aincom_mv;
>  	u32				fclk;
>  	u32				mode;
>  	u32				conf;
> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>  		/* Kelvin to Celsius */
>  		if (chan->type == IIO_TEMP)
>  			*val -= 273 * ad7192_get_temp_scale(unipolar);
> +		else if (st->aincom_mv && chan->channel2 == -1)
> +			*val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
> +						      st->scale_avail[gain][1]);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
> @@ -982,7 +986,7 @@ static const struct iio_info ad7195_info = {
>  #define AD7193_CHANNEL(_si, _channel1, _address) \
>  	AD7193_DIFF_CHANNEL(_si, _channel1, -1, _address)
>  
> -static const struct iio_chan_spec ad7192_channels[] = {
> +static 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),
> @@ -994,7 +998,7 @@ static const struct iio_chan_spec ad7192_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };
>  
> -static const struct iio_chan_spec ad7193_channels[] = {
> +static struct iio_chan_spec ad7193_channels[] = {
No to this.

Imagine you have 2 devices wired in parallel to the same host
but only one of them is using vincomm.  Which setting you
get will depend on which order the probe happens in.

You need to take a copy of the channels array or have enough
variants that you can pick between static const arrays depending
on the setting.  If that works here that is the preferred path to
take.  Have ad7193_channels + ad7193_channels_with_vincomm or
something like that.


>  	AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M),
>  	AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M),
>  	AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M),
> @@ -1048,11 +1052,27 @@ static void ad7192_reg_disable(void *reg)
>  	regulator_disable(reg);
>  }
>  
> +static int ad7192_config_channels(struct ad7192_state *st)
> +{
> +	struct iio_chan_spec *channels = st->chip_info->channels;
> +	int i;
> +
> +	if (!st->aincom_mv)
> +		for (i = 0; i < st->chip_info->num_channels; i++)
> +			if (channels[i].channel2 == -1) {
> +				channels[i].differential = 1;
> +				channels[i].channel2 = 0;
> +			}
> +
> +	return 0;
> +}
> +
>  static int ad7192_probe(struct spi_device *spi)
>  {
>  	struct ad7192_state *st;
>  	struct iio_dev *indio_dev;
> -	int ret;
> +	struct regulator *aincom;
> +	int ret = 0;
>  
>  	if (!spi->irq) {
>  		dev_err(&spi->dev, "no IRQ?\n");
> @@ -1067,6 +1087,26 @@ 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;
I think I'd make the two paths more explicit

		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;	
	}
or rely on default for st->aincom_mv already being zero and don't bother adding
the else, just move aincom_mv setting into the path with the voltage being read
and not the other path.

Setting it unconditionally by dividing 0 / 1000 is correct of course, but
not particularly obvious!	

		
> +
>  	st->avdd = devm_regulator_get(&spi->dev, "avdd");
>  	if (IS_ERR(st->avdd))
>  		return PTR_ERR(st->avdd);
> @@ -1113,6 +1153,11 @@ static int ad7192_probe(struct spi_device *spi)
>  	st->int_vref_mv = ret / 1000;
>  
>  	st->chip_info = spi_get_device_match_data(spi);
> +
> +	ret = ad7192_config_channels(st);
> +	if (ret)
> +		return ret;
> +
>  	indio_dev->name = st->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = st->chip_info->channels;

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

[David Lechner]

On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> AINCOM should actually be a supply. If present and it has a non-zero
> voltage, the pseudo-differential channels are configured as single-ended
> with an offset. Otherwise, they are configured as differential channels
> between AINx and AINCOM pins.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index ac737221beae..a9eb4fab39ca 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -175,7 +175,7 @@ enum {
>  struct ad7192_chip_info {
>         unsigned int                    chip_id;
>         const char                      *name;
> -       const struct iio_chan_spec      *channels;
> +       struct iio_chan_spec            *channels;
>         u8                              num_channels;
>         const struct iio_info           *info;
>  };
> @@ -186,6 +186,7 @@ struct ad7192_state {
>         struct regulator                *vref;
>         struct clk                      *mclk;
>         u16                             int_vref_mv;
> +       u16                             aincom_mv;

u32? (In case we have a future chip that can go above 6.5535V?

>         u32                             fclk;
>         u32                             mode;
>         u32                             conf;
> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>                 /* Kelvin to Celsius */

Not related to this patch, but I'm not a fan of the way the
temperature case writes over *val (maybe clean that up using a switch
statement instead in another patch while we are working on this?).
Adding the else if to this makes it even harder to follow.

>                 if (chan->type == IIO_TEMP)
>                         *val -= 273 * ad7192_get_temp_scale(unipolar);
> +               else if (st->aincom_mv && chan->channel2 == -1)

I think the logic should be !chan->differential instead of
chan->channel2 = -1 (more explanation on this below).

> +                       *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
> +                                                     st->scale_avail[gain][1]);
>                 return IIO_VAL_INT;
>         case IIO_CHAN_INFO_SAMP_FREQ:
>                 *val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);

...

>
> +static int ad7192_config_channels(struct ad7192_state *st)
> +{
> +       struct iio_chan_spec *channels = st->chip_info->channels;
> +       int i;
> +
> +       if (!st->aincom_mv)

As mentioned in my reply to the DT bindings patch, I don't think this
logic is correct. AINx/AINCOM input pairs are always
pseudo-differential regardless if AINCOM is tied to GND or is a
non-zero voltage.

Also, to be clear, for pseudo-differential inputs, we want
.differential = 0, so the existing channel specs look correct to me
and therefore we don't need any changes like this.

> +               for (i = 0; i < st->chip_info->num_channels; i++)
> +                       if (channels[i].channel2 == -1) {
> +                               channels[i].differential = 1;
> +                               channels[i].channel2 = 0;
> +                       }
> +
> +       return 0;
> +}
> +

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

[Alisa-Dariana Roman]

On 13.04.2024 22:10, David Lechner wrote:
> On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
>>
>> AINCOM should actually be a supply. If present and it has a non-zero
>> voltage, the pseudo-differential channels are configured as single-ended
>> with an offset. Otherwise, they are configured as differential channels
>> between AINx and AINCOM pins.
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>> ---
>>   drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 49 insertions(+), 4 deletions(-)

...

>> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>>                  /* Kelvin to Celsius */
> 
> Not related to this patch, but I'm not a fan of the way the
> temperature case writes over *val (maybe clean that up using a switch
> statement instead in another patch while we are working on this?).
> Adding the else if to this makes it even harder to follow.
> 
>>                  if (chan->type == IIO_TEMP)
>>                          *val -= 273 * ad7192_get_temp_scale(unipolar);
>> +               else if (st->aincom_mv && chan->channel2 == -1)
> 
> I think the logic should be !chan->differential instead of
> chan->channel2 = -1 (more explanation on this below).
> 
>> +                       *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
>> +                                                     st->scale_avail[gain][1]);
>>                  return IIO_VAL_INT;

Hi David,

I am very grateful for your suggestions!

	case IIO_CHAN_INFO_OFFSET:
		if (!unipolar)
			*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 */
		case IIO_TEMP:
			*val -= 273 * ad7192_get_temp_scale(unipolar);
			return IIO_VAL_INT;
		default:
			return -EINVAL;
		}

I added a switch because it looks neater indeed. But I would keep the if 
else for the unipolar in order not to have duplicate code. Is this alright?

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

[David Lechner]

On 4/14/24 8:58 AM, Alisa-Dariana Roman wrote:
> On 13.04.2024 22:10, David Lechner wrote:
>> On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
>> <alisadariana@gmail.com> wrote:
>>>
>>> AINCOM should actually be a supply. If present and it has a non-zero
>>> voltage, the pseudo-differential channels are configured as single-ended
>>> with an offset. Otherwise, they are configured as differential channels
>>> between AINx and AINCOM pins.
>>>
>>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>>> ---
>>>   drivers/iio/adc/ad7192.c | 53 +++++++++++++++++++++++++++++++++++++---
>>>   1 file changed, 49 insertions(+), 4 deletions(-)
> 
> ...
> 
>>> @@ -745,6 +746,9 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>>>                  /* Kelvin to Celsius */
>>
>> Not related to this patch, but I'm not a fan of the way the
>> temperature case writes over *val (maybe clean that up using a switch
>> statement instead in another patch while we are working on this?).
>> Adding the else if to this makes it even harder to follow.
>>
>>>                  if (chan->type == IIO_TEMP)
>>>                          *val -= 273 * ad7192_get_temp_scale(unipolar);
>>> +               else if (st->aincom_mv && chan->channel2 == -1)
>>
>> I think the logic should be !chan->differential instead of
>> chan->channel2 = -1 (more explanation on this below).
>>
>>> +                       *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * 1000000000,
>>> +                                                     st->scale_avail[gain][1]);
>>>                  return IIO_VAL_INT;
> 
> Hi David,
> 
> I am very grateful for your suggestions!
> 
>     case IIO_CHAN_INFO_OFFSET:
>         if (!unipolar)
>             *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 */
>         case IIO_TEMP:
>             *val -= 273 * ad7192_get_temp_scale(unipolar);
>             return IIO_VAL_INT;
>         default:
>             return -EINVAL;
>         }
> 
> I added a switch because it looks neater indeed. But I would keep the if else for the unipolar in order not to have duplicate code. Is this alright?
> 


I didn't notice before that the temperature channel could also be
unipolor or bipolar, so yes this seems fine.

[PATCH v5 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          | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index ba506af3b73e..855f0a2d7d75 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
 
@@ -104,8 +111,43 @@ required:
   - spi-cpol
   - spi-cpha
 
+patternProperties:
+  "^channel@[0-9]+$":
+    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
+
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          enum:
+            - adi,ad7190
+            - adi,ad7192
+            - adi,ad7193
+            - adi,ad7195
+    then:
+      patternProperties:
+        "^channel@[0-9]+$": false
 
 unevaluatedProperties: false
 
@@ -136,3 +178,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@1 {
+                reg = <1>;
+                diff-channels = <1 6>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                diff-channels = <16 5>;
+            };
+        };
+    };
-- 
2.34.1

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

[David Lechner]

On Sat, Apr 13, 2024 at 10:13 AM 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 add an example for AD7194 devicetree.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7192.yaml          | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index ba506af3b73e..855f0a2d7d75 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
>
> @@ -104,8 +111,43 @@ required:
>    - spi-cpol
>    - spi-cpha
>
> +patternProperties:
> +  "^channel@[0-9]+$":
> +    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

Don't we also need to allow 0 for AINCOM here? Or is this property
only for fully differential pairs and not pseudo-differential pairs?


> +
> +    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-9]+$": false
>
>  unevaluatedProperties: false
>
> @@ -136,3 +178,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@1 {
> +                reg = <1>;
> +                diff-channels = <1 6>;
> +            };
> +
> +            channel@2 {
> +                reg = <2>;
> +                diff-channels = <16 5>;
> +            };
> +        };
> +    };
> --
> 2.34.1
>

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

[Alisa-Dariana Roman]

On 13.04.2024 22:29, David Lechner wrote:
> On Sat, Apr 13, 2024 at 10:13 AM 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 add an example for AD7194 devicetree.
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>> ---
>>   .../bindings/iio/adc/adi,ad7192.yaml          | 74 +++++++++++++++++++
>>   1 file changed, 74 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> index ba506af3b73e..855f0a2d7d75 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
>>
>> @@ -104,8 +111,43 @@ required:
>>     - spi-cpol
>>     - spi-cpha
>>
>> +patternProperties:
>> +  "^channel@[0-9]+$":
>> +    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
> 
> Don't we also need to allow 0 for AINCOM here? Or is this property
> only for fully differential pairs and not pseudo-differential pairs?

I thought it would be a good idea to have the pseudo-differential pairs 
set in the driver (all from AIN1 to AIN16 referenced to AINCOM). Only 
differential ones are fully configurable in the devicetree.

Kind regards,
Alisa-Dariana Roman

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

[Krzysztof Kozlowski]

On 13/04/2024 17:11, 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          | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index ba506af3b73e..855f0a2d7d75 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
>  
> @@ -104,8 +111,43 @@ required:
>    - spi-cpol
>    - spi-cpha
>  
> +patternProperties:

This goes after properties:, so before required: block.

> +  "^channel@[0-9]+$":

Why restricting the pattern? If you have 256 channels, how are you going
to encode it?

> +    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
> +
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - adi,ad7190
> +            - adi,ad7192
> +            - adi,ad7193
> +            - adi,ad7195
> +    then:
> +      patternProperties:
> +        "^channel@[0-9]+$": false
>  
>  unevaluatedProperties: false
>  
> @@ -136,3 +178,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>;

compatible is always the first property. reg is second.


Best regards,
Krzysztof

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

[Alisa-Dariana Roman]

On 14.04.2024 00:19, Krzysztof Kozlowski wrote:
> On 13/04/2024 17:11, 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          | 74 +++++++++++++++++++
>>   1 file changed, 74 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> index ba506af3b73e..855f0a2d7d75 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml

...

> 
>> +  "^channel@[0-9]+$":
> 
> Why restricting the pattern? If you have 256 channels, how are you going
> to encode it?

Hi Krzysztof,

Thank you for your feedback! I applied the rest, but as for this one 
isn't channel@1 -> channel@256 encoding sufficient?

Kind regards,
Alisa-Dariana Roman

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

[David Lechner]

On 4/15/24 8:08 AM, Alisa-Dariana Roman wrote:
> On 14.04.2024 00:19, Krzysztof Kozlowski wrote:
>> On 13/04/2024 17:11, 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          | 74 +++++++++++++++++++
>>>   1 file changed, 74 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> index ba506af3b73e..855f0a2d7d75 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> 
> ...
> 
>>
>>> +  "^channel@[0-9]+$":
>>
>> Why restricting the pattern? If you have 256 channels, how are you going
>> to encode it?
> 
> Hi Krzysztof,
> 
> Thank you for your feedback! I applied the rest, but as for this one isn't channel@1 -> channel@256 encoding sufficient?
> 
> Kind regards,
> Alisa-Dariana Roman
> 
> 

The number after @ is hexidecimal (without leading 0x), so it should be
[0-9a-f]. channel@0 to channel@ff.

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

[Krzysztof Kozlowski]

On 15/04/2024 15:08, Alisa-Dariana Roman wrote:
> On 14.04.2024 00:19, Krzysztof Kozlowski wrote:
>> On 13/04/2024 17:11, 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          | 74 +++++++++++++++++++
>>>   1 file changed, 74 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> index ba506af3b73e..855f0a2d7d75 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> 
> ...
> 
>>
>>> +  "^channel@[0-9]+$":
>>
>> Why restricting the pattern? If you have 256 channels, how are you going
>> to encode it?
> 
> Hi Krzysztof,
> 
> Thank you for your feedback! I applied the rest, but as for this one 
> isn't channel@1 -> channel@256 encoding sufficient?

@256, so 0x256 = 589, so a bit too much. The unit addresses are hex.


Best regards,
Krzysztof

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

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

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/Kconfig  |  11 ++-
 drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 138 insertions(+), 13 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 a9eb4fab39ca..646ab56b87e3 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,17 +180,10 @@ enum {
 	ID_AD7190,
 	ID_AD7192,
 	ID_AD7193,
+	ID_AD7194,
 	ID_AD7195,
 };
 
-struct ad7192_chip_info {
-	unsigned int			chip_id;
-	const char			*name;
-	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;
@@ -201,6 +205,15 @@ struct ad7192_state {
 	struct ad_sigma_delta		sd;
 };
 
+struct ad7192_chip_info {
+	unsigned int			chip_id;
+	const char			*name;
+	struct iio_chan_spec		*channels;
+	u8				num_channels;
+	const struct iio_info		*info;
+	int (*parse_channels)(struct iio_dev *indio_dev);
+};
+
 static const char * const ad7192_syscalib_modes[] = {
 	[AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
 	[AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
@@ -925,6 +938,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,
@@ -1016,6 +1038,90 @@ static struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static int ad7192_parse_channel(struct fwnode_handle *child,
+				struct iio_chan_spec *ad7194_channels)
+{
+	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_channels->channel = ain[0];
+	ad7194_channels->channel2 = ain[1];
+	ad7194_channels->address = AD7194_CH_DIFF(ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7192_parse_channels(struct iio_dev *indio_dev)
+{
+	struct ad7192_state *st = iio_priv(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, sizeof(*ad7194_channels),
+				       num_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 = ad7192_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++) {
+		if (st->aincom_mv) {
+			*ad7194_channels = ad7194_chan;
+		} else {
+			*ad7194_channels = ad7194_chan_diff;
+			ad7194_channels->channel2 = 0;
+		}
+		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,
@@ -1038,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 = ad7192_parse_channels,
+	},
 	[ID_AD7195] = {
 		.chip_id = CHIPID_AD7195,
 		.name = "ad7195",
@@ -1164,6 +1276,12 @@ static int ad7192_probe(struct spi_device *spi)
 	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;
+	}
+
 	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
 	if (ret)
 		return ret;
@@ -1201,6 +1319,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] },
 	{}
 };
@@ -1210,6 +1329,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] },
 	{}
 };
@@ -1226,6 +1346,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 v5 5/5] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Sat, 13 Apr 2024 18:11:52 +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.
> 
> Moved ad7192_chip_info struct definition to allow use of callback
> function parse_channels().
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
The rest of this series looks good to me.

Jonathan

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

[David Lechner]

On Sat, Apr 13, 2024 at 10:13 AM 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.
>
> Moved ad7192_chip_info struct definition to allow use of callback
> function parse_channels().

It looks like this no longer needs to be moved in this revision.

>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |  11 ++-
>  drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 138 insertions(+), 13 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 a9eb4fab39ca..646ab56b87e3 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,17 +180,10 @@ enum {
>         ID_AD7190,
>         ID_AD7192,
>         ID_AD7193,
> +       ID_AD7194,
>         ID_AD7195,
>  };
>
> -struct ad7192_chip_info {
> -       unsigned int                    chip_id;
> -       const char                      *name;
> -       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;
> @@ -201,6 +205,15 @@ struct ad7192_state {
>         struct ad_sigma_delta           sd;
>  };
>
> +struct ad7192_chip_info {
> +       unsigned int                    chip_id;
> +       const char                      *name;
> +       struct iio_chan_spec            *channels;
> +       u8                              num_channels;
> +       const struct iio_info           *info;
> +       int (*parse_channels)(struct iio_dev *indio_dev);
> +};
> +
>  static const char * const ad7192_syscalib_modes[] = {
>         [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
>         [AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
> @@ -925,6 +938,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,
> @@ -1016,6 +1038,90 @@ static struct iio_chan_spec ad7193_channels[] = {
>         IIO_CHAN_SOFT_TIMESTAMP(14),
>  };
>
> +static int ad7192_parse_channel(struct fwnode_handle *child,
> +                               struct iio_chan_spec *ad7194_channels)

nit: this is only requiring one "channel" and is not used as
"channels" array, so I would leave out the "s".

> +{
> +       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_channels->channel = ain[0];
> +       ad7194_channels->channel2 = ain[1];
> +       ad7194_channels->address = AD7194_CH_DIFF(ain[0], ain[1]);
> +
> +       return 0;
> +}
> +
> +static int ad7192_parse_channels(struct iio_dev *indio_dev)

Better name might be ad7194_parse_channels() or
ad7192_parse_ad7194_channels() since this is specific to the ad7194
chip.

> +{
> +       struct ad7192_state *st = iio_priv(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, sizeof(*ad7194_channels),
> +                                      num_channels, GFP_KERNEL);

nit: technically, the argument order is supposed to be count then size

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

nit: It would seem more natural to have all voltage channels
altogether rather than having the temperature channel in between.

> +
> +       for (ain_chan = 1; ain_chan <= 16; ain_chan++) {
> +               if (st->aincom_mv) {
> +                       *ad7194_channels = ad7194_chan;
> +               } else {
> +                       *ad7194_channels = ad7194_chan_diff;
> +                       ad7194_channels->channel2 = 0;
> +               }

Same comment as on [PATCH 3/5] pseudo-differential channels have
differential = 0 and so nothing should depend on st->aincom_mv being 0
or not.

> +               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,
> @@ -1038,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 = ad7192_parse_channels,
> +       },
>         [ID_AD7195] = {
>                 .chip_id = CHIPID_AD7195,
>                 .name = "ad7195",
> @@ -1164,6 +1276,12 @@ static int ad7192_probe(struct spi_device *spi)
>         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;
> +       }

Take it or leave it, but I think it would be nice to move

    indio_dev->channels = st->chip_info->channels;
    indio_dev->num_channels = st->chip_info->num_channels;

into an else { } here to make it clear what parse_channels is doing.

> +
>         ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
>         if (ret)
>                 return ret;
> @@ -1201,6 +1319,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] },
>         {}
>  };
> @@ -1210,6 +1329,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] },
>         {}
>  };
> @@ -1226,6 +1346,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 v5 5/5] iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

On 13.04.2024 23:05, David Lechner wrote:
> On Sat, Apr 13, 2024 at 10:13 AM 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.
>>
>> Moved ad7192_chip_info struct definition to allow use of callback
>> function parse_channels().
> 
> It looks like this no longer needs to be moved in this revision.
> 
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>> ---
>>   drivers/iio/adc/Kconfig  |  11 ++-
>>   drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 138 insertions(+), 13 deletions(-)

...

> 
>> +       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 = ad7192_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++;
> 
> nit: It would seem more natural to have all voltage channels
> altogether rather than having the temperature channel in between.

I wrote the channels like this to match the other chips:

static const struct iio_chan_spec ad7193_channels[] = {
	AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M),
	AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M),
	AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M),
	AD7193_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M),
	AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP),
	AD7193_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M),
	AD7193_CHANNEL(6, 1, AD7193_CH_AIN1),
	AD7193_CHANNEL(7, 2, AD7193_CH_AIN2),
	AD7193_CHANNEL(8, 3, AD7193_CH_AIN3),
	AD7193_CHANNEL(9, 4, AD7193_CH_AIN4),
	AD7193_CHANNEL(10, 5, AD7193_CH_AIN5),
	AD7193_CHANNEL(11, 6, AD7193_CH_AIN6),
	AD7193_CHANNEL(12, 7, AD7193_CH_AIN7),
	AD7193_CHANNEL(13, 8, AD7193_CH_AIN8),
	IIO_CHAN_SOFT_TIMESTAMP(14),
};

Kind regards,
Alisa-Dariana Roman

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

[David Lechner]

On 4/14/24 3:14 PM, Alisa-Dariana Roman wrote:
> On 13.04.2024 23:05, David Lechner wrote:
>> On Sat, Apr 13, 2024 at 10:13 AM 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.
>>>
>>> Moved ad7192_chip_info struct definition to allow use of callback
>>> function parse_channels().
>>
>> It looks like this no longer needs to be moved in this revision.
>>
>>>
>>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>>> ---
>>>   drivers/iio/adc/Kconfig  |  11 ++-
>>>   drivers/iio/adc/ad7192.c | 140 ++++++++++++++++++++++++++++++++++++---
>>>   2 files changed, 138 insertions(+), 13 deletions(-)
> 
> ...
> 
>>
>>> +       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 = ad7192_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++;
>>
>> nit: It would seem more natural to have all voltage channels
>> altogether rather than having the temperature channel in between.
> 
> I wrote the channels like this to match the other chips:
> 
> static const struct iio_chan_spec ad7193_channels[] = {
>     AD7193_DIFF_CHANNEL(0, 1, 2, AD7193_CH_AIN1P_AIN2M),
>     AD7193_DIFF_CHANNEL(1, 3, 4, AD7193_CH_AIN3P_AIN4M),
>     AD7193_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M),
>     AD7193_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M),
>     AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP),
>     AD7193_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M),
>     AD7193_CHANNEL(6, 1, AD7193_CH_AIN1),
>     AD7193_CHANNEL(7, 2, AD7193_CH_AIN2),
>     AD7193_CHANNEL(8, 3, AD7193_CH_AIN3),
>     AD7193_CHANNEL(9, 4, AD7193_CH_AIN4),
>     AD7193_CHANNEL(10, 5, AD7193_CH_AIN5),
>     AD7193_CHANNEL(11, 6, AD7193_CH_AIN6),
>     AD7193_CHANNEL(12, 7, AD7193_CH_AIN7),
>     AD7193_CHANNEL(13, 8, AD7193_CH_AIN8),
>     IIO_CHAN_SOFT_TIMESTAMP(14),
> };
> 
> Kind regards,
> Alisa-Dariana Roman
> 

Consistency is good too. ;-)

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

[Andy Shevchenko]

On Sat, Apr 13, 2024 at 11:05 PM David Lechner <dlechner@baylibre.com> wrote:
> On Sat, Apr 13, 2024 at 10:13 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:

...

> > +static int ad7192_parse_channels(struct iio_dev *indio_dev)
>
> Better name might be ad7194_parse_channels() or
> ad7192_parse_ad7194_channels() since this is specific to the ad7194
> chip.

Quite unlikely we have the same chip to be supported by somewhere
else, so I would just name it simpler, i.e.

ad7194_parse_channels()

-- 
With Best Regards,
Andy Shevchenko