LinuxLists.cc - [PATCH v8 0/6] iio: adc: ad7192: Add AD7194 support

[permalink] [raw]

Subject: [PATCH v8 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-05-14 12:04:06

[permalink] [raw]

Subject: [PATCH v8 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 | 50 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index ace81e3817a1..7160929d32c9 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,24 @@ 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:
+ /*
+ * Only applies to pseudo-differential inputs.
+ * 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 +1068,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 +1084,35 @@ 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)) {
+ if (PTR_ERR(aincom) != -ENODEV)
+ return dev_err_probe(&spi->dev, PTR_ERR(aincom),
+ "Failed to get AINCOM supply\n");
+
+ 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 / MILLI;
+ }
+
st->avdd = devm_regulator_get(&spi->dev, "avdd");
if (IS_ERR(st->avdd))
return PTR_ERR(st->avdd);
--
2.34.1

2024-05-14 12:04:18

[permalink] [raw]

Subject: [PATCH v8 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]>
---
.../devicetree/bindings/iio/adc/adc.yaml | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
index 36775f8f71df..0a77592f7388 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-ended 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.
@@ -50,4 +58,15 @@ properties:
device design and can interact with other characteristics such as
settling time.

+anyOf:
+ - oneOf:
+ - required:
+ - reg
+ - diff-channels
+ - required:
+ - reg
+ - single-channel
+ - required:
+ - reg
+
additionalProperties: true
--
2.34.1

2024-05-14 12:04:50

[permalink] [raw]

Subject: [PATCH v8 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.

Add sigma_delta_info member to chip_info structure. Since AD7194 is the
only chip that has no channel sequencer, num_slots should remain
undefined.

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 | 147 +++++++++++++++++++++++++++++++++++++--
2 files changed, 150 insertions(+), 8 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 7160929d32c9..fe2d8d55fa76 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,22 @@
#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(p) (BIT(10) | AD7194_CH_POS(p))
+ /* 10th bit corresponds to CON18(Pseudo) */
+#define AD7194_DIFF_CH(p, n) (AD7194_CH_POS(p) | AD7194_CH_NEG(n))
+#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 +182,7 @@ enum {
ID_AD7190,
ID_AD7192,
ID_AD7193,
+ ID_AD7194,
ID_AD7195,
};

@@ -178,7 +191,9 @@ struct ad7192_chip_info {
const char *name;
const struct iio_chan_spec *channels;
u8 num_channels;
+ const struct ad_sigma_delta_info *sigma_delta_info;
const struct iio_info *info;
+ int (*parse_channels)(struct iio_dev *indio_dev);
};

struct ad7192_state {
@@ -346,6 +361,18 @@ static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
.irq_flags = IRQF_TRIGGER_FALLING,
};

+static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
+ .set_channel = ad7192_set_channel,
+ .append_status = ad7192_append_status,
+ .disable_all = ad7192_disable_all,
+ .set_mode = ad7192_set_mode,
+ .has_registers = true,
+ .addr_shift = 3,
+ .read_mask = BIT(6),
+ .status_ch_mask = GENMASK(3, 0),
+ .irq_flags = IRQF_TRIGGER_FALLING,
+};
+
static const struct ad_sd_calib_data ad7192_calib_arr[8] = {
{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN1},
{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN1},
@@ -937,6 +964,14 @@ 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,
+};
+
static const struct iio_info ad7195_info = {
.read_raw = ad7192_read_raw,
.write_raw = ad7192_write_raw,
@@ -1028,12 +1063,96 @@ 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;
+ const struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
+ const struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
+ const struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
+ const 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_scoped(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_DIFF_CH(ain[0], ain[1]);
+ } else {
+ ret = fwnode_property_read_u32(child, "single-channel",
+ &ain[0]);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Missing channel property\n");
+
+ 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]);
+ }
+ 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,
.name = "ad7190",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
[ID_AD7192] = {
@@ -1041,6 +1160,7 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
.name = "ad7192",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
[ID_AD7193] = {
@@ -1048,13 +1168,22 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
.name = "ad7193",
.channels = ad7193_channels,
.num_channels = ARRAY_SIZE(ad7193_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
+ [ID_AD7194] = {
+ .chip_id = CHIPID_AD7194,
+ .name = "ad7194",
+ .info = &ad7194_info,
+ .sigma_delta_info = &ad7194_sigma_delta_info,
+ .parse_channels = ad7194_parse_channels,
+ },
[ID_AD7195] = {
.chip_id = CHIPID_AD7195,
.name = "ad7195",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7195_info,
},
};
@@ -1161,11 +1290,17 @@ 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);
+ ret = ad_sd_init(&st->sd, indio_dev, spi, st->chip_info->sigma_delta_info);
if (ret)
return ret;

@@ -1202,6 +1337,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] },
{}
};
@@ -1211,6 +1347,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] },
{}
};
@@ -1227,6 +1364,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-05-14 12:05:30

[permalink] [raw]

Subject: [PATCH v8 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-05-14 12:06:54

[permalink] [raw]

Subject: [PATCH v8 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-05-14 13:10:57

[permalink] [raw]

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

On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> only chip that has no channel sequencer, num_slots should remain
> undefined.
>
> Also modify config AD7192 description for better scaling.

Some non-critical, mostly style related comments below.

..

This...

> +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> + /* 10th bit corresponds to CON18(Pseudo) */

..should be (you have broken indentation on the comment, btw):

/* 10th bit corresponds to CON18(Pseudo) */
#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))

But no need to resend because of this, let's wait others to comment, and
if everything fine I think Jonathan can massage this when applying.

..

> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */

Not sure that the comment has any value here.

..

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

While this uses traditional pattern, it might be better looking in a form of

if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
return 0;

return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);

But at the same time I would rather expect this to be in the caller and here
to have a boolean function

static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
{
return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
}

> +}

..

> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);

return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);

?

Or with limit

return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
num_channels);

..

> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);

ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);

?

Or

ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
GFP_KERNEL);

?

..

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

And here I would rather go for the traditional pattern, i.e.

if (ret) {
...
} else {
...
}

> + 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_DIFF_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Missing channel property\n");
> +
> + 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]);
> + }
> + ad7194_channels++;
> + }

--
With Best Regards,
Andy Shevchenko

2024-05-14 13:13:36

[permalink] [raw]

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

On Tue, May 14, 2024 at 03:02:19PM +0300, Alisa-Dariana Roman 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.

..

Possible cleanup with the help of

struct device *dev = &spi->dev;

> struct ad7192_state *st;
> struct iio_dev *indio_dev;
> + struct regulator *aincom;
> int ret;

..

> + aincom = devm_regulator_get_optional(&spi->dev, "aincom");

aincom = devm_regulator_get_optional(dev, "aincom");

..

> + return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> + "Failed to get AINCOM supply\n");

return dev_err_probe(dev, PTR_ERR(aincom),
"Failed to get AINCOM supply\n");

..

> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable specified AINCOM supply\n");

return dev_err_probe(dev, ret,
"Failed to enable specified AINCOM supply\n");

..

> + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);

ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);

..

> + return dev_err_probe(&spi->dev, ret,
> + "Device tree error, AINCOM voltage undefined\n");

return dev_err_probe(dev, ret,
"Device tree error, AINCOM voltage undefined\n");

--
With Best Regards,
Andy Shevchenko

2024-05-14 13:22:17

[permalink] [raw]

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

On Tue, May 14, 2024 at 03:02:16PM +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.

Jonathan, LGTM, I have left a few non-critical comments, they may be fixed
when applying or dropped (most of them), depending on your preference.

--
With Best Regards,
Andy Shevchenko

2024-05-14 18:57:35

by Conor Dooley

[permalink] [raw]

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

On Tue, May 14, 2024 at 03:02:20PM +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.
>
> 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]>

Reviewed-by: Conor Dooley <[email protected]>

Cheers,
Conor.

Attachments:

(No filename) (721.00 B)
signature.asc (235.00 B)
Download all attachments

2024-05-14 18:58:51

by Conor Dooley

[permalink] [raw]

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

On Tue, May 14, 2024 at 03:02:21PM +0300, Alisa-Dariana Roman wrote:
> 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]>

Reviewed-by: Conor Dooley <[email protected]>

Cheers,
Conor.

Attachments:

(No filename) (389.00 B)
signature.asc (235.00 B)
Download all attachments

2024-05-19 17:46:09

[permalink] [raw]

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

On Tue, 14 May 2024 15:02:17 +0300
Alisa-Dariana Roman <[email protected]> wrote:

> Replace custom attribute filter_low_pass_3db_frequency_available with
> standard attribute.
>
> Store the available values in ad7192_state struct.
>
> The function that used to compute those values replaced by
> ad7192_update_filter_freq_avail().
>
> Function ad7192_show_filter_avail() is no longer needed.
>
> Note that the initial available values are hardcoded.
>
> 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]>
I'm going to work my way through these, picking things up until I
hit anything that needs non trivial changes (hopefully I won't!)

Applied

2024-05-19 17:54:11

[permalink] [raw]

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

On Tue, 14 May 2024 16:13:05 +0300
Andy Shevchenko <[email protected]> wrote:

> On Tue, May 14, 2024 at 03:02:19PM +0300, Alisa-Dariana Roman 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.
>
> ...
>
> Possible cleanup with the help of
>
> struct device *dev = &spi->dev;
This is a good thing to have as a follow up as it applies much more widely
than what is visible in this patch. In ideal world it would have been
a precursor to this series, but I don't want to delay this for a v9 just
to add that.

Hence I'm not going to tweak this whilst applying.

Patch applied as is.

Thanks,

Jonathan

>
>
> > struct ad7192_state *st;
> > struct iio_dev *indio_dev;
> > + struct regulator *aincom;
> > int ret;
>
> ...
>
> > + aincom = devm_regulator_get_optional(&spi->dev, "aincom");
>
> aincom = devm_regulator_get_optional(dev, "aincom");
>
> ...
>
> > + return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> > + "Failed to get AINCOM supply\n");
>
> return dev_err_probe(dev, PTR_ERR(aincom),
> "Failed to get AINCOM supply\n");
>
> ...
>
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to enable specified AINCOM supply\n");
>
> return dev_err_probe(dev, ret,
> "Failed to enable specified AINCOM supply\n");
>
> ...
>
> > + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
>
> ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
>
> ...
>
> > + return dev_err_probe(&spi->dev, ret,
> > + "Device tree error, AINCOM voltage undefined\n");
>
> return dev_err_probe(dev, ret,
> "Device tree error, AINCOM voltage undefined\n");
>

2024-05-19 17:54:33

[permalink] [raw]

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

On Tue, 14 May 2024 15:02:18 +0300
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.
>
> Reviewed-by: Rob Herring (Arm) <[email protected]>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>

Whilst I'm not sure we've totally bottomed out on how to describe
all combinations of pseudo-differential this seem reasonable for this
particular device and harmless against any proposals we have had so far.

Hence applied.

Jonathan
> ---
> 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>;

2024-05-19 18:03:29

[permalink] [raw]

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

On Tue, 14 May 2024 16:09:32 +0300
Andy Shevchenko <[email protected]> wrote:

> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> > Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> > dynamically configure them in the devicetree.
> >
> > Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> > only chip that has no channel sequencer, num_slots should remain
> > undefined.
> >
> > Also modify config AD7192 description for better scaling.
>
> Some non-critical, mostly style related comments below.
>
Tweaked a bit. And applied. Please check the result in the testing branch
of iio.git.
> ...
>
> This...
>
> > +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> > + /* 10th bit corresponds to CON18(Pseudo) */
>
> ...should be (you have broken indentation on the comment, btw):
>
> /* 10th bit corresponds to CON18(Pseudo) */
> #define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
>
> But no need to resend because of this, let's wait others to comment, and
> if everything fine I think Jonathan can massage this when applying.

Fixed.

>
> ...
>
> > +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
>
> Not sure that the comment has any value here.
Dropped

>
> ...
>
> > +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;
>
> While this uses traditional pattern, it might be better looking in a form of
>
> if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> return 0;
>
> return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);
>
> But at the same time I would rather expect this to be in the caller and here
> to have a boolean function
>
Moved it.

> static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
> {
> return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> }
>
> > +}
>
> ...
>
> > + return dev_err_probe(dev, -EINVAL,
> > + "Too many channels: %u\n", num_channels);
>
> return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);
>
> ?
>
> Or with limit
>
> return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> num_channels);
>
>
This one.
> ...
>
> > + ad7194_channels = devm_kcalloc(dev, num_channels,
> > + sizeof(*ad7194_channels), GFP_KERNEL);
>
> ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);
>
> ?
>
> Or
>
> ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
> GFP_KERNEL);
Nope. too long in either case.

>
> ?
>
> ...
>
> > + device_for_each_child_node_scoped(dev, child) {
> > + ret = fwnode_property_read_u32_array(child, "diff-channels",
> > + ain, ARRAY_SIZE(ain));
> > + if (ret == 0) {
>
> And here I would rather go for the traditional pattern, i.e.
>
> if (ret) {
> ...
> } else {
> ...
> }
It's odd, as it's two good paths I've left this one alone.

>
> > + 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_DIFF_CH(ain[0], ain[1]);
> > + } else {
> > + ret = fwnode_property_read_u32(child, "single-channel",
> > + &ain[0]);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Missing channel property\n");
> > +
> > + 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]);
> > + }
> > + ad7194_channels++;
> > + }
>

2024-05-19 19:39:02

[permalink] [raw]

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

On 19.05.2024 21:03, Jonathan Cameron wrote:
> On Tue, 14 May 2024 16:09:32 +0300
> Andy Shevchenko <[email protected]> wrote:
>
>> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
>>> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
>>> dynamically configure them in the devicetree.
>>>
>>> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
>>> only chip that has no channel sequencer, num_slots should remain
>>> undefined.
>>>
>>> Also modify config AD7192 description for better scaling.
>>
>> Some non-critical, mostly style related comments below.
>>
> Tweaked a bit. And applied. Please check the result in the testing branch
> of iio.git.

Thank you guys for the feedback and for making the adjustments!

+/* 10th bit corresponds to CON18(Pseudo) */
+#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
+
I noticed this comment got away in the testing branch.

+static bool ad7194_validate_ain_channel(struct device *dev, u32 ain)
+{
+ return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
+}
And the negation got lost here.

With these little changes, tested on board to make sure, running perfectly!

Kind regards,
Alisa-Dariana Roman.

2024-05-20 12:28:38

[permalink] [raw]

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

On Mon, May 20, 2024 at 01:20:30PM +0100, Jonathan Cameron wrote:
> On Sun, 19 May 2024 22:37:58 +0300
> Alisa-Dariana Roman <[email protected]> wrote:
> > On 19.05.2024 21:03, Jonathan Cameron wrote:

..

> > +static bool ad7194_validate_ain_channel(struct device *dev, u32 ain)
> > +{
> > + return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> > +}
> > And the negation got lost here.
>
> Ouch :(

And negation most likely should be on the caller's side.

--
With Best Regards,
Andy Shevchenko

2024-05-22 09:50:45

[permalink] [raw]

Subject: [PATCH] fix

---

Would this fix be alright, since writing something like if(!ret) may be
confusing?

And regarding the comment, my bad, there is nothing wrong there.

drivers/iio/adc/ad7192.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 101afce49378..0789121236d6 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1101,14 +1101,12 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
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)
+ if (!ad7194_validate_ain_channel(dev, ain[0]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[0]);

- ret = ad7194_validate_ain_channel(dev, ain[1]);
- if (ret)
+ if (!ad7194_validate_ain_channel(dev, ain[1]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[1]);
@@ -1125,8 +1123,7 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
return dev_err_probe(dev, ret,
"Missing channel property\n");

- ret = ad7194_validate_ain_channel(dev, ain[0]);
- if (ret)
+ if (!ad7194_validate_ain_channel(dev, ain[0]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[0]);
--
2.34.1

2024-05-25 16:35:59