[PATCH 0/2] iio: adc: add new ad7380 driver

[PATCH 0/2] iio: adc: add new ad7380 driver

[David Lechner]

This series is adding a new driver for the Analog Devices Inc. AD7380,
AD7381, AD7383, and AD7384 ADCs. These chips are part of a family of
simultaneous sampling SAR ADCs.

To keep things simple, the initial driver implementation only supports
the 2-channel differential chips listed above. There are also 4-channel
differential chips and 4-channel single-ended chips in the family that
can be added later.

Furthermore, the driver is just implementing basic support for capturing
data. Additional features like interrupts, CRC, etc. can be added later.

Also, FYI, this driver will also be used as the base for an upcoming
series adding AXI SPI Engine offload support for these chips along with
[1].

This work is being done by BayLibre and on behalf of Analog Devices Inc.
hence the maintainers are @analog.com.

[1]: https://lore.kernel.org/linux-spi/20231204-axi-spi-engine-series-2-v1-0-063672323fce@baylibre.com/

---
David Lechner (2):
      dt-bindings: iio: adc: Add binding for AD7380 ADCs
      iio: adc: ad7380: new driver for AD7380 ADCs

 .../devicetree/bindings/iio/adc/adi,ad7380.yaml    | 102 +++++
 MAINTAINERS                                        |  10 +
 drivers/iio/adc/Kconfig                            |  16 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ad7380.c                           | 467 +++++++++++++++++++++
 5 files changed, 596 insertions(+)
---
base-commit: 18f78b5e609b19b56237f0dae47068d44b8b0ecd
change-id: 20231208-ad7380-mainline-e6c4fa7dbedd

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/2] dt-bindings: iio: adc: Add binding for AD7380 ADCs

[David Lechner]

This adds a binding specification for the Analog Devices Inc. AD7380
family of ADCs.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7380.yaml    | 102 +++++++++++++++++++++
 MAINTAINERS                                        |   9 ++
 2 files changed, 111 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
new file mode 100644
index 000000000000..e9a0b72cd9d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -0,0 +1,102 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7380.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices Simultaneous Sampling Analog to Digital Converters
+
+maintainers:
+  - Michael Hennerich <Michael.Hennerich@analog.com>
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  * https://www.analog.com/en/products/ad7380.html
+  * https://www.analog.com/en/products/ad7381.html
+  * https://www.analog.com/en/products/ad7383.html
+  * https://www.analog.com/en/products/ad7384.html
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7380
+      - adi,ad7381
+      - adi,ad7383
+      - adi,ad7384
+
+  reg: true
+
+  spi-max-frequency:
+    maximum: 80000000
+  spi-cpol: true
+  spi-cpha: true
+
+  adi,sdo-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ 1-wire, 2-wire ]
+    description:
+      In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
+      pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
+      is read from SDOA and data for input B is read from SDOB/ALERT (and the
+      ALERT interrupt signal is not available).
+
+  vcc-supply:
+    description: A 3V to 3.6V supply that powers the chip.
+
+  vlogic-supply:
+    description:
+      A 1.65V to 3.6V supply for the logic pins.
+
+  refio-supply:
+    description:
+      A 2.5V to 3.3V supply for the external reference voltage. When omitted,
+      the internal 2.5V reference is used.
+
+  interrupts:
+    description:
+      When the device is using 1-wire mode, this property is used to optionally
+      specify the ALERT interrupt.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - adi,sdo-mode
+  - vcc-supply
+  - vlogic-supply
+
+allOf:
+  - if:
+      properties:
+        adi,sdo-mode:
+          const: 2-wire
+    then:
+      properties:
+        interrupts: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad7380";
+            reg = <0>;
+
+            spi-cpol;
+            spi-cpha;
+            spi-max-frequency = <80000000>;
+
+            adi,sdo-mode = "1-wire";
+            interrupts = <27>;
+
+            vcc-supply = <&supply_3_3V>;
+            vlogic-supply = <&supply_3_3V>;
+            refio-supply = <&supply_2_5V>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index fe1f6f97f96a..e2a998be5879 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -430,6 +430,15 @@ W:	http://wiki.analog.com/AD7142
 W:	https://ez.analog.com/linux-software-drivers
 F:	drivers/input/misc/ad714x.c
 
+AD738X ADC DRIVER (AD7380/1/2/4)
+M:	Michael Hennerich <michael.hennerich@analog.com>
+M:	Nuno Sá <nuno.sa@analog.com>
+R:	David Lechner <dlechner@baylibre.com>
+S:	Supported
+W:	https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+
 AD7877 TOUCHSCREEN DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported

-- 
2.43.0

[PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[David Lechner]

This adds a new driver for the AD7380 family ADCs.

The driver currently implements basic support for the AD7380, AD7381,
AD7383, and AD7384 2-channel differential ADCs. Support for additional
single-ended and 4-channel chips that use the same register map as well
as additional features of the chip will be added in future patches.

Co-developed-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  16 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7380.c | 467 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 485 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e2a998be5879..5a54620a31b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -438,6 +438,7 @@ S:	Supported
 W:	https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+F:	drivers/iio/adc/ad7380.c
 
 AD7877 TOUCHSCREEN DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 35f9867da12c..cbfd626712e3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -122,6 +122,22 @@ config AD7298
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7298.
 
+config AD7380
+	tristate "Analog Devices AD7380 ADC driver"
+	depends on SPI_MASTER
+	select IIO_BUFFER
+	select IIO_TRIGGER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  AD7380 is a family of simultaneous sampling ADCs that share the same
+	  SPI register map and have similar pinouts.
+
+	  Say yes here to build support for Analog Devices AD7380 ADC and
+	  similar chips.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad7380.
+
 config AD7476
 	tristate "Analog Devices AD7476 1-channel ADCs driver and other similar devices from AD and TI"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index bee11d442af4..e046d8004f41 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
 obj-$(CONFIG_AD7292) += ad7292.o
 obj-$(CONFIG_AD7298) += ad7298.o
 obj-$(CONFIG_AD7923) += ad7923.o
+obj-$(CONFIG_AD7476) += ad7380.o
 obj-$(CONFIG_AD7476) += ad7476.o
 obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
 obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
new file mode 100644
index 000000000000..6a5ec59bd1fd
--- /dev/null
+++ b/drivers/iio/adc/ad7380.c
@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD738x Simultaneous Sampling SAR ADCs
+ *
+ * Copyright 2017 Analog Devices Inc.
+ * Copyright 2023 BayLibre, SAS
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* 2.5V internal reference voltage */
+#define AD7380_INTERNAL_REF_MV		2500
+
+/* reading and writing registers is more reliable at lower than max speed */
+#define AD7380_REG_WR_SPEED_HZ		10000000
+
+#define AD7380_REG_WR			BIT(15)
+#define AD7380_REG_REGADDR		GENMASK(14, 12)
+#define AD7380_REG_DATA			GENMASK(11, 0)
+
+#define AD7380_REG_ADDR_NOP		0x0
+#define AD7380_REG_ADDR_CONFIG1		0x1
+#define AD7380_REG_ADDR_CONFIG2		0x2
+#define AD7380_REG_ADDR_ALERT		0x3
+#define AD7380_REG_ADDR_ALERT_LOW_TH	0x4
+#define AD7380_REG_ADDR_ALERT_HIGH_TH	0x5
+
+#define AD7380_CONFIG1_OS_MODE		BIT(9)
+#define AD7380_CONFIG1_OSR		GENMASK(8, 6)
+#define AD7380_CONFIG1_CRC_W		BIT(5)
+#define AD7380_CONFIG1_CRC_R		BIT(4)
+#define AD7380_CONFIG1_ALERTEN		BIT(3)
+#define AD7380_CONFIG1_RES		BIT(2)
+#define AD7380_CONFIG1_REFSEL		BIT(1)
+#define AD7380_CONFIG1_PMODE		BIT(0)
+
+#define AD7380_CONFIG2_SDO2		GENMASK(9, 8)
+#define AD7380_CONFIG2_SDO		BIT(8)
+#define AD7380_CONFIG2_RESET		GENMASK(7, 0)
+
+#define AD7380_CONFIG2_RESET_SOFT	0x3C
+#define AD7380_CONFIG2_RESET_HARD	0xFF
+
+#define AD7380_ALERT_LOW_TH		GENMASK(11, 0)
+#define AD7380_ALERT_HIGH_TH		GENMASK(11, 0)
+
+struct ad7380_chip_info {
+	const char *name;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+};
+
+#define AD7380_DIFFERENTIAL_CHANNEL(index, bits) {		\
+	.type = IIO_VOLTAGE,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.indexed = 1,						\
+	.differential = 1,					\
+	.channel = 2 * (index),					\
+	.channel2 = 2 * (index) + 1,				\
+	.scan_index = (index),					\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = (bits),				\
+		.storagebits = 16,				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+#define DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(name, bits)	\
+static const struct iio_chan_spec name[] = {			\
+		AD7380_DIFFERENTIAL_CHANNEL(0, bits),		\
+		AD7380_DIFFERENTIAL_CHANNEL(1, bits),		\
+		IIO_CHAN_SOFT_TIMESTAMP(2),			\
+}
+
+/* fully differential */
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
+/* pseudo differential */
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
+
+static const struct ad7380_chip_info ad7380_chip_info = {
+	.name = "ad7380",
+	.channels = ad7380_channels,
+	.num_channels = ARRAY_SIZE(ad7380_channels),
+};
+
+static const struct ad7380_chip_info ad7381_chip_info = {
+	.name = "ad7381",
+	.channels = ad7381_channels,
+	.num_channels = ARRAY_SIZE(ad7381_channels),
+};
+
+static const struct ad7380_chip_info ad7383_chip_info = {
+	.name = "ad7383",
+	.channels = ad7383_channels,
+	.num_channels = ARRAY_SIZE(ad7383_channels),
+};
+
+static const struct ad7380_chip_info ad7384_chip_info = {
+	.name = "ad7384",
+	.channels = ad7384_channels,
+	.num_channels = ARRAY_SIZE(ad7384_channels),
+};
+
+struct ad7380_state {
+	const struct ad7380_chip_info *chip_info;
+	struct spi_device *spi;
+	struct regulator *vref;
+	struct regmap *regmap;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
+	 * aligned 64 bit timestamp.
+	 */
+	struct {
+		u16 raw[2];
+		s64 ts __aligned(8);
+	} scan_data __aligned(IIO_DMA_MINALIGN);
+	u16 tx[2];
+	u16 rx[2];
+};
+
+static int ad7380_regmap_reg_write(void *context, unsigned int reg,
+				   unsigned int val)
+{
+	struct ad7380_state *st = context;
+	struct spi_transfer xfer = {
+		.speed_hz = AD7380_REG_WR_SPEED_HZ,
+		.bits_per_word = 16,
+		.len = 2,
+		.tx_buf = &st->tx[0],
+	};
+
+	st->tx[0] = FIELD_PREP(AD7380_REG_WR, 1) |
+		    FIELD_PREP(AD7380_REG_REGADDR, reg) |
+		    FIELD_PREP(AD7380_REG_DATA, val);
+
+	return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static int ad7380_regmap_reg_read(void *context, unsigned int reg,
+				  unsigned int *val)
+{
+	struct ad7380_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.speed_hz = AD7380_REG_WR_SPEED_HZ,
+			.bits_per_word = 16,
+			.len = 2,
+			.tx_buf = &st->tx[0],
+			.cs_change = 1,
+			.cs_change_delay = {
+				.value = 10, /* t[CSH] */
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+		}, {
+			.speed_hz = AD7380_REG_WR_SPEED_HZ,
+			.bits_per_word = 16,
+			.len = 2,
+			.rx_buf = &st->rx[0],
+		},
+	};
+	int ret;
+
+	st->tx[0] = FIELD_PREP(AD7380_REG_WR, 0) |
+		    FIELD_PREP(AD7380_REG_REGADDR, reg) |
+		    FIELD_PREP(AD7380_REG_DATA, 0);
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret < 0)
+		return ret;
+
+	*val = FIELD_GET(AD7380_REG_DATA, st->rx[0]);
+
+	return 0;
+}
+
+const struct regmap_config ad7380_regmap_config = {
+	.reg_bits = 3,
+	.val_bits = 12,
+	.reg_read = ad7380_regmap_reg_read,
+	.reg_write = ad7380_regmap_reg_write,
+	.max_register = AD7380_REG_ADDR_ALERT_HIGH_TH,
+	.can_sleep = true,
+};
+
+static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
+				     u32 writeval, u32 *readval)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static irqreturn_t ad7380_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad7380_state *st = iio_priv(indio_dev);
+	struct spi_transfer xfer = {
+		.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
+		.len = 4,
+		.rx_buf = &st->scan_data,
+	};
+	int ret;
+
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+
+	if (ret == 0)
+		iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
+						   pf->timestamp);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int ad7380_read_direct(struct ad7380_state *st,
+			      struct iio_chan_spec const *chan, int *val)
+{
+	struct spi_transfer xfers[] = {
+		/* toggle CS (no data xfer) to trigger a conversion */
+		{
+			.speed_hz = AD7380_REG_WR_SPEED_HZ,
+			.bits_per_word = chan->scan_type.realbits,
+			.delay = {
+				.value = 190, /* t[CONVERT] */
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+			.cs_change = 1,
+			.cs_change_delay = {
+				.value = 10, /* t[CSH] */
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+		},
+		/* then read both channels */
+		{
+			.speed_hz = AD7380_REG_WR_SPEED_HZ,
+			.bits_per_word = chan->scan_type.realbits,
+			.rx_buf = &st->rx[0],
+			.len = 4,
+		},
+	};
+	int ret;
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+
+	if (ret < 0)
+		return ret;
+
+	*val = sign_extend32(st->rx[chan->scan_index],
+			     chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int ad7380_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7380_read_direct(st, chan, val);
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		if (st->vref) {
+			ret = regulator_get_voltage(st->vref);
+			if (ret < 0)
+				return ret;
+
+			*val = ret / 1000;
+		} else {
+			*val = AD7380_INTERNAL_REF_MV;
+		}
+
+		*val2 = chan->scan_type.realbits;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info ad7380_info = {
+	.read_raw = &ad7380_read_raw,
+	.debugfs_reg_access = &ad7380_debugfs_reg_access,
+};
+
+static int ad7380_init(struct ad7380_state *st)
+{
+	int ret;
+
+	/* perform hard reset */
+	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+				 AD7380_CONFIG2_RESET,
+				 FIELD_PREP(AD7380_CONFIG2_RESET,
+					    AD7380_CONFIG2_RESET_HARD));
+	if (ret < 0)
+		return ret;
+
+
+	/* select internal or external reference voltage */
+	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+				 AD7380_CONFIG1_REFSEL,
+				 FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st->vref));
+	if (ret < 0)
+		return ret;
+
+	/* SPI 1-wire mode */
+	return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+				  AD7380_CONFIG2_SDO,
+				  FIELD_PREP(AD7380_CONFIG2_SDO, 1));
+}
+
+static void ad7380_release_regulator(void *p)
+{
+	struct regulator *reg = p;
+
+	regulator_disable(reg);
+}
+
+static int ad7380_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct ad7380_state *st;
+	const char *str_val;
+	int ret;
+
+	ret = device_property_read_string(&spi->dev, "adi,sdo-mode", &str_val);
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to read adi,sdo-mode property\n");
+
+	if (strcmp(str_val, "1-wire"))
+		return dev_err_probe(&spi->dev, -EINVAL,
+				     "Only 1-wire SDO is supported\n");
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	st->chip_info = spi_get_device_match_data(spi);
+
+	st->vref = devm_regulator_get_optional(&spi->dev, "refio");
+	if (IS_ERR(st->vref)) {
+		/*
+		 * If there is no REFIO supply, then it means that we are using
+		 * the internal 2.5V reference.
+		 */
+		if (PTR_ERR(st->vref) == -ENODEV)
+			st->vref = NULL;
+		else
+			return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
+					     "Failed to get refio regulator\n");
+	}
+
+	if (st->vref) {
+		ret = regulator_enable(st->vref);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7380_release_regulator,
+					       st->vref);
+		if (ret)
+			return ret;
+	}
+
+	st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+				     "failed to allocate register map\n");
+
+
+	indio_dev->channels = st->chip_info->channels;
+	indio_dev->num_channels = st->chip_info->num_channels;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = st->chip_info->name;
+	indio_dev->info = &ad7380_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad7380_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = ad7380_init(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad7380_of_match_table[] = {
+	{ .compatible = "adi,ad7380", .data = &ad7380_chip_info },
+	{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+	{ .compatible = "adi,ad7383", .data = &ad7383_chip_info },
+	{ .compatible = "adi,ad7384", .data = &ad7384_chip_info },
+	{ }
+};
+
+static const struct spi_device_id ad7380_id_table[] = {
+	{ "ad7380", (kernel_ulong_t)&ad7380_chip_info },
+	{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+	{ "ad7383", (kernel_ulong_t)&ad7383_chip_info },
+	{ "ad7384", (kernel_ulong_t)&ad7384_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad7380_id_table);
+
+static struct spi_driver ad7380_driver = {
+	.driver = {
+		.name = "ad7380",
+		.of_match_table = ad7380_of_match_table,
+	},
+	.probe = ad7380_probe,
+	.id_table = ad7380_id_table,
+};
+module_spi_driver(ad7380_driver);
+
+MODULE_AUTHOR("Stefan Popa <stefan.popa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD738x ADC driver");
+MODULE_LICENSE("GPL");

-- 
2.43.0

Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[David Lechner]

On Fri, Dec 8, 2023 at 9:52 AM David Lechner <dlechner@baylibre.com> wrote:
>
> This adds a new driver for the AD7380 family ADCs.
>
> The driver currently implements basic support for the AD7380, AD7381,
> AD7383, and AD7384 2-channel differential ADCs. Support for additional
> single-ended and 4-channel chips that use the same register map as well
> as additional features of the chip will be added in future patches.
>
> Co-developed-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

...

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index bee11d442af4..e046d8004f41 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
>  obj-$(CONFIG_AD7292) += ad7292.o
>  obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7923) += ad7923.o
> +obj-$(CONFIG_AD7476) += ad7380.o

copy/paste oops: should be CONFIG_AD7380

>  obj-$(CONFIG_AD7476) += ad7476.o
>  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o

Re: [PATCH 1/2] dt-bindings: iio: adc: Add binding for AD7380 ADCs

[Krzysztof Kozlowski]

On 08/12/2023 16:51, David Lechner wrote:
> This adds a binding specification for the Analog Devices Inc. AD7380
> family of ADCs.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7380.yaml    | 102 +++++++++++++++++++++
>  MAINTAINERS                                        |   9 ++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> new file mode 100644
> index 000000000000..e9a0b72cd9d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7380.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Simultaneous Sampling Analog to Digital Converters
> +
> +maintainers:
> +  - Michael Hennerich <Michael.Hennerich@analog.com>
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  * https://www.analog.com/en/products/ad7380.html
> +  * https://www.analog.com/en/products/ad7381.html
> +  * https://www.analog.com/en/products/ad7383.html
> +  * https://www.analog.com/en/products/ad7384.html
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7380
> +      - adi,ad7381
> +      - adi,ad7383
> +      - adi,ad7384
> +
> +  reg: true

maxItems
(unless 256 items are really possible for this device)

> +
> +  spi-max-frequency:
> +    maximum: 80000000
> +  spi-cpol: true
> +  spi-cpha: true
> +


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: adc: Add binding for AD7380 ADCs

[Jonathan Cameron]

On Fri,  8 Dec 2023 09:51:40 -0600
David Lechner <dlechner@baylibre.com> wrote:

> This adds a binding specification for the Analog Devices Inc. AD7380
> family of ADCs.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Hi David,

Comments inline.  A question for Mark Brown on the 2-wire bit..
Do we have existing DT bindings for devices with parallel spi data
outputs?

> ---
>  .../devicetree/bindings/iio/adc/adi,ad7380.yaml    | 102 +++++++++++++++++++++
>  MAINTAINERS                                        |   9 ++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> new file mode 100644
> index 000000000000..e9a0b72cd9d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
...


> +  * https://www.analog.com/en/products/ad7380.html
> +  * https://www.analog.com/en/products/ad7381.html
> +  * https://www.analog.com/en/products/ad7383.html
> +  * https://www.analog.com/en/products/ad7384.html

> +  adi,sdo-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ 1-wire, 2-wire ]
> +    description:
> +      In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
> +      pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
> +      is read from SDOA and data for input B is read from SDOB/ALERT (and the
> +      ALERT interrupt signal is not available).

This is fun...  If I understand correctly 2-wire requires two SPI buses (or a complex
spi controller that does parallel serial channels).  What would description for that
look like in DT and can we not establish what is wanted here from that bus description
rather than an adi specific property?

Seems a bit like parallel-memories.

Mark, any insights into what we should do to describe this?

> +
> +  vcc-supply:
> +    description: A 3V to 3.6V supply that powers the chip.
> +
> +  vlogic-supply:
> +    description:
> +      A 1.65V to 3.6V supply for the logic pins.
> +
> +  refio-supply:
> +    description:
> +      A 2.5V to 3.3V supply for the external reference voltage. When omitted,
> +      the internal 2.5V reference is used.
> +
> +  interrupts:
> +    description:
> +      When the device is using 1-wire mode, this property is used to optionally
> +      specify the ALERT interrupt.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - adi,sdo-mode

Could define a default of 1-wire?  Simplifies things a little in the bindings.

> +  - vcc-supply
> +  - vlogic-supply

Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[Jonathan Cameron]

On Fri,  8 Dec 2023 09:51:41 -0600
David Lechner <dlechner@baylibre.com> wrote:

> This adds a new driver for the AD7380 family ADCs.
> 
> The driver currently implements basic support for the AD7380, AD7381,
> AD7383, and AD7384 2-channel differential ADCs. Support for additional
> single-ended and 4-channel chips that use the same register map as well
> as additional features of the chip will be added in future patches.
> 
> Co-developed-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Hi David / Stefan,

A few minor things inline.  Other than that question following through
from the bindings about 1-wire / 2-wire description it's all trivial
and some might be considered my odd tastes :)
Nice to see such a clean v1

Jonathan


> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> new file mode 100644
> index 000000000000..6a5ec59bd1fd
> --- /dev/null
> +++ b/drivers/iio/adc/ad7380.c

> +struct ad7380_state {
> +	const struct ad7380_chip_info *chip_info;
> +	struct spi_device *spi;
> +	struct regulator *vref;
> +	struct regmap *regmap;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
> +	 * aligned 64 bit timestamp.
> +	 */
> +	struct {
> +		u16 raw[2];
> +		s64 ts __aligned(8);
> +	} scan_data __aligned(IIO_DMA_MINALIGN);
> +	u16 tx[2];
> +	u16 rx[2];
> +};

...



> +static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	struct spi_transfer xfer = {
> +		.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
> +		.len = 4,
> +		.rx_buf = &st->scan_data,
I'd make it explicit you are reading into st->scan_data->raw rather than using
the address of the containing structure. 

> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +
> +	if (ret == 0)
I'm not keen on this pattern. It saves a bit of text but makes things slightly less
obvious when compared to all the other error paths.

	if (ret)
		goto error;

	iio_push_to_buffers...

error:
	iio_trigger_notify_done...

Is my preference.
> +		iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> +						   pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad7380_read_direct(struct ad7380_state *st,
> +			      struct iio_chan_spec const *chan, int *val)
> +{
> +	struct spi_transfer xfers[] = {
> +		/* toggle CS (no data xfer) to trigger a conversion */
> +		{
> +			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> +			.bits_per_word = chan->scan_type.realbits,
> +			.delay = {
> +				.value = 190, /* t[CONVERT] */
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +			.cs_change = 1,
> +			.cs_change_delay = {
> +				.value = 10, /* t[CSH] */
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +		},
> +		/* then read both channels */
> +		{
> +			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> +			.bits_per_word = chan->scan_type.realbits,
> +			.rx_buf = &st->rx[0],
> +			.len = 4,
> +		},
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +

Trivial, but no blank line here. It's good to keep error check and
the call in the same block.

> +	if (ret < 0)
> +		return ret;
> +
> +	*val = sign_extend32(st->rx[chan->scan_index],
> +			     chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +

...

> +
> +static int ad7380_init(struct ad7380_state *st)
> +{
> +	int ret;
> +
> +	/* perform hard reset */
> +	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> +				 AD7380_CONFIG2_RESET,
> +				 FIELD_PREP(AD7380_CONFIG2_RESET,
> +					    AD7380_CONFIG2_RESET_HARD));
> +	if (ret < 0)
> +		return ret;
> +
> +
Trivial: Single blank line

> +	/* select internal or external reference voltage */
> +	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> +				 AD7380_CONFIG1_REFSEL,
> +				 FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st->vref));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* SPI 1-wire mode */
> +	return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> +				  AD7380_CONFIG2_SDO,
> +				  FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> +}
> +
> +static void ad7380_release_regulator(void *p)
> +{
> +	struct regulator *reg = p;

The local variable doesn't really add anything over putting p
in the regulator_disable() call.

> +
> +	regulator_disable(reg);
> +}
> +
> +static int ad7380_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad7380_state *st;
> +	const char *str_val;
> +	int ret;
> +
> +	ret = device_property_read_string(&spi->dev, "adi,sdo-mode", &str_val);
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to read adi,sdo-mode property\n");
> +
> +	if (strcmp(str_val, "1-wire"))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "Only 1-wire SDO is supported\n");

Discussion on this binding in the dt-binding patch.
As mentioned there, it feels like a place where a default would be sensible.

> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	st->chip_info = spi_get_device_match_data(spi);
> +
> +	st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> +	if (IS_ERR(st->vref)) {
> +		/*
> +		 * If there is no REFIO supply, then it means that we are using
> +		 * the internal 2.5V reference.
> +		 */
> +		if (PTR_ERR(st->vref) == -ENODEV)
> +			st->vref = NULL;
> +		else
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> +					     "Failed to get refio regulator\n");
> +	}
> +
> +	if (st->vref) {
> +		ret = regulator_enable(st->vref);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev, ad7380_release_regulator,

Naming wise maybe ad7380_disable_regulator is less misleading that release (which in
my mind is the bit the unwind for devm_regulator_get_optional() is doing)

> +					       st->vref);
> +		if (ret)
> +			return ret;
> +	}

Re: [PATCH 1/2] dt-bindings: iio: adc: Add binding for AD7380 ADCs

[David Lechner]

On Sun, Dec 10, 2023 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri,  8 Dec 2023 09:51:40 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>
> > This adds a binding specification for the Analog Devices Inc. AD7380
> > family of ADCs.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> Hi David,
>
> Comments inline.  A question for Mark Brown on the 2-wire bit..
> Do we have existing DT bindings for devices with parallel spi data
> outputs?
>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7380.yaml    | 102 +++++++++++++++++++++
> >  MAINTAINERS                                        |   9 ++
> >  2 files changed, 111 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> > new file mode 100644
> > index 000000000000..e9a0b72cd9d3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> ...
>
>
> > +  * https://www.analog.com/en/products/ad7380.html
> > +  * https://www.analog.com/en/products/ad7381.html
> > +  * https://www.analog.com/en/products/ad7383.html
> > +  * https://www.analog.com/en/products/ad7384.html
>
> > +  adi,sdo-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ 1-wire, 2-wire ]
> > +    description:
> > +      In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
> > +      pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
> > +      is read from SDOA and data for input B is read from SDOB/ALERT (and the
> > +      ALERT interrupt signal is not available).
>
> This is fun...  If I understand correctly 2-wire requires two SPI buses (or a complex
> spi controller that does parallel serial channels).

No, it wouldn't work with two separate SPI busses. Only a special
controller with parallel serial channels.

> What would description for that
> look like in DT and can we not establish what is wanted here from that bus description
> rather than an adi specific property?
>
> Seems a bit like parallel-memories.

I don't think this is the same as parallel-memories. Looking at the
the patch [1] it looks like parallel memories requires multiple CS
lines to connect to multiple chips to make the multiple chips behave
as one chip but otherwise works like SPI_RX_DUAL.

This ADC, on the other hand, works as if two chips that share
everything except the MISO line. Each MISO line (SDOA and SDOB) shifts
out one bit of two different words in parallel on each line instead of
two bits of the same word like SPI_RX_DUAL busses.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-6.8&id=88a50c1663ffa9f6b31705c6bf7a887a2c8d9434

>
> Mark, any insights into what we should do to describe this?
>
> > +
> > +  vcc-supply:
> > +    description: A 3V to 3.6V supply that powers the chip.
> > +
> > +  vlogic-supply:
> > +    description:
> > +      A 1.65V to 3.6V supply for the logic pins.
> > +
> > +  refio-supply:
> > +    description:
> > +      A 2.5V to 3.3V supply for the external reference voltage. When omitted,
> > +      the internal 2.5V reference is used.
> > +
> > +  interrupts:
> > +    description:
> > +      When the device is using 1-wire mode, this property is used to optionally
> > +      specify the ALERT interrupt.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - adi,sdo-mode
>
> Could define a default of 1-wire?  Simplifies things a little in the bindings.
>
> > +  - vcc-supply
> > +  - vlogic-supply
>
>

Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[Nuno Sá]

Hi David,

minor stuff from me...

On Fri, 2023-12-08 at 09:51 -0600, David Lechner wrote:
> This adds a new driver for the AD7380 family ADCs.
> 
> The driver currently implements basic support for the AD7380, AD7381,
> AD7383, and AD7384 2-channel differential ADCs. Support for additional
> single-ended and 4-channel chips that use the same register map as well
> as additional features of the chip will be added in future patches.
> 
> Co-developed-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  16 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7380.c | 467
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 485 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e2a998be5879..5a54620a31b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -438,6 +438,7 @@ S:	Supported
>  W:	
> https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> +F:	drivers/iio/adc/ad7380.c
>  
>  AD7877 TOUCHSCREEN DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 35f9867da12c..cbfd626712e3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -122,6 +122,22 @@ config AD7298
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7298.
>  
> +config AD7380
> +	tristate "Analog Devices AD7380 ADC driver"
> +	depends on SPI_MASTER
> +	select IIO_BUFFER
> +	select IIO_TRIGGER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  AD7380 is a family of simultaneous sampling ADCs that share the
> same
> +	  SPI register map and have similar pinouts.
> +
> +	  Say yes here to build support for Analog Devices AD7380 ADC and
> +	  similar chips.
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad7380.
> +
>  config AD7476
>  	tristate "Analog Devices AD7476 1-channel ADCs driver and other
> similar devices from AD and TI"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index bee11d442af4..e046d8004f41 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
>  obj-$(CONFIG_AD7292) += ad7292.o
>  obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7923) += ad7923.o
> +obj-$(CONFIG_AD7476) += ad7380.o
>  obj-$(CONFIG_AD7476) += ad7476.o
>  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> new file mode 100644
> index 000000000000..6a5ec59bd1fd
> --- /dev/null
> +++ b/drivers/iio/adc/ad7380.c
> @@ -0,0 +1,467 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> + *
> + * Copyright 2017 Analog Devices Inc.
> + * Copyright 2023 BayLibre, SAS
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> 

...

> +
> +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> +				     u32 writeval, u32 *readval)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +

potential controversial take: do we really need locking in here? regmap already
has it's own lock (I think you're not disabling it) and if someone plays with
registers it shouldn't while buffering, well... This is a debug interface so I
would probably not worry much. One could anyways for write stuff by going
directly to regmap :)

That said, fine to be as-is from my side (just wanted to take it out of my
system :))...

> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	struct spi_transfer xfer = {
> +		.bits_per_word = st->chip_info-
> >channels[0].scan_type.realbits,
> +		.len = 4,
> +		.rx_buf = &st->scan_data,
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +
> +	if (ret == 0)
> +		iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> +						   pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad7380_read_direct(struct ad7380_state *st,
> +			      struct iio_chan_spec const *chan, int *val)
> +{
> +	struct spi_transfer xfers[] = {
> +		/* toggle CS (no data xfer) to trigger a conversion */
> +		{
> +			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> +			.bits_per_word = chan->scan_type.realbits,
> +			.delay = {
> +				.value = 190, /* t[CONVERT] */
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +			.cs_change = 1,
> +			.cs_change_delay = {
> +				.value = 10, /* t[CSH] */
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +		},
> +		/* then read both channels */
> +		{
> +			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> +			.bits_per_word = chan->scan_type.realbits,
> +			.rx_buf = &st->rx[0],
> +			.len = 4,
> +		},
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = sign_extend32(st->rx[chan->scan_index],
> +			     chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad7380_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad7380_read_direct(st, chan, val);
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (st->vref) {
> +			ret = regulator_get_voltage(st->vref);

nit: I wonder how likely it is for vref to change at runtime...

> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret / 1000;
> +		} else {
> +			*val = AD7380_INTERNAL_REF_MV;
> +		}
> +
> +		*val2 = chan->scan_type.realbits;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ad7380_info = {
> +	.read_raw = &ad7380_read_raw,
> +	.debugfs_reg_access = &ad7380_debugfs_reg_access,
> +};
> +
> +static int ad7380_init(struct ad7380_state *st)
> +{
> +	int ret;
> +
> +	/* perform hard reset */
> +	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> +				 AD7380_CONFIG2_RESET,
> +				 FIELD_PREP(AD7380_CONFIG2_RESET,
> +					    AD7380_CONFIG2_RESET_HARD));
> +	if (ret < 0)
> +		return ret;
> +
> +
> +	/* select internal or external reference voltage */
> +	ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> +				 AD7380_CONFIG1_REFSEL,
> +				 FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st-
> >vref));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* SPI 1-wire mode */
> +	return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> +				  AD7380_CONFIG2_SDO,
> +				  FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> +}
> +
> +static void ad7380_release_regulator(void *p)
> +{
> +	struct regulator *reg = p;
> +
> +	regulator_disable(reg);
> +}
> +
> +static int ad7380_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad7380_state *st;
> +	const char *str_val;
> +	int ret;
> +
> +	ret = device_property_read_string(&spi->dev, "adi,sdo-mode",
> &str_val);
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to read adi,sdo-mode
> property\n");
> +
> +	if (strcmp(str_val, "1-wire"))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "Only 1-wire SDO is supported\n");
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	st->chip_info = spi_get_device_match_data(spi);
> +

if (!st->chip_info) ?

> +	st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> +	if (IS_ERR(st->vref)) {
> +		/*
> +		 * If there is no REFIO supply, then it means that we are
> using
> +		 * the internal 2.5V reference.
> +		 */
> +		if (PTR_ERR(st->vref) == -ENODEV)
> +			st->vref = NULL;
> +		else
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> +					     "Failed to get refio
> regulator\n");
> +	}
> +
> +	if (st->vref) {
> +		ret = regulator_enable(st->vref);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> ad7380_release_regulator,
> +					       st->vref);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> &ad7380_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +				     "failed to allocate register map\n");
> +

Could we instead have a custom regmap_bus instead of bypass regmap with
reg_read() and reg_write()?
> +
> +	indio_dev->channels = st->chip_info->channels;
> +	indio_dev->num_channels = st->chip_info->num_channels;
> +	indio_dev->dev.parent = &spi->dev;

no need to assign parent (the core does it for you)...

- Nuno Sá

Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[David Lechner]

On Tue, Dec 12, 2023 at 4:16 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> Hi David,
>
> minor stuff from me...
>
> On Fri, 2023-12-08 at 09:51 -0600, David Lechner wrote:
> > This adds a new driver for the AD7380 family ADCs.
> >
> > The driver currently implements basic support for the AD7380, AD7381,
> > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > single-ended and 4-channel chips that use the same register map as well
> > as additional features of the chip will be added in future patches.
> >
> > Co-developed-by: Stefan Popa <stefan.popa@analog.com>
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  MAINTAINERS              |   1 +
> >  drivers/iio/adc/Kconfig  |  16 ++
> >  drivers/iio/adc/Makefile |   1 +
> >  drivers/iio/adc/ad7380.c | 467
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 485 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e2a998be5879..5a54620a31b8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -438,6 +438,7 @@ S:        Supported
> >  W:
> > https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
> >  W:   https://ez.analog.com/linux-software-drivers
> >  F:   Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> > +F:   drivers/iio/adc/ad7380.c
> >
> >  AD7877 TOUCHSCREEN DRIVER
> >  M:   Michael Hennerich <michael.hennerich@analog.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 35f9867da12c..cbfd626712e3 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -122,6 +122,22 @@ config AD7298
> >         To compile this driver as a module, choose M here: the
> >         module will be called ad7298.
> >
> > +config AD7380
> > +     tristate "Analog Devices AD7380 ADC driver"
> > +     depends on SPI_MASTER
> > +     select IIO_BUFFER
> > +     select IIO_TRIGGER
> > +     select IIO_TRIGGERED_BUFFER
> > +     help
> > +       AD7380 is a family of simultaneous sampling ADCs that share the
> > same
> > +       SPI register map and have similar pinouts.
> > +
> > +       Say yes here to build support for Analog Devices AD7380 ADC and
> > +       similar chips.
> > +
> > +       To compile this driver as a module, choose M here: the module will
> > be
> > +       called ad7380.
> > +
> >  config AD7476
> >       tristate "Analog Devices AD7476 1-channel ADCs driver and other
> > similar devices from AD and TI"
> >       depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index bee11d442af4..e046d8004f41 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
> >  obj-$(CONFIG_AD7292) += ad7292.o
> >  obj-$(CONFIG_AD7298) += ad7298.o
> >  obj-$(CONFIG_AD7923) += ad7923.o
> > +obj-$(CONFIG_AD7476) += ad7380.o
> >  obj-$(CONFIG_AD7476) += ad7476.o
> >  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> >  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > new file mode 100644
> > index 000000000000..6a5ec59bd1fd
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7380.c
> > @@ -0,0 +1,467 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> > + *
> > + * Copyright 2017 Analog Devices Inc.
> > + * Copyright 2023 BayLibre, SAS
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/sysfs.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> >
>
> ...
>
> > +
> > +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> > +                                  u32 writeval, u32 *readval)
> > +{
> > +     struct ad7380_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     ret = iio_device_claim_direct_mode(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
>
> potential controversial take: do we really need locking in here? regmap already
> has it's own lock (I think you're not disabling it) and if someone plays with
> registers it shouldn't while buffering, well... This is a debug interface so I
> would probably not worry much. One could anyways for write stuff by going
> directly to regmap :)
>
> That said, fine to be as-is from my side (just wanted to take it out of my
> system :))...

I put it in because we often run `iio_info` which reads register 0 via
debugfs. So even though we "shouldn't" be using debug while buffer is
enabled, it is easy to do unintentionally. (This was particularly
problem while working on offload support.)

>
> > +     if (readval)
> > +             ret = regmap_read(st->regmap, reg, readval);
> > +     else
> > +             ret = regmap_write(st->regmap, reg, writeval);
> > +
> > +     iio_device_release_direct_mode(indio_dev);
> > +
> > +     return ret;
> > +}
> > +
> > +static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> > +{
> > +     struct iio_poll_func *pf = p;
> > +     struct iio_dev *indio_dev = pf->indio_dev;
> > +     struct ad7380_state *st = iio_priv(indio_dev);
> > +     struct spi_transfer xfer = {
> > +             .bits_per_word = st->chip_info-
> > >channels[0].scan_type.realbits,
> > +             .len = 4,
> > +             .rx_buf = &st->scan_data,
> > +     };
> > +     int ret;
> > +
> > +     ret = spi_sync_transfer(st->spi, &xfer, 1);
> > +
> > +     if (ret == 0)
> > +             iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> > +                                                pf->timestamp);
> > +
> > +     iio_trigger_notify_done(indio_dev->trig);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int ad7380_read_direct(struct ad7380_state *st,
> > +                           struct iio_chan_spec const *chan, int *val)
> > +{
> > +     struct spi_transfer xfers[] = {
> > +             /* toggle CS (no data xfer) to trigger a conversion */
> > +             {
> > +                     .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > +                     .bits_per_word = chan->scan_type.realbits,
> > +                     .delay = {
> > +                             .value = 190, /* t[CONVERT] */
> > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +                     .cs_change = 1,
> > +                     .cs_change_delay = {
> > +                             .value = 10, /* t[CSH] */
> > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +             },
> > +             /* then read both channels */
> > +             {
> > +                     .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > +                     .bits_per_word = chan->scan_type.realbits,
> > +                     .rx_buf = &st->rx[0],
> > +                     .len = 4,
> > +             },
> > +     };
> > +     int ret;
> > +
> > +     ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     *val = sign_extend32(st->rx[chan->scan_index],
> > +                          chan->scan_type.realbits - 1);
> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
> > +static int ad7380_read_raw(struct iio_dev *indio_dev,
> > +                        struct iio_chan_spec const *chan,
> > +                        int *val, int *val2, long info)
> > +{
> > +     struct ad7380_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     switch (info) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = iio_device_claim_direct_mode(indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = ad7380_read_direct(st, chan, val);
> > +             iio_device_release_direct_mode(indio_dev);
> > +
> > +             return ret;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             if (st->vref) {
> > +                     ret = regulator_get_voltage(st->vref);
>
> nit: I wonder how likely it is for vref to change at runtime...
>
> > +                     if (ret < 0)
> > +                             return ret;
> > +
> > +                     *val = ret / 1000;
> > +             } else {
> > +                     *val = AD7380_INTERNAL_REF_MV;
> > +             }
> > +
> > +             *val2 = chan->scan_type.realbits;
> > +
> > +             return IIO_VAL_FRACTIONAL_LOG2;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static const struct iio_info ad7380_info = {
> > +     .read_raw = &ad7380_read_raw,
> > +     .debugfs_reg_access = &ad7380_debugfs_reg_access,
> > +};
> > +
> > +static int ad7380_init(struct ad7380_state *st)
> > +{
> > +     int ret;
> > +
> > +     /* perform hard reset */
> > +     ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > +                              AD7380_CONFIG2_RESET,
> > +                              FIELD_PREP(AD7380_CONFIG2_RESET,
> > +                                         AD7380_CONFIG2_RESET_HARD));
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +
> > +     /* select internal or external reference voltage */
> > +     ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> > +                              AD7380_CONFIG1_REFSEL,
> > +                              FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st-
> > >vref));
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* SPI 1-wire mode */
> > +     return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > +                               AD7380_CONFIG2_SDO,
> > +                               FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> > +}
> > +
> > +static void ad7380_release_regulator(void *p)
> > +{
> > +     struct regulator *reg = p;
> > +
> > +     regulator_disable(reg);
> > +}
> > +
> > +static int ad7380_probe(struct spi_device *spi)
> > +{
> > +     struct iio_dev *indio_dev;
> > +     struct ad7380_state *st;
> > +     const char *str_val;
> > +     int ret;
> > +
> > +     ret = device_property_read_string(&spi->dev, "adi,sdo-mode",
> > &str_val);
> > +     if (ret < 0)
> > +             return dev_err_probe(&spi->dev, ret,
> > +                                  "Failed to read adi,sdo-mode
> > property\n");
> > +
> > +     if (strcmp(str_val, "1-wire"))
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                                  "Only 1-wire SDO is supported\n");
> > +
> > +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     st = iio_priv(indio_dev);
> > +     st->spi = spi;
> > +     st->chip_info = spi_get_device_match_data(spi);
> > +
>
> if (!st->chip_info) ?

It looks like quite a few drivers don't check this, but I guess safer
to check anyway.


>
> > +     st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> > +     if (IS_ERR(st->vref)) {
> > +             /*
> > +              * If there is no REFIO supply, then it means that we are
> > using
> > +              * the internal 2.5V reference.
> > +              */
> > +             if (PTR_ERR(st->vref) == -ENODEV)
> > +                     st->vref = NULL;
> > +             else
> > +                     return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> > +                                          "Failed to get refio
> > regulator\n");
> > +     }
> > +
> > +     if (st->vref) {
> > +             ret = regulator_enable(st->vref);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_add_action_or_reset(&spi->dev,
> > ad7380_release_regulator,
> > +                                            st->vref);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> > &ad7380_regmap_config);
> > +     if (IS_ERR(st->regmap))
> > +             return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> > +                                  "failed to allocate register map\n");
> > +
>
> Could we instead have a custom regmap_bus instead of bypass regmap with
> reg_read() and reg_write()?
> > +
> > +     indio_dev->channels = st->chip_info->channels;
> > +     indio_dev->num_channels = st->chip_info->num_channels;
> > +     indio_dev->dev.parent = &spi->dev;
>
> no need to assign parent (the core does it for you)...
>
> - Nuno Sá
>

Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs

[Nuno Sá]

On Tue, 2023-12-12 at 16:42 +0100, David Lechner wrote:
> On Tue, Dec 12, 2023 at 4:16 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > Hi David,
> > 
> > minor stuff from me...
> > 
> > On Fri, 2023-12-08 at 09:51 -0600, David Lechner wrote:
> > > This adds a new driver for the AD7380 family ADCs.
> > > 
> > > The driver currently implements basic support for the AD7380, AD7381,
> > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > single-ended and 4-channel chips that use the same register map as well
> > > as additional features of the chip will be added in future patches.
> > > 
> > > Co-developed-by: Stefan Popa <stefan.popa@analog.com>
> > > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  MAINTAINERS              |   1 +
> > >  drivers/iio/adc/Kconfig  |  16 ++
> > >  drivers/iio/adc/Makefile |   1 +
> > >  drivers/iio/adc/ad7380.c | 467
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 485 insertions(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e2a998be5879..5a54620a31b8 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -438,6 +438,7 @@ S:        Supported
> > >  W:
> > > https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
> > >  W:   https://ez.analog.com/linux-software-drivers
> > >  F:   Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> > > +F:   drivers/iio/adc/ad7380.c
> > > 
> > >  AD7877 TOUCHSCREEN DRIVER
> > >  M:   Michael Hennerich <michael.hennerich@analog.com>
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 35f9867da12c..cbfd626712e3 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -122,6 +122,22 @@ config AD7298
> > >         To compile this driver as a module, choose M here: the
> > >         module will be called ad7298.
> > > 
> > > +config AD7380
> > > +     tristate "Analog Devices AD7380 ADC driver"
> > > +     depends on SPI_MASTER
> > > +     select IIO_BUFFER
> > > +     select IIO_TRIGGER
> > > +     select IIO_TRIGGERED_BUFFER
> > > +     help
> > > +       AD7380 is a family of simultaneous sampling ADCs that share the
> > > same
> > > +       SPI register map and have similar pinouts.
> > > +
> > > +       Say yes here to build support for Analog Devices AD7380 ADC and
> > > +       similar chips.
> > > +
> > > +       To compile this driver as a module, choose M here: the module will
> > > be
> > > +       called ad7380.
> > > +
> > >  config AD7476
> > >       tristate "Analog Devices AD7476 1-channel ADCs driver and other
> > > similar devices from AD and TI"
> > >       depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index bee11d442af4..e046d8004f41 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
> > >  obj-$(CONFIG_AD7292) += ad7292.o
> > >  obj-$(CONFIG_AD7298) += ad7298.o
> > >  obj-$(CONFIG_AD7923) += ad7923.o
> > > +obj-$(CONFIG_AD7476) += ad7380.o
> > >  obj-$(CONFIG_AD7476) += ad7476.o
> > >  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> > >  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > > new file mode 100644
> > > index 000000000000..6a5ec59bd1fd
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad7380.c
> > > @@ -0,0 +1,467 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> > > + *
> > > + * Copyright 2017 Analog Devices Inc.
> > > + * Copyright 2023 BayLibre, SAS
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/cleanup.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/sysfs.h>
> > > +
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +
> > > 
> > 
> > ...
> > 
> > > +
> > > +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> > > +                                  u32 writeval, u32 *readval)
> > > +{
> > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > +     int ret;
> > > +
> > > +     ret = iio_device_claim_direct_mode(indio_dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > 
> > potential controversial take: do we really need locking in here? regmap already
> > has it's own lock (I think you're not disabling it) and if someone plays with
> > registers it shouldn't while buffering, well... This is a debug interface so I
> > would probably not worry much. One could anyways for write stuff by going
> > directly to regmap :)
> > 
> > That said, fine to be as-is from my side (just wanted to take it out of my
> > system :))...
> 
> I put it in because we often run `iio_info` which reads register 0 via
> debugfs. So even though we "shouldn't" be using debug while buffer is
> enabled, it is easy to do unintentionally. (This was particularly
> problem while working on offload support.)
> 

fair enough...

> > 
> > > +     if (readval)
> > > +             ret = regmap_read(st->regmap, reg, readval);
> > > +     else
> > > +             ret = regmap_write(st->regmap, reg, writeval);
> > > +
> > > +     iio_device_release_direct_mode(indio_dev);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> > > +{
> > > +     struct iio_poll_func *pf = p;
> > > +     struct iio_dev *indio_dev = pf->indio_dev;
> > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > +     struct spi_transfer xfer = {
> > > +             .bits_per_word = st->chip_info-
> > > > channels[0].scan_type.realbits,
> > > +             .len = 4,
> > > +             .rx_buf = &st->scan_data,
> > > +     };
> > > +     int ret;
> > > +
> > > +     ret = spi_sync_transfer(st->spi, &xfer, 1);
> > > +
> > > +     if (ret == 0)
> > > +             iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> > > +                                                pf->timestamp);
> > > +
> > > +     iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int ad7380_read_direct(struct ad7380_state *st,
> > > +                           struct iio_chan_spec const *chan, int *val)
> > > +{
> > > +     struct spi_transfer xfers[] = {
> > > +             /* toggle CS (no data xfer) to trigger a conversion */
> > > +             {
> > > +                     .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > > +                     .bits_per_word = chan->scan_type.realbits,
> > > +                     .delay = {
> > > +                             .value = 190, /* t[CONVERT] */
> > > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > > +                     },
> > > +                     .cs_change = 1,
> > > +                     .cs_change_delay = {
> > > +                             .value = 10, /* t[CSH] */
> > > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > > +                     },
> > > +             },
> > > +             /* then read both channels */
> > > +             {
> > > +                     .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > > +                     .bits_per_word = chan->scan_type.realbits,
> > > +                     .rx_buf = &st->rx[0],
> > > +                     .len = 4,
> > > +             },
> > > +     };
> > > +     int ret;
> > > +
> > > +     ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > +
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     *val = sign_extend32(st->rx[chan->scan_index],
> > > +                          chan->scan_type.realbits - 1);
> > > +
> > > +     return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int ad7380_read_raw(struct iio_dev *indio_dev,
> > > +                        struct iio_chan_spec const *chan,
> > > +                        int *val, int *val2, long info)
> > > +{
> > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > +     int ret;
> > > +
> > > +     switch (info) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             ret = iio_device_claim_direct_mode(indio_dev);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = ad7380_read_direct(st, chan, val);
> > > +             iio_device_release_direct_mode(indio_dev);
> > > +
> > > +             return ret;
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             if (st->vref) {
> > > +                     ret = regulator_get_voltage(st->vref);
> > 
> > nit: I wonder how likely it is for vref to change at runtime...
> > 
> > > +                     if (ret < 0)
> > > +                             return ret;
> > > +
> > > +                     *val = ret / 1000;
> > > +             } else {
> > > +                     *val = AD7380_INTERNAL_REF_MV;
> > > +             }
> > > +
> > > +             *val2 = chan->scan_type.realbits;
> > > +
> > > +             return IIO_VAL_FRACTIONAL_LOG2;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_info ad7380_info = {
> > > +     .read_raw = &ad7380_read_raw,
> > > +     .debugfs_reg_access = &ad7380_debugfs_reg_access,
> > > +};
> > > +
> > > +static int ad7380_init(struct ad7380_state *st)
> > > +{
> > > +     int ret;
> > > +
> > > +     /* perform hard reset */
> > > +     ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > > +                              AD7380_CONFIG2_RESET,
> > > +                              FIELD_PREP(AD7380_CONFIG2_RESET,
> > > +                                         AD7380_CONFIG2_RESET_HARD));
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +
> > > +     /* select internal or external reference voltage */
> > > +     ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> > > +                              AD7380_CONFIG1_REFSEL,
> > > +                              FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st-
> > > > vref));
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* SPI 1-wire mode */
> > > +     return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > > +                               AD7380_CONFIG2_SDO,
> > > +                               FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> > > +}
> > > +
> > > +static void ad7380_release_regulator(void *p)
> > > +{
> > > +     struct regulator *reg = p;
> > > +
> > > +     regulator_disable(reg);
> > > +}
> > > +
> > > +static int ad7380_probe(struct spi_device *spi)
> > > +{
> > > +     struct iio_dev *indio_dev;
> > > +     struct ad7380_state *st;
> > > +     const char *str_val;
> > > +     int ret;
> > > +
> > > +     ret = device_property_read_string(&spi->dev, "adi,sdo-mode",
> > > &str_val);
> > > +     if (ret < 0)
> > > +             return dev_err_probe(&spi->dev, ret,
> > > +                                  "Failed to read adi,sdo-mode
> > > property\n");
> > > +
> > > +     if (strcmp(str_val, "1-wire"))
> > > +             return dev_err_probe(&spi->dev, -EINVAL,
> > > +                                  "Only 1-wire SDO is supported\n");
> > > +
> > > +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     st = iio_priv(indio_dev);
> > > +     st->spi = spi;
> > > +     st->chip_info = spi_get_device_match_data(spi);
> > > +
> > 
> > if (!st->chip_info) ?
> 
> It looks like quite a few drivers don't check this, but I guess safer
> to check anyway.
> 

Don't make it right :). I mean, FWIW, I'm not quite sure if you can really get NULL
in here but since it's a possibility (from the API point of view), better to do it
properly.


- Nuno Sá