[PATCH v0 0/3] Add support for AD7191

[PATCH v0 0/3] Add support for AD7191

[Alisa-Dariana Roman]

Dear maintainers,

Here is a series of patches adding support for AD7191, a chip from the AD719X
family, with similar functionalities. Since this chip is entirely
pin-programmable with no registers, I wrote a separate driver from ad7192.c.
Sigma delta adc drivers use the set_mode() callback function which contains a
register write to make sure the chip select line is asserted before waiting for
an interrupt. Devices such as AD7191 and AD7780 have no registers to write to,
so I added a helper function ad_sd_assert_cs() for these use cases.

Kind regards,
Alisa-Dariana Roman.

[PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

[Alisa-Dariana Roman]

Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
start conversion when CS is asserted. Add helper function to support
this use case by allowing devices to assert CS without performing
register operations.

This function can be used by drivers through their set_mode callback.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 24 ++++++++++++++++++++++++
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 0f355dac7813..c0f33d4baddf 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -48,6 +48,30 @@ void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm)
 }
 EXPORT_SYMBOL_NS_GPL(ad_sd_set_comm, "IIO_AD_SIGMA_DELTA");
 
+/**
+ * ad_sd_assert_cs() - Assert chip select line
+ *
+ * @sigma_delta: The sigma delta device
+ *
+ * Returns 0 on success, an error code otherwise.
+ **/
+int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta)
+{
+	struct spi_transfer t = {
+		.len = 0,
+		.cs_change = sigma_delta->keep_cs_asserted,
+	};
+	struct spi_message m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	if (sigma_delta->bus_locked)
+		return spi_sync_locked(sigma_delta->spi, &m);
+	return spi_sync(sigma_delta->spi, &m);
+}
+EXPORT_SYMBOL_NS_GPL(ad_sd_assert_cs, IIO_AD_SIGMA_DELTA);
+
 /**
  * ad_sd_write_reg() - Write a register
  *
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 417073c52380..99ab56d04793 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -178,6 +178,7 @@ static inline int ad_sigma_delta_postprocess_sample(struct ad_sigma_delta *sd,
 }
 
 void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm);
+int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta);
 int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 	unsigned int size, unsigned int val);
 int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
-- 
2.43.0

[PATCH v1 2/3] 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          | 128 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 135 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..f3e596918c22
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
@@ -0,0 +1,128 @@
+# 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
+
+  odr1-gpios:
+    description: GPIO connected to ODR1 pin for output data rate selection
+    maxItems: 1
+
+  odr2-gpios:
+    description: GPIO connected to ODR2 pin for output data rate selection
+    maxItems: 1
+
+  pga1-gpios:
+    description: GPIO connected to PGA1 pin for gain selection
+    maxItems: 1
+
+  pga2-gpios:
+    description: GPIO connected to PGA2 pin for gain selection
+    maxItems: 1
+
+  temp-gpios:
+    description: GPIO connected to TEMP pin for temperature sensor enable
+    maxItems: 1
+
+  chan-gpios:
+    description: GPIO connected to CHAN pin for input channel selection
+    maxItems: 1
+
+  clksel-gpios:
+    description: GPIO connected to CLKSEL pin for clock source selection
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - avdd-supply
+  - dvdd-supply
+  - vref-supply
+  - spi-cpol
+  - spi-cpha
+  - odr1-gpios
+  - odr2-gpios
+  - pga1-gpios
+  - pga2-gpios
+  - temp-gpios
+  - chan-gpios
+  - clksel-gpios
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi0 {
+        #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>;
+            odr1-gpios = <&gpio 23 GPIO_ACTIVE_HIGH>;
+            odr2-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+            pga1-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
+            pga2-gpios = <&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 8e5167443cea..254fbfe6144f 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 v1 3/3] 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  |  11 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7191.c | 513 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 526 insertions(+)
 create mode 100644 drivers/iio/adc/ad7191.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 254fbfe6144f..caaf1960ac85 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..434610af3d39 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -112,6 +112,17 @@ 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.
+	  If unsure, say N (but it's safe to say "Y").
+
+	  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..6509e974072a
--- /dev/null
+++ b/drivers/iio/adc/ad7191.c
@@ -0,0 +1,513 @@
+// 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/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/adc/ad_sigma_delta.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define AD7191_NAME			"ad7191"
+
+#define AD7191_TEMP_CODES_PER_DEGREE	2815
+
+#define AD7191_EXT_FREQ_HZ_MIN		2457600
+#define AD7191_EXT_FREQ_HZ_MAX		5120000
+#define AD7191_INT_FREQ_HZ		4915200
+
+#define AD7191_CHAN_MASK		BIT(0)
+#define AD7191_TEMP_MASK		BIT(1)
+
+#define AD7191_PGA1_MASK		BIT(0)
+#define AD7191_PGA2_MASK		BIT(1)
+
+#define AD7191_ODR1_MASK		BIT(0)
+#define AD7191_ODR2_MASK		BIT(1)
+
+#define AD7191_CH_AIN1_AIN2		0
+#define AD7191_CH_AIN3_AIN4		1
+#define AD7191_CH_TEMP			2
+
+/* 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_desc		*odr1_gpio;
+	struct gpio_desc		*odr2_gpio;
+	struct gpio_desc		*pga1_gpio;
+	struct gpio_desc		*pga2_gpio;
+	struct gpio_desc		*temp_gpio;
+	struct gpio_desc		*chan_gpio;
+	struct gpio_desc		*clksel_gpio;
+
+	u16				int_vref_mv;
+	u8				gain_index;
+	u32				scale_avail[4][2];
+	u8				samp_freq_index;
+	u32				samp_freq_avail[4];
+
+	struct clk			*mclk;
+	u32				fclk;
+};
+
+static struct ad7191_state *ad_sigma_delta_to_ad7191(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad7191_state, sd);
+}
+
+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 ad7191_set_mode(struct ad_sigma_delta *sd,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
+
+	return ad_sd_assert_cs(&st->sd);
+}
+
+static const struct ad_sigma_delta_info ad7191_sigma_delta_info = {
+	.set_channel = ad7191_set_channel,
+	.set_mode = ad7191_set_mode,
+	.has_registers = false,
+	.irq_flags = IRQF_TRIGGER_FALLING,
+};
+
+static inline bool ad7191_valid_external_frequency(u32 freq)
+{
+	return (freq >= AD7191_EXT_FREQ_HZ_MIN &&
+		freq <= AD7191_EXT_FREQ_HZ_MAX);
+}
+
+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_init_gpios(struct iio_dev *indio_dev)
+{
+	struct ad7191_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
+
+	st->odr1_gpio = devm_gpiod_get(dev, "odr1", GPIOD_OUT_LOW);
+	if (IS_ERR(st->odr1_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->odr1_gpio),
+				     "Failed to get odr1 gpio.\n");
+
+	st->odr2_gpio = devm_gpiod_get(dev, "odr2", GPIOD_OUT_LOW);
+	if (IS_ERR(st->odr2_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->odr2_gpio),
+				     "Failed to get odr2 gpio.\n");
+
+	st->pga1_gpio = devm_gpiod_get(dev, "pga1", GPIOD_OUT_LOW);
+	if (IS_ERR(st->pga1_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->pga1_gpio),
+				     "Failed to get pga1 gpio.\n");
+
+	st->pga2_gpio = devm_gpiod_get(dev, "pga2", GPIOD_OUT_LOW);
+	if (IS_ERR(st->pga2_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->pga2_gpio),
+				     "Failed to get pga2 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");
+
+	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");
+
+	return 0;
+}
+
+static int ad7191_clock_setup(struct ad7191_state *st)
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	if (!device_property_present(dev, "mclk")) {
+		gpiod_set_value(st->clksel_gpio, 1);
+		st->fclk = AD7191_INT_FREQ_HZ;
+		return 0;
+	}
+
+	gpiod_set_value(st->clksel_gpio, 0);
+
+	st->mclk = devm_clk_get_enabled(dev, "mclk");
+	if (IS_ERR(st->mclk))
+		return dev_err_probe(dev, PTR_ERR(st->mclk),
+				     "Failed to get clock source\n");
+
+	st->fclk = clk_get_rate(st->mclk);
+	if (!ad7191_valid_external_frequency(st->fclk))
+		return dev_err_probe(dev, -EINVAL,
+				     "External clock frequency out of bounds\n");
+
+	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;
+	int gain[4] = {1, 8, 64, 128};
+	int i;
+	int ret;
+
+	ret = ad7191_init_regulators(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7191_init_gpios(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7191_clock_setup(st);
+	if (ret)
+		return ret;
+
+	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:
+			mutex_lock(&st->lock);
+			*val = st->scale_avail[st->gain_index][0];
+			*val2 = st->scale_avail[st->gain_index][1];
+			mutex_unlock(&st->lock);
+			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->samp_freq_index];
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int ad7191_set_gain(struct ad7191_state *st, u8 gain_index)
+{
+	u8 pga1_gpio_val, pga2_gpio_val;
+
+	if (!FIELD_FIT(AD7191_PGA1_MASK | AD7191_PGA2_MASK, gain_index))
+		return -EINVAL;
+
+	st->gain_index = gain_index;
+
+	pga1_gpio_val = FIELD_GET(AD7191_PGA1_MASK, gain_index);
+	pga2_gpio_val = FIELD_GET(AD7191_PGA2_MASK, gain_index);
+
+	gpiod_set_value(st->pga1_gpio, pga1_gpio_val);
+	gpiod_set_value(st->pga2_gpio, pga2_gpio_val);
+
+	return 0;
+}
+
+static int ad7191_set_samp_freq(struct ad7191_state *st, u8 samp_freq_index)
+{
+	u8 odr1_gpio_val, odr2_gpio_val;
+
+	if (!FIELD_FIT(AD7191_ODR1_MASK | AD7191_ODR2_MASK, samp_freq_index))
+		return -EINVAL;
+
+	st->samp_freq_index = samp_freq_index;
+
+	odr1_gpio_val = FIELD_GET(AD7191_ODR1_MASK, samp_freq_index);
+	odr2_gpio_val = FIELD_GET(AD7191_ODR2_MASK, samp_freq_index);
+
+	gpiod_set_value(st->odr1_gpio, odr1_gpio_val);
+	gpiod_set_value(st->odr2_gpio, odr2_gpio_val);
+
+	return 0;
+}
+
+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, i;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	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 = ad7191_set_gain(st, i);
+				break;
+			}
+		mutex_unlock(&st->lock);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!val) {
+			ret = -EINVAL;
+			break;
+		}
+		mutex_lock(&st->lock);
+		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++)
+			if (val == st->samp_freq_avail[i]) {
+				ret = ad7191_set_samp_freq(st, i);
+				break;
+			}
+		mutex_unlock(&st->lock);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	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,
+};
+
+#define __AD719x_CHANNEL(_si, _channel1, _channel2, _address, _type, \
+	_differential, _mask_type_av) \
+	{ \
+		.type = (_type), \
+		.differential = _differential, \
+		.indexed = 1, \
+		.channel = (_channel1), \
+		.channel2 = (_channel2), \
+		.address = (_address), \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			BIT(IIO_CHAN_INFO_OFFSET), \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.info_mask_shared_by_type_available = (_mask_type_av), \
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.scan_index = (_si), \
+		.scan_type = { \
+			.sign = 'u', \
+			.realbits = 24, \
+			.storagebits = 24, \
+			.endianness = IIO_BE, \
+		}, \
+	}
+
+#define AD719x_DIFF_CHANNEL(_si, _channel1, _channel2, _address) \
+	__AD719x_CHANNEL(_si, _channel1, _channel2, _address, IIO_VOLTAGE, 1, \
+		BIT(IIO_CHAN_INFO_SCALE))
+
+#define AD719x_TEMP_CHANNEL(_si, _address) \
+	__AD719x_CHANNEL(_si, 0, -1, _address, IIO_TEMP, 0, 0)
+
+static const struct iio_chan_spec ad7191_channels[] = {
+	AD719x_TEMP_CHANNEL(0, AD7191_CH_TEMP),
+	AD719x_DIFF_CHANNEL(1, 1, 2, AD7191_CH_AIN1_AIN2),
+	AD719x_DIFF_CHANNEL(2, 3, 4, AD7191_CH_AIN3_AIN4),
+	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);
+
+	mutex_init(&st->lock);
+
+	indio_dev->name = AD7191_NAME;
+	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 struct spi_driver ad7191_driver = {
+	.driver = {
+		.name	= AD7191_NAME,
+		.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 v1 2/3] dt-bindings: iio: adc: add AD7191

[Conor Dooley]

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

On Sat, Dec 21, 2024 at 05:56:01PM +0200, 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          | 128 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 135 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..f3e596918c22
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> @@ -0,0 +1,128 @@
> +# 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.

Without clock-names, how do you tell the difference between driven ctal and
the cmos-compatible clock? That CLKSEL's job?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: AVdd voltage supply
> +
> +  dvdd-supply:
> +    description: DVdd voltage supply
> +
> +  vref-supply:
> +    description: Vref voltage supply
> +
> +  odr1-gpios:
> +    description: GPIO connected to ODR1 pin for output data rate selection
> +    maxItems: 1
> +
> +  odr2-gpios:
> +    description: GPIO connected to ODR2 pin for output data rate selection
> +    maxItems: 1
> +
> +  pga1-gpios:
> +    description: GPIO connected to PGA1 pin for gain selection
> +    maxItems: 1
> +
> +  pga2-gpios:
> +    description: GPIO connected to PGA2 pin for gain selection
> +    maxItems: 1
> +
> +  temp-gpios:
> +    description: GPIO connected to TEMP pin for temperature sensor enable
> +    maxItems: 1
> +
> +  chan-gpios:
> +    description: GPIO connected to CHAN pin for input channel selection
> +    maxItems: 1
> +
> +  clksel-gpios:
> +    description: GPIO connected to CLKSEL pin for clock source selection
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - avdd-supply
> +  - dvdd-supply
> +  - vref-supply
> +  - spi-cpol
> +  - spi-cpha

> +  - odr1-gpios
> +  - odr2-gpios
> +  - pga1-gpios
> +  - pga2-gpios

For these 4, since all are required, seems like grouping as odr-pgios
and pga-gpios would be a good idea?

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

Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

[Jonathan Cameron]

On Sat, 21 Dec 2024 17:56:00 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
> start conversion when CS is asserted. Add helper function to support
> this use case by allowing devices to assert CS without performing
> register operations.
Hi Alisa-Dariana,

I had a look at the ad7191 datasheet. Given this description,
I was expecting to see it do a pre pulse of the chip select to trigger
the acquisition.  However, what I see is a power down line (which is more
or less a chip select) but it just has a specified t1 delay before the
DOUT will change to the state for the first bit and the host
can start driving the clock.

That can be done by setting spi_device->cs_setup to whatever delay is
needed.  The text is spi_device docs are a little vague,
but I'd take it as t1 + t2 (maybe t3 to be safe).

That is going to be more reliable than trying to hold the cs across
messages / spi_sync() calls, particularly if the bus might not be
locked (which the code below suggests).

Jonathan


> 
> This function can be used by drivers through their set_mode callback.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 24 ++++++++++++++++++++++++
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 0f355dac7813..c0f33d4baddf 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -48,6 +48,30 @@ void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm)
>  }
>  EXPORT_SYMBOL_NS_GPL(ad_sd_set_comm, "IIO_AD_SIGMA_DELTA");
>  
> +/**
> + * ad_sd_assert_cs() - Assert chip select line
> + *
> + * @sigma_delta: The sigma delta device
> + *
> + * Returns 0 on success, an error code otherwise.
> + **/
> +int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta)
> +{
> +	struct spi_transfer t = {
> +		.len = 0,
> +		.cs_change = sigma_delta->keep_cs_asserted,
> +	};
> +	struct spi_message m;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +
> +	if (sigma_delta->bus_locked)
> +		return spi_sync_locked(sigma_delta->spi, &m);
> +	return spi_sync(sigma_delta->spi, &m);
> +}
> +EXPORT_SYMBOL_NS_GPL(ad_sd_assert_cs, IIO_AD_SIGMA_DELTA);
> +
>  /**
>   * ad_sd_write_reg() - Write a register
>   *
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 417073c52380..99ab56d04793 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -178,6 +178,7 @@ static inline int ad_sigma_delta_postprocess_sample(struct ad_sigma_delta *sd,
>  }
>  
>  void ad_sd_set_comm(struct ad_sigma_delta *sigma_delta, uint8_t comm);
> +int ad_sd_assert_cs(struct ad_sigma_delta *sigma_delta);
>  int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
>  	unsigned int size, unsigned int val);
>  int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,

Re: [PATCH v1 2/3] dt-bindings: iio: adc: add AD7191

[Jonathan Cameron]

On Sun, 22 Dec 2024 14:48:08 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Dec 21, 2024 at 05:56:01PM +0200, 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>

Maybe I'm over thinking things, but comments inline about possibility of
pinstrapping holding this device in a particular configuration, without
the GPIOS connected.

> > ---
> >  .../bindings/iio/adc/adi,ad7191.yaml          | 128 ++++++++++++++++++
> >  MAINTAINERS                                   |   7 +
> >  2 files changed, 135 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..f3e596918c22
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> > @@ -0,0 +1,128 @@
> > +# 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.  
> 
> Without clock-names, how do you tell the difference between driven ctal and
> the cmos-compatible clock? That CLKSEL's job?

Seems it's an unusual part and there is no config associated with whether we
have a clock or an xtal, so we probably don't need the name.

Related to that, in many cases I'd expect clksel to be tied to always
use the external clock or not (depending on whether one is connected)
not to be a gpio.  So you probably need to take that configuration into
account as well.

Similar may apply for the odr, and pga pins.  Sometimes people
hardwire those things.  There are examples in tree (I can't point at one
right now though) that deal with this. Fairly sure at least 1 ADI part
has a binding to handle that.  (the ad7606 does a bit of this as it needs
a particular pinstrap for sw-mode).

You should be fine with chan and temp always being GPIOs as it would be weird
to buy a part with the extra channels + temperature sensor and not wire it
to be useable.

> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  avdd-supply:
> > +    description: AVdd voltage supply
> > +
> > +  dvdd-supply:
> > +    description: DVdd voltage supply
> > +
> > +  vref-supply:
> > +    description: Vref voltage supply
> > +
> > +  odr1-gpios:
> > +    description: GPIO connected to ODR1 pin for output data rate selection
> > +    maxItems: 1
> > +
> > +  odr2-gpios:
> > +    description: GPIO connected to ODR2 pin for output data rate selection
> > +    maxItems: 1
> > +
> > +  pga1-gpios:
> > +    description: GPIO connected to PGA1 pin for gain selection
> > +    maxItems: 1
> > +
> > +  pga2-gpios:
> > +    description: GPIO connected to PGA2 pin for gain selection
> > +    maxItems: 1
> > +
> > +  temp-gpios:
> > +    description: GPIO connected to TEMP pin for temperature sensor enable
> > +    maxItems: 1
> > +
> > +  chan-gpios:
> > +    description: GPIO connected to CHAN pin for input channel selection
> > +    maxItems: 1
> > +
> > +  clksel-gpios:
> > +    description: GPIO connected to CLKSEL pin for clock source selection
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - avdd-supply
> > +  - dvdd-supply
> > +  - vref-supply
> > +  - spi-cpol
> > +  - spi-cpha  
> 
> > +  - odr1-gpios
> > +  - odr2-gpios
> > +  - pga1-gpios
> > +  - pga2-gpios  
> 
> For these 4, since all are required, seems like grouping as odr-pgios
> and pga-gpios would be a good idea?
Agreed except for the annoying option of pin strapping.  Maybe we ignore that
for now.  If it becomes a problem, we can add it safely as a driver predating
that will try to grab the gpios and fail if it sees a DT not providing them.
So will fail safe before we add pinstrapping.  Maybe we will never need to.

Jonathan

Re: [PATCH v1 3/3] iio: adc: ad7191: add AD7191

[Jonathan Cameron]

On Sat, 21 Dec 2024 17:56:02 +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,

Comments inline.

Note the date may be right for v2 (if there are significant changes),
but it's wrong now and you should never put a copyright date in the future.

Happy holidays and new year! :)

Jonathan


> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 849c90203071..434610af3d39 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -112,6 +112,17 @@ 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.
> +	  If unsure, say N (but it's safe to say "Y").

It does no harm, so get rid of this sentence.

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

> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..6509e974072a
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> @@ -0,0 +1,513 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7191 ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.

Impressive claim in 2024! :)

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

Sometimes we pull out the IIO ones, but not normally this
mix of other subsystem specific ones. I'd just put the lot
in alphabetical order (maybe with the two iio headers in an extra
block like this one).

> +
> +#define AD7191_NAME			"ad7191"
Same comment as I've given to a lot of drivers recently. 
It isn't a magic constant and the define just makes it harder to review
as I need to come and find it.  Just put the string inline.
There is no particular reason the driver name and iio_dev->name should
be the same, so it is clearer to me if I can just see the string in each
place.

> +
> +#define AD7191_TEMP_CODES_PER_DEGREE	2815
> +
> +#define AD7191_EXT_FREQ_HZ_MIN		2457600
> +#define AD7191_EXT_FREQ_HZ_MAX		5120000
> +#define AD7191_INT_FREQ_HZ		4915200
> +
> +#define AD7191_CHAN_MASK		BIT(0)
> +#define AD7191_TEMP_MASK		BIT(1)
> +
> +#define AD7191_PGA1_MASK		BIT(0)
> +#define AD7191_PGA2_MASK		BIT(1)
> +
> +#define AD7191_ODR1_MASK		BIT(0)
> +#define AD7191_ODR2_MASK		BIT(1)
> +
> +#define AD7191_CH_AIN1_AIN2		0
> +#define AD7191_CH_AIN3_AIN4		1
> +#define AD7191_CH_TEMP			2

These are arbitrary assigned values I think?  So use an enum.

> +
> +/* NOTE:
/*
 * 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_desc		*odr1_gpio;
> +	struct gpio_desc		*odr2_gpio;

Even if you don't do what Conor suggested in the DT you
can just make these a few 2 element arrays to reduce code length here.
	struct gpio_desc		*odr_gpio[2];
etc

> +	struct gpio_desc		*pga1_gpio;
> +	struct gpio_desc		*pga2_gpio;
> +	struct gpio_desc		*temp_gpio;
> +	struct gpio_desc		*chan_gpio;
> +	struct gpio_desc		*clksel_gpio;
> +
> +	u16				int_vref_mv;
> +	u8				gain_index;
> +	u32				scale_avail[4][2];
> +	u8				samp_freq_index;
> +	u32				samp_freq_avail[4];
> +
> +	struct clk			*mclk;
As for fclk. Not used after the initial function that fills it in.
Just use a local variable there until you have need of it somewhere else in the driver.

> +	u32				fclk;
Not sure what fclk is for as you don't use it.

> +};
> +
> +static struct ad7191_state *ad_sigma_delta_to_ad7191(struct ad_sigma_delta *sd)
> +{
> +	return container_of(sd, struct ad7191_state, sd);

We normally just do

#define ad_sigma_delta_to_ad7191(sigmad) container_of((sigmad), struct ad7191_state, sd)

Being careful with the parameter naming to not hit that second sd.
I guess a function is fine if you strongly prefer it though.

> +}

> +
> +static const struct ad_sigma_delta_info ad7191_sigma_delta_info = {
> +	.set_channel = ad7191_set_channel,
> +	.set_mode = ad7191_set_mode,
> +	.has_registers = false,
> +	.irq_flags = IRQF_TRIGGER_FALLING,

I thought we'd fixed this up so that the irq direction can be left to the
firmware?  That is the right thing to do if the ad_sigma_delta library
works with that.

> +};

> +static int ad7191_clock_setup(struct ad7191_state *st)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	if (!device_property_present(dev, "mclk")) {
> +		gpiod_set_value(st->clksel_gpio, 1);
> +		st->fclk = AD7191_INT_FREQ_HZ;
> +		return 0;
> +	}
> +
> +	gpiod_set_value(st->clksel_gpio, 0);

Is there some sequencing requirement in here that stops you just trying
to get the clock and checking for -ENODEV to decide there isn't one?

> +
> +	st->mclk = devm_clk_get_enabled(dev, "mclk");
> +	if (IS_ERR(st->mclk))
> +		return dev_err_probe(dev, PTR_ERR(st->mclk),
> +				     "Failed to get clock source\n");
> +
> +	st->fclk = clk_get_rate(st->mclk);

st->fclk is only used in here

> +	if (!ad7191_valid_external_frequency(st->fclk))

Typically we don't consider it a drivers problem to make sure a correct clock
is wired up.  So why check here?  If it's a configurable clock we should
perhaps be setting clk_set_max_rate() but I'm not sure what that does
for a fixed rate clock.  Perhaps do some experiments just to check.
  

> +		return dev_err_probe(dev, -EINVAL,
> +				     "External clock frequency out of bounds\n");
> +
> +	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;
> +	int gain[4] = {1, 8, 64, 128};

	const int gain[4] = { 1, 8, 64, 128 };

> +	int i;
> +	int ret;

	int i, ret;


> +
> +	ret = ad7191_init_regulators(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_init_gpios(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_clock_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	st->samp_freq_avail[0] = 120;
> +	st->samp_freq_avail[1] = 60;
> +	st->samp_freq_avail[2] = 50;
> +	st->samp_freq_avail[3] = 10;

Perhaps a few comments on what maths this is working out.

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

Combine some of these parameters onto one line.

> +{
> +	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:

I'd use a guard here as well, but doesn't really matter.

> +			mutex_lock(&st->lock);
> +			*val = st->scale_avail[st->gain_index][0];
> +			*val2 = st->scale_avail[st->gain_index][1];
> +			mutex_unlock(&st->lock);
> +			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->samp_freq_index];
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7191_set_gain(struct ad7191_state *st, u8 gain_index)
> +{
> +	u8 pga1_gpio_val, pga2_gpio_val;
> +
> +	if (!FIELD_FIT(AD7191_PGA1_MASK | AD7191_PGA2_MASK, gain_index))
> +		return -EINVAL;

As below.  Seems overkill to protect when we control the input.

> +
> +	st->gain_index = gain_index;
> +
> +	pga1_gpio_val = FIELD_GET(AD7191_PGA1_MASK, gain_index);
> +	pga2_gpio_val = FIELD_GET(AD7191_PGA2_MASK, gain_index);
> +
> +	gpiod_set_value(st->pga1_gpio, pga1_gpio_val);
> +	gpiod_set_value(st->pga2_gpio, pga2_gpio_val);
> +
> +	return 0;
> +}
> +
> +static int ad7191_set_samp_freq(struct ad7191_state *st, u8 samp_freq_index)
> +{
> +	u8 odr1_gpio_val, odr2_gpio_val;
> +
> +	if (!FIELD_FIT(AD7191_ODR1_MASK | AD7191_ODR2_MASK, samp_freq_index))
> +		return -EINVAL;

Why do you need this check?  Haven't you already explicitly matched to get
the samp_freq_index?

> +
> +	st->samp_freq_index = samp_freq_index;
> +
> +	odr1_gpio_val = FIELD_GET(AD7191_ODR1_MASK, samp_freq_index);
> +	odr2_gpio_val = FIELD_GET(AD7191_ODR2_MASK, samp_freq_index);
> +
> +	gpiod_set_value(st->odr1_gpio, odr1_gpio_val);
> +	gpiod_set_value(st->odr2_gpio, odr2_gpio_val);
> +
> +	return 0;
> +}
> +
> +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, i;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = -EINVAL;
> +		mutex_lock(&st->lock);
	case IIO_CHAN_INFO_SCALE: {

		ret = -EINVAL;

		guard(mutex)(&st->lock);
		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
			if (val != st->scale_avail[i][1])
				continue;
			ret = ad7191_set_gain(st, i);
			break;
		}
		
Maybe also factor out the switch statement into __ad7191_write_raw()
so that you can do direct returns to simplify the code and still always
release the direct mode claim.


			
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
> +			if (val2 == st->scale_avail[i][1]) {
> +				ret = ad7191_set_gain(st, i);
> +				break;
> +			}
> +		mutex_unlock(&st->lock);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!val) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		mutex_lock(&st->lock);
> +		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++)
> +			if (val == st->samp_freq_avail[i]) {
> +				ret = ad7191_set_samp_freq(st, i);
> +				break;
> +			}
> +		mutex_unlock(&st->lock);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
> +}

> +
> +#define __AD719x_CHANNEL(_si, _channel1, _channel2, _address, _type, \

Do you ever use ->address?  If not, don't set it.

> +	_differential, _mask_type_av) \
> +	{ \
> +		.type = (_type), \
> +		.differential = _differential, \
> +		.indexed = 1, \
> +		.channel = (_channel1), \
> +		.channel2 = (_channel2), \
> +		.address = (_address), \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			BIT(IIO_CHAN_INFO_OFFSET), \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +		.info_mask_shared_by_type_available = (_mask_type_av), \
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +		.scan_index = (_si), \
> +		.scan_type = { \
> +			.sign = 'u', \
> +			.realbits = 24, \
> +			.storagebits = 24, \
> +			.endianness = IIO_BE, \
> +		}, \
> +	}
> +
> +#define AD719x_DIFF_CHANNEL(_si, _channel1, _channel2, _address) \
> +	__AD719x_CHANNEL(_si, _channel1, _channel2, _address, IIO_VOLTAGE, 1, \
> +		BIT(IIO_CHAN_INFO_SCALE))
> +
> +#define AD719x_TEMP_CHANNEL(_si, _address) \
> +	__AD719x_CHANNEL(_si, 0, -1, _address, IIO_TEMP, 0, 0)

Channel 2 isn't used, but setting it to -1 is odd. Given there
is only one of this type I'd just put the full structure setup inline where
you use this macro.

Actually given there are only 2 ADC channels. I'd put those in inline as well.
Slightly more duplication but no need to figure out the macros to see what
ends up where.  Injecting the chanenl number in the TEMP_CHANNEL macro for
instance makes little sense

i.e.
static const struct iio_chan_spec ad7191_channels[] = {
	{
		.type = IIO_TEMP,
//no obvious reason to index temp.
		.address = AD7191_CH_TEMP,
		.info_mask_separate = ...
	}, {
		.type = IIO_VOLTAGE,
		.differential = 1,
		.indexed = 1,
//unusual to index from 1 but not technically wrong.  Just unusual.
		.channel = 1,
		.channel = 2,
etc


> +
> +static const struct iio_chan_spec ad7191_channels[] = {
> +	AD719x_TEMP_CHANNEL(0, AD7191_CH_TEMP),
> +	AD719x_DIFF_CHANNEL(1, 1, 2, AD7191_CH_AIN1_AIN2),
> +	AD719x_DIFF_CHANNEL(2, 3, 4, AD7191_CH_AIN3_AIN4),
> +	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);
> +
> +	mutex_init(&st->lock);
For new drivers, I'd prefer we use
	ret = devm_mutex_init(&st->lock);
	if (ret)
		return ret;

It only does anything useful in a tiny number of debug cases, but now we
have that autocleanup, we might as well do it.

> +
> +	indio_dev->name = AD7191_NAME;
> +	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 struct spi_driver ad7191_driver = {
> +	.driver = {
> +		.name	= AD7191_NAME,
> +		.of_match_table = ad7191_of_match,
> +	},
> +	.probe		= ad7191_probe,

Given alignment never works well (as seen for name and of_match_table
just don't do it at all.  One space before = rather than tabs is fine.

Need the id_table as well for autoprobing to work (I think that is
still true for SPI).

> +};
> +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 v1 2/3] dt-bindings: iio: adc: add AD7191

[Conor Dooley]

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

On Sun, Dec 22, 2024 at 06:18:57PM +0000, Jonathan Cameron wrote:
> On Sun, 22 Dec 2024 14:48:08 +0000
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Sat, Dec 21, 2024 at 05:56:01PM +0200, 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>
> 
> Maybe I'm over thinking things, but comments inline about possibility of
> pinstrapping holding this device in a particular configuration, without
> the GPIOS connected.
> 
> > > ---
> > >  .../bindings/iio/adc/adi,ad7191.yaml          | 128 ++++++++++++++++++
> > >  MAINTAINERS                                   |   7 +
> > >  2 files changed, 135 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..f3e596918c22
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> > > @@ -0,0 +1,128 @@
> > > +# 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.  
> > 
> > Without clock-names, how do you tell the difference between driven ctal and
> > the cmos-compatible clock? That CLKSEL's job?
> 
> Seems it's an unusual part and there is no config associated with whether we
> have a clock or an xtal, so we probably don't need the name.
> 
> Related to that, in many cases I'd expect clksel to be tied to always
> use the external clock or not (depending on whether one is connected)
> not to be a gpio.  So you probably need to take that configuration into
> account as well.
> 
> Similar may apply for the odr, and pga pins.  Sometimes people
> hardwire those things.  There are examples in tree (I can't point at one
> right now though) that deal with this. Fairly sure at least 1 ADI part
> has a binding to handle that.  (the ad7606 does a bit of this as it needs
> a particular pinstrap for sw-mode).
> 
> You should be fine with chan and temp always being GPIOs as it would be weird
> to buy a part with the extra channels + temperature sensor and not wire it
> to be useable.
> 
> > 
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  avdd-supply:
> > > +    description: AVdd voltage supply
> > > +
> > > +  dvdd-supply:
> > > +    description: DVdd voltage supply
> > > +
> > > +  vref-supply:
> > > +    description: Vref voltage supply
> > > +
> > > +  odr1-gpios:
> > > +    description: GPIO connected to ODR1 pin for output data rate selection
> > > +    maxItems: 1
> > > +
> > > +  odr2-gpios:
> > > +    description: GPIO connected to ODR2 pin for output data rate selection
> > > +    maxItems: 1
> > > +
> > > +  pga1-gpios:
> > > +    description: GPIO connected to PGA1 pin for gain selection
> > > +    maxItems: 1
> > > +
> > > +  pga2-gpios:
> > > +    description: GPIO connected to PGA2 pin for gain selection
> > > +    maxItems: 1
> > > +
> > > +  temp-gpios:
> > > +    description: GPIO connected to TEMP pin for temperature sensor enable
> > > +    maxItems: 1
> > > +
> > > +  chan-gpios:
> > > +    description: GPIO connected to CHAN pin for input channel selection
> > > +    maxItems: 1
> > > +
> > > +  clksel-gpios:
> > > +    description: GPIO connected to CLKSEL pin for clock source selection
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - avdd-supply
> > > +  - dvdd-supply
> > > +  - vref-supply
> > > +  - spi-cpol
> > > +  - spi-cpha  
> > 
> > > +  - odr1-gpios
> > > +  - odr2-gpios
> > > +  - pga1-gpios
> > > +  - pga2-gpios  
> > 
> > For these 4, since all are required, seems like grouping as odr-pgios
> > and pga-gpios would be a good idea?
> Agreed except for the annoying option of pin strapping.  Maybe we ignore that
> for now.  If it becomes a problem, we can add it safely as a driver predating
> that will try to grab the gpios and fail if it sees a DT not providing them.
> So will fail safe before we add pinstrapping.  Maybe we will never need to.

To me, it's a cosmetic nicety to merge them, so I'm happy with any valid
reason to not do so.

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

Re: [PATCH v1 2/3] dt-bindings: iio: adc: add AD7191

[Krzysztof Kozlowski]

On Sat, Dec 21, 2024 at 05:56:01PM +0200, 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>

Sob mismatch.

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also 'scripts/checkpatch.pl --strict' and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

> +  odr1-gpios:
> +    description: GPIO connected to ODR1 pin for output data rate selection

a/GPIO connected to//
That's just obvious, cannot be "SPI MOSI line connected to ODR1 pin",
right? Samee in all other places.

> +    maxItems: 1
> +
> +  odr2-gpios:
> +    description: GPIO connected to ODR2 pin for output data rate selection
> +    maxItems: 1
> +
> +  pga1-gpios:
> +    description: GPIO connected to PGA1 pin for gain selection
> +    maxItems: 1
> +
> +  pga2-gpios:
> +    description: GPIO connected to PGA2 pin for gain selection
> +    maxItems: 1
> +
> +  temp-gpios:
> +    description: GPIO connected to TEMP pin for temperature sensor enable
> +    maxItems: 1
> +
> +  chan-gpios:
> +    description: GPIO connected to CHAN pin for input channel selection
> +    maxItems: 1
> +
> +  clksel-gpios:
> +    description: GPIO connected to CLKSEL pin for clock source selection
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - avdd-supply
> +  - dvdd-supply
> +  - vref-supply
> +  - spi-cpol
> +  - spi-cpha
> +  - odr1-gpios
> +  - odr2-gpios
> +  - pga1-gpios
> +  - pga2-gpios
> +  - temp-gpios
> +  - chan-gpios
> +  - clksel-gpios
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi0 {

spi

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "adi,ad7191";

Best regards,
Krzysztof

Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

[Alisa-Dariana Roman]

On Sun, Dec 22, 2024 at 06:07:13PM +0000, Jonathan Cameron wrote:
> On Sat, 21 Dec 2024 17:56:00 +0200
> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> 
> > Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
> > start conversion when CS is asserted. Add helper function to support
> > this use case by allowing devices to assert CS without performing
> > register operations.
> Hi Alisa-Dariana,
> 
> I had a look at the ad7191 datasheet. Given this description,
> I was expecting to see it do a pre pulse of the chip select to trigger
> the acquisition.  However, what I see is a power down line (which is more
> or less a chip select) but it just has a specified t1 delay before the
> DOUT will change to the state for the first bit and the host
> can start driving the clock.
> 
> That can be done by setting spi_device->cs_setup to whatever delay is
> needed.  The text is spi_device docs are a little vague,
> but I'd take it as t1 + t2 (maybe t3 to be safe).
> 
> That is going to be more reliable than trying to hold the cs across
> messages / spi_sync() calls, particularly if the bus might not be
> locked (which the code below suggests).
> 
> Jonathan
> 
> 

Hello Jonathan! I am grateful for your and everyone's feedback, as
always!

I got a bit stuck on this part. The motivation for adding this function
is as following:

int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
	const struct iio_chan_spec *chan, int *val)
{

...
	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);

	ad_sd_enable_irq(sigma_delta);
	ret = wait_for_completion_interruptible_timeout(
			&sigma_delta->completion, HZ);
...
}

I noticed that adc drivers need to call the ad_sd_write_reg function in
their callback set_mode function, in order to keep the cs line pulled
down before waiting for the interrupt (if I understand correctly). But
since this component and AD7780 have no register I just copied the
functionality of ad_sd_write_reg without actually writing anything.

Should I change the description/name to more accurately present the
functionality? Or would it be a better idea to not use the single
conversion function and write something from scratch leveraging the
cs_setup?

Thank you!

Kind regards,
Alisa-Dariana Roman.

Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

[David Lechner]

On 1/21/25 3:36 AM, Alisa-Dariana Roman wrote:
> On Sun, Dec 22, 2024 at 06:07:13PM +0000, Jonathan Cameron wrote:
>> On Sat, 21 Dec 2024 17:56:00 +0200
>> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
>>
>>> Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
>>> start conversion when CS is asserted. Add helper function to support
>>> this use case by allowing devices to assert CS without performing
>>> register operations.
>> Hi Alisa-Dariana,
>>
>> I had a look at the ad7191 datasheet. Given this description,
>> I was expecting to see it do a pre pulse of the chip select to trigger
>> the acquisition.  However, what I see is a power down line (which is more
>> or less a chip select) but it just has a specified t1 delay before the
>> DOUT will change to the state for the first bit and the host
>> can start driving the clock.
>>
>> That can be done by setting spi_device->cs_setup to whatever delay is
>> needed.  The text is spi_device docs are a little vague,
>> but I'd take it as t1 + t2 (maybe t3 to be safe).
>>
>> That is going to be more reliable than trying to hold the cs across
>> messages / spi_sync() calls, particularly if the bus might not be
>> locked (which the code below suggests).
>>
>> Jonathan
>>
>>
> 
> Hello Jonathan! I am grateful for your and everyone's feedback, as
> always!
> 
> I got a bit stuck on this part. The motivation for adding this function
> is as following:
> 
> int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
> 	const struct iio_chan_spec *chan, int *val)
> {
> 
> ...
> 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
> 
> 	ad_sd_enable_irq(sigma_delta);
> 	ret = wait_for_completion_interruptible_timeout(
> 			&sigma_delta->completion, HZ);
> ...
> }
> 
> I noticed that adc drivers need to call the ad_sd_write_reg function in
> their callback set_mode function, in order to keep the cs line pulled
> down before waiting for the interrupt (if I understand correctly). But
> since this component and AD7780 have no register I just copied the
> functionality of ad_sd_write_reg without actually writing anything.
> 
> Should I change the description/name to more accurately present the
> functionality? Or would it be a better idea to not use the single
> conversion function and write something from scratch leveraging the
> cs_setup?

If the RDY interrupt handling wasn't so tricky, I would suggest to just
make a separate function. But to avoid duplicating that tricky code I
think using the existing function would be best. I think you have mostly
the right idea here. Here is how I would try to do it...

1)

	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);

In the implementation of this callback, call spi_bus_lock(), then do
the SPI xfer with no data that has cs_change set so that CS does not
deassert (using spi_sync_locked() since we manually control the lock).

2)

This is the main part of your question, I think. In this part of the
function...

	if (sigma_delta->info->data_reg != 0)
		data_reg = sigma_delta->info->data_reg;
	else
		data_reg = AD_SD_REG_DATA;

	ret = ad_sd_read_reg(sigma_delta, data_reg,
		DIV_ROUND_UP(chan->scan_type.realbits + chan->scan_type.shift, 8),
		&raw_sample);

I would add a new flag or create a sentinel value for sigma_delta->info->data_reg
(e.g. #define AD_SD_NO_REG ~0U) that indicates that this chip doesn't have registers.

Then modify the if statement a bit so that if the chip has registers, call the
existing ad_sd_read_reg() function or if the chip doesn't have registers, call
a new function that reads one sample and has cs_change set on the last SPI xfer
so that CS still does not deassert.

This way, we don't have to mess with modifying ad_sd_read_reg() to not read
a register and avoid the naming issue. :-)

3)

	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);

In the callback for this function, do an empty SPI xfer so that CS finally
deasserts. Then call spi_bus_unlock() to release the lock that was taken
earlier.


---

Also, thinking outside the box, could we use a GPIO instead of connecting
SPI CS to the powerdown pin? The DT bindings already have a powerdown-gpios
binding for that. The could simplify things a bit.

With this, the set_mode callback would just be poking the GPIO instead of
dealing with the SPI CS line. But otherwise would be the same as above.

Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

[Alisa-Dariana Roman]

On Tue, Jan 21, 2025 at 04:32:56PM -0600, David Lechner wrote:
> On 1/21/25 3:36 AM, Alisa-Dariana Roman wrote:
> > On Sun, Dec 22, 2024 at 06:07:13PM +0000, Jonathan Cameron wrote:
> >> On Sat, 21 Dec 2024 17:56:00 +0200
> >> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> >>
> >>> Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
> >>> start conversion when CS is asserted. Add helper function to support
> >>> this use case by allowing devices to assert CS without performing
> >>> register operations.
> >> Hi Alisa-Dariana,
> >>
> >> I had a look at the ad7191 datasheet. Given this description,
> >> I was expecting to see it do a pre pulse of the chip select to trigger
> >> the acquisition.  However, what I see is a power down line (which is more
> >> or less a chip select) but it just has a specified t1 delay before the
> >> DOUT will change to the state for the first bit and the host
> >> can start driving the clock.
> >>
> >> That can be done by setting spi_device->cs_setup to whatever delay is
> >> needed.  The text is spi_device docs are a little vague,
> >> but I'd take it as t1 + t2 (maybe t3 to be safe).
> >>
> >> That is going to be more reliable than trying to hold the cs across
> >> messages / spi_sync() calls, particularly if the bus might not be
> >> locked (which the code below suggests).
> >>
> >> Jonathan
> >>
> >>
> > 
> > Hello Jonathan! I am grateful for your and everyone's feedback, as
> > always!
> > 
> > I got a bit stuck on this part. The motivation for adding this function
> > is as following:
> > 
> > int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
> > 	const struct iio_chan_spec *chan, int *val)
> > {
> > 
> > ...
> > 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
> > 
> > 	ad_sd_enable_irq(sigma_delta);
> > 	ret = wait_for_completion_interruptible_timeout(
> > 			&sigma_delta->completion, HZ);
> > ...
> > }
> > 
> > I noticed that adc drivers need to call the ad_sd_write_reg function in
> > their callback set_mode function, in order to keep the cs line pulled
> > down before waiting for the interrupt (if I understand correctly). But
> > since this component and AD7780 have no register I just copied the
> > functionality of ad_sd_write_reg without actually writing anything.
> > 
> > Should I change the description/name to more accurately present the
> > functionality? Or would it be a better idea to not use the single
> > conversion function and write something from scratch leveraging the
> > cs_setup?
> 
> If the RDY interrupt handling wasn't so tricky, I would suggest to just
> make a separate function. But to avoid duplicating that tricky code I
> think using the existing function would be best. I think you have mostly
> the right idea here. Here is how I would try to do it...
> 
> 1)
> 
> 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
> 
> In the implementation of this callback, call spi_bus_lock(), then do
> the SPI xfer with no data that has cs_change set so that CS does not
> deassert (using spi_sync_locked() since we manually control the lock).
> 
> 2)
> 
> This is the main part of your question, I think. In this part of the
> function...
> 
> 	if (sigma_delta->info->data_reg != 0)
> 		data_reg = sigma_delta->info->data_reg;
> 	else
> 		data_reg = AD_SD_REG_DATA;
> 
> 	ret = ad_sd_read_reg(sigma_delta, data_reg,
> 		DIV_ROUND_UP(chan->scan_type.realbits + chan->scan_type.shift, 8),
> 		&raw_sample);
> 
> I would add a new flag or create a sentinel value for sigma_delta->info->data_reg
> (e.g. #define AD_SD_NO_REG ~0U) that indicates that this chip doesn't have registers.
> 
> Then modify the if statement a bit so that if the chip has registers, call the
> existing ad_sd_read_reg() function or if the chip doesn't have registers, call
> a new function that reads one sample and has cs_change set on the last SPI xfer
> so that CS still does not deassert.
> 
> This way, we don't have to mess with modifying ad_sd_read_reg() to not read
> a register and avoid the naming issue. :-)
> 
> 3)
> 
> 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> 
> In the callback for this function, do an empty SPI xfer so that CS finally
> deasserts. Then call spi_bus_unlock() to release the lock that was taken
> earlier.
> 
> 
> ---
> 
> Also, thinking outside the box, could we use a GPIO instead of connecting
> SPI CS to the powerdown pin? The DT bindings already have a powerdown-gpios
> binding for that. The could simplify things a bit.
> 
> With this, the set_mode callback would just be poking the GPIO instead of
> dealing with the SPI CS line. But otherwise would be the same as above.
> 

Hello, David! I really appreciate your suggestions! Things look a lot
clearer.

Regarding point 2) I looked a bit further into the read function and the
ad_sd_read_reg_raw() function seems to handle no register components as
you suggested:

...
			.cs_change = sigma_delta->bus_locked,
...
	if (sigma_delta->info->has_registers) {
		data[0] = reg << sigma_delta->info->addr_shift;
		data[0] |= sigma_delta->info->read_mask;
		data[0] |= sigma_delta->comm;
		spi_message_add_tail(&t[0], &m);
	}
	spi_message_add_tail(&t[1], &m);
...

So I will handle the AD_SD_MODE_SINGLE and AD_SD_MODE_IDLE cases in the
callback function by doing empty SPI xfers as you said. The bus seems to
be already locked before all the ad_sigma_delta_set_mode() and unlocked
after, so I think I can skip this part.

I will follow with the second version as soon as possible if this setup
looks alright!

---

I initially used a GPIO for the powerdown pin, but the first interrupt
was somehow always getting lost.

Kind regards,
Alisa-Dariana Roman.