Dear maintainers,
Thank you all for the feedback!
I am submitting the upgraded series of patches for the ad7192 driver.
Please consider applying in order.
Note that I dropped the patch related to the clock bindings. I will be
back with another series of patches related to the clock.
Thank you!
v2 -> v3
- add precursor patch to simply functions to only pass
ad7192_state
- add patch to replace custom attribute
- bindings patch: correct use of allOf and some minor changes to
the ad7194 example
- add ad7194 patch:
- use "ad7192 and similar"
- ad7194 no longer needs attribute group
- use callback function in chip_info to parse channels
- move struct ad7192_chip_info
- change position of parse functions
- drop clock bindings patch
v1 -> v2
- new commit with missing documentation for properties
- add constraint for channels in binding
- correct pattern for channels
- correct commit message by adding "()" to functions
- use in_range
- use preferred structure in Kconfig
Kind regards,
Alisa-Dariana Roman (5):
iio: adc: ad7192: Use device api
iio: adc: ad7192: Pass state directly
iio: adc: ad7192: Use standard attribute
dt-bindings: iio: adc: ad7192: Add AD7194 support
iio: adc: ad7192: Add AD7194 support
.../bindings/iio/adc/adi,ad7192.yaml | 75 ++++++
drivers/iio/adc/Kconfig | 11 +-
drivers/iio/adc/ad7192.c | 252 +++++++++++++-----
3 files changed, 269 insertions(+), 69 deletions(-)
--
2.34.1
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 <[email protected]>
---
drivers/iio/adc/ad7192.c | 67 ++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 37 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 5e8043865233..d8393ac048e7 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -187,6 +187,7 @@ struct ad7192_state {
u32 mode;
u32 conf;
u32 scale_avail[8][2];
+ u32 filter_freq_avail[4][2];
u32 oversampling_ratio_avail[4];
u8 gpocon;
u8 clock_sel;
@@ -470,6 +471,16 @@ static int ad7192_setup(struct ad7192_state *st)
st->oversampling_ratio_avail[2] = 8;
st->oversampling_ratio_avail[3] = 16;
+ st->filter_freq_avail[0][0] = 600;
+ st->filter_freq_avail[1][0] = 800;
+ st->filter_freq_avail[2][0] = 2300;
+ st->filter_freq_avail[3][0] = 2720;
+
+ st->filter_freq_avail[0][1] = 1000;
+ st->filter_freq_avail[1][1] = 1000;
+ st->filter_freq_avail[2][1] = 1000;
+ st->filter_freq_avail[3][1] = 1000;
+
return 0;
}
@@ -583,48 +594,24 @@ static int ad7192_get_f_adc(struct ad7192_state *st)
f_order * FIELD_GET(AD7192_MODE_RATE_MASK, st->mode));
}
-static void ad7192_get_available_filter_freq(struct ad7192_state *st,
- int *freq)
+static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
{
unsigned int fadc;
/* Formulas for filter at page 25 of the datasheet */
fadc = ad7192_compute_f_adc(st, false, true);
- freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+ st->filter_freq_avail[0][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
fadc = ad7192_compute_f_adc(st, true, true);
- freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+ st->filter_freq_avail[1][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
fadc = ad7192_compute_f_adc(st, false, false);
- freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
+ st->filter_freq_avail[2][0] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
fadc = ad7192_compute_f_adc(st, true, false);
- freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
+ st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
}
-static ssize_t ad7192_show_filter_avail(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct ad7192_state *st = iio_priv(indio_dev);
- unsigned int freq_avail[4], i;
- size_t len = 0;
-
- ad7192_get_available_filter_freq(st, freq_avail);
-
- for (i = 0; i < ARRAY_SIZE(freq_avail); i++)
- len += sysfs_emit_at(buf, len, "%d.%03d ", freq_avail[i] / 1000,
- freq_avail[i] % 1000);
-
- buf[len - 1] = '\n';
-
- return len;
-}
-
-static IIO_DEVICE_ATTR(filter_low_pass_3db_frequency_available,
- 0444, ad7192_show_filter_avail, NULL, 0);
-
static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
ad7192_show_bridge_switch, ad7192_set,
AD7192_REG_GPOCON);
@@ -634,7 +621,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
AD7192_REG_CONF);
static struct attribute *ad7192_attributes[] = {
- &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
&iio_dev_attr_bridge_switch_en.dev_attr.attr,
NULL
};
@@ -644,7 +630,6 @@ static const struct attribute_group ad7192_attribute_group = {
};
static struct attribute *ad7195_attributes[] = {
- &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
&iio_dev_attr_bridge_switch_en.dev_attr.attr,
&iio_dev_attr_ac_excitation_en.dev_attr.attr,
NULL
@@ -662,17 +647,15 @@ static unsigned int ad7192_get_temp_scale(bool unipolar)
static int ad7192_set_3db_filter_freq(struct ad7192_state *st,
int val, int val2)
{
- int freq_avail[4], i, ret, freq;
+ int i, ret, freq;
unsigned int diff_new, diff_old;
int idx = 0;
diff_old = U32_MAX;
freq = val * 1000 + val2;
- ad7192_get_available_filter_freq(st, freq_avail);
-
- for (i = 0; i < ARRAY_SIZE(freq_avail); i++) {
- diff_new = abs(freq - freq_avail[i]);
+ for (i = 0; i < ARRAY_SIZE(st->filter_freq_avail); i++) {
+ diff_new = abs(freq - st->filter_freq_avail[i][0]);
if (diff_new < diff_old) {
diff_old = diff_new;
idx = i;
@@ -823,6 +806,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
st->mode &= ~AD7192_MODE_RATE_MASK;
st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+ ad7192_update_filter_freq_avail(st);
break;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000);
@@ -843,6 +827,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
break;
}
mutex_unlock(&st->lock);
+ ad7192_update_filter_freq_avail(st);
break;
default:
ret = -EINVAL;
@@ -885,6 +870,12 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
/* Values are stored in a 2D matrix */
*length = ARRAY_SIZE(st->scale_avail) * 2;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ *vals = (int *)st->filter_freq_avail;
+ *type = IIO_VAL_FRACTIONAL;
+ *length = ARRAY_SIZE(st->filter_freq_avail) * 2;
+
return IIO_AVAIL_LIST;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
*vals = (int *)st->oversampling_ratio_avail;
@@ -953,7 +944,9 @@ static const struct iio_info ad7195_info = {
BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
(_mask_all), \
.info_mask_shared_by_type_available = (_mask_type_av), \
- .info_mask_shared_by_all_available = (_mask_all_av), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+ (_mask_all_av), \
.ext_info = (_ext_info), \
.scan_index = (_si), \
.scan_type = { \
--
2.34.1
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 <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
---
drivers/iio/adc/Kconfig | 11 ++-
drivers/iio/adc/ad7192.c | 150 ++++++++++++++++++++++++++++++++++++---
2 files changed, 148 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 59ae1d17b50d..8062a4d1cbe7 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -71,12 +71,17 @@ config AD7124
called ad7124.
config AD7192
- tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+ tristate "Analog Devices AD7192 and similar ADC driver"
depends on SPI
select AD_SIGMA_DELTA
help
- Say yes here to build support for Analog Devices AD7190,
- AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+ Say yes here to build support for Analog Devices SPI analog to digital
+ converters (ADC):
+ - AD7190
+ - AD7192
+ - AD7193
+ - AD7194
+ - AD7195
If unsure, say N (but it's safe to say "Y").
To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index d8393ac048e7..a3ff60ed6f63 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7192 and similar SPI ADC driver
*
* Copyright 2011-2015 Analog Devices Inc.
*/
@@ -125,10 +125,39 @@
#define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
#define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
+#define AD7194_CH_TEMP 0x100 /* Temp sensor */
+#define AD7194_CH_AIN1 0x400 /* AIN1 - AINCOM */
+#define AD7194_CH_AIN2 0x410 /* AIN2 - AINCOM */
+#define AD7194_CH_AIN3 0x420 /* AIN3 - AINCOM */
+#define AD7194_CH_AIN4 0x430 /* AIN4 - AINCOM */
+#define AD7194_CH_AIN5 0x440 /* AIN5 - AINCOM */
+#define AD7194_CH_AIN6 0x450 /* AIN6 - AINCOM */
+#define AD7194_CH_AIN7 0x460 /* AIN7 - AINCOM */
+#define AD7194_CH_AIN8 0x470 /* AIN8 - AINCOM */
+#define AD7194_CH_AIN9 0x480 /* AIN9 - AINCOM */
+#define AD7194_CH_AIN10 0x490 /* AIN10 - AINCOM */
+#define AD7194_CH_AIN11 0x4A0 /* AIN11 - AINCOM */
+#define AD7194_CH_AIN12 0x4B0 /* AIN12 - AINCOM */
+#define AD7194_CH_AIN13 0x4C0 /* AIN13 - AINCOM */
+#define AD7194_CH_AIN14 0x4D0 /* AIN14 - AINCOM */
+#define AD7194_CH_AIN15 0x4E0 /* AIN15 - AINCOM */
+#define AD7194_CH_AIN16 0x4F0 /* AIN16 - AINCOM */
+#define AD7194_CH_POS_MASK GENMASK(7, 4)
+#define AD7194_CH_POS(x) FIELD_PREP(AD7194_CH_POS_MASK, (x))
+#define AD7194_CH_NEG_MASK GENMASK(3, 0)
+#define AD7194_CH_NEG(x) FIELD_PREP(AD7194_CH_NEG_MASK, (x))
+#define AD7194_CH_DIFF(pos, neg) \
+ (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))
+#define AD7194_CH_DIFF_START 0
+#define AD7194_CH_DIFF_NR 8
+#define AD7194_CH_AIN_START 1
+#define AD7194_CH_AIN_NR 16
+
/* ID Register Bit Designations (AD7192_REG_ID) */
#define CHIPID_AD7190 0x4
#define CHIPID_AD7192 0x0
#define CHIPID_AD7193 0x2
+#define CHIPID_AD7194 0x3
#define CHIPID_AD7195 0x6
#define AD7192_ID_MASK GENMASK(3, 0)
@@ -166,17 +195,10 @@ enum {
ID_AD7190,
ID_AD7192,
ID_AD7193,
+ ID_AD7194,
ID_AD7195,
};
-struct ad7192_chip_info {
- unsigned int chip_id;
- const char *name;
- const struct iio_chan_spec *channels;
- u8 num_channels;
- const struct iio_info *info;
-};
-
struct ad7192_state {
const struct ad7192_chip_info *chip_info;
struct regulator *avdd;
@@ -197,6 +219,15 @@ struct ad7192_state {
struct ad_sigma_delta sd;
};
+struct ad7192_chip_info {
+ unsigned int chip_id;
+ const char *name;
+ const struct iio_chan_spec *channels;
+ u8 num_channels;
+ const struct iio_info *info;
+ int (*parse_channels)(struct ad7192_state *st);
+};
+
static const char * const ad7192_syscalib_modes[] = {
[AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
[AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
@@ -918,6 +949,15 @@ static const struct iio_info ad7192_info = {
.update_scan_mode = ad7192_update_scan_mode,
};
+static const struct iio_info ad7194_info = {
+ .read_raw = ad7192_read_raw,
+ .write_raw = ad7192_write_raw,
+ .write_raw_get_fmt = ad7192_write_raw_get_fmt,
+ .read_avail = ad7192_read_avail,
+ .validate_trigger = ad_sd_validate_trigger,
+ .update_scan_mode = ad7192_update_scan_mode,
+};
+
static const struct iio_info ad7195_info = {
.read_raw = ad7192_read_raw,
.write_raw = ad7192_write_raw,
@@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(14),
};
+static struct iio_chan_spec ad7194_channels[] = {
+ AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
+ AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
+ AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
+ AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
+ AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
+ AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
+ AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
+ AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
+ AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
+ AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
+ AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
+ AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
+ AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
+ AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
+ AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
+ AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
+ AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
+ AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
+ AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
+ AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
+ AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
+ AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
+ AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
+ AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
+ AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
+ IIO_CHAN_SOFT_TIMESTAMP(25),
+};
+
+static int ad7192_parse_channel(struct fwnode_handle *child)
+{
+ u32 reg, ain[2];
+ int ret;
+
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ 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 <[email protected]>");
-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
On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
<[email protected]> 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 <[email protected]>
> Reviewed-by: Nuno Sa <[email protected]>
> ---
> 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", ®);
> + 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 <[email protected]>");
> -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
>
On Thu, 8 Feb 2024 19:24:57 +0200
Alisa-Dariana Roman <[email protected]> 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 <[email protected]>
Looks good. Thanks for tidying this up.
I've nothing to add to other reviews that have already come in for v3.
Thanks,
Jonathan
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
> <[email protected]> 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
On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
<[email protected]> 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
> > <[email protected]> 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)
On Thu, 15 Feb 2024 11:13:19 -0600
David Lechner <[email protected]> wrote:
> On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> <[email protected]> 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
> > > <[email protected]> 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
On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <[email protected]> wrote:
>
> On Thu, 15 Feb 2024 11:13:19 -0600
> David Lechner <[email protected]> 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.
On Fri, 16 Feb 2024 10:57:33 -0600
David Lechner <[email protected]> wrote:
> On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Thu, 15 Feb 2024 11:13:19 -0600
> > David Lechner <[email protected]> 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
On Sat, Feb 17, 2024 at 10:25 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 16 Feb 2024 10:57:33 -0600
> David Lechner <[email protected]> wrote:
>
> > On Fri, Feb 16, 2024 at 8:22 AM Jonathan Cameron <[email protected]> wrote:
> > >
> > > On Thu, 15 Feb 2024 11:13:19 -0600
> > > David Lechner <[email protected]> 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.
On Thu, Feb 15, 2024 at 11:13 AM David Lechner <[email protected]> wrote:
>
> On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> <[email protected]> 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
> > > <[email protected]> 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)
On Mon, 19 Feb 2024 10:33:45 -0600
David Lechner <[email protected]> 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
> > <[email protected]> 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
> > > > <[email protected]> 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)