diff mbox series

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

Message ID 20240208172459.280189-6-alisa.roman@analog.com
State New
Headers
Series None |

Commit Message

Alisa-Dariana Roman Feb. 8, 2024, 5:24 p.m. UTC 
  Unlike the other AD719Xs, AD7194 has configurable differential
channels. The default configuration for these channels can be changed
from the devicetree.

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

Also modify config AD7192 description for better scaling.

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

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

Comments

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

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

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

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

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

#define AD7194_CH_AIN1    AD7194_CH_DIFF(1, 0)

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

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

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

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

> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> +};

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

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

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

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

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

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

..

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

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

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

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

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

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

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

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

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

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

Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
much more configurable than AD7192 when it comes to assigning
channels. There are basically no restrictions on which inputs can be
used together. So I am still confident that my suggestion is the way
to go for AD7194. (Although I didn't actually try it on hardware, so
can't be 100% confident. But at least 90% confident :-p)
Jonathan Cameron Feb. 16, 2024, 2:21 p.m. UTC | #4 
On Thu, 15 Feb 2024 11:13:19 -0600
David Lechner <dlechner@baylibre.com> wrote:

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

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

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

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

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

What fun.

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

..

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

Calling AINCOM a supply doesn't sound right to me since usually this
signal is coming somewhere external, i.e. you have a twisted pair
connected to AIN1 and AINCOM going to some signal source that may be
hot-pluggable and not known at compile time. As an example, if AINCOM
was modeled as a supply, then we would have to change the device tree
every time we changed the voltage offset on the signal generator while
we are testing using an evaluation board.
Jonathan Cameron Feb. 17, 2024, 4:25 p.m. UTC | #6 
On Fri, 16 Feb 2024 10:57:33 -0600
David Lechner <dlechner@baylibre.com> wrote:

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

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

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

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

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


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

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

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

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

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


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

Patch

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