This patch series adds support for the Analog Devices AD4111, AD4112,
AD4114, AD4115, AD4116 within the existing AD7173 driver.
The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications. They integrate an analog front end suitable for processing
fully differential/single-ended and bipolar voltage inputs.
- All ADCs have inputs with a precision voltage divider with a division
ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
shunt resistor.
Datasheets:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
This series depends on patch:
(iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify error paths.)
https://lore.kernel.org/all/[email protected]
Signed-off-by: Dumitru Ceclan <[email protected]>
---
Dumitru Ceclan (6):
dt-bindings: adc: ad7173: add support for ad411x
iio: adc: ad7173: fix buffers enablement for ad7176-2
iio: adc: ad7173: refactor channel configuration parsing
iio: adc: ad7173: refactor ain and vref selection
iio: adc: ad7173: Remove index from temp channel
iio: adc: ad7173: Add support for AD411x devices
.../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++-
drivers/iio/adc/ad7173.c | 318 ++++++++++++++++++---
2 files changed, 331 insertions(+), 46 deletions(-)
---
base-commit: 5ab61121a34759eb2418977f0b3589b7edc57776
change-id: 20240312-ad4111-7eeb34eb4a5f
Best regards,
--
Dumitru Ceclan <[email protected]>
From: Dumitru Ceclan <[email protected]>
Move configurations regarding number of channels from
*_fw_parse_device_config to *_fw_parse_channel_config.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8a95b1391826..699bc6970790 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -917,7 +917,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
struct device *dev = indio_dev->dev.parent;
struct iio_chan_spec *chan_arr, *chan;
unsigned int ain[2], chan_index = 0;
- int ref_sel, ret;
+ int ref_sel, ret, num_channels;
+
+ num_channels = device_get_child_node_count(dev);
+
+ if (st->info->has_temp)
+ num_channels++;
+
+ if (num_channels == 0)
+ return dev_err_probe(dev, -ENODATA, "No channels specified\n");
+
+ if (num_channels > st->info->num_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "Too many channels specified. Maximum is %d, not including temperature channel if supported.\n",
+ st->info->num_channels);
+
+ indio_dev->num_channels = num_channels;
+ st->num_channels = num_channels;
chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
st->num_channels, GFP_KERNEL);
@@ -1012,7 +1028,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
{
struct ad7173_state *st = iio_priv(indio_dev);
struct device *dev = indio_dev->dev.parent;
- unsigned int num_channels;
int ret;
st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
@@ -1071,16 +1086,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
ad7173_sigma_delta_info.irq_line = ret;
- num_channels = device_get_child_node_count(dev);
-
- if (st->info->has_temp)
- num_channels++;
-
- if (num_channels == 0)
- return dev_err_probe(dev, -ENODATA, "No channels specified\n");
- indio_dev->num_channels = num_channels;
- st->num_channels = num_channels;
-
return ad7173_fw_parse_channel_config(indio_dev);
}
--
2.43.0
From: Dumitru Ceclan <[email protected]>
Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
AD4111/AD4112 support current channels, usage is implemented by
specifying channel reg values bigger than 15.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++-
1 file changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ea6cfcd0aff4..bba2de0a52f3 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -19,7 +19,18 @@ description: |
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.
+ Analog Devices AD411x ADC's:
+ The AD411X family encompasses a series of low power, low noise, 24-bit,
+ sigma-delta analog-to-digital converters that offer a versatile range of
+ specifications. They integrate an analog front end suitable for processing
+ fully differential/single-ended and bipolar voltage inputs.
+
Datasheets for supported chips:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
@@ -31,6 +42,11 @@ description: |
properties:
compatible:
enum:
+ - adi,ad4111
+ - adi,ad4112
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
- adi,ad7172-2
- adi,ad7172-4
- adi,ad7173-8
@@ -125,10 +141,19 @@ patternProperties:
properties:
reg:
+ description:
+ Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
minimum: 0
- maximum: 15
+ maximum: 19
diff-channels:
+ description:
+ For using current channels specify only the positive channel.
+ (IIN2+, IIN2−) -> diff-channels = <2 0>
+
+ Family AD411x supports a dedicated VCOM voltage input.
+ To select it set the second channel to 16.
+ (VIN2, VCOM) -> diff-channels = <2 16>
items:
minimum: 0
maximum: 31
@@ -166,7 +191,6 @@ allOf:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
# Only ad7172-4, ad7173-8 and ad7175-8 support vref2
- # Other models have [0-3] channel registers
- if:
properties:
compatible:
@@ -187,6 +211,37 @@ allOf:
- vref
- refout-avss
- avdd
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
+ - adi,ad7173-8
+ - adi,ad7175-8
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]$":
+ properties:
+ reg:
+ maximum: 15
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad7172-2
+ - adi,ad7175-2
+ - adi,ad7176-2
+ - adi,ad7177-2
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]$":
+ properties:
reg:
maximum: 3
--
2.43.0
From: Dumitru Ceclan <[email protected]>
Move validation of analog inputs and reference voltage selection to
separate functions.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 59 +++++++++++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 699bc6970790..bf5a5b384fe2 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -910,6 +910,41 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
&st->int_clk_hw);
}
+static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
+ unsigned int ain[2])
+{
+ struct device *dev = &st->sd.spi->dev;
+
+ if (ain[0] >= st->info->num_inputs ||
+ ain[1] >= st->info->num_inputs)
+ return dev_err_probe(dev, -EINVAL,
+ "Input pin number out of range for pair (%d %d).\n",
+ ain[0], ain[1]);
+
+ return 0;
+}
+
+static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
+{
+ struct device *dev = &st->sd.spi->dev;
+ int ret;
+
+ if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
+ return dev_err_probe(dev, -EINVAL,
+ "Internal reference is not available on current model.\n");
+
+ if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
+ return dev_err_probe(dev, -EINVAL,
+ "External reference 2 is not available on current model.\n");
+
+ ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Cannot use reference %u\n", ref_sel);
+
+ return 0;
+}
+
static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
{
struct ad7173_channel *chans_st_arr, *chan_st_priv;
@@ -970,11 +1005,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
if (ret)
return ret;
- if (ain[0] >= st->info->num_inputs ||
- ain[1] >= st->info->num_inputs)
- return dev_err_probe(dev, -EINVAL,
- "Input pin number out of range for pair (%d %d).\n",
- ain[0], ain[1]);
+ ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ if (ret)
+ return ret;
ret = fwnode_property_match_property_string(child,
"adi,reference-select",
@@ -985,19 +1018,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
else
ref_sel = ret;
- if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
- !st->info->has_int_ref)
- return dev_err_probe(dev, -EINVAL,
- "Internal reference is not available on current model.\n");
-
- if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
- return dev_err_probe(dev, -EINVAL,
- "External reference 2 is not available on current model.\n");
-
- ret = ad7173_get_ref_voltage_milli(st, ref_sel);
- if (ret < 0)
- return dev_err_probe(dev, ret,
- "Cannot use reference %u\n", ref_sel);
+ ret = ad7173_validate_reference(st, ref_sel);
+ if (ret)
+ return ret;
if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
st->adc_mode |= AD7173_ADC_MODE_REF_EN;
--
2.43.0
From: Dumitru Ceclan <[email protected]>
AD7176-2 does not feature input buffers, enable buffers only on
supported models.
Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index f6d29abe1d04..8a95b1391826 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -145,6 +145,7 @@ struct ad7173_device_info {
unsigned int id;
char *name;
bool has_temp;
+ bool has_input_buf;
bool has_int_ref;
bool has_ref2;
u8 num_gpios;
@@ -212,6 +213,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 2 * HZ_PER_MHZ,
.sinc5_data_rates = ad7173_sinc5_data_rates,
@@ -224,6 +226,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = false,
+ .has_input_buf = true,
.has_ref2 = true,
.clock = 2 * HZ_PER_MHZ,
.sinc5_data_rates = ad7173_sinc5_data_rates,
@@ -237,6 +240,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.has_ref2 = true,
.clock = 2 * HZ_PER_MHZ,
@@ -251,6 +255,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 16 * HZ_PER_MHZ,
.sinc5_data_rates = ad7175_sinc5_data_rates,
@@ -263,6 +268,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.has_ref2 = true,
.clock = 16 * HZ_PER_MHZ,
@@ -289,6 +295,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 16 * HZ_PER_MHZ,
.odr_start_value = AD7177_ODR_START_VALUE,
@@ -932,7 +939,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
AD7173_CH_ADDRESS(chan_arr[chan_index].channel,
chan_arr[chan_index].channel2);
chan_st_priv->cfg.bipolar = false;
- chan_st_priv->cfg.input_buf = true;
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
st->adc_mode |= AD7173_ADC_MODE_REF_EN;
@@ -989,7 +996,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
chan_st_priv->chan_reg = chan_index;
- chan_st_priv->cfg.input_buf = true;
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.odr = 0;
chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
--
2.43.0
From: Dumitru Ceclan <[email protected]>
Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications.
This family of ADCs integrates an analog front end suitable for processing
both fully differential and single-ended, bipolar voltage inputs
addressing a wide array of industrial and instrumentation requirements.
- All ADCs have inputs with a precision voltage divider with a division
ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
shunt resistor.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 224 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 210 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 9526585e6929..ac32bd7dbd1e 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0+
/*
- * AD717x family SPI ADC driver
+ * AD717x and AD411x family SPI ADC driver
*
* Supported devices:
+ * AD4111/AD4112/AD4114/AD4115/AD4116
* AD7172-2/AD7172-4/AD7173-8/AD7175-2
* AD7175-8/AD7176-2/AD7177-2
*
@@ -72,6 +73,11 @@
#define AD7175_2_ID 0x0cd0
#define AD7172_4_ID 0x2050
#define AD7173_ID 0x30d0
+#define AD4111_ID 0x30d0
+#define AD4112_ID 0x30d0
+#define AD4114_ID 0x30d0
+#define AD4116_ID 0x34d0
+#define AD4115_ID 0x38d0
#define AD7175_8_ID 0x3cd0
#define AD7177_ID 0x4fd0
#define AD7173_ID_MASK GENMASK(15, 4)
@@ -120,11 +126,20 @@
#define AD7173_VOLTAGE_INT_REF_uV 2500000
#define AD7173_TEMP_SENSIIVITY_uV_per_C 477
#define AD7177_ODR_START_VALUE 0x07
+#define AD4111_SHUNT_RESISTOR_OHM 50
+#define AD4111_DIVIDER_RATIO 10
+#define AD411X_VCOM_INPUT 0X10
+#define AD4111_CURRENT_CHAN_CUTOFF 16
#define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
#define AD7173_MAX_CONFIGS 8
enum ad7173_ids {
+ ID_AD4111,
+ ID_AD4112,
+ ID_AD4114,
+ ID_AD4115,
+ ID_AD4116,
ID_AD7172_2,
ID_AD7172_4,
ID_AD7173_8,
@@ -134,16 +149,26 @@ enum ad7173_ids {
ID_AD7177_2,
};
+enum ad4111_current_channels {
+ AD4111_CURRENT_IN0P_IN0N,
+ AD4111_CURRENT_IN1P_IN1N,
+ AD4111_CURRENT_IN2P_IN2N,
+ AD4111_CURRENT_IN3P_IN3N,
+};
+
struct ad7173_device_info {
const unsigned int *sinc5_data_rates;
unsigned int num_sinc5_data_rates;
unsigned int odr_start_value;
+ unsigned int num_inputs_with_divider;
unsigned int num_channels;
unsigned int num_configs;
unsigned int num_inputs;
unsigned int clock;
unsigned int id;
char *name;
+ bool has_current_inputs;
+ bool has_vcom;
bool has_temp;
bool has_input_buf;
bool has_int_ref;
@@ -189,6 +214,24 @@ struct ad7173_state {
#endif
};
+static unsigned int ad4115_sinc5_data_rates[] = {
+ 24845000, 24845000, 20725000, 20725000, /* 0-3 */
+ 15564000, 13841000, 10390000, 10390000, /* 4-7 */
+ 4994000, 2499000, 1000000, 500000, /* 8-11 */
+ 395500, 200000, 100000, 59890, /* 12-15 */
+ 49920, 20000, 16660, 10000, /* 16-19 */
+ 5000, 2500, 2500, /* 20-22 */
+};
+
+static unsigned int ad4116_sinc5_data_rates[] = {
+ 12422360, 12422360, 12422360, 12422360, /* 0-3 */
+ 10362690, 10362690, 7782100, 6290530, /* 4-7 */
+ 5194800, 2496900, 1007600, 499900, /* 8-11 */
+ 390600, 200300, 100000, 59750, /* 12-15 */
+ 49840, 20000, 16650, 10000, /* 16-19 */
+ 5000, 2500, 1250, /* 20-22 */
+};
+
static const unsigned int ad7173_sinc5_data_rates[] = {
6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */
3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */
@@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
5000, /* 20 */
};
+static unsigned int ad4111_current_channel_config[] = {
+ [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
+ [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
+ [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
+ [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
+};
+
static const struct ad7173_device_info ad7173_device_info[] = {
+ [ID_AD4111] = {
+ .id = AD4111_ID,
+ .num_inputs_with_divider = 8,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 8,
+ .num_gpios = 2,
+ .has_temp = true,
+ .has_vcom = true,
+ .has_input_buf = true,
+ .has_current_inputs = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4112] = {
+ .id = AD4112_ID,
+ .num_inputs_with_divider = 8,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 8,
+ .num_gpios = 2,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_current_inputs = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4114] = {
+ .id = AD4114_ID,
+ .num_inputs_with_divider = 16,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4115] = {
+ .id = AD4115_ID,
+ .num_inputs_with_divider = 16,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 8 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad4115_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
+ },
+ [ID_AD4116] = {
+ .id = AD4116_ID,
+ .num_inputs_with_divider = 11,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 4 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad4116_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
+ },
[ID_AD7172_2] = {
.name = "ad7172-2",
.id = AD7172_2_ID,
@@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- if (chan->type == IIO_TEMP) {
+
+ switch (chan->type) {
+ case IIO_TEMP:
temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
*val = temp;
*val2 = chan->scan_type.realbits;
- } else {
+ break;
+ case IIO_VOLTAGE:
*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+
+ if (chan->channel < st->info->num_inputs_with_divider)
+ *val *= AD4111_DIVIDER_RATIO;
+ break;
+ case IIO_CURRENT:
+ *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+ *val /= AD4111_SHUNT_RESISTOR_OHM;
+ *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+ break;
+ default:
+ return -EINVAL;
}
return IIO_VAL_FRACTIONAL_LOG2;
case IIO_CHAN_INFO_OFFSET:
- if (chan->type == IIO_TEMP) {
+
+ switch (chan->type) {
+ case IIO_TEMP:
/* 0 Kelvin -> raw sample */
temp = -ABSOLUTE_ZERO_MILLICELSIUS;
temp *= AD7173_TEMP_SENSIIVITY_uV_per_C;
@@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
AD7173_VOLTAGE_INT_REF_uV *
MILLI);
*val = -temp;
- } else {
+ break;
+ case IIO_VOLTAGE:
+ case IIO_CURRENT:
*val = -BIT(chan->scan_type.realbits - 1);
+ break;
+ default:
+ return -EINVAL;
}
return IIO_VAL_INT;
case IIO_CHAN_INFO_SAMP_FREQ:
@@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
&st->int_clk_hw);
}
+static int ad4111_validate_current_ain(struct ad7173_state *st,
+ unsigned int ain[2])
+{
+ struct device *dev = &st->sd.spi->dev;
+
+ if (!st->info->has_current_inputs)
+ return dev_err_probe(dev, -EINVAL,
+ "Reg values equal to or higher than %d are restricted to models with current channels.\n",
+ AD4111_CURRENT_CHAN_CUTOFF);
+
+ if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
+ return dev_err_probe(dev, -EINVAL,
+ "For current channel diff-channels must be <[0-%d],0>\n",
+ ARRAY_SIZE(ad4111_current_channel_config) - 1);
+
+ return 0;
+}
+
static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
unsigned int ain[2])
{
@@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
struct device *dev = indio_dev->dev.parent;
struct iio_chan_spec *chan_arr, *chan;
unsigned int ain[2], chan_index = 0;
- int ref_sel, ret, num_channels;
+ int ref_sel, ret, reg, num_channels;
num_channels = device_get_child_node_count(dev);
@@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
if (ret)
return ret;
- ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ ret = fwnode_property_read_u32(child, "reg", ®);
if (ret)
return ret;
+ if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
+ ret = ad4111_validate_current_ain(st, ain);
+ if (ret)
+ return ret;
+ } else {
+ ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ if (ret)
+ return ret;
+ }
+
ret = fwnode_property_match_property_string(child,
"adi,reference-select",
ad7173_ref_sel_str,
@@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
*chan = ad7173_channel_template;
chan->address = chan_index;
chan->scan_index = chan_index;
- chan->channel = ain[0];
- chan->channel2 = ain[1];
- chan->differential = true;
- chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+ if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
+ chan->type = IIO_CURRENT;
+ chan->channel = ain[0];
+ chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
+ } else {
+ chan->channel = ain[0];
+ chan->channel2 = ain[1];
+ chan->differential = true;
+
+ chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
+ }
+
chan_st_priv->chan_reg = chan_index;
- chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.odr = 0;
-
chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
if (chan_st_priv->cfg.bipolar)
chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
@@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi)
}
static const struct of_device_id ad7173_of_match[] = {
+ { .compatible = "ad4111",
+ .data = &ad7173_device_info[ID_AD4111]},
+ { .compatible = "ad4112",
+ .data = &ad7173_device_info[ID_AD4112]},
+ { .compatible = "ad4114",
+ .data = &ad7173_device_info[ID_AD4114]},
+ { .compatible = "ad4115",
+ .data = &ad7173_device_info[ID_AD4115]},
{ .compatible = "adi,ad7172-2",
.data = &ad7173_device_info[ID_AD7172_2]},
{ .compatible = "adi,ad7172-4",
@@ -1186,6 +1377,11 @@ static const struct of_device_id ad7173_of_match[] = {
MODULE_DEVICE_TABLE(of, ad7173_of_match);
static const struct spi_device_id ad7173_id_table[] = {
+ { "ad4111", (kernel_ulong_t)&ad7173_device_info[ID_AD4111]},
+ { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]},
+ { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]},
+ { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]},
+ { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]},
{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
{ "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
@@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver);
MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
MODULE_AUTHOR("Lars-Peter Clausen <[email protected]>");
MODULE_AUTHOR("Dumitru Ceclan <[email protected]>");
-MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver");
MODULE_LICENSE("GPL");
--
2.43.0
From: Dumitru Ceclan <[email protected]>
Temperature channel is unique per device, index is not needed.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index bf5a5b384fe2..9526585e6929 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -797,7 +797,6 @@ static const struct iio_info ad7173_info = {
static const struct iio_chan_spec ad7173_channel_template = {
.type = IIO_VOLTAGE,
- .indexed = 1,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
--
2.43.0
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>
> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> AD4111/AD4112 support current channels, usage is implemented by
> specifying channel reg values bigger than 15.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++-
> 1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> index ea6cfcd0aff4..bba2de0a52f3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -19,7 +19,18 @@ description: |
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
>
> + Analog Devices AD411x ADC's:
> + The AD411X family encompasses a series of low power, low noise, 24-bit,
> + sigma-delta analog-to-digital converters that offer a versatile range of
> + specifications. They integrate an analog front end suitable for processing
> + fully differential/single-ended and bipolar voltage inputs.
> +
> Datasheets for supported chips:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> @@ -31,6 +42,11 @@ description: |
> properties:
> compatible:
> enum:
> + - adi,ad4111
> + - adi,ad4112
> + - adi,ad4114
> + - adi,ad4115
> + - adi,ad4116
> - adi,ad7172-2
> - adi,ad7172-4
> - adi,ad7173-8
> @@ -125,10 +141,19 @@ patternProperties:
>
> properties:
> reg:
> + description:
> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> minimum: 0
> - maximum: 15
> + maximum: 19
This looks wrong. Isn't reg describing the number of logical channels
(# of channel config registers)?
After reviewing the driver, I see that > 16 is used as a way of
flagging current inputs, but still seems like the wrong way to do it.
See suggestion below.
>
> diff-channels:
> + description:
> + For using current channels specify only the positive channel.
> + (IIN2+, IIN2−) -> diff-channels = <2 0>
I find this a bit confusing since 2 is already VIN2 and 0 is already
VIN0. I think it would make more sense to assign unique channel
numbers individually to the negative and positive current inputs.
Also, I think it makes sense to use the same numbers that the
registers in the datasheet use (8 - 11 for negative and 12 to 15 for
positive).
So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> +
> + Family AD411x supports a dedicated VCOM voltage input.
> + To select it set the second channel to 16.
> + (VIN2, VCOM) -> diff-channels = <2 16>
The 411x datasheets call this pin VINCOM so calling it VCOM here is a
bit confusing.
Also, do we need to add a vincom-supply to get this voltage? Or is it
safe to assume it is always connected to AVSS? The datasheet seems to
indicate that the latter is the case. But then it also has this
special case (at least for AD4116, didn't check all datasheets)
"VIN10, VINCOM (single-ended or differential pair)". If it can be used
as part of a fully differential input, we probably need some extra
flag to indicate that case.
Similarly, do we need special handling for ADCIN15 on AD4116? It has a
"(pseudo differential or differential pair)" notation that other
inputs don't. In other words, it is more like VINCOM than it is to the
other ADCINxx pins. So we probably need an adcin15-supply for this pin
to properly get the right channel configuration. I.e. the logic in the
IIO driver would be if adcin15-supply is present, any channels that
use this input are pseudo-differential, otherwise any channels that
use it are fully differential.
> items:
> minimum: 0
> maximum: 31
> @@ -166,7 +191,6 @@ allOf:
> - $ref: /schemas/spi/spi-peripheral-props.yaml#
>
> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> - # Other models have [0-3] channel registers
Did you forget to remove
reg:
maximum: 3
from this if statement that this comment is referring to?
> - if:
> properties:
> compatible:
> @@ -187,6 +211,37 @@ allOf:
> - vref
> - refout-avss
> - avdd
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,ad4114
> + - adi,ad4115
> + - adi,ad4116
> + - adi,ad7173-8
> + - adi,ad7175-8
> + then:
> + patternProperties:
> + "^channel@[0-9a-f]$":
> + properties:
> + reg:
> + maximum: 15
As with the previous reg comment, this if statement should not be
needed since maximum should not be changed to 19.
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,ad7172-2
> + - adi,ad7175-2
> + - adi,ad7176-2
> + - adi,ad7177-2
> + then:
> + patternProperties:
> + "^channel@[0-9a-f]$":
> + properties:
> reg:
> maximum: 3
It looks to me like AD7172-4 actually has 8 possible channels rather
than 16. So it would need a special condition as well. But that is a
bug in the previous bindings and should therefore be fixed in a
separate patch.
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> AD7176-2 does not feature input buffers, enable buffers only on
> supported models.
>
> Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
To be consistent with has_temp, maybe add `.has_input_buf = false,` to
ID_AD7176_2.
But either way:
Reviewed-by: David Lechner <[email protected]>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Move configurations regarding number of channels from
> *_fw_parse_device_config to *_fw_parse_channel_config.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
Commit messages need to explain _why_ the change is being made [1]. It
is not obvious to me why this needs to be moved.
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Move validation of analog inputs and reference voltage selection to
> separate functions.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
Same as my comment on PATCH 3/6. We would like to know why this change
is being made.
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Temperature channel is unique per device, index is not needed.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
This breaks userspace, so the commit message should explain why it is
safe to do this (e.g. driver hasn't reached mainline yet, so won't
break existing users since there are none).
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
>
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
>
> - All ADCs have inputs with a precision voltage divider with a division
> ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> shunt resistor.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> drivers/iio/adc/ad7173.c | 224 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 210 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 9526585e6929..ac32bd7dbd1e 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1,8 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * AD717x family SPI ADC driver
> + * AD717x and AD411x family SPI ADC driver
> *
> * Supported devices:
> + * AD4111/AD4112/AD4114/AD4115/AD4116
> * AD7172-2/AD7172-4/AD7173-8/AD7175-2
> * AD7175-8/AD7176-2/AD7177-2
> *
> @@ -72,6 +73,11 @@
> #define AD7175_2_ID 0x0cd0
> #define AD7172_4_ID 0x2050
> #define AD7173_ID 0x30d0
> +#define AD4111_ID 0x30d0
> +#define AD4112_ID 0x30d0
> +#define AD4114_ID 0x30d0
It might make it a bit more obvious that not all chips have a unique
ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather
than introducing multiple macros with the same value.
Or leave it as AD7173_ID to keep it short and add a comment where it
is used with 411x chips in ad7173_device_info[].
> +#define AD4116_ID 0x34d0
> +#define AD4115_ID 0x38d0
> #define AD7175_8_ID 0x3cd0
> #define AD7177_ID 0x4fd0
> #define AD7173_ID_MASK GENMASK(15, 4)
> @@ -120,11 +126,20 @@
> #define AD7173_VOLTAGE_INT_REF_uV 2500000
> #define AD7173_TEMP_SENSIIVITY_uV_per_C 477
> #define AD7177_ODR_START_VALUE 0x07
> +#define AD4111_SHUNT_RESISTOR_OHM 50
> +#define AD4111_DIVIDER_RATIO 10
> +#define AD411X_VCOM_INPUT 0X10
> +#define AD4111_CURRENT_CHAN_CUTOFF 16
>
> #define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
> #define AD7173_MAX_CONFIGS 8
>
> enum ad7173_ids {
> + ID_AD4111,
> + ID_AD4112,
> + ID_AD4114,
> + ID_AD4115,
> + ID_AD4116,
> ID_AD7172_2,
> ID_AD7172_4,
> ID_AD7173_8,
> @@ -134,16 +149,26 @@ enum ad7173_ids {
> ID_AD7177_2,
> };
>
> +enum ad4111_current_channels {
> + AD4111_CURRENT_IN0P_IN0N,
> + AD4111_CURRENT_IN1P_IN1N,
> + AD4111_CURRENT_IN2P_IN2N,
> + AD4111_CURRENT_IN3P_IN3N,
> +};
> +
> struct ad7173_device_info {
> const unsigned int *sinc5_data_rates;
> unsigned int num_sinc5_data_rates;
> unsigned int odr_start_value;
> + unsigned int num_inputs_with_divider;
> unsigned int num_channels;
> unsigned int num_configs;
> unsigned int num_inputs;
Probably a good idea to change num_inputs to num_voltage_inputs so it
isn't confused with the total number of inputs.
Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider.
Also could use a comment to make it clear if num_voltage_inputs
includes num_voltage_inputs_with_divider or not. And that it doesn't
include VINCOM.
Probably also need some flag here to differentiate ADCINxx voltage
inputs on AD4116.
> unsigned int clock;
> unsigned int id;
> char *name;
> + bool has_current_inputs;
Maybe more future-proof to have num_current_inputs instead of bool?
> + bool has_vcom;
For consistency: has_vcom_input
> bool has_temp;
> bool has_input_buf;
> bool has_int_ref;
> @@ -189,6 +214,24 @@ struct ad7173_state {
> #endif
> };
>
> +static unsigned int ad4115_sinc5_data_rates[] = {
> + 24845000, 24845000, 20725000, 20725000, /* 0-3 */
> + 15564000, 13841000, 10390000, 10390000, /* 4-7 */
> + 4994000, 2499000, 1000000, 500000, /* 8-11 */
> + 395500, 200000, 100000, 59890, /* 12-15 */
> + 49920, 20000, 16660, 10000, /* 16-19 */
> + 5000, 2500, 2500, /* 20-22 */
> +};
> +
> +static unsigned int ad4116_sinc5_data_rates[] = {
> + 12422360, 12422360, 12422360, 12422360, /* 0-3 */
> + 10362690, 10362690, 7782100, 6290530, /* 4-7 */
> + 5194800, 2496900, 1007600, 499900, /* 8-11 */
> + 390600, 200300, 100000, 59750, /* 12-15 */
> + 49840, 20000, 16650, 10000, /* 16-19 */
> + 5000, 2500, 1250, /* 20-22 */
> +};
> +
> static const unsigned int ad7173_sinc5_data_rates[] = {
> 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */
> 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */
> @@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
> 5000, /* 20 */
> };
>
> +static unsigned int ad4111_current_channel_config[] = {
> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
> +};
As mentioned in the DT bindings review, it would make more sense to
just use the datasheet numbers for the current input channels in the
diff-channels DT property, then we don't need this lookup table.
> +
> static const struct ad7173_device_info ad7173_device_info[] = {
> + [ID_AD4111] = {
> + .id = AD4111_ID,
> + .num_inputs_with_divider = 8,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 8,
> + .num_gpios = 2,
> + .has_temp = true,
> + .has_vcom = true,
> + .has_input_buf = true,
> + .has_current_inputs = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4112] = {
> + .id = AD4112_ID,
> + .num_inputs_with_divider = 8,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 8,
> + .num_gpios = 2,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_current_inputs = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4114] = {
> + .id = AD4114_ID,
> + .num_inputs_with_divider = 16,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4115] = {
> + .id = AD4115_ID,
> + .num_inputs_with_divider = 16,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 8 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad4115_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
> + },
> + [ID_AD4116] = {
> + .id = AD4116_ID,
> + .num_inputs_with_divider = 11,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 4 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad4116_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
> + },
> [ID_AD7172_2] = {
> .name = "ad7172-2",
> .id = AD7172_2_ID,
> @@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> - if (chan->type == IIO_TEMP) {
> +
> + switch (chan->type) {
> + case IIO_TEMP:
> temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
> temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
> *val = temp;
> *val2 = chan->scan_type.realbits;
> - } else {
> + break;
can we just return here instead of break?
> + case IIO_VOLTAGE:
> *val = ad7173_get_ref_voltage_milli(st, ch->cfgref_sel);
> *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +
> + if (chan->channel < st->info->num_inputs_with_divider)
> + *val *= AD4111_DIVIDER_RATIO;
> + break;
same: return here
> + case IIO_CURRENT:
> + *val = ad7173_get_ref_voltage_milli(st, ch->cfgref_sel);
> + *val /= AD4111_SHUNT_RESISTOR_OHM;
> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
Static analysis tools like to complain about using bool as int.
Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
> + break;
return here
> + default:
> + return -EINVAL;
> }
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> - if (chan->type == IIO_TEMP) {
> +
> + switch (chan->type) {
> + case IIO_TEMP:
> /* 0 Kelvin -> raw sample */
> temp = -ABSOLUTE_ZERO_MILLICELSIUS;
> temp *= AD7173_TEMP_SENSIIVITY_uV_per_C;
> @@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
> AD7173_VOLTAGE_INT_REF_uV *
> MILLI);
> *val = -temp;
> - } else {
> + break;
return ...
> + case IIO_VOLTAGE:
> + case IIO_CURRENT:
> *val = -BIT(chan->scan_type.realbits - 1);
Expecting a special case here, at least when ADCIN15 is configured for
pseudo-differential inputs.
> + break;
return ...
> + default:
> + return -EINVAL;
> }
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SAMP_FREQ:
> @@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
> &st->int_clk_hw);
> }
>
> +static int ad4111_validate_current_ain(struct ad7173_state *st,
> + unsigned int ain[2])
> +{
> + struct device *dev = &st->sd.spi->dev;
> +
> + if (!st->info->has_current_inputs)
> + return dev_err_probe(dev, -EINVAL,
> + "Reg values equal to or higher than %d are restricted to models with current channels.\n",
> + AD4111_CURRENT_CHAN_CUTOFF);
> +
> + if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
> + return dev_err_probe(dev, -EINVAL,
> + "For current channel diff-channels must be <[0-%d],0>\n",
> + ARRAY_SIZE(ad4111_current_channel_config) - 1);
> +
> + return 0;
> +}
> +
> static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> unsigned int ain[2])
> {
> @@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> struct device *dev = indio_dev->dev.parent;
> struct iio_chan_spec *chan_arr, *chan;
> unsigned int ain[2], chan_index = 0;
> - int ref_sel, ret, num_channels;
> + int ref_sel, ret, reg, num_channels;
>
> num_channels = device_get_child_node_count(dev);
>
> @@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> if (ret)
> return ret;
>
> - ret = ad7173_validate_voltage_ain_inputs(st, ain);
> + ret = fwnode_property_read_u32(child, "reg", ®);
> if (ret)
> return ret;
>
> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
As mentioned in the DT bindings review, using reg to determine if an a
channel is a current input or voltage input seems wrong/fragile. We
should be able to use the has_current_inputs flag and the input
numbers instead to determine the type of input.
> + ret = ad4111_validate_current_ain(st, ain);
> + if (ret)
> + return ret;
> + } else {
> + ret = ad7173_validate_voltage_ain_inputs(st, ain);
> + if (ret)
> + return ret;
> + }
> +
> ret = fwnode_property_match_property_string(child,
> "adi,reference-select",
> ad7173_ref_sel_str,
> @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> *chan = ad7173_channel_template;
> chan->address = chan_index;
> chan->scan_index = chan_index;
> - chan->channel = ain[0];
> - chan->channel2 = ain[1];
> - chan->differential = true;
>
> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> + chan->type = IIO_CURRENT;
> + chan->channel = ain[0];
> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> + } else {
> + chan->channel = ain[0];
> + chan->channel2 = ain[1];
> + chan->differential = true;
Expecting chan->differential = false when ADCIN15 is configured for
pseudo-differential inputs.
Also, perhaps missed in previous reviews, I would expect
chan->differential = false when channels are used as single-ended.
> +
> + chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> + chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> + }
> +
> chan_st_priv->chan_reg = chan_index;
> - chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.odr = 0;
> -
> chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
> if (chan_st_priv->cfg.bipolar)
> chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> @@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi)
> }
>
> static const struct of_device_id ad7173_of_match[] = {
> + { .compatible = "ad4111",
> + .data = &ad7173_device_info[ID_AD4111]},
> + { .compatible = "ad4112",
> + .data = &ad7173_device_info[ID_AD4112]},
> + { .compatible = "ad4114",
> + .data = &ad7173_device_info[ID_AD4114]},
> + { .compatible = "ad4115",
> + .data = &ad7173_device_info[ID_AD4115]},
> { .compatible = "adi,ad7172-2",
> .data = &ad7173_device_info[ID_AD7172_2]},
> { .compatible = "adi,ad7172-4",
> @@ -1186,6 +1377,11 @@ static const struct of_device_id ad7173_of_match[] = {
> MODULE_DEVICE_TABLE(of, ad7173_of_match);
>
> static const struct spi_device_id ad7173_id_table[] = {
> + { "ad4111", (kernel_ulong_t)&ad7173_device_info[ID_AD4111]},
> + { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]},
> + { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]},
> + { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]},
> + { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]},
> { "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
> { "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
> { "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
> @@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver);
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
> MODULE_AUTHOR("Lars-Peter Clausen <[email protected]>");
> MODULE_AUTHOR("Dumitru Ceclan <[email protected]>");
> -MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
> +MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver");
> MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
>
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
> >
> > From: Dumitru Ceclan <[email protected]>
> >
> > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> >
> > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> > AD4111/AD4112 support current channels, usage is implemented by
> > specifying channel reg values bigger than 15.
> >
> > Signed-off-by: Dumitru Ceclan <[email protected]>
> > ---
> > .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++-
> > 1 file changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > index ea6cfcd0aff4..bba2de0a52f3 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
Also, I just noticed that AD411x have only one AVDD input instead of
AVDD1 and AVDD2. So we need an if statement that says if properties:
compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
> >
> > From: Dumitru Ceclan <[email protected]>
> >
> > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> >
> > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> > AD4111/AD4112 support current channels, usage is implemented by
> > specifying channel reg values bigger than 15.
> >
> > Signed-off-by: Dumitru Ceclan <[email protected]>
> > ---
..
> > @@ -125,10 +141,19 @@ patternProperties:
> >
> > properties:
> > reg:
> > + description:
> > + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> > minimum: 0
> > - maximum: 15
> > + maximum: 19
>
> This looks wrong. Isn't reg describing the number of logical channels
> (# of channel config registers)?
>
> After reviewing the driver, I see that > 16 is used as a way of
> flagging current inputs, but still seems like the wrong way to do it.
> See suggestion below.
>
> >
> > diff-channels:
> > + description:
> > + For using current channels specify only the positive channel.
> > + (IIN2+, IIN2−) -> diff-channels = <2 0>
>
> I find this a bit confusing since 2 is already VIN2 and 0 is already
> VIN0. I think it would make more sense to assign unique channel
> numbers individually to the negative and positive current inputs.
> Also, I think it makes sense to use the same numbers that the
> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> positive).
>
> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
Thinking about this a bit more...
Since the current inputs have dedicated pins and aren't mix-and-match
with multiple valid wiring configurations like the voltage inputs, do
we even need to describe them in the devicetree?
In the driver, the current channels would just be hard-coded like the
temperature channel since there isn't any application-specific
variation.
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<[email protected]> wrote:
>
> From: Dumitru Ceclan <[email protected]>
>
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
>
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
>
> - All ADCs have inputs with a precision voltage divider with a division
> ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> shunt resistor.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
..
> @@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> struct device *dev = indio_dev->dev.parent;
> struct iio_chan_spec *chan_arr, *chan;
> unsigned int ain[2], chan_index = 0;
> - int ref_sel, ret, num_channels;
> + int ref_sel, ret, reg, num_channels;
>
> num_channels = device_get_child_node_count(dev);
>
Another thing that is missing in this function both for the chips
being added here and the existing chips are channels for _all_
possible inputs. The driver is adding a fixed input channel for the
temperature sensor, as it should. But all of the chips also have a
similar input channel configuration that measures the reference
voltage. Currently, there doesn't seem to be a way to make use of this
feature. I would expect a hard-coded voltage input channel that is
always added for this reference voltage similar to the temperature
channel.
The ad717x chips (except AD7173-8 and AD7176-2) also have a common
mode voltage input ("((AVDD1 − AVSS)/5)") that could work the same.
In the case of the ad717x chips though, it looks like these channels
are not "fixed" like they are in ad411x. It looks like these inputs
can be mixed and matched with AINx inputs and/or each other as
differential pairs. So if that is actually the case, I would expect
the DT bindings for ad717x to look like adi,ad4130.yaml where these
additional input sources are listed in the diff-channels property
instead of having hard-coded channels in the driver like I have
suggested above.
But, as always, fixes for ad717x should be in a separate patch series.
Still, I think adding a hard-coded channel for the reference voltage
input for ad411x chips in this patch makes sense.
On Mon, Apr 1, 2024 at 2:45 PM David Lechner <[email protected]> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
> >
> > From: Dumitru Ceclan <[email protected]>
> >
> > Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> >
..
> > @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> > *chan = ad7173_channel_template;
> > chan->address = chan_index;
> > chan->scan_index = chan_index;
> > - chan->channel = ain[0];
> > - chan->channel2 = ain[1];
> > - chan->differential = true;
> >
> > - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> > + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> > + chan->type = IIO_CURRENT;
> > + chan->channel = ain[0];
> > + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> > + } else {
> > + chan->channel = ain[0];
> > + chan->channel2 = ain[1];
> > + chan->differential = true;
>
> Expecting chan->differential = false when ADCIN15 is configured for
> pseudo-differential inputs.
>
> Also, perhaps missed in previous reviews, I would expect
> chan->differential = false when channels are used as single-ended.
>
After sleeping on it, I came to the concision that these parts are
probably too complex to try to worry about differential vs.
pseudo-differential/single-ended (what the datasheet calls
single-ended is really pseudo-differential).
So I take back my comments about expecting differential = false in those cases.
On 01/04/2024 22:37, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
>>
>> From: Dumitru Ceclan <[email protected]>
..
>> properties:
>> reg:
>> + description:
>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>> minimum: 0
>> - maximum: 15
>> + maximum: 19
>
> This looks wrong. Isn't reg describing the number of logical channels
> (# of channel config registers)?
>
> After reviewing the driver, I see that > 16 is used as a way of
> flagging current inputs, but still seems like the wrong way to do it.
> See suggestion below.
>
This was a suggestion from Jonathan, maybe I implemented it wrong.
Other alternative that came to my mind: attribute "adi,current-channel".
>>
>> diff-channels:
>> + description:
>> + For using current channels specify only the positive channel.
>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>
> I find this a bit confusing since 2 is already VIN2 and 0 is already
> VIN0. I think it would make more sense to assign unique channel
> numbers individually to the negative and positive current inputs.
> Also, I think it makes sense to use the same numbers that the
> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> positive).
>
> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>
>
It would mean for the user to look in the datasheet at the possible
channel INPUT configurations values, decode the bit field into two
integer values and place it here (0110101010) -> 13 10. This is
cumbersome for just choosing current input 2.
>> +
>> + Family AD411x supports a dedicated VCOM voltage input.
>> + To select it set the second channel to 16.
>> + (VIN2, VCOM) -> diff-channels = <2 16>
>
> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> bit confusing.
>
Sure, I'll rename to VINCOM.
> Also, do we need to add a vincom-supply to get this voltage? Or is it
> safe to assume it is always connected to AVSS? The datasheet seems to
> indicate that the latter is the case. But then it also has this
> special case (at least for AD4116, didn't check all datasheets)
> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> as part of a fully differential input, we probably need some extra
> flag to indicate that case.
>
I cannot see any configuration options for these use cases. All inputs
are routed to the same mux and routed to the differential positive and
negative ADC inputs.
"VIN10, VINCOM (single-ended or differential pair)" the only difference
between these two use cases is if you connected VINCOM to AVSS (with
unipolar coding) or not with bipolar encoding. The channel is still
measuring the difference between the two selected inputs and comparing
to the selected reference.
> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> "(pseudo differential or differential pair)" notation that other
> inputs don't. In other words, it is more like VINCOM than it is to the
> other ADCINxx pins. So we probably need an adcin15-supply for this pin
> to properly get the right channel configuration. I.e. the logic in the
> IIO driver would be if adcin15-supply is present, any channels that
> use this input are pseudo-differential, otherwise any channels that
> use it are fully differential.
>
I cannot seem to understand what would a adcin15-supply be needed for.
This input, the same as all others, enters the mux and is routed to
either positive or negative input of the ADC.
The voltage on the ADCIN15 pin is not important to the user, just the
difference in voltage between that pin and the other one selected.
>> items:
>> minimum: 0
>> maximum: 31
>> @@ -166,7 +191,6 @@ allOf:
>> - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>
>> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
>> - # Other models have [0-3] channel registers
>
> Did you forget to remove
>
> reg:
> maximum: 3
>
> from this if statement that this comment is referring to?
>
>
Other way around, forgot in a previous patch to remove the comment.
I'll move this change to a precursor patch.
>> - if:
>> properties:
>> compatible:
>> @@ -187,6 +211,37 @@ allOf:
>> - vref
>> - refout-avss
>> - avdd
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - adi,ad4114
>> + - adi,ad4115
>> + - adi,ad4116
>> + - adi,ad7173-8
>> + - adi,ad7175-8
>> + then:
>> + patternProperties:
>> + "^channel@[0-9a-f]$":
>> + properties:
>> + reg:
>> + maximum: 15
>
> As with the previous reg comment, this if statement should not be
> needed since maximum should not be changed to 19.
>
We'll see what is the best approach regarding the current channels,
perhaps the one you mentioned in the later reply with always configuring
like the temp channel.
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - adi,ad7172-2
>> + - adi,ad7175-2
>> + - adi,ad7176-2
>> + - adi,ad7177-2
>> + then:
>> + patternProperties:
>> + "^channel@[0-9a-f]$":
>> + properties:
>> reg:
>> maximum: 3
>
> It looks to me like AD7172-4 actually has 8 possible channels rather
> than 16. So it would need a special condition as well. But that is a
> bug in the previous bindings and should therefore be fixed in a
> separate patch.
It is addressed already in the binding:
"
- if:
properties:
compatible:
contains:
const: adi,ad7172-4
[...]
maximum: 7
"
On 02/04/2024 00:16, David Lechner wrote:
> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>>
>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>> <[email protected]> wrote:
>>>
>>> From: Dumitru Ceclan <[email protected]>
>>>
..
>>>
>>> properties:
>>> reg:
>>> + description:
>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>> minimum: 0
>>> - maximum: 15
>>> + maximum: 19
>>
>> This looks wrong. Isn't reg describing the number of logical channels
>> (# of channel config registers)?
>>
>> After reviewing the driver, I see that > 16 is used as a way of
>> flagging current inputs, but still seems like the wrong way to do it.
>> See suggestion below.
>>
>>>
>>> diff-channels:
>>> + description:
>>> + For using current channels specify only the positive channel.
>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>>
>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>> VIN0. I think it would make more sense to assign unique channel
>> numbers individually to the negative and positive current inputs.
>> Also, I think it makes sense to use the same numbers that the
>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>> positive).
>>
>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>
> Thinking about this a bit more...
>
> Since the current inputs have dedicated pins and aren't mix-and-match
> with multiple valid wiring configurations like the voltage inputs, do
> we even need to describe them in the devicetree?
>
> In the driver, the current channels would just be hard-coded like the
> temperature channel since there isn't any application-specific
> variation.
Sure, but we still need to offer the user a way to configure which
current inputs he wants and if they should use bipolar or unipolar coding.
One other issue that arises is the fact that we will reserve 5
(including temp) channels out of the 15 that the user has the option to
configure. While the binding will configure only 11 channels for
example, the driver probe will error out with the message "Too many
channels specified".
On 01/04/2024 23:22, David Lechner wrote:
> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>>
>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>> <[email protected]> wrote:
..
>
> Also, I just noticed that AD411x have only one AVDD input instead of
> AVDD1 and AVDD2. So we need an if statement that says if properties:
> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
Already addressed by this:
"
# Only ad7172-4, ad7173-8 and ad7175-8 support vref2
- if:
properties:
compatible:
not:
contains:
enum:
- adi,ad7172-4
- adi,ad7173-8
- adi,ad7175-8
then:
properties:
vref2-supply: false
patternProperties:
"^channel@[0-9a-f]$":
properties:
adi,reference-select:
enum:
- vref
- refout-avss
- avdd
"
On 02/04/2024 00:53, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
>>
>> From: Dumitru Ceclan <[email protected]>
>>
>> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>>
>> The AD411X family encompasses a series of low power, low noise, 24-bit,
>> sigma-delta analog-to-digital converters that offer a versatile range of
>> specifications.
>>
>> This family of ADCs integrates an analog front end suitable for processing
>> both fully differential and single-ended, bipolar voltage inputs
>> addressing a wide array of industrial and instrumentation requirements.
>>
>> - All ADCs have inputs with a precision voltage divider with a division
>> ratio of 10.
>> - AD4116 has 5 low level inputs without a voltage divider.
>> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
>> shunt resistor.
>>
>> Signed-off-by: Dumitru Ceclan <[email protected]>
>> ---
>
> ...
>
>> @@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>> struct device *dev = indio_dev->dev.parent;
>> struct iio_chan_spec *chan_arr, *chan;
>> unsigned int ain[2], chan_index = 0;
>> - int ref_sel, ret, num_channels;
>> + int ref_sel, ret, reg, num_channels;
>>
>> num_channels = device_get_child_node_count(dev);
>>
>
> Another thing that is missing in this function both for the chips
> being added here and the existing chips are channels for _all_
> possible inputs. The driver is adding a fixed input channel for the
> temperature sensor, as it should. But all of the chips also have a
> similar input channel configuration that measures the reference
> voltage. Currently, there doesn't seem to be a way to make use of this
> feature. I would expect a hard-coded voltage input channel that is
> always added for this reference voltage similar to the temperature
> channel.
>
AD7173-8:
Channel input configs:
AINPOS0: REF+ 10101: 21
AINNEG0: REF- 10110: 22
For the user to define the REF measurement channel:
diff-channels = <21 22>;
So it is possible from the binding side. It would just need support from
the driver as currently any value above the stated number of inputs is
rejected. Maybe document this in a comment like you suggested below.
I really do not agree with using up channels without letting the user
decide. I can accept to dedicate one for the temp where applicable but
more than that and it feels like we are restricting the usage too much.
> The ad717x chips (except AD7173-8 and AD7176-2) also have a common
> mode voltage input ("((AVDD1 − AVSS)/5)") that could work the same.
>
Again, would be resolved if I added support from the driver.
> In the case of the ad717x chips though, it looks like these channels
> are not "fixed" like they are in ad411x. It looks like these inputs
> can be mixed and matched with AINx inputs and/or each other as
> differential pairs. So if that is actually the case, I would expect
> the DT bindings for ad717x to look like adi,ad4130.yaml where these
> additional input sources are listed in the diff-channels property
> instead of having hard-coded channels in the driver like I have
> suggested above.
>
Yep, agree.
> But, as always, fixes for ad717x should be in a separate patch series.
> Still, I think adding a hard-coded channel for the reference voltage
> input for ad411x chips in this patch makes sense.
As stated above, not comfortable with using up channels with hard-coded
values.
On 01/04/2024 22:45, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
>>
>> From: Dumitru Ceclan <[email protected]>
>>
..
>> #define AD7175_2_ID 0x0cd0
>> #define AD7172_4_ID 0x2050
>> #define AD7173_ID 0x30d0
>> +#define AD4111_ID 0x30d0
>> +#define AD4112_ID 0x30d0
>> +#define AD4114_ID 0x30d0
>
> It might make it a bit more obvious that not all chips have a unique
> ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather
> than introducing multiple macros with the same value.
>
> Or leave it as AD7173_ID to keep it short and add a comment where it
> is used with 411x chips in ad7173_device_info[].
>
Sure
>> +#define AD4116_ID 0x34d0
>> +#define AD4115_ID 0x38d0
>> #define AD7175_8_ID 0x3cd0
>> #define AD7177_ID 0x4fd0
>> #define AD7173_ID_MASK GENMASK(15, 4)
..
>> struct ad7173_device_info {
>> const unsigned int *sinc5_data_rates;
>> unsigned int num_sinc5_data_rates;
>> unsigned int odr_start_value;
>> + unsigned int num_inputs_with_divider;
>> unsigned int num_channels;
>> unsigned int num_configs;
>> unsigned int num_inputs;
>
> Probably a good idea to change num_inputs to num_voltage_inputs so it
> isn't confused with the total number of inputs.
>
> Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider.
>
> Also could use a comment to make it clear if num_voltage_inputs
> includes num_voltage_inputs_with_divider or not. And that it doesn't
> include VINCOM.
>
Alright for these 3 statements above.
> Probably also need some flag here to differentiate ADCINxx voltage
> inputs on AD4116.
>
That is the purpose of num_inputs_with_divider. Mangled some changes
when splitting into individual patches. Will include in V2.
"
if (ain[1] == AD411X_VCOM_INPUT &&
ain[0] >= st->info->num_inputs_with_divider)
return dev_err_probe(dev, -EINVAL,
"VCOM must be paired with inputs having divider.\n");
"
..
>>
>> +static unsigned int ad4111_current_channel_config[] = {
>> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
>> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
>> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
>> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
>> +};
>
> As mentioned in the DT bindings review, it would make more sense to
> just use the datasheet numbers for the current input channels in the
> diff-channels DT property, then we don't need this lookup table.
>
Yet, the datasheet does not specify the numbers, just a single bitfield
for each pair. It is too much of a churn to need to decode that bitfield
into individual values when the user just wants to select a single pair.
..
>> + case IIO_CURRENT:
>> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
>> + *val /= AD4111_SHUNT_RESISTOR_OHM;
>> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
>
> Static analysis tools like to complain about using bool as int.
> Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
>
Maybe it does not apply here, but i followed this advice:
Andy Shevchenko V1 of AD7173 (named initially ad717x)
"
> + return (bool)(value & mask);
This is weird. You have int which you get from bool, wouldn't be better
to use
!!(...) as other GPIO drivers do?
"
>> + case IIO_CURRENT:
>> *val = -BIT(chan->scan_type.realbits - 1);
>
> Expecting a special case here, at least when ADCIN15 is configured for
> pseudo-differential inputs.
>
And what configuration would that be?
The only configurable part is the BI_UNIPOLARx bit in the channel
register, which is addressed here.
There seems to be a confusion similar to what we had with single-ended
channels. The ADC is differential. Pseudo-differential in this datasheet
just means that you wired a fixed voltage(higher than 0) to the negative
analog input.
Which you can also do on the other inputs with a divider.
..
>> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
>> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
>> + chan->type = IIO_CURRENT;
>> + chan->channel = ain[0];
>> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
>> + } else {
>> + chan->channel = ain[0];
>> + chan->channel2 = ain[1];
>> + chan->differential = true;
>
> Expecting chan->differential = false when ADCIN15 is configured for
> pseudo-differential inputs.
>
> Also, perhaps missed in previous reviews, I would expect
> chan->differential = false when channels are used as single-ended.
>
Why?
Also, how would one detect if you are using single-ended channels?
The ADC is still differential. Single ended is represented as connecting
AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate.
In the IIO framework the only difference this makes is in the naming of
the channel:
voltage0-voltage1 vs just voltage0
All channels are differential. Pseudo differential: still differential.
On 02/04/2024 17:00, David Lechner wrote:
> On Mon, Apr 1, 2024 at 2:45 PM David Lechner <[email protected]> wrote:
>>
>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>> <[email protected]> wrote:
..
>>> *chan = ad7173_channel_template;
>>> chan->address = chan_index;
>>> chan->scan_index = chan_index;
>>> - chan->channel = ain[0];
>>> - chan->channel2 = ain[1];
>>> - chan->differential = true;
>>>
>>> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
>>> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
>>> + chan->type = IIO_CURRENT;
>>> + chan->channel = ain[0];
>>> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
>>> + } else {
>>> + chan->channel = ain[0];
>>> + chan->channel2 = ain[1];
>>> + chan->differential = true;
>>
>> Expecting chan->differential = false when ADCIN15 is configured for
>> pseudo-differential inputs.
>>
>> Also, perhaps missed in previous reviews, I would expect
>> chan->differential = false when channels are used as single-ended.
>>
>
> After sleeping on it, I came to the concision that these parts are
> probably too complex to try to worry about differential vs.
> pseudo-differential/single-ended (what the datasheet calls
> single-ended is really pseudo-differential).
>
> So I take back my comments about expecting differential = false in those cases.
Alrighty then
On 01/04/2024 22:40, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
>>
>> From: Dumitru Ceclan <[email protected]>
>>
>> Move validation of analog inputs and reference voltage selection to
>> separate functions.
>>
>> Signed-off-by: Dumitru Ceclan <[email protected]>
>> ---
>
> Same as my comment on PATCH 3/6. We would like to know why this change
> is being made.
Move validation of analog inputs and reference voltage selection to
separate functions to reduce the size of the channel config parsing function.
Good?
On 03/04/2024 10:45, Ceclan, Dumitru wrote:
> On 01/04/2024 23:22, David Lechner wrote:
>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>>>
>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>> <[email protected]> wrote:
>
> ...
>
>>
>> Also, I just noticed that AD411x have only one AVDD input instead of
>> AVDD1 and AVDD2. So we need an if statement that says if properties:
>> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
>
> Already addressed by this:
> "
> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> - if:
> properties:
> compatible:
> not:
> contains:
> enum:
> - adi,ad7172-4
> - adi,ad7173-8
> - adi,ad7175-8
> then:
> properties:
> vref2-supply: false
> patternProperties:
> "^channel@[0-9a-f]$":
> properties:
> adi,reference-select:
> enum:
> - vref
> - refout-avss
> - avdd
> "
Mistaken vref2-supply to avdd2-supply.
But still, the presence of avdd2-supply does not influence anything at all.
Driver does not use it, you cannot select it for channel conversions.
Would a restriction like this really be required?
On 01/04/2024 22:39, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <[email protected]> wrote:
>>
>> From: Dumitru Ceclan <[email protected]>
>>
>> Move configurations regarding number of channels from
>> *_fw_parse_device_config to *_fw_parse_channel_config.
>>
>> Signed-off-by: Dumitru Ceclan <[email protected]>
>> ---
>
> Commit messages need to explain _why_ the change is being made [1]. It
> is not obvious to me why this needs to be moved.
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
Jonathan Cameron:
"
> + if (num_channels == 0)
> + return dev_err_probe(dev, -ENODATA, "No channels specified\n");
> + indio_dev->num_channels = num_channels;
> + st->num_channels = num_channels;
I'm not seeing benefit of duplication here really and logically it feels like
a lot of this last chunk would sit better in ad7173_fw_parse_channel_config()
Perhaps that's a job for a future tidying up patch.
"
https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
On Wed, Apr 3, 2024 at 5:08 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
> On 03/04/2024 10:45, Ceclan, Dumitru wrote:
> > On 01/04/2024 23:22, David Lechner wrote:
> >> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
> >>>
> >>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>> <[email protected]> wrote:
> >
> > ...
> >
> >>
> >> Also, I just noticed that AD411x have only one AVDD input instead of
> >> AVDD1 and AVDD2. So we need an if statement that says if properties:
> >> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
> >
> > Already addressed by this:
> > "
> > # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> > - if:
> > properties:
> > compatible:
> > not:
> > contains:
> > enum:
> > - adi,ad7172-4
> > - adi,ad7173-8
> > - adi,ad7175-8
> > then:
> > properties:
> > vref2-supply: false
> > patternProperties:
> > "^channel@[0-9a-f]$":
> > properties:
> > adi,reference-select:
> > enum:
> > - vref
> > - refout-avss
> > - avdd
> > "
>
> Mistaken vref2-supply to avdd2-supply.
>
> But still, the presence of avdd2-supply does not influence anything at all.
> Driver does not use it, you cannot select it for channel conversions.
> Would a restriction like this really be required?
It is true that it is not likely to cause any problems we don't fix
this but why would we want the bindings to intentionally be incorrect
when there is an easy fix? Driver implementations should not influence
leaving something out of the bindings [1].
[1]: https://www.kernel.org/doc/html//v5.10/devicetree/bindings/writing-bindings.html#overall-design
On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
> On 01/04/2024 22:37, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <[email protected]> wrote:
> >>
> >> From: Dumitru Ceclan <[email protected]>
>
> ...
>
> >> properties:
> >> reg:
> >> + description:
> >> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> >> minimum: 0
> >> - maximum: 15
> >> + maximum: 19
> >
> > This looks wrong. Isn't reg describing the number of logical channels
> > (# of channel config registers)?
> >
> > After reviewing the driver, I see that > 16 is used as a way of
> > flagging current inputs, but still seems like the wrong way to do it.
> > See suggestion below.
> >
>
> This was a suggestion from Jonathan, maybe I implemented it wrong.
> Other alternative that came to my mind: attribute "adi,current-channel".
Having a boolean flag like this would make more sense to me if we
don't agree that the suggestion below is simpler.
> >>
> >> diff-channels:
> >> + description:
> >> + For using current channels specify only the positive channel.
> >> + (IIN2+, IIN2−) -> diff-channels = <2 0>
> >
> > I find this a bit confusing since 2 is already VIN2 and 0 is already
> > VIN0. I think it would make more sense to assign unique channel
> > numbers individually to the negative and positive current inputs.
> > Also, I think it makes sense to use the same numbers that the
> > registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> > positive).
> >
> > So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> >
> >
> It would mean for the user to look in the datasheet at the possible
> channel INPUT configurations values, decode the bit field into two
> integer values and place it here (0110101010) -> 13 10. This is
> cumbersome for just choosing current input 2.
It could be documented in the devicetree bindings, just as it is done
in adi,ad4130.yaml so that users of the bindings don't have to
decipher the datasheet.
>
> >> +
> >> + Family AD411x supports a dedicated VCOM voltage input.
> >> + To select it set the second channel to 16.
> >> + (VIN2, VCOM) -> diff-channels = <2 16>
> >
> > The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> > bit confusing.
> >
>
> Sure, I'll rename to VINCOM.
>
> > Also, do we need to add a vincom-supply to get this voltage? Or is it
> > safe to assume it is always connected to AVSS? The datasheet seems to
> > indicate that the latter is the case. But then it also has this
> > special case (at least for AD4116, didn't check all datasheets)
> > "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> > as part of a fully differential input, we probably need some extra
> > flag to indicate that case.
> >
>
> I cannot see any configuration options for these use cases. All inputs
> are routed to the same mux and routed to the differential positive and
> negative ADC inputs.
>
> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> between these two use cases is if you connected VINCOM to AVSS (with
> unipolar coding) or not with bipolar encoding. The channel is still
> measuring the difference between the two selected inputs and comparing
> to the selected reference.
>
> > Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> > "(pseudo differential or differential pair)" notation that other
> > inputs don't. In other words, it is more like VINCOM than it is to the
> > other ADCINxx pins. So we probably need an adcin15-supply for this pin
> > to properly get the right channel configuration. I.e. the logic in the
> > IIO driver would be if adcin15-supply is present, any channels that
> > use this input are pseudo-differential, otherwise any channels that
> > use it are fully differential.
> >
>
> I cannot seem to understand what would a adcin15-supply be needed for.
> This input, the same as all others, enters the mux and is routed to
> either positive or negative input of the ADC.
>
> The voltage on the ADCIN15 pin is not important to the user, just the
> difference in voltage between that pin and the other one selected.
>
These suggestions come from some recent discussion about
pseudo-differential vs. fully differential inputs (e.g. search the IIO
mailing list for AD7380).
So what I suggested here might be more technically correct according
to what I got out of that discussion. But for this specific case, I
agree it is good enough to just treat all inputs as always
fully-differential to keep things from getting too unwieldy.
> >> items:
> >> minimum: 0
> >> maximum: 31
> >> @@ -166,7 +191,6 @@ allOf:
> >> - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >>
> >> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> >> - # Other models have [0-3] channel registers
> >
> > Did you forget to remove
> >
> > reg:
> > maximum: 3
> >
> > from this if statement that this comment is referring to?
> >
> >
>
>
> Other way around, forgot in a previous patch to remove the comment.
> I'll move this change to a precursor patch.
>
> >> - if:
> >> properties:
> >> compatible:
> >> @@ -187,6 +211,37 @@ allOf:
> >> - vref
> >> - refout-avss
> >> - avdd
> >> +
> >> + - if:
> >> + properties:
> >> + compatible:
> >> + contains:
> >> + enum:
> >> + - adi,ad4114
> >> + - adi,ad4115
> >> + - adi,ad4116
> >> + - adi,ad7173-8
> >> + - adi,ad7175-8
> >> + then:
> >> + patternProperties:
> >> + "^channel@[0-9a-f]$":
> >> + properties:
> >> + reg:
> >> + maximum: 15
> >
> > As with the previous reg comment, this if statement should not be
> > needed since maximum should not be changed to 19.
> >
>
> We'll see what is the best approach regarding the current channels,
> perhaps the one you mentioned in the later reply with always configuring
> like the temp channel.
>
> >> +
> >> + - if:
> >> + properties:
> >> + compatible:
> >> + contains:
> >> + enum:
> >> + - adi,ad7172-2
> >> + - adi,ad7175-2
> >> + - adi,ad7176-2
> >> + - adi,ad7177-2
> >> + then:
> >> + patternProperties:
> >> + "^channel@[0-9a-f]$":
> >> + properties:
> >> reg:
> >> maximum: 3
> >
> > It looks to me like AD7172-4 actually has 8 possible channels rather
> > than 16. So it would need a special condition as well. But that is a
> > bug in the previous bindings and should therefore be fixed in a
> > separate patch.
>
> It is addressed already in the binding:
> "
> - if:
> properties:
> compatible:
> contains:
> const: adi,ad7172-4
> [...]
> maximum: 7
> "
Ah, I missed it hiding with adi,reference-select overrides.
On Wed, Apr 3, 2024 at 5:01 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
> On 01/04/2024 22:39, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <[email protected]> wrote:
> >>
> >> From: Dumitru Ceclan <[email protected]>
> >>
> >> Move configurations regarding number of channels from
> >> *_fw_parse_device_config to *_fw_parse_channel_config.
> >>
> >> Signed-off-by: Dumitru Ceclan <[email protected]>
> >> ---
> >
> > Commit messages need to explain _why_ the change is being made [1]. It
> > is not obvious to me why this needs to be moved.
> >
> > [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
>
>
> Jonathan Cameron:
>
> "
> > + if (num_channels == 0)
> > + return dev_err_probe(dev, -ENODATA, "No channels specified\n");
> > + indio_dev->num_channels = num_channels;
> > + st->num_channels = num_channels;
>
> I'm not seeing benefit of duplication here really and logically it feels like
> a lot of this last chunk would sit better in ad7173_fw_parse_channel_config()
>
> Perhaps that's a job for a future tidying up patch.
> "
> https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
>
Thanks.
A Link: and Suggested-by: in the commit message with this info would
be a reasonable way to communicate this.
I looks like this is also adding an additional check " if
(num_channels > st->info->num_channels)" in addition to moving
existing code. It would be helpful to have the reason for this in the
commit message as well.
With the suggested additions to the commit message...
Reviewed-by: David Lechner <[email protected]>
On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
> On 02/04/2024 00:16, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibrecom> wrote:
> >>
> >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >> <[email protected]> wrote:
> >>>
> >>> From: Dumitru Ceclan <[email protected]>
> >>>
>
> ...
>
> >>>
> >>> properties:
> >>> reg:
> >>> + description:
> >>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> >>> minimum: 0
> >>> - maximum: 15
> >>> + maximum: 19
> >>
> >> This looks wrong. Isn't reg describing the number of logical channels
> >> (# of channel config registers)?
> >>
> >> After reviewing the driver, I see that > 16 is used as a way of
> >> flagging current inputs, but still seems like the wrong way to do it.
> >> See suggestion below.
> >>
> >>>
> >>> diff-channels:
> >>> + description:
> >>> + For using current channels specify only the positive channel.
> >>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
> >>
> >> I find this a bit confusing since 2 is already VIN2 and 0 is already
> >> VIN0. I think it would make more sense to assign unique channel
> >> numbers individually to the negative and positive current inputs.
> >> Also, I think it makes sense to use the same numbers that the
> >> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> >> positive).
> >>
> >> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> >
> > Thinking about this a bit more...
> >
> > Since the current inputs have dedicated pins and aren't mix-and-match
> > with multiple valid wiring configurations like the voltage inputs, do
> > we even need to describe them in the devicetree?
> >
> > In the driver, the current channels would just be hard-coded like the
> > temperature channel since there isn't any application-specific
> > variation.
>
> Sure, but we still need to offer the user a way to configure which
> current inputs he wants and if they should use bipolar or unipolar coding.
From the datasheet, it looks like only positive current input is
allowed so I'm not sure bipolar applies here. But, yes, if there is
some other variation in wiring or electrical signal that needs to be
describe here, then it makes sense to allow a channel configuration
node for it.
>
> One other issue that arises is the fact that we will reserve 5
> (including temp) channels out of the 15 that the user has the option to
> configure. While the binding will configure only 11 channels for
> example, the driver probe will error out with the message "Too many
> channels specified".
>
Surely the driver could be changed to fix this, if needed. :-)
On Wed, Apr 3, 2024 at 5:03 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
> On 01/04/2024 22:40, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <[email protected]> wrote:
> >>
> >> From: Dumitru Ceclan <[email protected]>
> >>
> >> Move validation of analog inputs and reference voltage selection to
> >> separate functions.
> >>
> >> Signed-off-by: Dumitru Ceclan <[email protected]>
> >> ---
> >
> > Same as my comment on PATCH 3/6. We would like to know why this change
> > is being made.
>
> Move validation of analog inputs and reference voltage selection to
> separate functions to reduce the size of the channel config parsing function.
>
> Good?
Better. But it still only says what is being done and doesn't answer
the question "why?".
"to reduce the size of the function to make it easier to read"
explains why reducing the size of the function makes it an
improvement.
On Wed, Apr 3, 2024 at 4:53 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote:
>
>
> On 01/04/2024 22:45, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <[email protected]> wrote:
> >>
> >> From: Dumitru Ceclan <[email protected]>
> >>
>
> ...
>
> >> #define AD7175_2_ID 0x0cd0
> >> #define AD7172_4_ID 0x2050
> >> #define AD7173_ID 0x30d0
> >> +#define AD4111_ID 0x30d0
> >> +#define AD4112_ID 0x30d0
> >> +#define AD4114_ID 0x30d0
> >
> > It might make it a bit more obvious that not all chips have a unique
> > ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather
> > than introducing multiple macros with the same value.
> >
> > Or leave it as AD7173_ID to keep it short and add a comment where it
> > is used with 411x chips in ad7173_device_info[].
> >
>
> Sure
>
> >> +#define AD4116_ID 0x34d0
> >> +#define AD4115_ID 0x38d0
> >> #define AD7175_8_ID 0x3cd0
> >> #define AD7177_ID 0x4fd0
> >> #define AD7173_ID_MASK GENMASK(15, 4)
>
> ...
>
> >> struct ad7173_device_info {
> >> const unsigned int *sinc5_data_rates;
> >> unsigned int num_sinc5_data_rates;
> >> unsigned int odr_start_value;
> >> + unsigned int num_inputs_with_divider;
> >> unsigned int num_channels;
> >> unsigned int num_configs;
> >> unsigned int num_inputs;
> >
> > Probably a good idea to change num_inputs to num_voltage_inputs so it
> > isn't confused with the total number of inputs.
> >
> > Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider.
> >
> > Also could use a comment to make it clear if num_voltage_inputs
> > includes num_voltage_inputs_with_divider or not. And that it doesn't
> > include VINCOM.
> >
>
> Alright for these 3 statements above.
>
> > Probably also need some flag here to differentiate ADCINxx voltage
> > inputs on AD4116.
> >
>
> That is the purpose of num_inputs_with_divider. Mangled some changes
> when splitting into individual patches. Will include in V2.
> "
> if (ain[1] == AD411X_VCOM_INPUT &&
> ain[0] >= st->info->num_inputs_with_divider)
> return dev_err_probe(dev, -EINVAL,
> "VCOM must be paired with inputs having divider.\n");
> "
>
> ...
>
> >>
> >> +static unsigned int ad4111_current_channel_config[] = {
> >> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
> >> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
> >> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
> >> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
> >> +};
> >
> > As mentioned in the DT bindings review, it would make more sense to
> > just use the datasheet numbers for the current input channels in the
> > diff-channels DT property, then we don't need this lookup table.
> >
> Yet, the datasheet does not specify the numbers, just a single bitfield
> for each pair. It is too much of a churn to need to decode that bitfield
> into individual values when the user just wants to select a single pair.
>
> ...
>
> >> + case IIO_CURRENT:
> >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> >> + *val /= AD4111_SHUNT_RESISTOR_OHM;
> >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> >
> > Static analysis tools like to complain about using bool as int.
> > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
> >
> Maybe it does not apply here, but i followed this advice:
>
> Andy Shevchenko V1 of AD7173 (named initially ad717x)
> "
> > + return (bool)(value & mask);
>
> This is weird. You have int which you get from bool, wouldn't be better
> to use
> !!(...) as other GPIO drivers do?
As long as the build bots don't complain, there isn't a reason to
change it. It is just a matter of personal preference at that point.
I got a sparse warning for something like this recently [1], but maybe
that case was just because it was inside of a FIELD_PREP() using it as
bit logic instead of addition and we won't get any warnings here.
[1]: https://lore.kernel.org/linux-iio/[email protected]/
>
> "
>
>
> >> + case IIO_CURRENT:
> >> *val = -BIT(chan->scan_type.realbits - 1);
> >
> > Expecting a special case here, at least when ADCIN15 is configured for
> > pseudo-differential inputs.
> >
>
> And what configuration would that be?
> The only configurable part is the BI_UNIPOLARx bit in the channel
> register, which is addressed here.
>
> There seems to be a confusion similar to what we had with single-ended
> channels. The ADC is differential. Pseudo-differential in this datasheet
> just means that you wired a fixed voltage(higher than 0) to the negative
> analog input.
>
> Which you can also do on the other inputs with a divider.
>
As discussed elsewhere, you can disregard this suggestion.
> ...
>
> >> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> >> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> >> + chan->type = IIO_CURRENT;
> >> + chan->channel = ain[0];
> >> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> >> + } else {
> >> + chan->channel = ain[0];
> >> + chan->channel2 = ain[1];
> >> + chan->differential = true;
> >
> > Expecting chan->differential = false when ADCIN15 is configured for
> > pseudo-differential inputs.
> >
> > Also, perhaps missed in previous reviews, I would expect
> > chan->differential = false when channels are used as single-ended.
> >
> Why?
> Also, how would one detect if you are using single-ended channels?
>
> The ADC is still differential. Single ended is represented as connecting
> AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate.
>
> In the IIO framework the only difference this makes is in the naming of
> the channel:
> voltage0-voltage1 vs just voltage0
>
> All channels are differential. Pseudo differential: still differential.
In the discussions on the AD7380 patch series, we came to the
conclusion that pseduo-differential is technically not differential in
the context of the .differential flag in IIO.
But as mentioned in my follow up, for this driver it is going to make
things far simpler if we just ignore that and treat
pseudo-differential the same as fully differential.
On 03/04/2024 18:22, David Lechner wrote:
> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <[email protected]> wrote:
>> On 02/04/2024 00:16, David Lechner wrote:
>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>> <[email protected]> wrote:
>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>
>> ...
>>
>>>>> properties:
>>>>> reg:
>>>>> + description:
>>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>>>> minimum: 0
>>>>> - maximum: 15
>>>>> + maximum: 19
>>>> This looks wrong. Isn't reg describing the number of logical channels
>>>> (# of channel config registers)?
>>>>
>>>> After reviewing the driver, I see that > 16 is used as a way of
>>>> flagging current inputs, but still seems like the wrong way to do it.
>>>> See suggestion below.
>>>>
>>>>> diff-channels:
>>>>> + description:
>>>>> + For using current channels specify only the positive channel.
>>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>>>> VIN0. I think it would make more sense to assign unique channel
>>>> numbers individually to the negative and positive current inputs.
>>>> Also, I think it makes sense to use the same numbers that the
>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>>>> positive).
>>>>
>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>>> Thinking about this a bit more...
>>>
>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>> with multiple valid wiring configurations like the voltage inputs, do
>>> we even need to describe them in the devicetree?
>>>
>>> In the driver, the current channels would just be hard-coded like the
>>> temperature channel since there isn't any application-specific
>>> variation.
>> Sure, but we still need to offer the user a way to configure which
>> current inputs he wants and if they should use bipolar or unipolar coding.
> From the datasheet, it looks like only positive current input is
> allowed so I'm not sure bipolar applies here. But, yes, if there is
> some other variation in wiring or electrical signal that needs to be
> describe here, then it makes sense to allow a channel configuration
> node for it.
AD4111 datasheet pg.29:
When the ADC is configured for bipolar operation, the output
code is offset binary with a negative full-scale voltage resulting
in a code of 000 … 000, a zero differential input voltage resulting in
a code of 100 … 000, and a positive full-scale input voltage
resulting in a code of 111 … 111. The output code for any
analog input voltage can be represented as
Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
The output code for any input current is represented as
Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
I would say bipolar applies here, not a great idea because of the limitation on
the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
is available.
On Thu, 4 Apr 2024 16:08:56 +0300
"Ceclan, Dumitru" <[email protected]> wrote:
> On 03/04/2024 18:22, David Lechner wrote:
> > On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <[email protected]> wrote:
> >> On 02/04/2024 00:16, David Lechner wrote:
> >>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
> >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>> <[email protected]> wrote:
> >>>>> From: Dumitru Ceclan <[email protected]>
> >>>>>
> >> ...
> >>
> >>>>> properties:
> >>>>> reg:
> >>>>> + description:
> >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> >>>>> minimum: 0
> >>>>> - maximum: 15
> >>>>> + maximum: 19
> >>>> This looks wrong. Isn't reg describing the number of logical channels
> >>>> (# of channel config registers)?
> >>>>
> >>>> After reviewing the driver, I see that > 16 is used as a way of
> >>>> flagging current inputs, but still seems like the wrong way to do it.
> >>>> See suggestion below.
> >>>>
> >>>>> diff-channels:
> >>>>> + description:
> >>>>> + For using current channels specify only the positive channel.
> >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
> >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
> >>>> VIN0. I think it would make more sense to assign unique channel
> >>>> numbers individually to the negative and positive current inputs.
> >>>> Also, I think it makes sense to use the same numbers that the
> >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> >>>> positive).
> >>>>
> >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> >>> Thinking about this a bit more...
> >>>
> >>> Since the current inputs have dedicated pins and aren't mix-and-match
> >>> with multiple valid wiring configurations like the voltage inputs, do
> >>> we even need to describe them in the devicetree?
> >>>
> >>> In the driver, the current channels would just be hard-coded like the
> >>> temperature channel since there isn't any application-specific
> >>> variation.
> >> Sure, but we still need to offer the user a way to configure which
> >> current inputs he wants and if they should use bipolar or unipolar coding.
> > From the datasheet, it looks like only positive current input is
> > allowed so I'm not sure bipolar applies here. But, yes, if there is
> > some other variation in wiring or electrical signal that needs to be
> > describe here, then it makes sense to allow a channel configuration
> > node for it.
>
> AD4111 datasheet pg.29:
> When the ADC is configured for bipolar operation, the output
> code is offset binary with a negative full-scale voltage resulting
> in a code of 000 … 000, a zero differential input voltage resulting in
> a code of 100 … 000, and a positive full-scale input voltage
> resulting in a code of 111 … 111. The output code for any
> analog input voltage can be represented as
> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
> The output code for any input current is represented as
> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
>
> I would say bipolar applies here, not a great idea because of the limitation on
> the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
> is available.
Just to check I am correct in thinking you 'might' use bipolar if you want
to be able to measure small negative currents, but the range is much larger
in the positive direction?
J
>
On Wed, 3 Apr 2024 10:40:39 -0500
David Lechner <[email protected]> wrote:
> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
> >
> > On 01/04/2024 22:37, David Lechner wrote:
> > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > > <[email protected]> wrote:
> > >>
> > >> From: Dumitru Ceclan <[email protected]>
> >
> > ...
> >
> > >> properties:
> > >> reg:
> > >> + description:
> > >> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> > >> minimum: 0
> > >> - maximum: 15
> > >> + maximum: 19
> > >
> > > This looks wrong. Isn't reg describing the number of logical channels
> > > (# of channel config registers)?
> > >
> > > After reviewing the driver, I see that > 16 is used as a way of
> > > flagging current inputs, but still seems like the wrong way to do it.
> > > See suggestion below.
> > >
> >
> > This was a suggestion from Jonathan, maybe I implemented it wrong.
Maybe Jonathan was wrong! I was younger then than now :)
However, reg values for child nodes are unique so can't just use a flag these
need to be different values.
> > Other alternative that came to my mind: attribute "adi,current-channel"
>
> Having a boolean flag like this would make more sense to me if we
> don't agree that the suggestion below is simpler.
>
> > >>
> > >> diff-channels:
> > >> + description:
> > >> + For using current channels specify only the positive channel.
> > >> + (IIN2+, IIN2−) -> diff-channels = <2 0>
> > >
> > > I find this a bit confusing since 2 is already VIN2 and 0 is already
> > > VIN0. I think it would make more sense to assign unique channel
> > > numbers individually to the negative and positive current inputs.
> > > Also, I think it makes sense to use the same numbers that the
> > > registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> > > positive).
> > >
> > > So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> > >
> > >
> > It would mean for the user to look in the datasheet at the possible
> > channel INPUT configurations values, decode the bit field into two
> > integer values and place it here (0110101010) -> 13 10. This is
> > cumbersome for just choosing current input 2.
>
> It could be documented in the devicetree bindings, just as it is done
> in adi,ad4130.yaml so that users of the bindings don't have to
> decipher the datasheet.
The <13 10> option makes sense to me and avoids suggesting a common negative
input.
The 'fun' bit here is that diff-channels doesn't actually tell us anything.
So we could just not provide it and rely on documentation of reg = 16-19 meaning
the current channels?
>
> >
> > >> +
> > >> + Family AD411x supports a dedicated VCOM voltage input.
> > >> + To select it set the second channel to 16.
> > >> + (VIN2, VCOM) -> diff-channels = <2 16>
> > >
> > > The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> > > bit confusing.
> > >
> >
> > Sure, I'll rename to VINCOM.
> >
> > > Also, do we need to add a vincom-supply to get this voltage? Or is it
> > > safe to assume it is always connected to AVSS? The datasheet seems to
> > > indicate that the latter is the case. But then it also has this
> > > special case (at least for AD4116, didn't check all datasheets)
> > > "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> > > as part of a fully differential input, we probably need some extra
> > > flag to indicate that case.
> > >
> >
> > I cannot see any configuration options for these use cases. All inputs
> > are routed to the same mux and routed to the differential positive and
> > negative ADC inputs.
> >
> > "VIN10, VINCOM (single-ended or differential pair)" the only difference
> > between these two use cases is if you connected VINCOM to AVSS (with
> > unipolar coding) or not with bipolar encoding. The channel is still
> > measuring the difference between the two selected inputs and comparing
> > to the selected reference.
> >
> > > Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> > > "(pseudo differential or differential pair)" notation that other
> > > inputs don't. In other words, it is more like VINCOM than it is to the
> > > other ADCINxx pins. So we probably need an adcin15-supply for this pin
> > > to properly get the right channel configuration. I.e. the logic in the
> > > IIO driver would be if adcin15-supply is present, any channels that
> > > use this input are pseudo-differential, otherwise any channels that
> > > use it are fully differential.
> > >
> >
> > I cannot seem to understand what would a adcin15-supply be needed for.
> > This input, the same as all others, enters the mux and is routed to
> > either positive or negative input of the ADC.
> >
> > The voltage on the ADCIN15 pin is not important to the user, just the
> > difference in voltage between that pin and the other one selected.
> >
>
> These suggestions come from some recent discussion about
> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> mailing list for AD7380).
>
> So what I suggested here might be more technically correct according
> to what I got out of that discussion. But for this specific case, I
> agree it is good enough to just treat all inputs as always
> fully-differential to keep things from getting too unwieldy.
Hmm. That whole approach to pseudo differential does get messy if
we have the common line routed through the main MUX rather than an opt
in only on the negative side.
If I read this right, its almost a trick to support a pseudo differential
wiring with simple registers (I guess reflecting MUX limitations).
So what could we do?
We could assume that VINCOM is used like a conventional pseudo
differential negative signal and have supply-vincom + non diffferential
channels if that's the configuration wanted.
Then for differential channels can support all the VINX VINX+1
and swapped options.
For VIN10 it gets fun as non differential and differential options
I think map to same actual config. Don't see reason we need to express
that in the binding though so let that have VIN10 VINCOM (probably using
a magic channel number) and VIN10 pseudo differential.
Similar setup for ADCIN15 equivalent usage
Code wise this probably won't be particular hard to support in the driver
(obviously I haven't tried though :) is it worth the effort to keep
it inline with other devices that support pseudo differential channesl.
>
> > >> items:
> > >> minimum: 0
> > >> maximum: 31
> > >> @@ -166,7 +191,6 @@ allOf:
> > >> - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > >>
> > >> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> > >> - # Other models have [0-3] channel registers
> > >
> > > Did you forget to remove
> > >
> > > reg:
> > > maximum: 3
> > >
> > > from this if statement that this comment is referring to?
> > >
> > >
> >
> >
> > Other way around, forgot in a previous patch to remove the comment.
> > I'll move this change to a precursor patch.
> >
> > >> - if:
> > >> properties:
> > >> compatible:
> > >> @@ -187,6 +211,37 @@ allOf:
> > >> - vref
> > >> - refout-avss
> > >> - avdd
> > >> +
> > >> + - if:
> > >> + properties:
> > >> + compatible:
> > >> + contains:
> > >> + enum:
> > >> + - adi,ad4114
> > >> + - adi,ad4115
> > >> + - adi,ad4116
> > >> + - adi,ad7173-8
> > >> + - adi,ad7175-8
> > >> + then:
> > >> + patternProperties:
> > >> + "^channel@[0-9a-f]$":
> > >> + properties:
> > >> + reg:
> > >> + maximum: 15
> > >
> > > As with the previous reg comment, this if statement should not be
> > > needed since maximum should not be changed to 19.
> > >
> >
> > We'll see what is the best approach regarding the current channels,
> > perhaps the one you mentioned in the later reply with always configuring
> > like the temp channel.
> >
That's an option as well. In many early drivers we just provided all the
channels. Somewhat depends on whether people buy devices with lots of
channels they don't wire. Mostly I suspect they don't as that's money
wasted!
Jonathan
On Mon, 01 Apr 2024 18:32:20 +0300
Dumitru Ceclan via B4 Relay <[email protected]> wrote:
> From: Dumitru Ceclan <[email protected]>
>
> AD7176-2 does not feature input buffers, enable buffers only on
> supported models.
>
> Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
> Signed-off-by: Dumitru Ceclan <[email protected]>
How bad is this? If you can find out if writing those bits does anything
harmful (they are reserved and datasheet says should be written 0 I think)
That will help people decide whether to backport the fix?
> ---
> drivers/iio/adc/ad7173.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index f6d29abe1d04..8a95b1391826 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -145,6 +145,7 @@ struct ad7173_device_info {
> unsigned int id;
> char *name;
> bool has_temp;
> + bool has_input_buf;
> bool has_int_ref;
> bool has_ref2;
> u8 num_gpios;
> @@ -212,6 +213,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 4,
> .num_gpios = 2,
> .has_temp = true,
> + .has_input_buf = true,
> .has_int_ref = true,
> .clock = 2 * HZ_PER_MHZ,
> .sinc5_data_rates = ad7173_sinc5_data_rates,
> @@ -224,6 +226,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 8,
> .num_gpios = 4,
> .has_temp = false,
> + .has_input_buf = true,
> .has_ref2 = true,
> .clock = 2 * HZ_PER_MHZ,
> .sinc5_data_rates = ad7173_sinc5_data_rates,
> @@ -237,6 +240,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 8,
> .num_gpios = 4,
> .has_temp = true,
> + .has_input_buf = true,
> .has_int_ref = true,
> .has_ref2 = true,
> .clock = 2 * HZ_PER_MHZ,
> @@ -251,6 +255,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 4,
> .num_gpios = 2,
> .has_temp = true,
> + .has_input_buf = true,
> .has_int_ref = true,
> .clock = 16 * HZ_PER_MHZ,
> .sinc5_data_rates = ad7175_sinc5_data_rates,
> @@ -263,6 +268,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 8,
> .num_gpios = 4,
> .has_temp = true,
> + .has_input_buf = true,
> .has_int_ref = true,
> .has_ref2 = true,
> .clock = 16 * HZ_PER_MHZ,
> @@ -289,6 +295,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
> .num_configs = 4,
> .num_gpios = 2,
> .has_temp = true,
> + .has_input_buf = true,
> .has_int_ref = true,
> .clock = 16 * HZ_PER_MHZ,
> .odr_start_value = AD7177_ODR_START_VALUE,
> @@ -932,7 +939,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> AD7173_CH_ADDRESS(chan_arr[chan_index].channel,
> chan_arr[chan_index].channel2);
> chan_st_priv->cfg.bipolar = false;
> - chan_st_priv->cfg.input_buf = true;
> + chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>
> @@ -989,7 +996,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>
> chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> chan_st_priv->chan_reg = chan_index;
> - chan_st_priv->cfg.input_buf = true;
> + chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.odr = 0;
>
> chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
>
On Mon, 01 Apr 2024 18:32:22 +0300
Dumitru Ceclan via B4 Relay <[email protected]> wrote:
> From: Dumitru Ceclan <[email protected]>
>
> Move validation of analog inputs and reference voltage selection to
> separate functions.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
A few line wrapping comments inline.
> ---
> drivers/iio/adc/ad7173.c | 59 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 699bc6970790..bf5a5b384fe2 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -910,6 +910,41 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
> &st->int_clk_hw);
> }
>
> +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> + unsigned int ain[2])
> +{
> + struct device *dev = &st->sd.spi->dev;
> +
> + if (ain[0] >= st->info->num_inputs ||
> + ain[1] >= st->info->num_inputs)
No need to line wrap the above - its under 80 chars on one line with the
new reduced indent due to factoring this out.
> + return dev_err_probe(dev, -EINVAL,
> + "Input pin number out of range for pair (%d %d).\n",
> + ain[0], ain[1]);
> +
> + return 0;
> +}
> +
> +static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
> +{
> + struct device *dev = &st->sd.spi->dev;
> + int ret;
> +
> + if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
> + return dev_err_probe(dev, -EINVAL,
> + "Internal reference is not available on current model.\n");
> +
> + if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> + return dev_err_probe(dev, -EINVAL,
> + "External reference 2 is not available on current model.\n");
> +
> + ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Cannot use reference %u\n", ref_sel);
Can pull the string to previous line and then align ref_sel just after (
whilst still remaining under 80 chars and end up a little prettier.
> +
> + return 0;
> +}
> +
> static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> {
> struct ad7173_channel *chans_st_arr, *chan_st_priv;
> @@ -970,11 +1005,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> if (ret)
> return ret;
>
> - if (ain[0] >= st->info->num_inputs ||
> - ain[1] >= st->info->num_inputs)
> - return dev_err_probe(dev, -EINVAL,
> - "Input pin number out of range for pair (%d %d).\n",
> - ain[0], ain[1]);
> + ret = ad7173_validate_voltage_ain_inputs(st, ain);
> + if (ret)
> + return ret;
>
> ret = fwnode_property_match_property_string(child,
> "adi,reference-select",
> @@ -985,19 +1018,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> else
> ref_sel = ret;
>
> - if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
> - !st->info->has_int_ref)
> - return dev_err_probe(dev, -EINVAL,
> - "Internal reference is not available on current model.\n");
> -
> - if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> - return dev_err_probe(dev, -EINVAL,
> - "External reference 2 is not available on current model.\n");
> -
> - ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> - if (ret < 0)
> - return dev_err_probe(dev, ret,
> - "Cannot use reference %u\n", ref_sel);
> + ret = ad7173_validate_reference(st, ref_sel);
> + if (ret)
> + return ret;
>
> if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
> st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>
> >
> > >> + case IIO_CURRENT:
> > >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> > >> + *val /= AD4111_SHUNT_RESISTOR_OHM;
> > >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> > >
> > > Static analysis tools like to complain about using bool as int.
> > > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
> > >
> > Maybe it does not apply here, but i followed this advice:
> >
> > Andy Shevchenko V1 of AD7173 (named initially ad717x)
> > "
> > > + return (bool)(value & mask);
> >
> > This is weird. You have int which you get from bool, wouldn't be better
> > to use
> > !!(...) as other GPIO drivers do?
>
> As long as the build bots don't complain, there isn't a reason to
> change it. It is just a matter of personal preference at that point.
>
> I got a sparse warning for something like this recently [1], but maybe
> that case was just because it was inside of a FIELD_PREP() using it as
> bit logic instead of addition and we won't get any warnings here.
>
> [1]: https://lore.kernel.org/linux-iio/[email protected]/
It was common to use !! for a number of years, but then it got a
comment from Linus Torvalds making reasonable point that it isn't
easy to read, so slight preference these days is for a ternary.
On 06/04/2024 17:56, Jonathan Cameron wrote:
> On Mon, 01 Apr 2024 18:32:20 +0300
> Dumitru Ceclan via B4 Relay <[email protected]> wrote:
>
>> From: Dumitru Ceclan <[email protected]>
>>
>> AD7176-2 does not feature input buffers, enable buffers only on
>> supported models.
>>
>> Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
>> Signed-off-by: Dumitru Ceclan <[email protected]>
> How bad is this? If you can find out if writing those bits does anything
> harmful (they are reserved and datasheet says should be written 0 I think)
> That will help people decide whether to backport the fix?
The bits are marked as read-only and there does not seem to be any effect on the ADC.
So drop this one?
On 06/04/2024 17:53, Jonathan Cameron wrote:
> On Wed, 3 Apr 2024 10:40:39 -0500
> David Lechner <[email protected]> wrote:
>
>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
>>>
>>> On 01/04/2024 22:37, David Lechner wrote:
>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>> <[email protected]> wrote:
>>>>>
>>>>> From: Dumitru Ceclan <[email protected]>
>>>
>>> ...
>>>
>>>>> properties:
>>>>> reg:
>>>>> + description:
>>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>>>> minimum: 0
>>>>> - maximum: 15
>>>>> + maximum: 19
>>>>
>>>> This looks wrong. Isn't reg describing the number of logical channels
>>>> (# of channel config registers)?
>>>>
>>>> After reviewing the driver, I see that > 16 is used as a way of
>>>> flagging current inputs, but still seems like the wrong way to do it.
>>>> See suggestion below.
>>>>
>>>
>>> This was a suggestion from Jonathan, maybe I implemented it wrong.
>
> Maybe Jonathan was wrong! I was younger then than now :)
>
> However, reg values for child nodes are unique so can't just use a flag these
> need to be different values.
>
I do not see where the restriction appears when using just the flag, when defining
the channels you would still define unique reg values.
>>> Other alternative that came to my mind: attribute "adi,current-channel".
>>
>> Having a boolean flag like this would make more sense to me if we
>> don't agree that the suggestion below is simpler.
>>
>>>>>
>>>>> diff-channels:
>>>>> + description:
>>>>> + For using current channels specify only the positive channel.
>>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>>>>
>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>>>> VIN0. I think it would make more sense to assign unique channel
>>>> numbers individually to the negative and positive current inputs.
>>>> Also, I think it makes sense to use the same numbers that the
>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>>>> positive).
>>>>
>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>>>>
>>>>
>>> It would mean for the user to look in the datasheet at the possible
>>> channel INPUT configurations values, decode the bit field into two
>>> integer values and place it here (0110101010) -> 13 10. This is
>>> cumbersome for just choosing current input 2.
>>
>> It could be documented in the devicetree bindings, just as it is done
>> in adi,ad4130.yaml so that users of the bindings don't have to
>> decipher the datasheet.
>
> The <13 10> option makes sense to me and avoids suggesting a common negative
> input.
>
> The 'fun' bit here is that diff-channels doesn't actually tell us anything.
> So we could just not provide it and rely on documentation of reg = 16-19 meaning
> the current channels?
>
So a channel without diff-channels defined and reg=16 means IN0+ IN0-?
>>
>>>
>>>>> +
>>>>> + Family AD411x supports a dedicated VCOM voltage input.
>>>>> + To select it set the second channel to 16.
>>>>> + (VIN2, VCOM) -> diff-channels = <2 16>
>>>>
>>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
>>>> bit confusing.
>>>>
>>>
>>> Sure, I'll rename to VINCOM.
>>>
>>>> Also, do we need to add a vincom-supply to get this voltage? Or is it
>>>> safe to assume it is always connected to AVSS? The datasheet seems to
>>>> indicate that the latter is the case. But then it also has this
>>>> special case (at least for AD4116, didn't check all datasheets)
>>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
>>>> as part of a fully differential input, we probably need some extra
>>>> flag to indicate that case.
>>>>
>>>
>>> I cannot see any configuration options for these use cases. All inputs
>>> are routed to the same mux and routed to the differential positive and
>>> negative ADC inputs.
>>>
>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
>>> between these two use cases is if you connected VINCOM to AVSS (with
>>> unipolar coding) or not with bipolar encoding. The channel is still
>>> measuring the difference between the two selected inputs and comparing
>>> to the selected reference.
>>>
>>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
>>>> "(pseudo differential or differential pair)" notation that other
>>>> inputs don't. In other words, it is more like VINCOM than it is to the
>>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin
>>>> to properly get the right channel configuration. I.e. the logic in the
>>>> IIO driver would be if adcin15-supply is present, any channels that
>>>> use this input are pseudo-differential, otherwise any channels that
>>>> use it are fully differential.
>>>>
>>>
>>> I cannot seem to understand what would a adcin15-supply be needed for.
>>> This input, the same as all others, enters the mux and is routed to
>>> either positive or negative input of the ADC.
>>>
>>> The voltage on the ADCIN15 pin is not important to the user, just the
>>> difference in voltage between that pin and the other one selected.
>>>
>>
>> These suggestions come from some recent discussion about
>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
>> mailing list for AD7380).
>>
>> So what I suggested here might be more technically correct according
>> to what I got out of that discussion. But for this specific case, I
>> agree it is good enough to just treat all inputs as always
>> fully-differential to keep things from getting too unwieldy.
>
> Hmm. That whole approach to pseudo differential does get messy if
> we have the common line routed through the main MUX rather than an opt
> in only on the negative side.
>
> If I read this right, its almost a trick to support a pseudo differential
> wiring with simple registers (I guess reflecting MUX limitations).
>
> So what could we do?
>
> We could assume that VINCOM is used like a conventional pseudo
> differential negative signal and have supply-vincom + non diffferential
> channels if that's the configuration wanted.
>
> Then for differential channels can support all the VINX VINX+1
> and swapped options.
> For VIN10 it gets fun as non differential and differential options
> I think map to same actual config. Don't see reason we need to express
> that in the binding though so let that have VIN10 VINCOM (probably using
> a magic channel number) and VIN10 pseudo differential.
>
> Similar setup for ADCIN15 equivalent usage
>
> Code wise this probably won't be particular hard to support in the driver
> (obviously I haven't tried though :) is it worth the effort to keep
> it inline with other devices that support pseudo differential channesl.
Then this would need to be done to any fully differential ADC as support
for pseudo differential channels is present (connect a fixed non 0 voltage
to the negative input).
The AD717x family supports pseudo differential channels as well... should
this change be applied to them too? It is just the case that the documentation
does not mentions this use case.
I think that a distinction needs to be made here:
- When a device is only pseudo differential, sure, it is in a different category
- When a device is fully differential and you are using it as pseudo-differential
you are having two inputs compared to one another
I would need more clarification is why would supply-vincom be a requirement.
The voltage supplied to VINCOM will not be used in any computation within
the driver. From the perspective of getting the data it doesn't matter if
you are using the channel in a pseudo-differential, single ended or fully
differential manner.
Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
"Due to the matching resistors on the analog front end, the
differential inputs must be paired together in the following
pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
VIN6 and VIN7. If any two voltage inputs are paired in a
configuration other than what is described in this data sheet,
the accuracy of the device cannot be guaranteed."
Tried the device and it works as fully differential when pairing any
VINx with VINCOM. Still works when selecting VINCOM as the positive
input of the ADC.
I really see this as overly complicated and unnecessary. These families
of ADCs are fully differential. If you are using it to measure a single ended
(Be it compared to 0V or pseudo differential where you are comparing to Vref/2
and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
the common voltage.
On 06/04/2024 17:26, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 16:08:56 +0300
> "Ceclan, Dumitru" <[email protected]> wrote:
>
>> On 03/04/2024 18:22, David Lechner wrote:
>>> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <[email protected]> wrote:
>>>> On 02/04/2024 00:16, David Lechner wrote:
>>>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <[email protected]> wrote:
>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>> <[email protected]> wrote:
>>>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>>>
>>>> ...
>>>>
>>>>>>> properties:
>>>>>>> reg:
>>>>>>> + description:
>>>>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>>>>>> minimum: 0
>>>>>>> - maximum: 15
>>>>>>> + maximum: 19
>>>>>> This looks wrong. Isn't reg describing the number of logical channels
>>>>>> (# of channel config registers)?
>>>>>>
>>>>>> After reviewing the driver, I see that > 16 is used as a way of
>>>>>> flagging current inputs, but still seems like the wrong way to do it.
>>>>>> See suggestion below.
>>>>>>
>>>>>>> diff-channels:
>>>>>>> + description:
>>>>>>> + For using current channels specify only the positive channel.
>>>>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>>>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>>>>>> VIN0. I think it would make more sense to assign unique channel
>>>>>> numbers individually to the negative and positive current inputs.
>>>>>> Also, I think it makes sense to use the same numbers that the
>>>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>>>>>> positive).
>>>>>>
>>>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>>>>> Thinking about this a bit more...
>>>>>
>>>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>>>> with multiple valid wiring configurations like the voltage inputs, do
>>>>> we even need to describe them in the devicetree?
>>>>>
>>>>> In the driver, the current channels would just be hard-coded like the
>>>>> temperature channel since there isn't any application-specific
>>>>> variation.
>>>> Sure, but we still need to offer the user a way to configure which
>>>> current inputs he wants and if they should use bipolar or unipolar coding.
>>> From the datasheet, it looks like only positive current input is
>>> allowed so I'm not sure bipolar applies here. But, yes, if there is
>>> some other variation in wiring or electrical signal that needs to be
>>> describe here, then it makes sense to allow a channel configuration
>>> node for it.
>>
>> AD4111 datasheet pg.29:
>> When the ADC is configured for bipolar operation, the output
>> code is offset binary with a negative full-scale voltage resulting
>> in a code of 000 … 000, a zero differential input voltage resulting in
>> a code of 100 … 000, and a positive full-scale input voltage
>> resulting in a code of 111 … 111. The output code for any
>> analog input voltage can be represented as
>> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
>> The output code for any input current is represented as
>> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
>>
>> I would say bipolar applies here, not a great idea because of the limitation on
>> the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
>> is available.
> Just to check I am correct in thinking you 'might' use bipolar if you want
> to be able to measure small negative currents, but the range is much larger
> in the positive direction?
>
> J
Yes, exactly
On Tue, 9 Apr 2024 11:08:28 +0300
"Ceclan, Dumitru" <[email protected]> wrote:
> On 06/04/2024 17:53, Jonathan Cameron wrote:
> > On Wed, 3 Apr 2024 10:40:39 -0500
> > David Lechner <[email protected]> wrote:
> >
> >> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
> >>>
> >>> On 01/04/2024 22:37, David Lechner wrote:
> >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> From: Dumitru Ceclan <[email protected]>
> >>>
> >>> ...
> >>>
> >>>>> properties:
> >>>>> reg:
> >>>>> + description:
> >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> >>>>> minimum: 0
> >>>>> - maximum: 15
> >>>>> + maximum: 19
> >>>>
> >>>> This looks wrong. Isn't reg describing the number of logical channels
> >>>> (# of channel config registers)?
> >>>>
> >>>> After reviewing the driver, I see that > 16 is used as a way of
> >>>> flagging current inputs, but still seems like the wrong way to do it.
> >>>> See suggestion below.
> >>>>
> >>>
> >>> This was a suggestion from Jonathan, maybe I implemented it wrong.
> >
> > Maybe Jonathan was wrong! I was younger then than now :)
> >
> > However, reg values for child nodes are unique so can't just use a flag these
> > need to be different values.
> >
>
> I do not see where the restriction appears when using just the flag, when defining
> the channels you would still define unique reg values.
Good point. I'd got it into my head we had reg matching the channel index
but that doesn't work anyway because those can repeat. Sorry for misdirection!
>
> >>> Other alternative that came to my mind: attribute "adi,current-channel".
> >>
> >> Having a boolean flag like this would make more sense to me if we
> >> don't agree that the suggestion below is simpler.
> >>
> >>>>>
> >>>>> diff-channels:
> >>>>> + description:
> >>>>> + For using current channels specify only the positive channel.
> >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
> >>>>
> >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
> >>>> VIN0. I think it would make more sense to assign unique channel
> >>>> numbers individually to the negative and positive current inputs.
> >>>> Also, I think it makes sense to use the same numbers that the
> >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> >>>> positive).
> >>>>
> >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> >>>>
> >>>>
> >>> It would mean for the user to look in the datasheet at the possible
> >>> channel INPUT configurations values, decode the bit field into two
> >>> integer values and place it here (0110101010) -> 13 10. This is
> >>> cumbersome for just choosing current input 2.
> >>
> >> It could be documented in the devicetree bindings, just as it is done
> >> in adi,ad4130.yaml so that users of the bindings don't have to
> >> decipher the datasheet.
> >
> > The <13 10> option makes sense to me and avoids suggesting a common negative
> > input.
> >
> > The 'fun' bit here is that diff-channels doesn't actually tell us anything.
> > So we could just not provide it and rely on documentation of reg = 16-19 meaning
> > the current channels?
> >
>
> So a channel without diff-channels defined and reg=16 means IN0+ IN0-?
Yes, but with you correcting my error above maybe this isn't as clear cut as
I'd falsely recalled.
We do directly relate reg to channel numbers in drivers like the ad7292 (where not
all channels are differential) I'm not convinced either way on what is best
here where reg is currently just an index into a channel specification, not
meaningful for which pins are involved.
It doesn't seem worth adding an equivalent of diff-channels for a single channel
setup but I guess it would be more consistent.
>
> >>
> >>>
> >>>>> +
> >>>>> + Family AD411x supports a dedicated VCOM voltage input.
> >>>>> + To select it set the second channel to 16.
> >>>>> + (VIN2, VCOM) -> diff-channels = <2 16>
> >>>>
> >>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> >>>> bit confusing.
> >>>>
> >>>
> >>> Sure, I'll rename to VINCOM.
> >>>
> >>>> Also, do we need to add a vincom-supply to get this voltage? Or is it
> >>>> safe to assume it is always connected to AVSS? The datasheet seems to
> >>>> indicate that the latter is the case. But then it also has this
> >>>> special case (at least for AD4116, didn't check all datasheets)
> >>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> >>>> as part of a fully differential input, we probably need some extra
> >>>> flag to indicate that case.
> >>>>
> >>>
> >>> I cannot see any configuration options for these use cases. All inputs
> >>> are routed to the same mux and routed to the differential positive and
> >>> negative ADC inputs.
> >>>
> >>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> >>> between these two use cases is if you connected VINCOM to AVSS (with
> >>> unipolar coding) or not with bipolar encoding. The channel is still
> >>> measuring the difference between the two selected inputs and comparing
> >>> to the selected reference.
> >>>
> >>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> >>>> "(pseudo differential or differential pair)" notation that other
> >>>> inputs don't. In other words, it is more like VINCOM than it is to the
> >>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin
> >>>> to properly get the right channel configuration. I.e. the logic in the
> >>>> IIO driver would be if adcin15-supply is present, any channels that
> >>>> use this input are pseudo-differential, otherwise any channels that
> >>>> use it are fully differential.
> >>>>
> >>>
> >>> I cannot seem to understand what would a adcin15-supply be needed for.
> >>> This input, the same as all others, enters the mux and is routed to
> >>> either positive or negative input of the ADC.
> >>>
> >>> The voltage on the ADCIN15 pin is not important to the user, just the
> >>> difference in voltage between that pin and the other one selected.
> >>>
That statement is the root of disagreement I think.
If they are wired for pseudo differential measurement ADCIN15 a reference voltage
not a varying signal. It can equally be used as a negative channel of
a differential pair. Not different from point of view of hardware
config, but potentially different from point of view of how the
analog wiring is done and how we may want to present it to userspace.
> >>
> >> These suggestions come from some recent discussion about
> >> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> >> mailing list for AD7380).
> >>
> >> So what I suggested here might be more technically correct according
> >> to what I got out of that discussion. But for this specific case, I
> >> agree it is good enough to just treat all inputs as always
> >> fully-differential to keep things from getting too unwieldy.
> >
> > Hmm. That whole approach to pseudo differential does get messy if
> > we have the common line routed through the main MUX rather than an opt
> > in only on the negative side.
> >
> > If I read this right, its almost a trick to support a pseudo differential
> > wiring with simple registers (I guess reflecting MUX limitations).
> >
> > So what could we do?
> >
> > We could assume that VINCOM is used like a conventional pseudo
> > differential negative signal and have supply-vincom + non diffferential
> > channels if that's the configuration wanted.
> >
> > Then for differential channels can support all the VINX VINX+1
> > and swapped options.
> > For VIN10 it gets fun as non differential and differential options
> > I think map to same actual config. Don't see reason we need to express
> > that in the binding though so let that have VIN10 VINCOM (probably using
> > a magic channel number) and VIN10 pseudo differential.
> >
> > Similar setup for ADCIN15 equivalent usage
> >
> > Code wise this probably won't be particular hard to support in the driver
> > (obviously I haven't tried though :) is it worth the effort to keep
> > it inline with other devices that support pseudo differential channesl.
>
> Then this would need to be done to any fully differential ADC as support
> for pseudo differential channels is present (connect a fixed non 0 voltage
> to the negative input).
Whilst you could argue that, I'd counter that a clearly stated pseudo
differential mode with a simple choice of negative input (typically
only one pin is used for these modes), is a feature of the ADC, rather
than a wiring choice such as tying all negative inputs together and to
a reference supply.
>
> The AD717x family supports pseudo differential channels as well... should
> this change be applied to them too? It is just the case that the documentation
> does not mentions this use case.
Maybe you could argue that if we used the REF- for the negative input.
Otherwise I think it falls into the category where there isn't a clearly defined
pseudo differential mode.
>
> I think that a distinction needs to be made here:
> - When a device is only pseudo differential, sure, it is in a different category
> - When a device is fully differential and you are using it as pseudo-differential
> you are having two inputs compared to one another
>
> I would need more clarification is why would supply-vincom be a requirement.
> The voltage supplied to VINCOM will not be used in any computation within
> the driver. From the perspective of getting the data it doesn't matter if
> you are using the channel in a pseudo-differential, single ended or fully
> differential manner.
I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
ground so indeed nothing to turn on in this case and no offset to supply
(the offset will be 0 so we don't present it).
I'll note the datasheet describes the VINCOM as follows.
Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
as single-ended. Connect AINCOM to analog ground.
The reference to single ended is pretty clear hint to me that this case
is not a differential channel. The more complex case is the one David
raised of the AD4116 where we have actual pseudo differential inputs.
>
> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> "Due to the matching resistors on the analog front end, the
> differential inputs must be paired together in the following
> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
> VIN6 and VIN7. If any two voltage inputs are paired in a
> configuration other than what is described in this data sheet,
> the accuracy of the device cannot be guaranteed."
OK, but I'll assume no 'good' customer of ADI will do that as any support
engineer would grumpily point at that statement if they ever reported
a problem :)
>
> Tried the device and it works as fully differential when pairing any
> VINx with VINCOM. Still works when selecting VINCOM as the positive
> input of the ADC.
>
> I really see this as overly complicated and unnecessary. These families
> of ADCs are fully differential. If you are using it to measure a single ended
> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> the common voltage.
For single ended VINCOM should be tied to analog 0V. If the chip docs allowed
you to tie it to a different voltage then the single ended mode would be offset
wrt to that value.
For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
that is not connected to analog 0V. If the device is being used in a pseudo differential
mode that provides a fixed offset voltage.
So my preference (though I could maybe be convinced it's not worth the effort)
is to treat pseudo differential as single ended channels where 'negative' pin is
providing a fixed voltage (or 0V if that's relevant). Thus measurements provided
to userspace include the information of that offset.
We haven't handled pseudo differential channels that well in the past, but the
recent discussions have lead to a cleaner overall solution and it would be good
to be consistent going forwards. We could deprecate the previous bindings in
existing drivers, but that is a job for another day (possibly never happens!)
Jonathan
>
>
>
On Mon, 8 Apr 2024 19:40:26 +0300
"Ceclan, Dumitru" <[email protected]> wrote:
> On 06/04/2024 17:56, Jonathan Cameron wrote:
> > On Mon, 01 Apr 2024 18:32:20 +0300
> > Dumitru Ceclan via B4 Relay <[email protected]> wrote:
> >
> >> From: Dumitru Ceclan <[email protected]>
> >>
> >> AD7176-2 does not feature input buffers, enable buffers only on
> >> supported models.
> >>
> >> Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
> >> Signed-off-by: Dumitru Ceclan <[email protected]>
> > How bad is this? If you can find out if writing those bits does anything
> > harmful (they are reserved and datasheet says should be written 0 I think)
> > That will help people decide whether to backport the fix?
>
> The bits are marked as read-only and there does not seem to be any effect on the ADC.
> So drop this one?
Patch is good as makes the driver more consistent, just drop the Fixes tag so we
don't end up backporting this. That is basically treat it as code improvement
rather than a fix. Add a note on the bits being read only so this not fixing
a bug, just an inconsistency.
Thanks,
Jonathan
On 13/04/2024 13:49, Jonathan Cameron wrote:
> On Tue, 9 Apr 2024 11:08:28 +0300
> "Ceclan, Dumitru" <[email protected]> wrote:
>
>> On 06/04/2024 17:53, Jonathan Cameron wrote:
>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>> David Lechner <[email protected]> wrote:
>>>
>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
>>>>>
>>>>> On 01/04/2024 22:37, David Lechner wrote:
>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>
..
>>
>>>>> Other alternative that came to my mind: attribute "adi,current-channel".
>>>>
>>>> Having a boolean flag like this would make more sense to me if we
>>>> don't agree that the suggestion below is simpler.
>>>>
..
>
> We do directly relate reg to channel numbers in drivers like the ad7292 (where not
> all channels are differential) I'm not convinced either way on what is best
> here where reg is currently just an index into a channel specification, not
> meaningful for which pins are involved.
>
> It doesn't seem worth adding an equivalent of diff-channels for a single channel
> setup but I guess it would be more consistent.
>
Would you agree with the attribute adi,current-channel within the channel and
diff-channels set to the correspondent current inputs (13 10 for pair IN2)?
>>
>>>>
>>>>>
>>>>>>> +
>>>>>>> + Family AD411x supports a dedicated VCOM voltage input.
>>>>>>> + To select it set the second channel to 16.
>>>>>>> + (VIN2, VCOM) -> diff-channels = <2 16>
>>>>>>
>>>>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
>>>>>> bit confusing.
>>>>>>
>>>>>
>>>>> Sure, I'll rename to VINCOM.
>>>>>
>>>>>> Also, do we need to add a vincom-supply to get this voltage? Or is it
>>>>>> safe to assume it is always connected to AVSS? The datasheet seems to
>>>>>> indicate that the latter is the case. But then it also has this
>>>>>> special case (at least for AD4116, didn't check all datasheets)
>>>>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
>>>>>> as part of a fully differential input, we probably need some extra
>>>>>> flag to indicate that case.
>>>>>>
>>>>>
>>>>> I cannot see any configuration options for these use cases. All inputs
>>>>> are routed to the same mux and routed to the differential positive and
>>>>> negative ADC inputs.
>>>>>
>>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
>>>>> between these two use cases is if you connected VINCOM to AVSS (with
>>>>> unipolar coding) or not with bipolar encoding. The channel is still
>>>>> measuring the difference between the two selected inputs and comparing
>>>>> to the selected reference.
>>>>>
>>>>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
>>>>>> "(pseudo differential or differential pair)" notation that other
>>>>>> inputs don't. In other words, it is more like VINCOM than it is to the
>>>>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin
>>>>>> to properly get the right channel configuration. I.e. the logic in the
>>>>>> IIO driver would be if adcin15-supply is present, any channels that
>>>>>> use this input are pseudo-differential, otherwise any channels that
>>>>>> use it are fully differential.
>>>>>>
>>>>>
>>>>> I cannot seem to understand what would a adcin15-supply be needed for.
>>>>> This input, the same as all others, enters the mux and is routed to
>>>>> either positive or negative input of the ADC.
>>>>>
>>>>> The voltage on the ADCIN15 pin is not important to the user, just the
>>>>> difference in voltage between that pin and the other one selected.
>>>>>
>
> That statement is the root of disagreement I think.
> If they are wired for pseudo differential measurement ADCIN15 a reference voltage
> not a varying signal. It can equally be used as a negative channel of
> a differential pair. Not different from point of view of hardware
> config, but potentially different from point of view of how the
> analog wiring is done and how we may want to present it to userspace.
>
>>>>
>>>> These suggestions come from some recent discussion about
>>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
>>>> mailing list for AD7380).
>>>>
>>>> So what I suggested here might be more technically correct according
>>>> to what I got out of that discussion. But for this specific case, I
>>>> agree it is good enough to just treat all inputs as always
>>>> fully-differential to keep things from getting too unwieldy.
>>>
>>> Hmm. That whole approach to pseudo differential does get messy if
>>> we have the common line routed through the main MUX rather than an opt
>>> in only on the negative side.
>>>
>>> If I read this right, its almost a trick to support a pseudo differential
>>> wiring with simple registers (I guess reflecting MUX limitations).
>>>
>>> So what could we do?
>>>
>>> We could assume that VINCOM is used like a conventional pseudo
>>> differential negative signal and have supply-vincom + non diffferential
>>> channels if that's the configuration wanted.
>>>
>>> Then for differential channels can support all the VINX VINX+1
>>> and swapped options.
>>> For VIN10 it gets fun as non differential and differential options
>>> I think map to same actual config. Don't see reason we need to express
>>> that in the binding though so let that have VIN10 VINCOM (probably using
>>> a magic channel number) and VIN10 pseudo differential.
>>>
>>> Similar setup for ADCIN15 equivalent usage
>>>
>>> Code wise this probably won't be particular hard to support in the driver
>>> (obviously I haven't tried though :) is it worth the effort to keep
>>> it inline with other devices that support pseudo differential channesl.
>>
>> Then this would need to be done to any fully differential ADC as support
>> for pseudo differential channels is present (connect a fixed non 0 voltage
>> to the negative input).
>
> Whilst you could argue that, I'd counter that a clearly stated pseudo
> differential mode with a simple choice of negative input (typically
> only one pin is used for these modes), is a feature of the ADC, rather
> than a wiring choice such as tying all negative inputs together and to
> a reference supply.
>
>>
>> The AD717x family supports pseudo differential channels as well... should
>> this change be applied to them too? It is just the case that the documentation
>> does not mentions this use case.
>
> Maybe you could argue that if we used the REF- for the negative input.
> Otherwise I think it falls into the category where there isn't a clearly defined
> pseudo differential mode.
>
While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
"Pseudo Differential Inputs
The user can also choose to measure four different single-ended
analog inputs. In this case, each of the analog inputs is converted
as being the difference between the single-ended input to be
measured and a set analog input common pin. Because there is
a crosspoint multiplexer, the user can set any of the analog inputs
as the common pin. An example of such a scenario is to connect
the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
+ 2.5 V) and select this input when configuring the crosspoint
multiplexer. When using the AD7176-2 with pseudo differential
inputs, the INL specification is degraded."
As the crosspoint mux is present on all models it really makes me think that this
paragraph applies to all models in the family
>>
>> I think that a distinction needs to be made here:
>> - When a device is only pseudo differential, sure, it is in a different category
>> - When a device is fully differential and you are using it as pseudo-differential
>> you are having two inputs compared to one another
>>
>> I would need more clarification is why would supply-vincom be a requirement.
>> The voltage supplied to VINCOM will not be used in any computation within
>> the driver. From the perspective of getting the data it doesn't matter if
>> you are using the channel in a pseudo-differential, single ended or fully
>> differential manner.
>
> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> ground so indeed nothing to turn on in this case and no offset to supply
> (the offset will be 0 so we don't present it).
>
> I'll note the datasheet describes the VINCOM as follows.
>
> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> as single-ended. Connect AINCOM to analog ground.
>
> The reference to single ended is pretty clear hint to me that this case
> is not a differential channel. The more complex case is the one David
> raised of the AD4116 where we have actual pseudo differential inputs.
>
Alright, from my perspective they all pass through the same mux but okay,
not differential. The only issue would differentiating cases in AD4116 where
the pair VIN10 - VINCOM is specified as single-ended or differential pair.
Also, AD4116:
"0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
Not really sure where the "actual pseudo differential" sits.
Would you agree with having device tree flags that specifies how is the
channel used: single-ended, pseudo-differential, differential.
For the first two, the differential flag will not be set in IIO.
>>
>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>> "Due to the matching resistors on the analog front end, the
>> differential inputs must be paired together in the following
>> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>> VIN6 and VIN7. If any two voltage inputs are paired in a
>> configuration other than what is described in this data sheet,
>> the accuracy of the device cannot be guaranteed."
>
> OK, but I'll assume no 'good' customer of ADI will do that as any support
> engineer would grumpily point at that statement if they ever reported
> a problem :)
>
>>
>> Tried the device and it works as fully differential when pairing any
>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>> input of the ADC.
>>
>> I really see this as overly complicated and unnecessary. These families
>> of ADCs are fully differential. If you are using it to measure a single ended
>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>> the common voltage.
>
> For single ended VINCOM should be tied to analog 0V. If the chip docs allowed
> you to tie it to a different voltage then the single ended mode would be offset
> wrt to that value.
>
> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> that is not connected to analog 0V. If the device is being used in a pseudo differential
> mode that provides a fixed offset voltage.
>
> So my preference (though I could maybe be convinced it's not worth the effort)
> is to treat pseudo differential as single ended channels where 'negative' pin is
> providing a fixed voltage (or 0V if that's relevant). Thus measurements provided
> to userspace include the information of that offset.
>
What do you mean by offset? I currently understand that the user will have
a way of reading the voltage of that specific supply from the driver.
If you mean provide a different channel offset value when using it as
pseudo-differential then I would disagree
> We haven't handled pseudo differential channels that well in the past, but the
> recent discussions have lead to a cleaner overall solution and it would be good
> to be consistent going forwards. We could deprecate the previous bindings in
> existing drivers, but that is a job for another day (possibly never happens!)
>
I really hope that a clean solution could be obtained for this driver as well :)
On Mon, 15 Apr 2024 21:42:50 +0300
"Ceclan, Dumitru" <[email protected]> wrote:
> On 13/04/2024 13:49, Jonathan Cameron wrote:
> > On Tue, 9 Apr 2024 11:08:28 +0300
> > "Ceclan, Dumitru" <[email protected]> wrote:
> >
> >> On 06/04/2024 17:53, Jonathan Cameron wrote:
> >>> On Wed, 3 Apr 2024 10:40:39 -0500
> >>> David Lechner <[email protected]> wrote:
> >>>
> >>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
> >>>>>
> >>>>> On 01/04/2024 22:37, David Lechner wrote:
> >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>>>> <[email protected]> wrote:
> >>>>>>>
> >>>>>>> From: Dumitru Ceclan <[email protected]>
> >>>>>
> ...
> >>
> >>>>> Other alternative that came to my mind: attribute "adi,current-channel".
> >>>>
> >>>> Having a boolean flag like this would make more sense to me if we
> >>>> don't agree that the suggestion below is simpler.
> >>>>
>
> ...
>
> >
> > We do directly relate reg to channel numbers in drivers like the ad7292 (where not
> > all channels are differential) I'm not convinced either way on what is best
> > here where reg is currently just an index into a channel specification, not
> > meaningful for which pins are involved.
> >
> > It doesn't seem worth adding an equivalent of diff-channels for a single channel
> > setup but I guess it would be more consistent.
> >
>
> Would you agree with the attribute adi,current-channel within the channel and
> diff-channels set to the correspondent current inputs (13 10 for pair IN2)?
From another thread today I've concluded we do need a single-channel
equivalent of diff-channels, but you are right that here it is a differential
channel so <13 10> seems like the best option to me.
>
> >>
> >>>>
> >>>>>
> >>>>>>> +
> >>>>>>> + Family AD411x supports a dedicated VCOM voltage input.
> >>>>>>> + To select it set the second channel to 16.
> >>>>>>> + (VIN2, VCOM) -> diff-channels = <2 16>
> >>>>>>
> >>>>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> >>>>>> bit confusing.
> >>>>>>
> >>>>>
> >>>>> Sure, I'll rename to VINCOM.
> >>>>>
> >>>>>> Also, do we need to add a vincom-supply to get this voltage? Or is it
> >>>>>> safe to assume it is always connected to AVSS? The datasheet seems to
> >>>>>> indicate that the latter is the case. But then it also has this
> >>>>>> special case (at least for AD4116, didn't check all datasheets)
> >>>>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> >>>>>> as part of a fully differential input, we probably need some extra
> >>>>>> flag to indicate that case.
> >>>>>>
> >>>>>
> >>>>> I cannot see any configuration options for these use cases. All inputs
> >>>>> are routed to the same mux and routed to the differential positive and
> >>>>> negative ADC inputs.
> >>>>>
> >>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> >>>>> between these two use cases is if you connected VINCOM to AVSS (with
> >>>>> unipolar coding) or not with bipolar encoding. The channel is still
> >>>>> measuring the difference between the two selected inputs and comparing
> >>>>> to the selected reference.
> >>>>>
> >>>>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> >>>>>> "(pseudo differential or differential pair)" notation that other
> >>>>>> inputs don't. In other words, it is more like VINCOM than it is to the
> >>>>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin
> >>>>>> to properly get the right channel configuration. I.e. the logic in the
> >>>>>> IIO driver would be if adcin15-supply is present, any channels that
> >>>>>> use this input are pseudo-differential, otherwise any channels that
> >>>>>> use it are fully differential.
> >>>>>>
> >>>>>
> >>>>> I cannot seem to understand what would a adcin15-supply be needed for.
> >>>>> This input, the same as all others, enters the mux and is routed to
> >>>>> either positive or negative input of the ADC.
> >>>>>
> >>>>> The voltage on the ADCIN15 pin is not important to the user, just the
> >>>>> difference in voltage between that pin and the other one selected.
> >>>>>
> >
> > That statement is the root of disagreement I think.
> > If they are wired for pseudo differential measurement ADCIN15 a reference voltage
> > not a varying signal. It can equally be used as a negative channel of
> > a differential pair. Not different from point of view of hardware
> > config, but potentially different from point of view of how the
> > analog wiring is done and how we may want to present it to userspace.
> >
> >>>>
> >>>> These suggestions come from some recent discussion about
> >>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> >>>> mailing list for AD7380).
> >>>>
> >>>> So what I suggested here might be more technically correct according
> >>>> to what I got out of that discussion. But for this specific case, I
> >>>> agree it is good enough to just treat all inputs as always
> >>>> fully-differential to keep things from getting too unwieldy.
> >>>
> >>> Hmm. That whole approach to pseudo differential does get messy if
> >>> we have the common line routed through the main MUX rather than an opt
> >>> in only on the negative side.
> >>>
> >>> If I read this right, its almost a trick to support a pseudo differential
> >>> wiring with simple registers (I guess reflecting MUX limitations).
> >>>
> >>> So what could we do?
> >>>
> >>> We could assume that VINCOM is used like a conventional pseudo
> >>> differential negative signal and have supply-vincom + non diffferential
> >>> channels if that's the configuration wanted.
> >>>
> >>> Then for differential channels can support all the VINX VINX+1
> >>> and swapped options.
> >>> For VIN10 it gets fun as non differential and differential options
> >>> I think map to same actual config. Don't see reason we need to express
> >>> that in the binding though so let that have VIN10 VINCOM (probably using
> >>> a magic channel number) and VIN10 pseudo differential.
> >>>
> >>> Similar setup for ADCIN15 equivalent usage
> >>>
> >>> Code wise this probably won't be particular hard to support in the driver
> >>> (obviously I haven't tried though :) is it worth the effort to keep
> >>> it inline with other devices that support pseudo differential channesl.
> >>
> >> Then this would need to be done to any fully differential ADC as support
> >> for pseudo differential channels is present (connect a fixed non 0 voltage
> >> to the negative input).
> >
> > Whilst you could argue that, I'd counter that a clearly stated pseudo
> > differential mode with a simple choice of negative input (typically
> > only one pin is used for these modes), is a feature of the ADC, rather
> > than a wiring choice such as tying all negative inputs together and to
> > a reference supply.
> >
> >>
> >> The AD717x family supports pseudo differential channels as well... should
> >> this change be applied to them too? It is just the case that the documentation
> >> does not mentions this use case.
> >
> > Maybe you could argue that if we used the REF- for the negative input.
> > Otherwise I think it falls into the category where there isn't a clearly defined
> > pseudo differential mode.
> >
>
> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
> "Pseudo Differential Inputs
> The user can also choose to measure four different single-ended
> analog inputs. In this case, each of the analog inputs is converted
> as being the difference between the single-ended input to be
> measured and a set analog input common pin. Because there is
> a crosspoint multiplexer, the user can set any of the analog inputs
> as the common pin. An example of such a scenario is to connect
> the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
> + 2.5 V) and select this input when configuring the crosspoint
> multiplexer. When using the AD7176-2 with pseudo differential
> inputs, the INL specification is degraded."
>
> As the crosspoint mux is present on all models it really makes me think that this
> paragraph applies to all models in the family
Interesting indeed. So is your thinking that we need to support this
or take that "degraded" comment to imply that we should not bother
(at least until someone actually shouts that they want to do this?)
>
> >>
> >> I think that a distinction needs to be made here:
> >> - When a device is only pseudo differential, sure, it is in a different category
> >> - When a device is fully differential and you are using it as pseudo-differential
> >> you are having two inputs compared to one another
> >>
> >> I would need more clarification is why would supply-vincom be a requirement.
> >> The voltage supplied to VINCOM will not be used in any computation within
> >> the driver. From the perspective of getting the data it doesn't matter if
> >> you are using the channel in a pseudo-differential, single ended or fully
> >> differential manner.
> >
> > I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> > ground so indeed nothing to turn on in this case and no offset to supply
> > (the offset will be 0 so we don't present it).
> >
> > I'll note the datasheet describes the VINCOM as follows.
> >
> > Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> > as single-ended. Connect AINCOM to analog ground.
> >
> > The reference to single ended is pretty clear hint to me that this case
> > is not a differential channel. The more complex case is the one David
> > raised of the AD4116 where we have actual pseudo differential inputs.
> >
>
> Alright, from my perspective they all pass through the same mux but okay,
> not differential. The only issue would differentiating cases in AD4116 where
> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
>
> Also, AD4116:
> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
> 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
> 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
> 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
>
> Not really sure where the "actual pseudo differential" sits.
>
> Would you agree with having device tree flags that specifies how is the
> channel used: single-ended, pseudo-differential, differential.
> For the first two, the differential flag will not be set in IIO.
Yes. I think that makes sense - though as you observe in some cases
the actual device settings end up the same (the ad4116 note above).
If a given channel supports single-ended and pseudo-differential is
that really just a low reference change (I assume from an input to the
the IO ground)? Or is there more going on?
If it's the reference, then can we provide that as the binding control
signal? We have other drivers that do that (though we could perhaps make
it more generic) e.g. adi,ad7124 with adi,reference-select
I don't like that binding because it always ends up have a local enum
of values, but can't really think of a better solution.
>
> >>
> >> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> >> "Due to the matching resistors on the analog front end, the
> >> differential inputs must be paired together in the following
> >> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
> >> VIN6 and VIN7. If any two voltage inputs are paired in a
> >> configuration other than what is described in this data sheet,
> >> the accuracy of the device cannot be guaranteed."
> >
> > OK, but I'll assume no 'good' customer of ADI will do that as any support
> > engineer would grumpily point at that statement if they ever reported
> > a problem :)
> >
> >>
> >> Tried the device and it works as fully differential when pairing any
> >> VINx with VINCOM. Still works when selecting VINCOM as the positive
> >> input of the ADC.
> >>
> >> I really see this as overly complicated and unnecessary. These families
> >> of ADCs are fully differential. If you are using it to measure a single ended
> >> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> >> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> >> the common voltage.
> >
> > For single ended VINCOM should be tied to analog 0V. If the chip docs allowed
> > you to tie it to a different voltage then the single ended mode would be offset
> > wrt to that value.
> >
> > For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> > that is not connected to analog 0V. If the device is being used in a pseudo differential
> > mode that provides a fixed offset voltage.
> >
> > So my preference (though I could maybe be convinced it's not worth the effort)
> > is to treat pseudo differential as single ended channels where 'negative' pin is
> > providing a fixed voltage (or 0V if that's relevant). Thus measurements provided
> > to userspace include the information of that offset.
> >
>
> What do you mean by offset? I currently understand that the user will have
> a way of reading the voltage of that specific supply from the driver.
How? We could do it that way, but we don't have existing ABI for this that
I can think of.
>
> If you mean provide a different channel offset value when using it as
> pseudo-differential then I would disagree
Provided to user space as _offset on the channel, userspace can either
incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
or ignore it if it only wants the difference from the reference value.
I'm open to discussion other ABI options, but this is the one we most naturally have
available.
>
>
> > We haven't handled pseudo differential channels that well in the past, but the
> > recent discussions have lead to a cleaner overall solution and it would be good
> > to be consistent going forwards. We could deprecate the previous bindings in
> > existing drivers, but that is a job for another day (possibly never happens!)
> >
>
> I really hope that a clean solution could be obtained for this driver as well :)
I bet you wish sometimes that you had easier parts to write drivers for! :)
These continue to stretch the boundaries which is good, but slow.
Jonathan
>
>
On 20/04/2024 17:33, Jonathan Cameron wrote:
> On Mon, 15 Apr 2024 21:42:50 +0300
> "Ceclan, Dumitru" <[email protected]> wrote:
>
>> On 13/04/2024 13:49, Jonathan Cameron wrote:
>>> On Tue, 9 Apr 2024 11:08:28 +0300
>>> "Ceclan, Dumitru" <[email protected]> wrote:
>>>
>>>> On 06/04/2024 17:53, Jonathan Cameron wrote:
>>>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>>>> David Lechner <[email protected]> wrote:
>>>>>
>>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
>>>>>>>
>>>>>>> On 01/04/2024 22:37, David Lechner wrote:
>>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>>>> <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>>>
..
>>>>
>>>> The AD717x family supports pseudo differential channels as well... should
>>>> this change be applied to them too? It is just the case that the documentation
>>>> does not mentions this use case.
>>>
>>> Maybe you could argue that if we used the REF- for the negative input.
>>> Otherwise I think it falls into the category where there isn't a clearly defined
>>> pseudo differential mode.
>>>
>>
>> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
>> "Pseudo Differential Inputs
>> The user can also choose to measure four different single-ended
>> analog inputs. In this case, each of the analog inputs is converted
>> as being the difference between the single-ended input to be
>> measured and a set analog input common pin. Because there is
>> a crosspoint multiplexer, the user can set any of the analog inputs
>> as the common pin. An example of such a scenario is to connect
>> the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
>> + 2.5 V) and select this input when configuring the crosspoint
>> multiplexer. When using the AD7176-2 with pseudo differential
>> inputs, the INL specification is degraded."
>>
>> As the crosspoint mux is present on all models it really makes me think that this
>> paragraph applies to all models in the family
>
> Interesting indeed. So is your thinking that we need to support this
> or take that "degraded" comment to imply that we should not bother
> (at least until someone actually shouts that they want to do this?)
>
My perspective is that support for this is already existent, the chips do not
need any special configuration in that use-case. If we want to be correct in
how the channel will be presented to the user, besides setting to false the IIO
differential flag I do not see what else should be done.
>>
>>>>
>>>> I think that a distinction needs to be made here:
>>>> - When a device is only pseudo differential, sure, it is in a different category
>>>> - When a device is fully differential and you are using it as pseudo-differential
>>>> you are having two inputs compared to one another
>>>>
>>>> I would need more clarification is why would supply-vincom be a requirement.
>>>> The voltage supplied to VINCOM will not be used in any computation within
>>>> the driver. From the perspective of getting the data it doesn't matter if
>>>> you are using the channel in a pseudo-differential, single ended or fully
>>>> differential manner.
>>>
>>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
>>> ground so indeed nothing to turn on in this case and no offset to supply
>>> (the offset will be 0 so we don't present it).
>>>
>>> I'll note the datasheet describes the VINCOM as follows.
>>>
>>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
>>> as single-ended. Connect AINCOM to analog ground.
>>>
>>> The reference to single ended is pretty clear hint to me that this case
>>> is not a differential channel. The more complex case is the one David
>>> raised of the AD4116 where we have actual pseudo differential inputs.
>>>
>>
>> Alright, from my perspective they all pass through the same mux but okay,
>> not differential. The only issue would differentiating cases in AD4116 where
>> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
>>
>> Also, AD4116:
>> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
>> 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
>> 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
>> 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
>>
>> Not really sure where the "actual pseudo differential" sits.
>>
>> Would you agree with having device tree flags that specifies how is the
>> channel used: single-ended, pseudo-differential, differential.
>> For the first two, the differential flag will not be set in IIO.
>
> Yes. I think that makes sense - though as you observe in some cases
> the actual device settings end up the same (the ad4116 note above).
>
This precisely why I suggest this approach, because a channel used as
single-ended, pseudo or fully differential will have the same register
configuration on all models. I do not see any other way to know from
the driver this information.
> If a given channel supports single-ended and pseudo-differential is
> that really just a low reference change (I assume from an input to the
> the IO ground)? Or is there more going on?
>
I'm not sure if I understood what was said here. The reference specified
in the channel setup does not need to change.
> If it's the reference, then can we provide that as the binding control
> signal? We have other drivers that do that (though we could perhaps make
> it more generic) e.g. adi,ad7124 with adi,reference-select
> We already have adi,reference-select in the binding and driver, I do not
see how it could help the driver differentiate between (single, pseudo...)
> I don't like that binding because it always ends up have a local enum
> of values, but can't really think of a better solution.
>
>>
>>>>
>>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>>>> "Due to the matching resistors on the analog front end, the
>>>> differential inputs must be paired together in the following
>>>> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>>>> VIN6 and VIN7. If any two voltage inputs are paired in a
>>>> configuration other than what is described in this data sheet,
>>>> the accuracy of the device cannot be guaranteed."
>>>
>>> OK, but I'll assume no 'good' customer of ADI will do that as any support
>>> engineer would grumpily point at that statement if they ever reported
>>> a problem :)
>>>
>>>>
>>>> Tried the device and it works as fully differential when pairing any
>>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>>>> input of the ADC.
>>>>
>>>> I really see this as overly complicated and unnecessary. These families
>>>> of ADCs are fully differential. If you are using it to measure a single ended
>>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>>>> the common voltage.
>>>
>>> For single ended VINCOM should be tied to analog 0V. If the chip docs allowed
>>> you to tie it to a different voltage then the single ended mode would be offset
>>> wrt to that value.
>>>
>>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
>>> that is not connected to analog 0V. If the device is being used in a pseudo differential
>>> mode that provides a fixed offset voltage.
>>>
>>> So my preference (though I could maybe be convinced it's not worth the effort)
>>> is to treat pseudo differential as single ended channels where 'negative' pin is
>>> providing a fixed voltage (or 0V if that's relevant). Thus measurements provided
>>> to userspace include the information of that offset.
>>>
>>
>> What do you mean by offset? I currently understand that the user will have
>> a way of reading the voltage of that specific supply from the driver.
>
> How? We could do it that way, but we don't have existing ABI for this that
> I can think of.
>
Expose a voltage channel which is not reading from the device...but that is
too much of a hack to be accepted here
>>
>> If you mean provide a different channel offset value when using it as
>> pseudo-differential then I would disagree
>
> Provided to user space as _offset on the channel, userspace can either
> incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
> or ignore it if it only wants the difference from the reference value.
>
> I'm open to discussion other ABI options, but this is the one we most naturally have
> available.
_offset is already used when the bipolar coding is enabled on the channel
and is computed along datasheet specifications of how data should be processed,
this is why I disagree with this.
This feels over-engineered, most of the times if a channel is pseudo
differential, the relevant measurement will be the differences between
those two inputs.
If a user needs to know the voltage on the common input, he just needs to
also configure a single ended channel with the common input where the
negative AIN is connected to AVSS.
>>
>>
>>> We haven't handled pseudo differential channels that well in the past, but the
>>> recent discussions have lead to a cleaner overall solution and it would be good
>>> to be consistent going forwards. We could deprecate the previous bindings in
>>> existing drivers, but that is a job for another day (possibly never happens!)
>>>
>>
>> I really hope that a clean solution could be obtained for this driver as well :)
>
> I bet you wish sometimes that you had easier parts to write drivers for! :)
> These continue to stretch the boundaries which is good, but slow.
>
> Jonathan
Not easier, fewer crammed into the same driver :)
On Tue, 23 Apr 2024 11:18:47 +0300
"Ceclan, Dumitru" <[email protected]> wrote:
> On 20/04/2024 17:33, Jonathan Cameron wrote:
> > On Mon, 15 Apr 2024 21:42:50 +0300
> > "Ceclan, Dumitru" <[email protected]> wrote:
> >
> >> On 13/04/2024 13:49, Jonathan Cameron wrote:
> >>> On Tue, 9 Apr 2024 11:08:28 +0300
> >>> "Ceclan, Dumitru" <[email protected]> wrote:
> >>>
> >>>> On 06/04/2024 17:53, Jonathan Cameron wrote:
> >>>>> On Wed, 3 Apr 2024 10:40:39 -0500
> >>>>> David Lechner <[email protected]> wrote:
> >>>>>
> >>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <[email protected]> wrote:
> >>>>>>>
> >>>>>>> On 01/04/2024 22:37, David Lechner wrote:
> >>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>>>>>> <[email protected]> wrote:
> >>>>>>>>>
> >>>>>>>>> From: Dumitru Ceclan <[email protected]>
> >>>>>>>
>
> ...
>
> >>>>
> >>>> The AD717x family supports pseudo differential channels as well... should
> >>>> this change be applied to them too? It is just the case that the documentation
> >>>> does not mentions this use case.
> >>>
> >>> Maybe you could argue that if we used the REF- for the negative input.
> >>> Otherwise I think it falls into the category where there isn't a clearly defined
> >>> pseudo differential mode.
> >>>
> >>
> >> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
> >> "Pseudo Differential Inputs
> >> The user can also choose to measure four different single-ended
> >> analog inputs. In this case, each of the analog inputs is converted
> >> as being the difference between the single-ended input to be
> >> measured and a set analog input common pin. Because there is
> >> a crosspoint multiplexer, the user can set any of the analog inputs
> >> as the common pin. An example of such a scenario is to connect
> >> the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
> >> + 2.5 V) and select this input when configuring the crosspoint
> >> multiplexer. When using the AD7176-2 with pseudo differential
> >> inputs, the INL specification is degraded."
> >>
> >> As the crosspoint mux is present on all models it really makes me think that this
> >> paragraph applies to all models in the family
> >
> > Interesting indeed. So is your thinking that we need to support this
> > or take that "degraded" comment to imply that we should not bother
> > (at least until someone actually shouts that they want to do this?)
> >
>
> My perspective is that support for this is already existent, the chips do not
> need any special configuration in that use-case. If we want to be correct in
> how the channel will be presented to the user, besides setting to false the IIO
> differential flag I do not see what else should be done.
ah. The degraded bit bothered me. That wording makes me thing no effort
should be applied to support this unless a user shouts that they really want it.
If we get it for free or near free than all is good!.
>
> >>
> >>>>
> >>>> I think that a distinction needs to be made here:
> >>>> - When a device is only pseudo differential, sure, it is in a different category
> >>>> - When a device is fully differential and you are using it as pseudo-differential
> >>>> you are having two inputs compared to one another
> >>>>
> >>>> I would need more clarification is why would supply-vincom be a requirement.
> >>>> The voltage supplied to VINCOM will not be used in any computation within
> >>>> the driver. From the perspective of getting the data it doesn't matter if
> >>>> you are using the channel in a pseudo-differential, single ended or fully
> >>>> differential manner.
> >>>
> >>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> >>> ground so indeed nothing to turn on in this case and no offset to supply
> >>> (the offset will be 0 so we don't present it).
> >>>
> >>> I'll note the datasheet describes the VINCOM as follows.
> >>>
> >>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> >>> as single-ended. Connect AINCOM to analog ground.
> >>>
> >>> The reference to single ended is pretty clear hint to me that this case
> >>> is not a differential channel. The more complex case is the one David
> >>> raised of the AD4116 where we have actual pseudo differential inputs.
> >>>
> >>
> >> Alright, from my perspective they all pass through the same mux but okay,
> >> not differential. The only issue would differentiating cases in AD4116 where
> >> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
> >>
> >> Also, AD4116:
> >> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
> >> 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
> >> 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
> >> 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
> >>
> >> Not really sure where the "actual pseudo differential" sits.
> >>
> >> Would you agree with having device tree flags that specifies how is the
> >> channel used: single-ended, pseudo-differential, differential.
> >> For the first two, the differential flag will not be set in IIO.
> >
> > Yes. I think that makes sense - though as you observe in some cases
> > the actual device settings end up the same (the ad4116 note above).
> >
> This precisely why I suggest this approach, because a channel used as
> single-ended, pseudo or fully differential will have the same register
> configuration on all models. I do not see any other way to know from
> the driver this information.
>
> > If a given channel supports single-ended and pseudo-differential is
> > that really just a low reference change (I assume from an input to the
> > the IO ground)? Or is there more going on?
> >
> I'm not sure if I understood what was said here. The reference specified
> in the channel setup does not need to change.
So what is the effective difference? My assumption was that single-ended
means reference to 0V in all cases. Pseudo differential means reference
to an input that is common across multiple channels, but not necessarily 0V?
>
> > If it's the reference, then can we provide that as the binding control
> > signal? We have other drivers that do that (though we could perhaps make
> > it more generic) e.g. adi,ad7124 with adi,reference-select
> > We already have adi,reference-select in the binding and driver, I do not
> see how it could help the driver differentiate between (single, pseudo...)
Indeed that doesn't work. Problem in this discussion is I've normally forgotten
the earlier discussion when I come back to it :(
>
> > I don't like that binding because it always ends up have a local enum
> > of values, but can't really think of a better solution.
> >
>
> >>
> >>>>
> >>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> >>>> "Due to the matching resistors on the analog front end, the
> >>>> differential inputs must be paired together in the following
> >>>> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
> >>>> VIN6 and VIN7. If any two voltage inputs are paired in a
> >>>> configuration other than what is described in this data sheet,
> >>>> the accuracy of the device cannot be guaranteed."
> >>>
> >>> OK, but I'll assume no 'good' customer of ADI will do that as any support
> >>> engineer would grumpily point at that statement if they ever reported
> >>> a problem :)
> >>>
> >>>>
> >>>> Tried the device and it works as fully differential when pairing any
> >>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
> >>>> input of the ADC.
> >>>>
> >>>> I really see this as overly complicated and unnecessary. These families
> >>>> of ADCs are fully differential. If you are using it to measure a single ended
> >>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> >>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> >>>> the common voltage.
> >>>
> >>> For single ended VINCOM should be tied to analog 0V. If the chip docs allowed
> >>> you to tie it to a different voltage then the single ended mode would be offset
> >>> wrt to that value.
> >>>
> >>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> >>> that is not connected to analog 0V. If the device is being used in a pseudo differential
> >>> mode that provides a fixed offset voltage.
> >>>
> >>> So my preference (though I could maybe be convinced it's not worth the effort)
> >>> is to treat pseudo differential as single ended channels where 'negative' pin is
> >>> providing a fixed voltage (or 0V if that's relevant). Thus measurements provided
> >>> to userspace include the information of that offset.
> >>>
> >>
> >> What do you mean by offset? I currently understand that the user will have
> >> a way of reading the voltage of that specific supply from the driver.
> >
> > How? We could do it that way, but we don't have existing ABI for this that
> > I can think of.
> >
> Expose a voltage channel which is not reading from the device...but that is
> too much of a hack to be accepted here
We have done things like that for a few corner cases where we were really stuck
but it is indeed nasty and hard to comprehend. Also so far they've been 'out'
channels I think which doesn't make sense here.
> >>
> >> If you mean provide a different channel offset value when using it as
> >> pseudo-differential then I would disagree
> >
> > Provided to user space as _offset on the channel, userspace can either
> > incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
> > or ignore it if it only wants the difference from the reference value.
> >
> > I'm open to discussion other ABI options, but this is the one we most naturally have
> > available.
> _offset is already used when the bipolar coding is enabled on the channel
> and is computed along datasheet specifications of how data should be processed,
> this is why I disagree with this.
OK. It would be easy enough to apply an offset to that, but it would
complicate the driver and could seem a little odd.
>
> This feels over-engineered, most of the times if a channel is pseudo
> differential, the relevant measurement will be the differences between
> those two inputs.
>
> If a user needs to know the voltage on the common input, he just needs to
> also configure a single ended channel with the common input where the
> negative AIN is connected to AVSS.
OK. I'm somewhat convinced that this is enough of a pain to describe that
we should just rely on them having some other way to get that offset if they
are deliberately using it to shift the range. We can revisit if it ever
becomes a problem.
So, I think the conclusion is just don't represent AIN-COMM (or similar)
as an explicit voltage we can measure.
> >>
> >>
> >>> We haven't handled pseudo differential channels that well in the past, but the
> >>> recent discussions have lead to a cleaner overall solution and it would be good
> >>> to be consistent going forwards. We could deprecate the previous bindings in
> >>> existing drivers, but that is a job for another day (possibly never happens!)
> >>>
> >>
> >> I really hope that a clean solution could be obtained for this driver as well :)
> >
> > I bet you wish sometimes that you had easier parts to write drivers for! :)
> > These continue to stretch the boundaries which is good, but slow.
> >
> > Jonathan
>
> Not easier, fewer crammed into the same driver :)
I sympathise! It's been an annoyingly busy kernel cycle in the day job. I was hoping to
get back to you sooner so that more of this was fresh(ish) in my mind :(
My gut feeling is that this is a case for documentation / really detailed cover
letter for the next version to make sure we have come to at least a (mostly)
consistent conclusion.
Jonathan