[PATCH v2 0/2] Add support for AD7191

[PATCH v2 0/2] Add support for AD7191

[Alisa-Dariana Roman]

Dear maintainers,

Thank you for your feedback! Here is the updated series of patches for
adding AD7191 support.

Kind regards,
Alisa-Dariana Roman.

---

v1: https://lore.kernel.org/all/20241221155926.81954-1-alisa.roman@analog.com/

v1 -> v2:
	- removed patch adding function in ad_sigma_delta.h/.c
	- added a function set_cs() for asserting/deasserting the cs
	- handle pinstrapping cases
	- refactored all clock handling
	- updated bindings: corrected and added new things
	- -> address of the channels is used in set_channel()
	- addressed all the other changes

[PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

[Alisa-Dariana Roman]

AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
designed for precision bridge sensor measurements. It features two
differential analog input channels, selectable output rates,
programmable gain, internal temperature sensor and simultaneous
50Hz/60Hz rejection.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../bindings/iio/adc/adi,ad7191.yaml          | 175 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 182 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
new file mode 100644
index 000000000000..c0a6bed7a9cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
@@ -0,0 +1,175 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2025 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7191.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7191 ADC device driver
+
+maintainers:
+  - Alisa-Dariana Roman <alisa.roman@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD7191 ADC device. Datasheet can be
+  found here:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7191.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7191
+
+  reg:
+    maxItems: 1
+
+  spi-cpol: true
+
+  spi-cpha: true
+
+  clocks:
+    maxItems: 1
+    description:
+      Optionally, either a crystal can be attached externally between MCLK1 and
+      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
+      pin. If absent, internal 4.92MHz clock is used.
+
+  interrupts:
+    maxItems: 1
+
+  avdd-supply:
+    description: AVdd voltage supply
+
+  dvdd-supply:
+    description: DVdd voltage supply
+
+  vref-supply:
+    description: Vref voltage supply
+
+  odr-gpios:
+    description: |
+      ODR1 and ODR2 pins for output data rate selection. Should be defined if
+      adi,odr-state is absent.
+    maxItems: 2
+
+  adi,odr-state:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Should be present if ODR pins are pin-strapped. Value corresponds to:
+      0: 120 Hz (ODR1=0, ODR2=0)
+      1: 60 Hz (ODR1=0, ODR2=1)
+      2: 50 Hz (ODR1=1, ODR2=0)
+      3: 10 Hz (ODR1=1, ODR2=1)
+      If defined, odr-gpios must be absent.
+    enum: [0, 1, 2, 3]
+
+  pga-gpios:
+    description: |
+      PGA1 and PGA2 pins for gain selection. Should be defined if adi,pga-state
+      is absent.
+    maxItems: 2
+
+  adi,pga-state:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Should be present if PGA pins are pin-strapped. Value corresponds to:
+      0: Gain 1 (PGA1=0, PGA2=0)
+      1: Gain 8 (PGA1=0, PGA2=1)
+      2: Gain 64 (PGA1=1, PGA2=0)
+      3: Gain 128 (PGA1=1, PGA2=1)
+      If defined, pga-gpios must be absent.
+    enum: [0, 1, 2, 3]
+
+  temp-gpios:
+    description: TEMP pin for temperature sensor enable.
+    maxItems: 1
+
+  chan-gpios:
+    description: CHAN pin for input channel selection.
+    maxItems: 1
+
+  clksel-gpios:
+    description: |
+      Clock source selection pin (internal or external). Should be defined if
+      clksel-config is absent.
+    maxItems: 1
+
+  adi,clksel-state:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Should be present if CLKSEL is pin-strapped. 0 selects an external clock,
+      1 selects the internal clock. If defined, clksel-gpios must be absent.
+    enum: [0, 1]
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - avdd-supply
+  - dvdd-supply
+  - vref-supply
+  - spi-cpol
+  - spi-cpha
+  - temp-gpios
+  - chan-gpios
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      required:
+        - adi,odr-state
+    then:
+      properties:
+        odr-gpios: false
+    else:
+      required:
+        - odr-gpios
+  - if:
+      required:
+        - adi,pga-state
+    then:
+      properties:
+        pga-gpios: false
+    else:
+      required:
+        - pga-gpios
+  - if:
+      required:
+        - adi,clksel-state
+    then:
+      properties:
+        clksel-gpios: false
+    else:
+      required:
+        - clksel-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad7191";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+            spi-cpha;
+            clocks = <&ad7191_mclk>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-parent = <&gpio>;
+            avdd-supply = <&avdd>;
+            dvdd-supply = <&dvdd>;
+            vref-supply = <&vref>;
+            odr-gpios = <&gpio 23 GPIO_ACTIVE_HIGH>, <&gpio 24 GPIO_ACTIVE_HIGH>;
+            pga-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>, <&gpio 6 GPIO_ACTIVE_HIGH>;
+            temp-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
+            chan-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
+            clksel-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 98a3c1e46311..262beced3143 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1302,6 +1302,13 @@ W:	http://ez.analog.com/community/linux-device-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r*
 F:	drivers/iio/adc/ad7091r*
 
+ANALOG DEVICES INC AD7191 DRIVER
+M:	Alisa-Dariana Roman <alisa.roman@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
+
 ANALOG DEVICES INC AD7192 DRIVER
 M:	Alisa-Dariana Roman <alisa.roman@analog.com>
 L:	linux-iio@vger.kernel.org
-- 
2.43.0

[PATCH v2 2/2] iio: adc: ad7191: add AD7191

[Alisa-Dariana Roman]

AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
designed for precision bridge sensor measurements. It features two
differential analog input channels, selectable output rates,
programmable gain, internal temperature sensor and simultaneous
50Hz/60Hz rejection.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  10 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7191.c | 570 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 582 insertions(+)
 create mode 100644 drivers/iio/adc/ad7191.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 262beced3143..1340c27d9e5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1308,6 +1308,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
+F:	drivers/iio/adc/ad7191.c
 
 ANALOG DEVICES INC AD7192 DRIVER
 M:	Alisa-Dariana Roman <alisa.roman@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 849c90203071..70a662846aa2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -112,6 +112,16 @@ config AD7173
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7173.
 
+config AD7191
+	tristate "Analog Devices AD7191 ADC driver"
+	depends on SPI
+	select AD_SIGMA_DELTA
+	help
+	  Say yes here to build support for Analog Devices AD7191.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7191.
+
 config AD7192
 	tristate "Analog Devices AD7192 and similar ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ee19afba62b7..54335c613988 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7091R5) += ad7091r5.o
 obj-$(CONFIG_AD7091R8) += ad7091r8.o
 obj-$(CONFIG_AD7124) += ad7124.o
 obj-$(CONFIG_AD7173) += ad7173.o
+obj-$(CONFIG_AD7191) += ad7191.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
new file mode 100644
index 000000000000..dd8151ad3f3f
--- /dev/null
+++ b/drivers/iio/adc/ad7191.c
@@ -0,0 +1,570 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7191 ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define ad_sigma_delta_to_ad7191(sigmad)	container_of((sigmad), struct ad7191_state, sd)
+
+#define AD7191_TEMP_CODES_PER_DEGREE	2815
+
+#define AD7191_EXT_CLK_ENABLE		0
+#define AD7191_INT_CLK_ENABLE		1
+
+#define AD7191_CHAN_MASK		BIT(0)
+#define AD7191_TEMP_MASK		BIT(1)
+
+#define AD7191_MAX_ODR_STATE		3
+#define AD7191_MAX_PGA_STATE		3
+
+enum ad7191_channel {
+	AD7191_CH_AIN1_AIN2 = 0,
+	AD7191_CH_AIN3_AIN4,
+	AD7191_CH_TEMP
+};
+
+/*
+ * NOTE:
+ * The AD7191 features a dual-use data out ready DOUT/RDY output.
+ * In order to avoid contentions on the SPI bus, it's therefore necessary
+ * to use SPI bus locking.
+ *
+ * The DOUT/RDY output must also be wired to an interrupt-capable GPIO.
+ */
+
+struct ad7191_state {
+	struct ad_sigma_delta		sd;
+	struct mutex			lock; // to protect sensor state
+
+	struct gpio_descs		*odr_gpios;
+	struct gpio_descs		*pga_gpios;
+	struct gpio_desc		*temp_gpio;
+	struct gpio_desc		*chan_gpio;
+	struct gpio_desc		*clksel_gpio;
+
+	u16				int_vref_mv;
+	u32				pga_state;
+	u32				scale_avail[4][2];
+	u32				odr_state;
+	u32				samp_freq_avail[4];
+
+	struct clk			*mclk;
+	u32				clksel_state;
+};
+
+static int ad7191_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
+{
+	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
+	u8 temp_gpio_val, chan_gpio_val;
+
+	if (!FIELD_FIT(AD7191_CHAN_MASK | AD7191_TEMP_MASK, channel))
+		return -EINVAL;
+
+	chan_gpio_val = FIELD_GET(AD7191_CHAN_MASK, channel);
+	temp_gpio_val = FIELD_GET(AD7191_TEMP_MASK, channel);
+
+	gpiod_set_value(st->chan_gpio, chan_gpio_val);
+	gpiod_set_value(st->temp_gpio, temp_gpio_val);
+
+	return 0;
+}
+
+static int set_cs(struct ad_sigma_delta *sigma_delta, int pull_down)
+{
+	struct spi_transfer t = {
+		.len = 0,
+		.cs_change = pull_down,
+	};
+	struct spi_message m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	return spi_sync_locked(sigma_delta->spi, &m);
+}
+
+static int ad7191_set_mode(struct ad_sigma_delta *sd,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
+
+	switch (mode) {
+	case AD_SD_MODE_CONTINUOUS:
+	case AD_SD_MODE_SINGLE:
+		return set_cs(&st->sd, 1);
+	case AD_SD_MODE_IDLE:
+		return set_cs(&st->sd, 0);
+	default:
+		return 0;
+	}
+}
+
+static const struct ad_sigma_delta_info ad7191_sigma_delta_info = {
+	.set_channel = ad7191_set_channel,
+	.set_mode = ad7191_set_mode,
+	.has_registers = false,
+};
+
+static int ad7191_init_regulators(struct iio_dev *indio_dev)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
+	int ret;
+
+	ret = devm_regulator_get_enable(dev, "avdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable specified AVdd supply\n");
+
+	ret = devm_regulator_get_enable(dev, "dvdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get Vref voltage\n");
+
+	st->int_vref_mv = ret / 1000;
+
+	return 0;
+}
+
+static int ad7191_gpio_setup(struct iio_dev *indio_dev)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
+
+	if (device_property_read_u32(dev, "adi,odr-state", &st->odr_state) == 0) {
+		if (st->odr_state > AD7191_MAX_ODR_STATE)
+			return dev_err_probe(dev, -EINVAL, "Invalid ODR state.\n");
+
+		dev_info(dev, "ODR is pin-strapped to %d\n", st->odr_state);
+		st->odr_gpios = NULL;
+	} else {
+		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
+		if (IS_ERR(st->odr_gpios))
+			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
+					     "Failed to get odr gpios.\n");
+	}
+
+	if (device_property_read_u32(dev, "adi,pga-state", &st->pga_state) == 0) {
+		if (st->odr_state > AD7191_MAX_PGA_STATE)
+			return dev_err_probe(dev, -EINVAL, "Invalid PGA state.\n");
+
+		dev_info(dev, "PGA is pin-strapped to %d\n", st->pga_state);
+		st->pga_gpios = NULL;
+	} else {
+		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
+		if (IS_ERR(st->pga_gpios))
+			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
+					     "Failed to get pga gpios.\n");
+	}
+
+	if (device_property_read_u32(dev, "adi,clksel-state", &st->clksel_state) == 0) {
+		dev_info(dev, "CLKSEL is pin-strapped to %d\n", st->clksel_state);
+		st->clksel_gpio = NULL;
+	} else {
+		st->clksel_gpio = devm_gpiod_get(dev, "clksel", GPIOD_OUT_HIGH);
+		if (IS_ERR(st->clksel_gpio))
+			return dev_err_probe(dev, PTR_ERR(st->clksel_gpio),
+					     "Failed to get clksel gpio.\n");
+	}
+
+	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
+	if (IS_ERR(st->temp_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->temp_gpio),
+				     "Failed to get temp gpio.\n");
+
+	st->chan_gpio = devm_gpiod_get(dev, "chan", GPIOD_OUT_LOW);
+	if (IS_ERR(st->chan_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->chan_gpio),
+				     "Failed to get chan gpio.\n");
+
+	return 0;
+}
+
+static int ad7191_clock_setup(struct ad7191_state *st)
+{
+	struct device *dev = &st->sd.spi->dev;
+	u8 clksel_value;
+
+	st->mclk = devm_clk_get_enabled(dev, "mclk");
+	if (IS_ERR(st->mclk)) {
+		if (PTR_ERR(st->mclk) != -ENOENT)
+			return dev_err_probe(dev, PTR_ERR(st->mclk),
+					     "Failed to get mclk.\n");
+
+		/*
+		 * No external clock found, default to internal clock.
+		 */
+		clksel_value = AD7191_INT_CLK_ENABLE;
+		if (!st->clksel_gpio && st->clksel_state != AD7191_INT_CLK_ENABLE)
+			return dev_err_probe(dev, -EINVAL,
+				"Invalid CLKSEL state. To use the internal clock, CLKSEL must be high.\n");
+
+		dev_info(dev, "Using internal clock.\n");
+	} else {
+		clksel_value = AD7191_EXT_CLK_ENABLE;
+		if (!st->clksel_gpio && st->clksel_state != AD7191_EXT_CLK_ENABLE)
+			return dev_err_probe(dev, -EINVAL,
+				"Invalid CLKSEL state. To use the external clock, CLKSEL must be low.\n");
+
+		dev_info(dev, "Using external clock.\n");
+	}
+
+	if (st->clksel_gpio)
+		gpiod_set_value(st->clksel_gpio, clksel_value);
+
+	return 0;
+}
+
+static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+	u64 scale_uv;
+	const int gain[4] = {1, 8, 64, 128};
+	int i, ret;
+
+	ret = ad7191_init_regulators(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7191_gpio_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7191_clock_setup(st);
+	if (ret)
+		return ret;
+
+	/*
+	 * Sampling frequencies in Hz, available in the documentation, Table 5.
+	 */
+	st->samp_freq_avail[0] = 120;
+	st->samp_freq_avail[1] = 60;
+	st->samp_freq_avail[2] = 50;
+	st->samp_freq_avail[3] = 10;
+
+	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
+		scale_uv = ((u64)st->int_vref_mv * NANO) >>
+			   (indio_dev->channels[0].scan_type.realbits - 1);
+		do_div(scale_uv, gain[i]);
+		st->scale_avail[i][1] = do_div(scale_uv, NANO);
+		st->scale_avail[i][0] = scale_uv;
+	}
+
+	return 0;
+}
+
+static int ad7191_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long m)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			guard(mutex)(&st->lock);
+			*val = st->scale_avail[st->pga_state][0];
+			*val2 = st->scale_avail[st->pga_state][1];
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_TEMP:
+			*val = 0;
+			*val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
+			return IIO_VAL_INT_PLUS_NANO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(1 << (chan->scan_type.realbits - 1));
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->samp_freq_avail[st->odr_state];
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
+{
+	unsigned long value = gain_index;
+
+	if (!st->pga_gpios)
+		return -EPERM;
+
+	st->pga_state = gain_index;
+
+	return gpiod_set_array_value(2, st->pga_gpios->desc,
+				     st->pga_gpios->info, &value);
+
+	return 0;
+}
+
+static int ad7191_set_samp_freq(struct ad7191_state *st, int samp_freq_index)
+{
+	unsigned long value = samp_freq_index;
+
+	if (!st->odr_gpios)
+		return -EPERM;
+
+	st->odr_state = samp_freq_index;
+
+	return gpiod_set_array_value(2, st->odr_gpios->desc,
+				     st->odr_gpios->info, &value);
+}
+
+static int __ad7191_write_raw(struct ad7191_state *st,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		guard(mutex)(&st->lock);
+		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
+			if (val2 != st->scale_avail[i][1])
+				continue;
+			return ad7191_set_gain(st, i);
+		}
+		return -EINVAL;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!val)
+			return -EINVAL;
+
+		guard(mutex)(&st->lock);
+		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++) {
+			if (val != st->samp_freq_avail[i])
+				continue;
+			return ad7191_set_samp_freq(st, i);
+		}
+		return -EINVAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7191_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = __ad7191_write_raw(st, chan, val, val2, mask);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static int ad7191_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7191_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, const int **vals,
+			     int *type, int *length, long mask)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)st->scale_avail;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*length = ARRAY_SIZE(st->scale_avail) * 2;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)st->samp_freq_avail;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(st->samp_freq_avail);
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info ad7191_info = {
+	.read_raw = ad7191_read_raw,
+	.write_raw = ad7191_write_raw,
+	.write_raw_get_fmt = ad7191_write_raw_get_fmt,
+	.read_avail = ad7191_read_avail,
+	.validate_trigger = ad_sd_validate_trigger,
+};
+
+static const struct iio_chan_spec ad7191_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.address = AD7191_CH_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 24,
+			.storagebits = 24,
+			.endianness = IIO_BE,
+		},
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.differential = 1,
+		.indexed = 1,
+		.channel = 1,
+		.channel2 = 2,
+		.address = AD7191_CH_AIN1_AIN2,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 24,
+			.storagebits = 24,
+			.endianness = IIO_BE,
+		},
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.differential = 1,
+		.indexed = 1,
+		.channel = 3,
+		.channel2 = 4,
+		.address = AD7191_CH_AIN3_AIN4,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 24,
+			.storagebits = 24,
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static int ad7191_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ad7191_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!spi->irq) {
+		dev_err(dev, "no IRQ?\n");
+		return -ENODEV;
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "ad7191";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ad7191_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ad7191_channels);
+	indio_dev->info = &ad7191_info;
+
+	ad_sd_init(&st->sd, indio_dev, spi, &ad7191_sigma_delta_info);
+
+	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7191_setup(indio_dev, dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ad7191_of_match[] = {
+	{
+		.compatible = "adi,ad7191",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7191_of_match);
+
+static const struct spi_device_id ad7191_id_table[] = {
+	{ "ad7191", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad7191_id_table);
+
+static struct spi_driver ad7191_driver = {
+	.driver = {
+		.name = "ad7191",
+		.of_match_table = ad7191_of_match,
+	},
+	.probe = ad7191_probe,
+};
+module_spi_driver(ad7191_driver);
+
+MODULE_AUTHOR("Alisa-Dariana Roman <alisa.roman@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7191 ADC");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
-- 
2.43.0

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On Wed, Jan 22, 2025 at 03:20:39PM +0200, Alisa-Dariana Roman wrote:

> +  adi,odr-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if ODR pins are pin-strapped. Value corresponds to:
> +      0: 120 Hz (ODR1=0, ODR2=0)
> +      1: 60 Hz (ODR1=0, ODR2=1)
> +      2: 50 Hz (ODR1=1, ODR2=0)
> +      3: 10 Hz (ODR1=1, ODR2=1)
> +      If defined, odr-gpios must be absent.
> +    enum: [0, 1, 2, 3]

This should be a property in hertz

> +  pga-gpios:
> +    description: |
> +      PGA1 and PGA2 pins for gain selection. Should be defined if adi,pga-state
> +      is absent.
> +    maxItems: 2
> +
> +  adi,pga-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if PGA pins are pin-strapped. Value corresponds to:
> +      0: Gain 1 (PGA1=0, PGA2=0)
> +      1: Gain 8 (PGA1=0, PGA2=1)
> +      2: Gain 64 (PGA1=1, PGA2=0)
> +      3: Gain 128 (PGA1=1, PGA2=1)
> +      If defined, pga-gpios must be absent.
> +    enum: [0, 1, 2, 3]

And I think this one should be in units of "gain".


> +  adi,clksel-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if CLKSEL is pin-strapped. 0 selects an external clock,
> +      1 selects the internal clock. If defined, clksel-gpios must be absent.
> +    enum: [0, 1]

IMO this one should be a string, options of "external" and "internal". 0
& 1 means nothing to a dts reader/author and needs to be cross checked
with the binding to obtain the meanings.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

[David Lechner]

On 1/22/25 7:20 AM, Alisa-Dariana Roman wrote:
> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7191.yaml          | 175 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 182 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> new file mode 100644
> index 000000000000..c0a6bed7a9cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2025 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7191.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7191 ADC device driver
> +
> +maintainers:
> +  - Alisa-Dariana Roman <alisa.roman@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD7191 ADC device. Datasheet can be
> +  found here:
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7191.pdf

If we are not going to have a powerdown-gpios, I think we should mention in the
description that it is expected that the PDOWN pin is connected to the SPI
controller chip select.

> +
> +properties:

...

> +
> +  clksel-gpios:

Do we really need this one? I don't think we have a chip yet that wants to
change the clock at runtime. We don't have this for any of the other similar
ADI sigma delta chips that have already been upstreamed.

If there is an evaluation board where the pin is wired to a GPIO, we can just
use gpio-hog to select the correct state.

> +    description: |
> +      Clock source selection pin (internal or external). Should be defined if
> +      clksel-config is absent.
> +    maxItems: 1
> +
> +  adi,clksel-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if CLKSEL is pin-strapped. 0 selects an external clock,
> +      1 selects the internal clock. If defined, clksel-gpios must be absent.
> +    enum: [0, 1]

I don't think we need this one either. If the clocks property is present, then
we can assume that CLKSEL is going to be hardwired to indicate an external
clock and if the clocks property is absent, then we know it must be hardwired
to select the internal clock.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - avdd-supply
> +  - dvdd-supply
> +  - vref-supply
> +  - spi-cpol
> +  - spi-cpha
> +  - temp-gpios
> +  - chan-gpios
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      required:
> +        - adi,odr-state
> +    then:
> +      properties:
> +        odr-gpios: false
> +    else:
> +      required:
> +        - odr-gpios

I think we could simplify these with:

- oneOf:
  - required:
     - adi,odr-state
  - required:
     - odr-gpios

> +  - if:
> +      required:
> +        - adi,pga-state
> +    then:
> +      properties:
> +        pga-gpios: false
> +    else:
> +      required:
> +        - pga-gpios
> +  - if:
> +      required:
> +        - adi,clksel-state
> +    then:
> +      properties:
> +        clksel-gpios: false
> +    else:
> +      required:
> +        - clksel-gpios
> +
> +unevaluatedProperties: false
> +

Re: [PATCH v2 2/2] iio: adc: ad7191: add AD7191

[David Lechner]

On 1/22/25 7:20 AM, Alisa-Dariana Roman wrote:
> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC

ultra-low

> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

...

> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..dd8151ad3f3f
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> @@ -0,0 +1,570 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7191 ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>

prefer alphabetical order

> +
> +#define ad_sigma_delta_to_ad7191(sigmad)	container_of((sigmad), struct ad7191_state, sd)
> +
> +#define AD7191_TEMP_CODES_PER_DEGREE	2815
> +
> +#define AD7191_EXT_CLK_ENABLE		0
> +#define AD7191_INT_CLK_ENABLE		1
> +
> +#define AD7191_CHAN_MASK		BIT(0)
> +#define AD7191_TEMP_MASK		BIT(1)
> +
> +#define AD7191_MAX_ODR_STATE		3
> +#define AD7191_MAX_PGA_STATE		3
> +
> +enum ad7191_channel {
> +	AD7191_CH_AIN1_AIN2 = 0,

0 isn't needed here.

> +	AD7191_CH_AIN3_AIN4,
> +	AD7191_CH_TEMP
> +};
> +
> +/*
> + * NOTE:
> + * The AD7191 features a dual-use data out ready DOUT/RDY output.
> + * In order to avoid contentions on the SPI bus, it's therefore necessary
> + * to use SPI bus locking.
> + *
> + * The DOUT/RDY output must also be wired to an interrupt-capable GPIO.

Probably worth mentioning that the SPI controller CS gets wired to PDOWN pin
on the ADC here since that isn't very obvious.

> + */
> +
> +struct ad7191_state {
> +	struct ad_sigma_delta		sd;
> +	struct mutex			lock; // to protect sensor state
> +
> +	struct gpio_descs		*odr_gpios;
> +	struct gpio_descs		*pga_gpios;
> +	struct gpio_desc		*temp_gpio;
> +	struct gpio_desc		*chan_gpio;
> +	struct gpio_desc		*clksel_gpio;
> +
> +	u16				int_vref_mv;
> +	u32				pga_state;
> +	u32				scale_avail[4][2];
> +	u32				odr_state;
> +	u32				samp_freq_avail[4];
> +
> +	struct clk			*mclk;
> +	u32				clksel_state;
> +};
> +
> +static int ad7191_set_channel(struct ad_sigma_delta *sd, unsigned int channel)

Would be less confusing to me if the channel parameter was changed to "address"
since the actual value is the channel spec .address field.

> +{
> +	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
> +	u8 temp_gpio_val, chan_gpio_val;
> +
> +	if (!FIELD_FIT(AD7191_CHAN_MASK | AD7191_TEMP_MASK, channel))
> +		return -EINVAL;
> +
> +	chan_gpio_val = FIELD_GET(AD7191_CHAN_MASK, channel);
> +	temp_gpio_val = FIELD_GET(AD7191_TEMP_MASK, channel);
> +
> +	gpiod_set_value(st->chan_gpio, chan_gpio_val);
> +	gpiod_set_value(st->temp_gpio, temp_gpio_val);
> +
> +	return 0;
> +}
> +
> +static int set_cs(struct ad_sigma_delta *sigma_delta, int pull_down)

Make it ad7191_set_cs() to be consistent. And "assert" is probably a more common
name instead of pull_down.

> +{
> +	struct spi_transfer t = {
> +		.len = 0,
> +		.cs_change = pull_down,
> +	};
> +	struct spi_message m;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);

Can make this one line with spi_message_init_with_transfers().

> +
> +	return spi_sync_locked(sigma_delta->spi, &m);
> +}
> +
> +static int ad7191_set_mode(struct ad_sigma_delta *sd,
> +			   enum ad_sigma_delta_mode mode)
> +{
> +	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
> +
> +	switch (mode) {
> +	case AD_SD_MODE_CONTINUOUS:
> +	case AD_SD_MODE_SINGLE:
> +		return set_cs(&st->sd, 1);
> +	case AD_SD_MODE_IDLE:
> +		return set_cs(&st->sd, 0);
> +	default:
> +		return 0;

Should default return an error?

> +	}
> +}
> +
> +static const struct ad_sigma_delta_info ad7191_sigma_delta_info = {
> +	.set_channel = ad7191_set_channel,
> +	.set_mode = ad7191_set_mode,
> +	.has_registers = false,
> +};
> +
> +static int ad7191_init_regulators(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +	int ret;
> +
> +	ret = devm_regulator_get_enable(dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable specified AVdd supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "dvdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get Vref voltage\n");
> +
> +	st->int_vref_mv = ret / 1000;
> +
> +	return 0;
> +}
> +
> +static int ad7191_gpio_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	if (device_property_read_u32(dev, "adi,odr-state", &st->odr_state) == 0) {

Usually we check for a specific error. Otherwise, if someone does something like
using a string instead of an int in the .dts file, we will get the default value
rather than an error.

> +		if (st->odr_state > AD7191_MAX_ODR_STATE)
> +			return dev_err_probe(dev, -EINVAL, "Invalid ODR state.\n");
> +
> +		dev_info(dev, "ODR is pin-strapped to %d\n", st->odr_state);

dev_dbg(). or remove it. we can get the info by reading the samping_frequency
attribute if needed.

> +		st->odr_gpios = NULL;
> +	} else {
> +		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->odr_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> +					     "Failed to get odr gpios.\n");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,pga-state", &st->pga_state) == 0) {
> +		if (st->odr_state > AD7191_MAX_PGA_STATE)

Looks like copy/paste mistake. Should be checking pga_state here.

> +			return dev_err_probe(dev, -EINVAL, "Invalid PGA state.\n");
> +
> +		dev_info(dev, "PGA is pin-strapped to %d\n", st->pga_state);
> +		st->pga_gpios = NULL;
> +	} else {
> +		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->pga_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> +					     "Failed to get pga gpios.\n");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,clksel-state", &st->clksel_state) == 0) {
> +		dev_info(dev, "CLKSEL is pin-strapped to %d\n", st->clksel_state);
> +		st->clksel_gpio = NULL;
> +	} else {
> +		st->clksel_gpio = devm_gpiod_get(dev, "clksel", GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->clksel_gpio))
> +			return dev_err_probe(dev, PTR_ERR(st->clksel_gpio),
> +					     "Failed to get clksel gpio.\n");
> +	}
> +
> +	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->temp_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->temp_gpio),
> +				     "Failed to get temp gpio.\n");
> +
> +	st->chan_gpio = devm_gpiod_get(dev, "chan", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->chan_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->chan_gpio),
> +				     "Failed to get chan gpio.\n");
> +
> +	return 0;
> +}
> +
> +static int ad7191_clock_setup(struct ad7191_state *st)
> +{

As mentioned in the dt-bindings patch review, this could be simpified a bit if
we remove some of the reduant/unnecessary properties.

> +	struct device *dev = &st->sd.spi->dev;
> +	u8 clksel_value;
> +
> +	st->mclk = devm_clk_get_enabled(dev, "mclk");
> +	if (IS_ERR(st->mclk)) {
> +		if (PTR_ERR(st->mclk) != -ENOENT)
> +			return dev_err_probe(dev, PTR_ERR(st->mclk),
> +					     "Failed to get mclk.\n");
> +
> +		/*
> +		 * No external clock found, default to internal clock.
> +		 */
> +		clksel_value = AD7191_INT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_INT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the internal clock, CLKSEL must be high.\n");
> +
> +		dev_info(dev, "Using internal clock.\n");
> +	} else {
> +		clksel_value = AD7191_EXT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_EXT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the external clock, CLKSEL must be low.\n");
> +
> +		dev_info(dev, "Using external clock.\n");
> +	}
> +
> +	if (st->clksel_gpio)
> +		gpiod_set_value(st->clksel_gpio, clksel_value);
> +
> +	return 0;
> +}
> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	u64 scale_uv;
> +	const int gain[4] = {1, 8, 64, 128};
> +	int i, ret;
> +
> +	ret = ad7191_init_regulators(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_gpio_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_clock_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Sampling frequencies in Hz, available in the documentation, Table 5.
> +	 */
> +	st->samp_freq_avail[0] = 120;
> +	st->samp_freq_avail[1] = 60;
> +	st->samp_freq_avail[2] = 50;
> +	st->samp_freq_avail[3] = 10;

Looks like this one could just be static const data. Or if ODR pins are hard-
wired, maybe this should only have 1 value.

> +
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +		scale_uv = ((u64)st->int_vref_mv * NANO) >>
> +			   (indio_dev->channels[0].scan_type.realbits - 1);
> +		do_div(scale_uv, gain[i]);
> +		st->scale_avail[i][1] = do_div(scale_uv, NANO);
> +		st->scale_avail[i][0] = scale_uv;

Same here, if gain pins are hard-wired, then the other options aren't really
available.

> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7191_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long m)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			guard(mutex)(&st->lock);
> +			*val = st->scale_avail[st->pga_state][0];
> +			*val2 = st->scale_avail[st->pga_state][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_TEMP:
> +			*val = 0;
> +			*val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->samp_freq_avail[st->odr_state];
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
> +{
> +	unsigned long value = gain_index;
> +
> +	if (!st->pga_gpios)
> +		return -EPERM;
> +
> +	st->pga_state = gain_index;
> +
> +	return gpiod_set_array_value(2, st->pga_gpios->desc,

Replace hard-coded 2 with st->pga_gpios->ndescs.

Also, gpiod_set_array_value_cansleep() should be OK.

> +				     st->pga_gpios->info, &value);
> +
> +	return 0;
> +}
> +
> +static int ad7191_set_samp_freq(struct ad7191_state *st, int samp_freq_index)
> +{
> +	unsigned long value = samp_freq_index;
> +
> +	if (!st->odr_gpios)
> +		return -EPERM;
> +
> +	st->odr_state = samp_freq_index;
> +
> +	return gpiod_set_array_value(2, st->odr_gpios->desc,
> +				     st->odr_gpios->info, &value);

ditto

> +}
> +
> +static int __ad7191_write_raw(struct ad7191_state *st,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		guard(mutex)(&st->lock);
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +			if (val2 != st->scale_avail[i][1])
> +				continue;
> +			return ad7191_set_gain(st, i);
> +		}
> +		return -EINVAL;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!val)
> +			return -EINVAL;

This check seems reduandant since we would get the same result below without it.

> +
> +		guard(mutex)(&st->lock);
> +		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++) {
> +			if (val != st->samp_freq_avail[i])
> +				continue;
> +			return ad7191_set_samp_freq(st, i);
> +		}
> +		return -EINVAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +static const struct iio_chan_spec ad7191_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.address = AD7191_CH_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 24,

IIO buffers are "natrually" aligned, so storagebits for anything > 16, <= 32 is
going to be a 32-bit integer.

> +			.endianness = IIO_BE,
> +		},
> +	},
> +	{
> +		.type = IIO_VOLTAGE,
> +		.differential = 1,
> +		.indexed = 1,
> +		.channel = 1,
> +		.channel2 = 2,
> +		.address = AD7191_CH_AIN1_AIN2,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),

A bit odd to have offset separate and scale by type, but I guess it isn't
wrong.

> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 24,
> +			.endianness = IIO_BE,
> +		},
> +	},
> +	{
> +		.type = IIO_VOLTAGE,
> +		.differential = 1,
> +		.indexed = 1,
> +		.channel = 3,
> +		.channel2 = 4,
> +		.address = AD7191_CH_AIN3_AIN4,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 2,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 24,
> +			.endianness = IIO_BE,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static int ad7191_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ad7191_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!spi->irq) {
> +		dev_err(dev, "no IRQ?\n");
> +		return -ENODEV;

return dev_err_probe(...);

Or should we just let ad_sd_init() handle it?


> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad7191";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7191_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7191_channels);
> +	indio_dev->info = &ad7191_info;
> +
> +	ad_sd_init(&st->sd, indio_dev, spi, &ad7191_sigma_delta_info);

Need to check return value.

> +
> +	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_setup(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad7191_of_match[] = {
> +	{
> +		.compatible = "adi,ad7191",
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7191_of_match);
> +
> +static const struct spi_device_id ad7191_id_table[] = {
> +	{ "ad7191", 0 },

Could leave out the 0 here.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad7191_id_table);
> +
> +static struct spi_driver ad7191_driver = {
> +	.driver = {
> +		.name = "ad7191",
> +		.of_match_table = ad7191_of_match,
> +	},
> +	.probe = ad7191_probe,

Still missing .id_table here.

> +};
> +module_spi_driver(ad7191_driver);
> +
> +MODULE_AUTHOR("Alisa-Dariana Roman <alisa.roman@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7191 ADC");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

[Jonathan Cameron]

On Wed, 22 Jan 2025 15:20:39 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7191.yaml          | 175 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 182 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> new file mode 100644
> index 000000000000..c0a6bed7a9cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2025 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7191.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7191 ADC device driver
Not a "device driver" so drop that bit or say 'binding' instead.

> +
> +maintainers:
> +  - Alisa-Dariana Roman <alisa.roman@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD7191 ADC device. Datasheet can be
> +  found here:
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7191.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7191
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpol: true
> +
> +  spi-cpha: true
> +
> +  clocks:
> +    maxItems: 1
> +    description:
> +      Optionally, either a crystal can be attached externally between MCLK1 and
> +      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> +      pin. If absent, internal 4.92MHz clock is used.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: AVdd voltage supply
> +
> +  dvdd-supply:
> +    description: DVdd voltage supply
> +
> +  vref-supply:
> +    description: Vref voltage supply
> +
> +  odr-gpios:
> +    description: |
> +      ODR1 and ODR2 pins for output data rate selection. Should be defined if
> +      adi,odr-state is absent.
> +    maxItems: 2

minItems also 2? i guess we aren't coping with situation of one pin wired
until some board designer decides to do that.


> +
> +  adi,odr-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if ODR pins are pin-strapped. Value corresponds to:
> +      0: 120 Hz (ODR1=0, ODR2=0)
> +      1: 60 Hz (ODR1=0, ODR2=1)
> +      2: 50 Hz (ODR1=1, ODR2=0)
> +      3: 10 Hz (ODR1=1, ODR2=1)
> +      If defined, odr-gpios must be absent.
> +    enum: [0, 1, 2, 3]
> +
> +  pga-gpios:
> +    description: |
> +      PGA1 and PGA2 pins for gain selection. Should be defined if adi,pga-state
> +      is absent.
> +    maxItems: 2
minItems here as well I think.

> +
Jonathan

Re: [PATCH v2 2/2] iio: adc: ad7191: add AD7191

[Jonathan Cameron]

On Wed, 22 Jan 2025 15:20:40 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Hi Alisa-Dariana,

A few additional comments inline. Hopefully not too much overlap
with other reviews

Jonathan

> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..dd8151ad3f3f
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> @@ -0,0 +1,570 @@


> +static int set_cs(struct ad_sigma_delta *sigma_delta, int pull_down)
> +{
> +	struct spi_transfer t = {
> +		.len = 0,
> +		.cs_change = pull_down,
> +	};
> +	struct spi_message m;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);

spi_message_init_with_transfers() given there is no spi_sync_transfers_locked()
and probably not enough users to make it worth adding.


> +
> +	return spi_sync_locked(sigma_delta->spi, &m);
> +}
>

> +static int ad7191_gpio_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	if (device_property_read_u32(dev, "adi,odr-state", &st->odr_state) == 0) {
> +		if (st->odr_state > AD7191_MAX_ODR_STATE)
> +			return dev_err_probe(dev, -EINVAL, "Invalid ODR state.\n");
> +
> +		dev_info(dev, "ODR is pin-strapped to %d\n", st->odr_state);

dev_dbg for all these. 

> +		st->odr_gpios = NULL;
> +	} else {
> +		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->odr_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> +					     "Failed to get odr gpios.\n");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,pga-state", &st->pga_state) == 0) {
> +		if (st->odr_state > AD7191_MAX_PGA_STATE)
> +			return dev_err_probe(dev, -EINVAL, "Invalid PGA state.\n");
> +
> +		dev_info(dev, "PGA is pin-strapped to %d\n", st->pga_state);
> +		st->pga_gpios = NULL;
> +	} else {
> +		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->pga_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> +					     "Failed to get pga gpios.\n");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,clksel-state", &st->clksel_state) == 0) {
> +		dev_info(dev, "CLKSEL is pin-strapped to %d\n", st->clksel_state);
> +		st->clksel_gpio = NULL;
> +	} else {
> +		st->clksel_gpio = devm_gpiod_get(dev, "clksel", GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->clksel_gpio))
> +			return dev_err_probe(dev, PTR_ERR(st->clksel_gpio),
> +					     "Failed to get clksel gpio.\n");
> +	}
> +
> +	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->temp_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->temp_gpio),
> +				     "Failed to get temp gpio.\n");
> +
> +	st->chan_gpio = devm_gpiod_get(dev, "chan", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->chan_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->chan_gpio),
> +				     "Failed to get chan gpio.\n");
> +
> +	return 0;
> +}
> +
> +static int ad7191_clock_setup(struct ad7191_state *st)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	u8 clksel_value;
> +
> +	st->mclk = devm_clk_get_enabled(dev, "mclk");
> +	if (IS_ERR(st->mclk)) {
> +		if (PTR_ERR(st->mclk) != -ENOENT)
> +			return dev_err_probe(dev, PTR_ERR(st->mclk),
> +					     "Failed to get mclk.\n");
> +
> +		/*
> +		 * No external clock found, default to internal clock.
Single line comment if anything.  Kind of obvious from code anyway
so could drop the comment..

> +		 */
> +		clksel_value = AD7191_INT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_INT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the internal clock, CLKSEL must be high.\n");
> +
> +		dev_info(dev, "Using internal clock.\n");

dev_dbg() for this stuff.  Driver shouldn't be that noisy about details like this

> +	} else {
> +		clksel_value = AD7191_EXT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_EXT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the external clock, CLKSEL must be low.\n");
> +
> +		dev_info(dev, "Using external clock.\n");
> +	}
> +
> +	if (st->clksel_gpio)
> +		gpiod_set_value(st->clksel_gpio, clksel_value);
> +
> +	return 0;
> +}
> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	u64 scale_uv;
> +	const int gain[4] = {1, 8, 64, 128};
> +	int i, ret;
> +
> +	ret = ad7191_init_regulators(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_gpio_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_clock_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Sampling frequencies in Hz, available in the documentation, Table 5.

I'd just go with
	 /* Sampling frequencies in Hz, see Table 5 */

Unlikely it would be anywhere else and 'in the documentation' isn't
very specific.

> +	 */
> +	st->samp_freq_avail[0] = 120;
> +	st->samp_freq_avail[1] = 60;
> +	st->samp_freq_avail[2] = 50;
> +	st->samp_freq_avail[3] = 10;
> +
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +		scale_uv = ((u64)st->int_vref_mv * NANO) >>
> +			   (indio_dev->channels[0].scan_type.realbits - 1);
> +		do_div(scale_uv, gain[i]);
> +		st->scale_avail[i][1] = do_div(scale_uv, NANO);
> +		st->scale_avail[i][0] = scale_uv;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7191_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long m)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:

As below.  Make the scope explicit with {}

> +			guard(mutex)(&st->lock);
> +			*val = st->scale_avail[st->pga_state][0];
> +			*val2 = st->scale_avail[st->pga_state][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_TEMP:
> +			*val = 0;
> +			*val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->samp_freq_avail[st->odr_state];
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}

> +
> +static int __ad7191_write_raw(struct ad7191_state *st,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
scope should be defined and obvious if you want to use guard.

	case IIO_CHAN_INFO_SCALE: {
		guard(mutex)(&st->lock);
...

		return -EINVAL;
	}
> +		guard(mutex)(&st->lock);
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +			if (val2 != st->scale_avail[i][1])
> +				continue;
> +			return ad7191_set_gain(st, i);
> +		}
> +		return -EINVAL;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!val)
> +			return -EINVAL;
> +
> +		guard(mutex)(&st->lock);

Same here.

> +		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++) {
> +			if (val != st->samp_freq_avail[i])
> +				continue;
> +			return ad7191_set_samp_freq(st, i);
> +		}
> +		return -EINVAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int ad7191_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ad7191_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!spi->irq) {
> +		dev_err(dev, "no IRQ?\n");

Can't the ad_sigma_delta library figure this out for all devices?
Feels odd to check it here when this code never directly uses it.


> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad7191";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7191_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7191_channels);
> +	indio_dev->info = &ad7191_info;
> +
> +	ad_sd_init(&st->sd, indio_dev, spi, &ad7191_sigma_delta_info);
> +
> +	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_setup(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}