2024-04-30 16:30:32

by Alisa-Dariana Roman

Subject: [PATCH v7 0/6] iio: adc: ad7192: Add AD7194 support

Dear maintainers,

Thank you all for the feedback!

I am submitting the upgraded series of patches for the ad7192 driver.

Please consider applying in order.

Thank you!

v6 -> v7
- patch1: move mutex lock and unlock to protect whole switch statement
- patch3: use NANO from units.h
- patch3: add comment
- patch3: use dev_err_probe
- patch4: new patch to add single-channel property
- patch5: modify maximum number of channels to include single-ended channels
- patch5: add single-channel property to bindings for single-ended channels
- patch5: modify example to include single-channel property
- patch5: modify channel pattern to "^channel@[0-9a-f]+$"
- patch5: modify required properties for channel node
- patch6: add function to validate ain channel
- patch6: remove function to parse one channel
- patch6: single-ended channels are now also configured in the devicetree
- patch6: modified some names to reflect the changes

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

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

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

v2 -> v3
- add precursor patch to simply functions to only pass
ad7192_state
- add patch to replace custom attribute
- bindings patch: correct use of allOf and some minor changes to
the ad7194 example
- add ad7194 patch:
- use "ad7192 and similar"
- ad7194 no longer needs attribute group
- use callback function in chip_info to parse channels
- move struct ad7192_chip_info
- change position of parse functions
- drop clock bindings patch

v1 -> v2
- new commit with missing documentation for properties
- add constraint for channels in binding
- correct pattern for channels
- correct commit message by adding "()" to functions
- use in_range
- use preferred structure in Kconfig

Kind regards,

Alisa-Dariana Roman (6):
iio: adc: ad7192: Use standard attribute
dt-bindings: iio: adc: ad7192: Add aincom supply
iio: adc: ad7192: Add aincom supply
dt-bindings: iio: adc: Add single-channel property
dt-bindings: iio: adc: ad7192: Add AD7194 support
iio: adc: ad7192: Add AD7194 support

.../devicetree/bindings/iio/adc/adc.yaml | 8 +
.../bindings/iio/adc/adi,ad7192.yaml | 95 +++++++
drivers/iio/adc/Kconfig | 11 +-
drivers/iio/adc/ad7192.c | 245 ++++++++++++++----
4 files changed, 309 insertions(+), 50 deletions(-)

--
2.34.1

2024-04-30 16:30:38

by Alisa-Dariana Roman

Subject: [PATCH v7 1/6] iio: adc: ad7192: Use standard attribute

Replace custom attribute filter_low_pass_3db_frequency_available with
standard attribute.

Store the available values in ad7192_state struct.

The function that used to compute those values replaced by
ad7192_update_filter_freq_avail().

Function ad7192_show_filter_avail() is no longer needed.

Note that the initial available values are hardcoded.

Also moved the mutex lock and unlock in order to protect the whole
switch statement since each branch modifies the state of the device.

Reviewed-by: David Lechner <[email protected]>
Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
drivers/iio/adc/ad7192.c | 75 ++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 7bcc7e2aa2a2..ace81e3817a1 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -190,6 +190,7 @@ struct ad7192_state {
u32 mode;
u32 conf;
u32 scale_avail[8][2];
+ u32 filter_freq_avail[4][2];
u32 oversampling_ratio_avail[4];
u8 gpocon;
u8 clock_sel;
@@ -473,6 +474,16 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
st->oversampling_ratio_avail[2] = 8;
st->oversampling_ratio_avail[3] = 16;

+ st->filter_freq_avail[0][0] = 600;
+ st->filter_freq_avail[1][0] = 800;
+ st->filter_freq_avail[2][0] = 2300;
+ st->filter_freq_avail[3][0] = 2720;
+
+ st->filter_freq_avail[0][1] = 1000;
+ st->filter_freq_avail[1][1] = 1000;
+ st->filter_freq_avail[2][1] = 1000;
+ st->filter_freq_avail[3][1] = 1000;
+
return 0;
}

@@ -586,48 +597,24 @@ static int ad7192_get_f_adc(struct ad7192_state *st)
f_order * FIELD_GET(AD7192_MODE_RATE_MASK, st->mode));
}

-static void ad7192_get_available_filter_freq(struct ad7192_state *st,
- int *freq)
+static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
{
unsigned int fadc;

/* Formulas for filter at page 25 of the datasheet */
fadc = ad7192_compute_f_adc(st, false, true);
- freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+ st->filter_freq_avail[0][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);

fadc = ad7192_compute_f_adc(st, true, true);
- freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+ st->filter_freq_avail[1][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);

fadc = ad7192_compute_f_adc(st, false, false);
- freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
+ st->filter_freq_avail[2][0] = DIV_ROUND_CLOSEST(fadc * 230, 1024);

fadc = ad7192_compute_f_adc(st, true, false);
- freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
+ st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
}

-static ssize_t ad7192_show_filter_avail(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct ad7192_state *st = iio_priv(indio_dev);
- unsigned int freq_avail[4], i;
- size_t len = 0;
-
- ad7192_get_available_filter_freq(st, freq_avail);
-
- for (i = 0; i < ARRAY_SIZE(freq_avail); i++)
- len += sysfs_emit_at(buf, len, "%d.%03d ", freq_avail[i] / 1000,
- freq_avail[i] % 1000);
-
- buf[len - 1] = '\n';
-
- return len;
-}
-
-static IIO_DEVICE_ATTR(filter_low_pass_3db_frequency_available,
- 0444, ad7192_show_filter_avail, NULL, 0);
-
static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
ad7192_show_bridge_switch, ad7192_set,
AD7192_REG_GPOCON);
@@ -637,7 +624,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
AD7192_REG_CONF);

static struct attribute *ad7192_attributes[] = {
- &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
&iio_dev_attr_bridge_switch_en.dev_attr.attr,
NULL
};
@@ -647,7 +633,6 @@ static const struct attribute_group ad7192_attribute_group = {
};

static struct attribute *ad7195_attributes[] = {
- &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
&iio_dev_attr_bridge_switch_en.dev_attr.attr,
&iio_dev_attr_ac_excitation_en.dev_attr.attr,
NULL
@@ -665,17 +650,15 @@ static unsigned int ad7192_get_temp_scale(bool unipolar)
static int ad7192_set_3db_filter_freq(struct ad7192_state *st,
int val, int val2)
{
- int freq_avail[4], i, ret, freq;
+ int i, ret, freq;
unsigned int diff_new, diff_old;
int idx = 0;

diff_old = U32_MAX;
freq = val * 1000 + val2;

- ad7192_get_available_filter_freq(st, freq_avail);
-
- for (i = 0; i < ARRAY_SIZE(freq_avail); i++) {
- diff_new = abs(freq - freq_avail[i]);
+ for (i = 0; i < ARRAY_SIZE(st->filter_freq_avail); i++) {
+ diff_new = abs(freq - st->filter_freq_avail[i][0]);
if (diff_new < diff_old) {
diff_old = diff_new;
idx = i;
@@ -792,10 +775,11 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
if (ret)
return ret;

+ mutex_lock(&st->lock);
+
switch (mask) {
case IIO_CHAN_INFO_SCALE:
ret = -EINVAL;
- mutex_lock(&st->lock);
for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
if (val2 == st->scale_avail[i][1]) {
ret = 0;
@@ -809,7 +793,6 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
ad7192_calibrate_all(st);
break;
}
- mutex_unlock(&st->lock);
break;
case IIO_CHAN_INFO_SAMP_FREQ:
if (!val) {
@@ -826,13 +809,13 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
st->mode &= ~AD7192_MODE_RATE_MASK;
st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+ ad7192_update_filter_freq_avail(st);
break;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000);
break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
ret = -EINVAL;
- mutex_lock(&st->lock);
for (i = 0; i < ARRAY_SIZE(st->oversampling_ratio_avail); i++)
if (val == st->oversampling_ratio_avail[i]) {
ret = 0;
@@ -845,12 +828,14 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
3, st->mode);
break;
}
- mutex_unlock(&st->lock);
+ ad7192_update_filter_freq_avail(st);
break;
default:
ret = -EINVAL;
}

+ mutex_unlock(&st->lock);
+
iio_device_release_direct_mode(indio_dev);

return ret;
@@ -888,6 +873,12 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
/* Values are stored in a 2D matrix */
*length = ARRAY_SIZE(st->scale_avail) * 2;

+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ *vals = (int *)st->filter_freq_avail;
+ *type = IIO_VAL_FRACTIONAL;
+ *length = ARRAY_SIZE(st->filter_freq_avail) * 2;
+
return IIO_AVAIL_LIST;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
*vals = (int *)st->oversampling_ratio_avail;
@@ -956,7 +947,9 @@ static const struct iio_info ad7195_info = {
BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
(_mask_all), \
.info_mask_shared_by_type_available = (_mask_type_av), \
- .info_mask_shared_by_all_available = (_mask_all_av), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+ (_mask_all_av), \
.ext_info = (_ext_info), \
.scan_index = (_si), \
.scan_type = { \
--
2.34.1

2024-04-30 16:30:54

by Alisa-Dariana Roman

Subject: [PATCH v7 2/6] dt-bindings: iio: adc: ad7192: Add aincom supply

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

Reviewed-by: Rob Herring (Arm) <[email protected]>
Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 16def2985ab4..cf5c568f140a 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -41,6 +41,11 @@ properties:
interrupts:
maxItems: 1

+ aincom-supply:
+ description: |
+ AINCOM voltage supply. Analog inputs AINx are referenced to this input
+ when configured for pseudo-differential operation.
+
dvdd-supply:
description: DVdd voltage supply

@@ -117,6 +122,7 @@ examples:
clock-names = "mclk";
interrupts = <25 0x2>;
interrupt-parent = <&gpio>;
+ aincom-supply = <&aincom>;
dvdd-supply = <&dvdd>;
avdd-supply = <&avdd>;
vref-supply = <&vref>;
--
2.34.1

2024-04-30 16:31:07

by Alisa-Dariana Roman

Subject: [PATCH v7 3/6] iio: adc: ad7192: Add aincom supply

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

Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
drivers/iio/adc/ad7192.c | 41 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index ace81e3817a1..3e797ff48086 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/property.h>
+#include <linux/units.h>

#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
@@ -186,6 +187,7 @@ struct ad7192_state {
struct regulator *vref;
struct clk *mclk;
u16 int_vref_mv;
+ u32 aincom_mv;
u32 fclk;
u32 mode;
u32 conf;
@@ -742,10 +744,19 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
*val = -(1 << (chan->scan_type.realbits - 1));
else
*val = 0;
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (st->aincom_mv && !chan->differential)
+ *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * NANO,
+ st->scale_avail[gain][1]);
+ return IIO_VAL_INT;
/* Kelvin to Celsius */
- if (chan->type == IIO_TEMP)
+ case IIO_TEMP:
*val -= 273 * ad7192_get_temp_scale(unipolar);
- return IIO_VAL_INT;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_SAMP_FREQ:
*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
return IIO_VAL_INT;
@@ -1052,6 +1063,7 @@ static int ad7192_probe(struct spi_device *spi)
{
struct ad7192_state *st;
struct iio_dev *indio_dev;
+ struct regulator *aincom;
int ret;

if (!spi->irq) {
@@ -1067,6 +1079,31 @@ static int ad7192_probe(struct spi_device *spi)

mutex_init(&st->lock);

+ /*
+ * Regulator aincom is optional to maintain compatibility with older DT.
+ * Newer firmware should provide a zero volt fixed supply if wired to
+ * ground.
+ */
+ aincom = devm_regulator_get_optional(&spi->dev, "aincom");
+ if (IS_ERR(aincom)) {
+ st->aincom_mv = 0;
+ } else {
+ ret = regulator_enable(aincom);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to enable specified AINCOM supply\n");
+
+ ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
+ if (ret)
+ return ret;
+
+ ret = regulator_get_voltage(aincom);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "Device tree error, AINCOM voltage undefined\n");
+ st->aincom_mv = ret / 1000;
+ }
+
st->avdd = devm_regulator_get(&spi->dev, "avdd");
if (IS_ERR(st->avdd))
return PTR_ERR(st->avdd);
--
2.34.1

2024-04-30 16:31:22

by Alisa-Dariana Roman

Subject: [PATCH v7 4/6] dt-bindings: iio: adc: Add single-channel property

Devices that have both single-ended channels and differential channels
cause a bit of confusion when the channels are configured in the
devicetree.

Clarify difference between these two types of channels for such devices
by adding single-channel property alongside diff-channels. They should
be mutually exclusive.

Devices that have only single-ended channels can still use reg property
to reference a channel like before.

Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
Documentation/devicetree/bindings/iio/adc/adc.yaml | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
index 36775f8f71df..0c3eae580732 100644
--- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
@@ -38,6 +38,14 @@ properties:
The first value specifies the positive input pin, the second
specifies the negative input pin.

+ single-channel:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ When devices combine single and differential channels, allow the channel
+ for a single element to be specified, independent of reg (as for
+ differential channels). If this and diff-channels are not present reg
+ shall be used instead.
+
settling-time-us:
description:
Time between enabling the channel and first stable readings.
--
2.34.1

2024-04-30 16:31:52

by Alisa-Dariana Roman

Subject: [PATCH v7 6/6] iio: adc: ad7192: Add AD7194 support

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

Also modify config AD7192 description for better scaling.

Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
drivers/iio/adc/Kconfig | 11 +++-
drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++--
2 files changed, 133 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..74fecc284f1a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -88,12 +88,17 @@ config AD7173
called ad7173.

config AD7192
- tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+ tristate "Analog Devices AD7192 and similar ADC driver"
depends on SPI
select AD_SIGMA_DELTA
help
- Say yes here to build support for Analog Devices AD7190,
- AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+ Say yes here to build support for Analog Devices SPI analog to digital
+ converters (ADC):
+ - AD7190
+ - AD7192
+ - AD7193
+ - AD7194
+ - AD7195
If unsure, say N (but it's safe to say "Y").

To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 3e797ff48086..0f6ecf953559 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7192 and similar SPI ADC driver
*
* Copyright 2011-2015 Analog Devices Inc.
*/
@@ -129,10 +129,21 @@
#define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
#define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */

+#define AD7194_CH_POS(x) (((x) - 1) << 4)
+#define AD7194_CH_NEG(x) ((x) - 1)
+#define AD7194_CH(pos, neg) \
+ (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))
+#define AD7194_CH_TEMP 0x100 /* Temp sensor */
+#define AD7194_CH_BASE_NR 2
+#define AD7194_CH_AIN_START 1
+#define AD7194_CH_AIN_NR 16
+#define AD7194_CH_MAX_NR 272
+
/* ID Register Bit Designations (AD7192_REG_ID) */
#define CHIPID_AD7190 0x4
#define CHIPID_AD7192 0x0
#define CHIPID_AD7193 0x2
+#define CHIPID_AD7194 0x3
#define CHIPID_AD7195 0x6
#define AD7192_ID_MASK GENMASK(3, 0)

@@ -170,6 +181,7 @@ enum {
ID_AD7190,
ID_AD7192,
ID_AD7193,
+ ID_AD7194,
ID_AD7195,
};

@@ -179,6 +191,7 @@ struct ad7192_chip_info {
const struct iio_chan_spec *channels;
u8 num_channels;
const struct iio_info *info;
+ int (*parse_channels)(struct iio_dev *indio_dev);
};

struct ad7192_state {
@@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = {
.update_scan_mode = ad7192_update_scan_mode,
};

+static const struct iio_info ad7194_info = {
+ .read_raw = ad7192_read_raw,
+ .write_raw = ad7192_write_raw,
+ .write_raw_get_fmt = ad7192_write_raw_get_fmt,
+ .read_avail = ad7192_read_avail,
+ .validate_trigger = ad_sd_validate_trigger,
+ .update_scan_mode = ad7192_update_scan_mode,
+};
+
static const struct iio_info ad7195_info = {
.read_raw = ad7192_read_raw,
.write_raw = ad7192_write_raw,
@@ -1023,6 +1045,91 @@ static const struct iio_chan_spec ad7193_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(14),
};

+static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
+{
+ if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid AIN channel: %u\n", ain);
+
+ return 0;
+}
+
+static int ad7194_parse_channels(struct iio_dev *indio_dev)
+{
+ struct device *dev = indio_dev->dev.parent;
+ struct iio_chan_spec *ad7194_channels;
+ struct fwnode_handle *child;
+ struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
+ struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
+ struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
+ struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
+ unsigned int num_channels, index = 0;
+ u32 ain[2];
+ int ret;
+
+ num_channels = device_get_child_node_count(dev);
+ if (num_channels > AD7194_CH_MAX_NR)
+ return dev_err_probe(dev, -EINVAL,
+ "Too many channels: %u\n", num_channels);
+
+ num_channels += AD7194_CH_BASE_NR;
+
+ ad7194_channels = devm_kcalloc(dev, num_channels,
+ sizeof(*ad7194_channels), GFP_KERNEL);
+ if (!ad7194_channels)
+ return -ENOMEM;
+
+ indio_dev->channels = ad7194_channels;
+ indio_dev->num_channels = num_channels;
+
+ device_for_each_child_node(dev, child) {
+ ret = fwnode_property_read_u32_array(child, "diff-channels",
+ ain, ARRAY_SIZE(ain));
+ if (ret == 0) {
+ ret = ad7194_validate_ain_channel(dev, ain[0]);
+ if (ret)
+ return ret;
+
+ ret = ad7194_validate_ain_channel(dev, ain[1]);
+ if (ret)
+ return ret;
+
+ *ad7194_channels = ad7194_chan_diff;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->channel = ain[0];
+ ad7194_channels->channel2 = ain[1];
+ ad7194_channels->address = AD7194_CH(ain[0], ain[1]);
+ } else {
+ ret = fwnode_property_read_u32(child, "single-channel",
+ &ain[0]);
+ if (ret) {
+ fwnode_handle_put(child);
+ return ret;
+ }
+
+ ret = ad7194_validate_ain_channel(dev, ain[0]);
+ if (ret)
+ return ret;
+
+ *ad7194_channels = ad7194_chan;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->channel = ain[0];
+ ad7194_channels->address = AD7194_CH(ain[0], 0);
+ }
+ ad7194_channels++;
+ }
+
+ *ad7194_channels = ad7194_chan_temp;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->address = AD7194_CH_TEMP;
+ ad7194_channels++;
+
+ *ad7194_channels = ad7194_chan_timestamp;
+ ad7194_channels->scan_index = index;
+
+ return 0;
+}
+
static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
[ID_AD7190] = {
.chip_id = CHIPID_AD7190,
@@ -1045,6 +1152,12 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
.num_channels = ARRAY_SIZE(ad7193_channels),
.info = &ad7192_info,
},
+ [ID_AD7194] = {
+ .chip_id = CHIPID_AD7194,
+ .name = "ad7194",
+ .info = &ad7194_info,
+ .parse_channels = ad7194_parse_channels,
+ },
[ID_AD7195] = {
.chip_id = CHIPID_AD7195,
.name = "ad7195",
@@ -1152,9 +1265,15 @@ static int ad7192_probe(struct spi_device *spi)
st->chip_info = spi_get_device_match_data(spi);
indio_dev->name = st->chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->channels = st->chip_info->channels;
- indio_dev->num_channels = st->chip_info->num_channels;
indio_dev->info = st->chip_info->info;
+ if (st->chip_info->parse_channels) {
+ ret = st->chip_info->parse_channels(indio_dev);
+ if (ret)
+ return ret;
+ } else {
+ indio_dev->channels = st->chip_info->channels;
+ indio_dev->num_channels = st->chip_info->num_channels;
+ }

ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
if (ret)
@@ -1193,6 +1312,7 @@ static const struct of_device_id ad7192_of_match[] = {
{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+ { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
{}
};
@@ -1202,6 +1322,7 @@ static const struct spi_device_id ad7192_ids[] = {
{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
+ { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
{}
};
@@ -1218,6 +1339,6 @@ static struct spi_driver ad7192_driver = {
module_spi_driver(ad7192_driver);

MODULE_AUTHOR("Michael Hennerich <[email protected]>");
-MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
+MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
MODULE_LICENSE("GPL v2");
MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
--
2.34.1

2024-04-30 16:47:23

by Alisa-Dariana Roman

Subject: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

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

Also add an example for AD7194 devicetree.

Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
.../bindings/iio/adc/adi,ad7192.yaml | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index cf5c568f140a..a03da9489ed9 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -21,8 +21,15 @@ properties:
- adi,ad7190
- adi,ad7192
- adi,ad7193
+ - adi,ad7194
- adi,ad7195

+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
reg:
maxItems: 1

@@ -89,6 +96,42 @@ properties:
description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
type: boolean

+patternProperties:
+ "^channel@[0-9a-f]+$":
+ type: object
+ $ref: adc.yaml
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description: The channel index.
+ minimum: 0
+ maximum: 271
+
+ diff-channels:
+ description:
+ Both inputs can be connected to pins AIN1 to AIN16 by choosing the
+ appropriate value from 1 to 16.
+ items:
+ minimum: 1
+ maximum: 16
+
+ single-channel:
+ description:
+ Positive input can be connected to pins AIN1 to AIN16 by choosing the
+ appropriate value from 1 to 16. Negative input is connected to AINCOM.
+ items:
+ minimum: 1
+ maximum: 16
+
+ oneOf:
+ - required:
+ - reg
+ - diff-channels
+ - required:
+ - reg
+ - single-channel
+
required:
- compatible
- reg
@@ -103,6 +146,17 @@ required:

allOf:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
+ - if:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7190
+ - adi,ad7192
+ - adi,ad7193
+ - adi,ad7195
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]+$": false

unevaluatedProperties: false

@@ -133,3 +187,38 @@ examples:
adi,burnout-currents-enable;
};
};
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad7194";
+ reg = <0>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ spi-max-frequency = <1000000>;
+ spi-cpol;
+ spi-cpha;
+ clocks = <&ad7192_mclk>;
+ clock-names = "mclk";
+ interrupts = <25 0x2>;
+ interrupt-parent = <&gpio>;
+ aincom-supply = <&aincom>;
+ dvdd-supply = <&dvdd>;
+ avdd-supply = <&avdd>;
+ vref-supply = <&vref>;
+
+ channel@0 {
+ reg = <0>;
+ diff-channels = <1 6>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ single-channel = <1>;
+ };
+ };
+ };
--
2.34.1

2024-04-30 17:15:50

by Conor Dooley

Subject: Re: [PATCH v7 4/6] dt-bindings: iio: adc: Add single-channel property

On Tue, Apr 30, 2024 at 07:29:44PM +0300, Alisa-Dariana Roman wrote:
> Devices that have both single-ended channels and differential channels
> cause a bit of confusion when the channels are configured in the
> devicetree.
>
> Clarify difference between these two types of channels for such devices
> by adding single-channel property alongside diff-channels. They should
> be mutually exclusive.

I think that this mutual exclusion and the requirement for reg in the
absence of either property should be enforced by the binding, not
described in a commit message or free-form text in the property
description.

>
> Devices that have only single-ended channels can still use reg property
> to reference a channel like before.
>
> Suggested-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> Documentation/devicetree/bindings/iio/adc/adc.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
> index 36775f8f71df..0c3eae580732 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
> @@ -38,6 +38,14 @@ properties:
> The first value specifies the positive input pin, the second
> specifies the negative input pin.
>
> + single-channel:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:

> + When devices combine single

s/single/single-ended/, no?

Cheers,
Conor.

> and differential channels, allow the channel
> + for a single element to be specified, independent of reg (as for
> + differential channels). If this and diff-channels are not present reg
> + shall be used instead.
> +
> settling-time-us:
> description:
> Time between enabling the channel and first stable readings.
> --
> 2.34.1
>

Attachments:

2024-04-30 17:19:21

by Conor Dooley

Subject: Re: [PATCH v7 0/6] iio: adc: ad7192: Add AD7194 support

On Tue, Apr 30, 2024 at 07:29:40PM +0300, Alisa-Dariana Roman wrote:
> Dear maintainers,
>
> Thank you all for the feedback!
>
> I am submitting the upgraded series of patches for the ad7192 driver.
>
> Please consider applying in order.
>

Delivery has failed to these recipients or groups:

[email protected]
The email address you entered couldn't be found. Please check the recipient's email address and try to resend the message. If the problem continues, please contact your email admin.

rg [email protected]
MAINTAINERS
1199:M: Alexandru Tachici <[email protected]>

drivers/hwmon/pmbus/adm1266.c
509:MODULE_AUTHOR("Alexandru Tachici <[email protected]>");

drivers/hwmon/ltc2992.c
941:MODULE_AUTHOR("Alexandru Tachici <[email protected]>");

Documentation/hwmon/adm1266.rst
11:Author: Alexandru Tachici <[email protected]>

Documentation/hwmon/ltc2992.rst
11:Author: Alexandru Tachici <[email protected]>

drivers/net/ethernet/adi/adin1110.c
1736:MODULE_AUTHOR("Alexandru Tachici <[email protected]>");

Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
10: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/iio/dac/adi,ad5770r.yaml
11: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/iio/imu/adi,adis16480.yaml
10: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
11: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/hwmon/adi,ltc2992.yaml
10: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/net/adi,adin.yaml
10: - Alexandru Tachici <[email protected]>

Documentation/devicetree/bindings/net/adi,adin1110.yaml
10: - Alexandru Tachici <[email protected]>

You may wanna consider finding someone else in your company to maintain
these devices.

Thanks,
Conor.

Attachments:

2024-04-30 17:21:18

by Conor Dooley

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> + diff-channels:
> + description:
> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> + appropriate value from 1 to 16.
> + items:
> + minimum: 1
> + maximum: 16
> +
> + single-channel:
> + description:
> + Positive input can be connected to pins AIN1 to AIN16 by choosing the
> + appropriate value from 1 to 16. Negative input is connected to AINCOM.
> + items:
> + minimum: 1
> + maximum: 16

Up to 16 differential channels and 16 single-ended channels, but only 16
pins? Would the number of differential channels not max out at 8?

Attachments:

2024-04-30 17:27:53

by Conor Dooley

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> From: Alisa-Dariana Roman <[email protected]>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>

btw, the whole series has a from != signoff email problem. My git
send-email handles that for me without me having to do anything by
inserting a From: header in the patches, like so:
https://lore.kernel.org/all/20240424-tabby-plural-5f1d9fe44f47@spud/

I am not sure what option you're missing to suggest, but it may be as
setting the `--from` arg (or sendemail.from in your gitconfig).

Thanks,
Conor.

Attachments:

2024-04-30 20:29:20

by Andy Shevchenko

Subject: Re: [PATCH v7 3/6] iio: adc: ad7192: Add aincom supply

On Tue, Apr 30, 2024 at 7:30 PM Alisa-Dariana Roman
<[email protected]> wrote:
>
> AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
> in pseudo-differential operation mode. AINCOM voltage represents the
> offset of corresponding channels.

..

> + ret = regulator_enable(aincom);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable specified AINCOM supply\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(aincom);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "Device tree error, AINCOM voltage undefined\n");
> + st->aincom_mv = ret / 1000;

MILLI ?

--
With Best Regards,
Andy Shevchenko

2024-04-30 20:34:05

by Andy Shevchenko

Subject: Re: [PATCH v7 6/6] iio: adc: ad7192: Add AD7194 support

On Tue, Apr 30, 2024 at 7:30 PM Alisa-Dariana Roman
<[email protected]> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.

..

> +static int ad7194_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *ad7194_channels;
> + struct fwnode_handle *child;
> + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
> + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
> + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
> + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
> + unsigned int num_channels, index = 0;
> + u32 ain[2];
> + int ret;
> +
> + num_channels = device_get_child_node_count(dev);
> + if (num_channels > AD7194_CH_MAX_NR)
> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);
> +
> + num_channels += AD7194_CH_BASE_NR;
> +
> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);
> + if (!ad7194_channels)
> + return -ENOMEM;
> +
> + indio_dev->channels = ad7194_channels;
> + indio_dev->num_channels = num_channels;
> +
> + device_for_each_child_node(dev, child) {

You wanted _scoped version...

> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + ain, ARRAY_SIZE(ain));
> + if (ret == 0) {
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)

..otherwise here is the reference count leakage.

> + return ret;
> +
> + ret = ad7194_validate_ain_channel(dev, ain[1]);
> + if (ret)

Or here.

> + return ret;
> +
> + *ad7194_channels = ad7194_chan_diff;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->channel2 = ain[1];
> + ad7194_channels->address = AD7194_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret) {
> + fwnode_handle_put(child);
> + return ret;
> + }
> +
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)

Or here.

> + return ret;
> +
> + *ad7194_channels = ad7194_chan;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->address = AD7194_CH(ain[0], 0);
> + }
> + ad7194_channels++;
> + }
> +
> + *ad7194_channels = ad7194_chan_temp;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->address = AD7194_CH_TEMP;
> + ad7194_channels++;
> +
> + *ad7194_channels = ad7194_chan_timestamp;
> + ad7194_channels->scan_index = index;
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko

2024-05-01 17:01:59

by David Lechner

Subject: Re: [PATCH v7 3/6] iio: adc: ad7192: Add aincom supply

On Tue, Apr 30, 2024 at 11:30 AM Alisa-Dariana Roman
<[email protected]> wrote:
>
> AINCOM should actually be a supply. AINx inputs are referenced to AINCOM
> in pseudo-differential operation mode. AINCOM voltage represents the
> offset of corresponding channels.
>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> drivers/iio/adc/ad7192.c | 41 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index ace81e3817a1..3e797ff48086 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -20,6 +20,7 @@
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/property.h>
> +#include <linux/units.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> @@ -186,6 +187,7 @@ struct ad7192_state {
> struct regulator *vref;
> struct clk *mclk;
> u16 int_vref_mv;
> + u32 aincom_mv;
> u32 fclk;
> u32 mode;
> u32 conf;
> @@ -742,10 +744,19 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
> *val = -(1 << (chan->scan_type.realbits - 1));
> else
> *val = 0;

nit: add blank line before switch for readability

> + switch (chan->type) {
> + case IIO_VOLTAGE:

Comment could be helpful here as a few things might not be obvious:
* Only applies to pseudo-differential inputs (!chan->differential)
* AINCOM voltage has to be converted to "raw" units.

> + if (st->aincom_mv && !chan->differential)
> + *val += DIV_ROUND_CLOSEST_ULL((u64)st->aincom_mv * NANO,
> + st->scale_avail[gain][1]);
> + return IIO_VAL_INT;
> /* Kelvin to Celsius */
> - if (chan->type == IIO_TEMP)
> + case IIO_TEMP:
> *val -= 273 * ad7192_get_temp_scale(unipolar);
> - return IIO_VAL_INT;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> case IIO_CHAN_INFO_SAMP_FREQ:
> *val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
> return IIO_VAL_INT;
> @@ -1052,6 +1063,7 @@ static int ad7192_probe(struct spi_device *spi)
> {
> struct ad7192_state *st;
> struct iio_dev *indio_dev;
> + struct regulator *aincom;
> int ret;
>
> if (!spi->irq) {
> @@ -1067,6 +1079,31 @@ static int ad7192_probe(struct spi_device *spi)
>
> mutex_init(&st->lock);
>
> + /*
> + * Regulator aincom is optional to maintain compatibility with older DT.
> + * Newer firmware should provide a zero volt fixed supply if wired to
> + * ground.
> + */
> + aincom = devm_regulator_get_optional(&spi->dev, "aincom");

Do we need to consider other errors separate from -EINVAL?

I've done something like this in other drivers:

if (IS_ERR(aincom)) {
if (PTR_ERR(aincom) != -EINVAL)
return dev_err_probe(...);

st->aincom_mv = 0;
} else {
...
st->aincom_mv = ...;
}
> + if (IS_ERR(aincom)) {
> + st->aincom_mv = 0;
> + } else {
> + ret = regulator_enable(aincom);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable specified AINCOM supply\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(aincom);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "Device tree error, AINCOM voltage undefined\n");
> + st->aincom_mv = ret / 1000;
> + }
> +
> st->avdd = devm_regulator_get(&spi->dev, "avdd");
> if (IS_ERR(st->avdd))
> return PTR_ERR(st->avdd);
> --
> 2.34.1
>

2024-05-01 17:54:46

by David Lechner

Subject: Re: [PATCH v7 6/6] iio: adc: ad7192: Add AD7194 support

On Tue, Apr 30, 2024 at 11:30 AM Alisa-Dariana Roman
<[email protected]> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> drivers/iio/adc/Kconfig | 11 +++-
> drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 133 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..74fecc284f1a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -88,12 +88,17 @@ config AD7173
> called ad7173.
>
> config AD7192
> - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> + tristate "Analog Devices AD7192 and similar ADC driver"
> depends on SPI
> select AD_SIGMA_DELTA
> help
> - Say yes here to build support for Analog Devices AD7190,
> - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> + Say yes here to build support for Analog Devices SPI analog to digital
> + converters (ADC):
> + - AD7190
> + - AD7192
> + - AD7193
> + - AD7194
> + - AD7195
> If unsure, say N (but it's safe to say "Y").
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 3e797ff48086..0f6ecf953559 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7192 and similar SPI ADC driver
> *
> * Copyright 2011-2015 Analog Devices Inc.
> */
> @@ -129,10 +129,21 @@
> #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
> #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
>
> +#define AD7194_CH_POS(x) (((x) - 1) << 4)
> +#define AD7194_CH_NEG(x) ((x) - 1)
> +#define AD7194_CH(pos, neg) \
> + (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))

I think this needs a comment that BIT(10) is the CON18 flag for
pseudo-differential channels.

Also, it would probably be easier to understand if there were two
different macros, one for "single-channel" and one for "diff-channels"
rather than having neg==0 magically turn in to the pseudo flag.

> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
> +#define AD7194_CH_BASE_NR 2
> +#define AD7194_CH_AIN_START 1
> +#define AD7194_CH_AIN_NR 16
> +#define AD7194_CH_MAX_NR 272
> +
> /* ID Register Bit Designations (AD7192_REG_ID) */
> #define CHIPID_AD7190 0x4
> #define CHIPID_AD7192 0x0
> #define CHIPID_AD7193 0x2
> +#define CHIPID_AD7194 0x3
> #define CHIPID_AD7195 0x6
> #define AD7192_ID_MASK GENMASK(3, 0)
>
> @@ -170,6 +181,7 @@ enum {
> ID_AD7190,
> ID_AD7192,
> ID_AD7193,
> + ID_AD7194,
> ID_AD7195,
> };
>
> @@ -179,6 +191,7 @@ struct ad7192_chip_info {
> const struct iio_chan_spec *channels;
> u8 num_channels;
> const struct iio_info *info;
> + int (*parse_channels)(struct iio_dev *indio_dev);
> };
>
> struct ad7192_state {
> @@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = {
> .update_scan_mode = ad7192_update_scan_mode,
> };
>
> +static const struct iio_info ad7194_info = {
> + .read_raw = ad7192_read_raw,
> + .write_raw = ad7192_write_raw,
> + .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> + .read_avail = ad7192_read_avail,
> + .validate_trigger = ad_sd_validate_trigger,
> + .update_scan_mode = ad7192_update_scan_mode,

It looks like ad7192_update_scan_mode() won't work on ad7194 since the
channels are specified via DT for this chip and could be anything.
Each scan_index no longer corresponds a specific bit in
AD7192_REG_CONF so it would end up with some random configuration.

> +};
> +

The rest looks fine other than what Andy mentioned already.

2024-05-05 19:46:39

by Jonathan Cameron

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Tue, 30 Apr 2024 18:21:01 +0100
Conor Dooley <[email protected]> wrote:

> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> > + diff-channels:
> > + description:
> > + Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> > + appropriate value from 1 to 16.
> > + items:
> > + minimum: 1
> > + maximum: 16
> > +
> > + single-channel:
> > + description:
> > + Positive input can be connected to pins AIN1 to AIN16 by choosing the
> > + appropriate value from 1 to 16. Negative input is connected to AINCOM.
> > + items:
> > + minimum: 1
> > + maximum: 16
>
> Up to 16 differential channels and 16 single-ended channels, but only 16
> pins? Would the number of differential channels not max out at 8?

May not really be limited to 16 differential. Many chips use general purpose
muxes on both sides so you can do all combinations. In practice that's normally
pointless.

A more useful case is to do all but one channel as positive inputs and the remaining
channel as the negative for those 15 differential channels.
This is effectively the same as doing pseudo differential channels, but
on more flexible hardware. This is in contrast to a device that only supports
pseudo differential where there is a special pin for the negative
(this device has that as well as full muxes on the other 16 lines).

Having said all that. The ad7194 datasheet says 8 differential channels..
I have no idea why though... Maybe something to do with the mux switching?
Or maybe assumption is that if you want to do pseudo differential you'll use
the pseudo differential mode rather than wasting hardware?

Jonathan

2024-05-06 14:08:05

by Jonathan Cameron

Subject: Re: [PATCH v7 6/6] iio: adc: ad7192: Add AD7194 support

On Tue, 30 Apr 2024 19:29:46 +0300
Alisa-Dariana Roman <[email protected]> wrote:

> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>

Hi Alisa-Dariana,

I only had one minor thing to add to the other reviews,

Thanks,

Jonathan

> To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 3e797ff48086..0f6ecf953559 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
..

> +static int ad7194_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *ad7194_channels;
> + struct fwnode_handle *child;
> + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);

I think you are only using these as templates that get copied?
If so declare them const.

> + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
> + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
> + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
> + unsigned int num_channels, index = 0;
> + u32 ain[2];
> + int ret;
> +
> + num_channels = device_get_child_node_count(dev);
> + if (num_channels > AD7194_CH_MAX_NR)
> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);
> +
> + num_channels += AD7194_CH_BASE_NR;
> +
> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);
> + if (!ad7194_channels)
> + return -ENOMEM;
> +
> + indio_dev->channels = ad7194_channels;
> + indio_dev->num_channels = num_channels;
> +
> + device_for_each_child_node(dev, child) {

Andy pointed out the need for scoped here to avoid leaking the fwnode in error paths.

> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + ain, ARRAY_SIZE(ain));
> + if (ret == 0) {
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + ret = ad7194_validate_ain_channel(dev, ain[1]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan_diff;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->channel2 = ain[1];
> + ad7194_channels->address = AD7194_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret) {
> + fwnode_handle_put(child);

With scoped variant above you can drop this one case of (currently correct)
release of the fwnode.

> + return ret;
> + }
> +
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->address = AD7194_CH(ain[0], 0);
> + }
> + ad7194_channels++;
> + }
> +
> + *ad7194_channels = ad7194_chan_temp;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->address = AD7194_CH_TEMP;
> + ad7194_channels++;
> +
> + *ad7194_channels = ad7194_chan_timestamp;
> + ad7194_channels->scan_index = index;
> +
> + return 0;
> +}

2024-05-06 15:54:34

by Conor Dooley

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Sun, May 05, 2024 at 08:46:02PM +0100, Jonathan Cameron wrote:
> On Tue, 30 Apr 2024 18:21:01 +0100
> Conor Dooley <[email protected]> wrote:
>
> > On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> > > + diff-channels:
> > > + description:
> > > + Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> > > + appropriate value from 1 to 16.
> > > + items:
> > > + minimum: 1
> > > + maximum: 16
> > > +
> > > + single-channel:
> > > + description:
> > > + Positive input can be connected to pins AIN1 to AIN16 by choosing the
> > > + appropriate value from 1 to 16. Negative input is connected to AINCOM.
> > > + items:
> > > + minimum: 1
> > > + maximum: 16
> >
> > Up to 16 differential channels and 16 single-ended channels, but only 16
> > pins? Would the number of differential channels not max out at 8?
>
> May not really be limited to 16 differential. Many chips use general purpose
> muxes on both sides so you can do all combinations. In practice that's normally
> pointless.
>
> A more useful case is to do all but one channel as positive inputs and the remaining
> channel as the negative for those 15 differential channels.

Yah, 15 is what I had in my head as the highest reasonable number given
the information given about the AIN# pins.

> This is effectively the same as doing pseudo differential channels, but
> on more flexible hardware. This is in contrast to a device that only supports
> pseudo differential where there is a special pin for the negative
> (this device has that as well as full muxes on the other 16 lines).
>
> Having said all that. The ad7194 datasheet says 8 differential channels..
> I have no idea why though... Maybe something to do with the mux switching?
> Or maybe assumption is that if you want to do pseudo differential you'll use
> the pseudo differential mode rather than wasting hardware?

I didn't look at the datasheet tbf, I was just asking given the
description didn't make sense to me and looking for an explanation from
the author.

Attachments:

2024-05-10 10:05:48

by Alisa-Dariana Roman

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On 30.04.2024 20:21, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
>> + diff-channels:
>> + description:
>> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the
>> + appropriate value from 1 to 16.
>> + items:
>> + minimum: 1
>> + maximum: 16
>> +
>> + single-channel:
>> + description:
>> + Positive input can be connected to pins AIN1 to AIN16 by choosing the
>> + appropriate value from 1 to 16. Negative input is connected to AINCOM.
>> + items:
>> + minimum: 1
>> + maximum: 16
>
> Up to 16 differential channels and 16 single-ended channels, but only 16
> pins? Would the number of differential channels not max out at 8?

Hello, Conor! I really appreciate the feedback!

The way I thought about it, the only thing constraining the number of
channels is the reg number (minimum: 0, maximum: 271). 272 channels
cover all possible combinations (16*16 differential and 16 single ended)
and I thought there is no need for anything stricter. I added items:
minimum:1 maximum:16 to make sure the numbers are from 1 to 16,
corresponding to AIN1-AIN16.

Please let me know what should be improved!

Kind regards,
Alisa-Dariana Roman.

2024-05-10 14:22:37

by David Lechner

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote:
> On 30.04.2024 20:21, Conor Dooley wrote:
>> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
>>> +      diff-channels:
>>> +        description:
>>> +          Both inputs can be connected to pins AIN1 to AIN16 by choosing the
>>> +          appropriate value from 1 to 16.
>>> +        items:
>>> +          minimum: 1
>>> +          maximum: 16
>>> +
>>> +      single-channel:
>>> +        description:
>>> +          Positive input can be connected to pins AIN1 to AIN16 by choosing the
>>> +          appropriate value from 1 to 16. Negative input is connected to AINCOM.
>>> +        items:
>>> +          minimum: 1
>>> +          maximum: 16
>>
>> Up to 16 differential channels and 16 single-ended channels, but only 16
>> pins? Would the number of differential channels not max out at 8?
>
> Hello, Conor! I really appreciate the feedback!
>
> The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16.
>
> Please let me know what should be improved!
>
> Kind regards,
> Alisa-Dariana Roman.
>

Having looked at the datasheet for this and other similar chips, I agree
that this reasoning makes sense. Some of the similar chips that have fixed
channel assignments still have, e.g. a channel where + and - are both
AIN2 (I assume for diagnostics). So I think it makes sense to allow for
doing something similar here even if the most common use cases will
probably have at most 16 channels defined in the .dts.

2024-05-10 21:26:38

by Conor Dooley

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Fri, May 10, 2024 at 09:21:37AM -0500, David Lechner wrote:
> On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote:
> > On 30.04.2024 20:21, Conor Dooley wrote:
> >> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> >>> +????? diff-channels:
> >>> +??????? description:
> >>> +????????? Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> >>> +????????? appropriate value from 1 to 16.
> >>> +??????? items:
> >>> +????????? minimum: 1
> >>> +????????? maximum: 16
> >>> +
> >>> +????? single-channel:
> >>> +??????? description:
> >>> +????????? Positive input can be connected to pins AIN1 to AIN16 by choosing the
> >>> +????????? appropriate value from 1 to 16. Negative input is connected to AINCOM.
> >>> +??????? items:
> >>> +????????? minimum: 1
> >>> +????????? maximum: 16
> >>
> >> Up to 16 differential channels and 16 single-ended channels, but only 16
> >> pins? Would the number of differential channels not max out at 8?
> >
> > Hello, Conor! I really appreciate the feedback!
> >
> > The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16.
> >
> > Please let me know what should be improved!
> >
> > Kind regards,
> > Alisa-Dariana Roman.
> >
>
> Having looked at the datasheet for this and other similar chips, I agree
> that this reasoning makes sense. Some of the similar chips that have fixed
> channel assignments still have, e.g. a channel where + and - are both
> AIN2 (I assume for diagnostics). So I think it makes sense to allow for
> doing something similar here even if the most common use cases will
> probably have at most 16 channels defined in the .dts.

Actually, I think there were a bunch of whiffs on this one by either
misreading the property in question (me) or not realising that I had done
that and trying to explain what the possible combinations are.
Looking at it now, I dunno wtf I was smoking because there's no way that
this would be a functional binding if the min/max in the quote above
constraining the number of channels. I can hardly blame y'all for that
though, I am supposed to know how bindings work after all...

Attachments:

2024-05-11 11:30:19

by Jonathan Cameron

Subject: Re: [PATCH v7 5/6] dt-bindings: iio: adc: ad7192: Add AD7194 support

On Fri, 10 May 2024 22:26:22 +0100
Conor Dooley <[email protected]> wrote:

> On Fri, May 10, 2024 at 09:21:37AM -0500, David Lechner wrote:
> > On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote:
> > > On 30.04.2024 20:21, Conor Dooley wrote:
> > >> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote:
> > >>> +      diff-channels:
> > >>> +        description:
> > >>> +          Both inputs can be connected to pins AIN1 to AIN16 by choosing the
> > >>> +          appropriate value from 1 to 16.
> > >>> +        items:
> > >>> +          minimum: 1
> > >>> +          maximum: 16
> > >>> +
> > >>> +      single-channel:
> > >>> +        description:
> > >>> +          Positive input can be connected to pins AIN1 to AIN16 by choosing the
> > >>> +          appropriate value from 1 to 16. Negative input is connected to AINCOM.
> > >>> +        items:
> > >>> +          minimum: 1
> > >>> +          maximum: 16
> > >>
> > >> Up to 16 differential channels and 16 single-ended channels, but only 16
> > >> pins? Would the number of differential channels not max out at 8?
> > >
> > > Hello, Conor! I really appreciate the feedback!
> > >
> > > The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16.
> > >
> > > Please let me know what should be improved!
> > >
> > > Kind regards,
> > > Alisa-Dariana Roman.
> > >
> >
> > Having looked at the datasheet for this and other similar chips, I agree
> > that this reasoning makes sense. Some of the similar chips that have fixed
> > channel assignments still have, e.g. a channel where + and - are both
> > AIN2 (I assume for diagnostics). So I think it makes sense to allow for
> > doing something similar here even if the most common use cases will
> > probably have at most 16 channels defined in the .dts.
>
> Actually, I think there were a bunch of whiffs on this one by either
> misreading the property in question (me) or not realising that I had done
> that and trying to explain what the possible combinations are.
> Looking at it now, I dunno wtf I was smoking because there's no way that
> this would be a functional binding if the min/max in the quote above
> constraining the number of channels. I can hardly blame y'all for that
> though, I am supposed to know how bindings work after all...

Me too :( I also failed to register this doesn't constrain
channel counts at all.

Anyhow, all's well that ends well!

J