[PATCH 0/2] iio: adc: ad7944: implement chain mode support

[PATCH 0/2] iio: adc: ad7944: implement chain mode support

[David Lechner]

This series adds support and documentation for the chain mode wiring
configuration to the ad7944 driver. In this configuration, multiple
chips are daisy-chained together in series via the SPI data lines.
So it appears on the SPI bus as a single device with multiple channels.

---
David Lechner (2):
      iio: adc: ad7944: add support for chain mode
      docs: iio: ad7944: add documentation for chain mode

 Documentation/iio/ad7944.rst |  30 ++++++-
 drivers/iio/adc/ad7944.c     | 186 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 204 insertions(+), 12 deletions(-)
---
base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
change-id: 20240424-iio-ad7944-chain-mode-f5c4e6a856f6

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

[PATCH 1/2] iio: adc: ad7944: add support for chain mode

[David Lechner]

This adds support for the chain mode of the AD7944 ADC. This mode allows
multiple ADCs to be daisy-chained together. Data from all of the ADCs in
is read by reading multiple words from the first ADC in the chain.

Each chip in the chain adds an extra IIO input voltage channel to the
IIO device.

Only the wiring configuration where the SPI controller CS line is
connected to the CNV pin of all of the ADCs in the chain is supported
in this patch.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7944.c | 186 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 176 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index 4af574ffa864..1a87f70ce57f 100644
--- a/drivers/iio/adc/ad7944.c
+++ b/drivers/iio/adc/ad7944.c
@@ -6,6 +6,7 @@
  * Copyright 2024 BayLibre, SAS
  */
 
+#include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
@@ -53,6 +54,7 @@ struct ad7944_adc {
 	enum ad7944_spi_mode spi_mode;
 	struct spi_transfer xfers[3];
 	struct spi_message msg;
+	void *chain_mode_buf;
 	/* Chip-specific timing specifications. */
 	const struct ad7944_timing_spec *timing_spec;
 	/* GPIO connected to CNV pin. */
@@ -214,6 +216,46 @@ static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc
 	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
 }
 
+static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
+				      const struct iio_chan_spec *chan,
+				      u32 n_chain_dev)
+{
+	struct spi_transfer *xfers = adc->xfers;
+	int ret;
+
+	/*
+	 * NB: SCLK has to be low before we toggle CS to avoid triggering the
+	 * busy indication.
+	 */
+	if (adc->spi->mode & SPI_CPOL)
+		return dev_err_probe(dev, -EINVAL,
+				     "chain mode requires ~SPI_CPOL\n");
+
+	/*
+	 * We only support CNV connected to CS in chain mode and we need CNV
+	 * to be high during the transfer to trigger the conversion.
+	 */
+	if (!(adc->spi->mode & SPI_CS_HIGH))
+		return dev_err_probe(dev, -EINVAL,
+				     "chain mode requires SPI_CS_HIGH\n");
+
+	/* CNV has to be high for full conversion time before reading data. */
+	xfers[0].delay.value = adc->timing_spec->conv_ns;
+	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	xfers[1].rx_buf = adc->chain_mode_buf;
+	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits) * n_chain_dev;
+	xfers[1].bits_per_word = chan->scan_type.realbits;
+
+	spi_message_init_with_transfers(&adc->msg, xfers, 2);
+
+	ret = spi_optimize_message(adc->spi, &adc->msg);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
+}
+
 /**
  * ad7944_convert_and_acquire - Perform a single conversion and acquisition
  * @adc: The ADC device structure
@@ -223,7 +265,8 @@ static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc
  * Perform a conversion and acquisition of a single sample using the
  * pre-optimized adc->msg.
  *
- * Upon successful return adc->sample.raw will contain the conversion result.
+ * Upon successful return adc->sample.raw will contain the conversion result
+ * (or adc->chain_mode_buf if the device is using chain mode).
  */
 static int ad7944_convert_and_acquire(struct ad7944_adc *adc,
 				      const struct iio_chan_spec *chan)
@@ -252,10 +295,17 @@ static int ad7944_single_conversion(struct ad7944_adc *adc,
 	if (ret)
 		return ret;
 
-	if (chan->scan_type.storagebits > 16)
-		*val = adc->sample.raw.u32;
-	else
-		*val = adc->sample.raw.u16;
+	if (adc->spi_mode == AD7944_SPI_MODE_CHAIN) {
+		if (chan->scan_type.storagebits > 16)
+			*val = ((u32 *)adc->chain_mode_buf)[chan->scan_index];
+		else
+			*val = ((u16 *)adc->chain_mode_buf)[chan->scan_index];
+	} else {
+		if (chan->scan_type.storagebits > 16)
+			*val = adc->sample.raw.u32;
+		else
+			*val = adc->sample.raw.u16;
+	}
 
 	if (chan->scan_type.sign == 's')
 		*val = sign_extend32(*val, chan->scan_type.realbits - 1);
@@ -315,8 +365,12 @@ static irqreturn_t ad7944_trigger_handler(int irq, void *p)
 	if (ret)
 		goto out;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
-					   pf->timestamp);
+	if (adc->spi_mode == AD7944_SPI_MODE_CHAIN)
+		iio_push_to_buffers_with_timestamp(indio_dev, adc->chain_mode_buf,
+						   pf->timestamp);
+	else
+		iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
+						   pf->timestamp);
 
 out:
 	iio_trigger_notify_done(indio_dev->trig);
@@ -324,6 +378,90 @@ static irqreturn_t ad7944_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+/**
+ * ad7944_chain_mode_alloc - allocate and initialize channel specs and buffers
+ *                           for daisy-chained devices
+ * @dev: The device for devm_ functions
+ * @chan_template: The channel template for the devices (array of 2 channels
+ *                 voltage and timestamp)
+ * @n_chain_dev: The number of devices in the chain
+ * @chain_chan: Pointer to receive the allocated channel specs
+ * @chain_mode_buf: Pointer to receive the allocated rx buffer
+ * @chain_scan_masks: Pointer to receive the allocated scan masks
+ * Return: 0 on success, a negative error code on failure
+ */
+static int ad7944_chain_mode_alloc(struct device *dev,
+				   const struct iio_chan_spec *chan_template,
+				   u32 n_chain_dev,
+				   struct iio_chan_spec **chain_chan,
+				   void **chain_mode_buf,
+				   unsigned long **chain_scan_masks)
+{
+	struct iio_chan_spec *chan;
+	size_t chain_mode_buf_size;
+	unsigned long *scan_masks;
+	void *buf;
+	int i;
+
+	/* 1 channel for each device in chain plus 1 for soft timestamp */
+
+	chan = devm_kcalloc(dev, n_chain_dev + 1, sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	for (i = 0; i < n_chain_dev; i++) {
+		chan[i] = chan_template[0];
+
+		if (chan_template[0].differential) {
+			chan[i].channel = 2 * i;
+			chan[i].channel2 = 2 * i + 1;
+		} else {
+			chan[i].channel = i;
+		}
+
+		chan[i].scan_index = i;
+	}
+
+	/* soft timestamp */
+	chan[i] = chan_template[1];
+	chan[i].scan_index = i;
+
+	*chain_chan = chan;
+
+	/* 1 word for each voltage channel + aligned u64 for timestamp */
+
+	chain_mode_buf_size = ALIGN(n_chain_dev *
+		BITS_TO_BYTES(chan[0].scan_type.storagebits), sizeof(u64))
+		+ sizeof(u64);
+	buf = devm_kmalloc(dev, chain_mode_buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	*chain_mode_buf = buf;
+
+	/*
+	 * Have to limit n_chain_dev due to current implementation of
+	 * available_scan_masks.
+	 */
+	if (n_chain_dev > BITS_PER_LONG)
+		return dev_err_probe(dev, -EINVAL,
+				     "chain is limited to 32 devices\n");
+
+	scan_masks = devm_kcalloc(dev, 2, sizeof(*scan_masks), GFP_KERNEL);
+	if (!scan_masks)
+		return -ENOMEM;
+
+	/*
+	 * Scan mask is needed since we always have to read all devices in the
+	 * chain in one SPI transfer.
+	 */
+	scan_masks[0] = GENMASK(n_chain_dev - 1, 0);
+
+	*chain_scan_masks = scan_masks;
+
+	return 0;
+}
+
 static const char * const ad7944_power_supplies[] = {
 	"avdd",	"dvdd",	"bvdd", "vio"
 };
@@ -341,6 +479,9 @@ static int ad7944_probe(struct spi_device *spi)
 	struct ad7944_adc *adc;
 	bool have_refin = false;
 	struct regulator *ref;
+	struct iio_chan_spec *chain_chan;
+	unsigned long *chain_scan_masks;
+	u32 n_chain_dev;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
@@ -474,14 +615,39 @@ static int ad7944_probe(struct spi_device *spi)
 
 		break;
 	case AD7944_SPI_MODE_CHAIN:
-		return dev_err_probe(dev, -EINVAL, "chain mode is not implemented\n");
+		ret = device_property_read_u32(dev, "#daisy-chained-devices",
+					       &n_chain_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					"failed to get #daisy-chained-devices\n");
+
+		ret = ad7944_chain_mode_alloc(dev, chip_info->channels,
+					      n_chain_dev, &chain_chan,
+					      &adc->chain_mode_buf,
+					      &chain_scan_masks);
+		if (ret)
+			return ret;
+
+		ret = ad7944_chain_mode_init_msg(dev, adc, &chain_chan[0],
+						 n_chain_dev);
+		if (ret)
+			return ret;
+
+		break;
 	}
 
 	indio_dev->name = chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ad7944_iio_info;
-	indio_dev->channels = chip_info->channels;
-	indio_dev->num_channels = ARRAY_SIZE(chip_info->channels);
+
+	if (adc->spi_mode == AD7944_SPI_MODE_CHAIN) {
+		indio_dev->available_scan_masks = chain_scan_masks;
+		indio_dev->channels = chain_chan;
+		indio_dev->num_channels = n_chain_dev + 1;
+	} else {
+		indio_dev->channels = chip_info->channels;
+		indio_dev->num_channels = ARRAY_SIZE(chip_info->channels);
+	}
 
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,

-- 
2.43.2

[PATCH 2/2] docs: iio: ad7944: add documentation for chain mode

[David Lechner]

Add documentation for chain mode support that was recently added to the
AD7944 ADC driver.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/iio/ad7944.rst | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/iio/ad7944.rst b/Documentation/iio/ad7944.rst
index f418ab1288ae..0d26e56aba88 100644
--- a/Documentation/iio/ad7944.rst
+++ b/Documentation/iio/ad7944.rst
@@ -24,7 +24,7 @@ Supported features
 SPI wiring modes
 ----------------
 
-The driver currently supports two of the many possible SPI wiring configurations.
+The driver currently supports three of the many possible SPI wiring configurations.
 
 CS mode, 3-wire, without busy indicator
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -68,6 +68,27 @@ CS mode, 4-wire, without busy indicator
 To select this mode in the device tree, omit the ``adi,spi-mode`` property and
 provide the ``cnv-gpios`` property.
 
+Chain mode, without busy indicator
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. code-block::
+
+                                                                 +-------------+
+                  +-------------------------+--------------------| CS          |
+                  v                         v                    |             |
+        +--------------------+    +--------------------+         |     HOST    |
+        |        CNV         |    |        CNV         |         |             |
+   +--->| SDI   AD7944   SDO |--->| SDI   AD7944   SDO |-------->| SDI         |
+   |    |        SCK         |    |        SCK         |         |             |
+  GND   +--------------------+    +--------------------+         |             |
+                  ^                         ^                    |             |
+                  +-------------------------+--------------------| SCLK        |
+                                                                 +-------------+
+
+To select this mode in the device tree, set the ``adi,spi-mode`` property to
+``"chain"``, add the ``spi-cs-high`` flag, add the ``#daisy-chained-devices``
+property, and omit the ``cnv-gpios`` property.
+
 Reference voltage
 -----------------
 
@@ -86,7 +107,6 @@ Unimplemented features
 
 - ``BUSY`` indication
 - ``TURBO`` mode
-- Daisy chain mode
 
 
 Device attributes
@@ -108,6 +128,9 @@ AD7944 and AD7985 are pseudo-differential ADCs and have the following attributes
 | ``in_voltage0_scale``                 | Scale factor to convert raw value to mV.                     |
 +---------------------------------------+--------------------------------------------------------------+
 
+In "chain" mode, additional chips will appear as additional voltage input
+channels, e.g. ``in_voltage1_raw``.
+
 Fully-differential ADCs
 -----------------------
 
@@ -121,6 +144,9 @@ AD7986 is a fully-differential ADC and has the following attributes:
 | ``in_voltage0-voltage1_scale``        | Scale factor to convert raw value to mV.                     |
 +---------------------------------------+--------------------------------------------------------------+
 
+In "chain" mode, additional chips will appear as additional voltage input
+channels, e.g. ``in_voltage2-voltage3_raw``.
+
 
 Device buffers
 ==============

-- 
2.43.2

Re: [PATCH 1/2] iio: adc: ad7944: add support for chain mode

[Jonathan Cameron]

On Thu, 25 Apr 2024 09:09:59 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds support for the chain mode of the AD7944 ADC. This mode allows
> multiple ADCs to be daisy-chained together. Data from all of the ADCs in
> is read by reading multiple words from the first ADC in the chain.
> 
> Each chip in the chain adds an extra IIO input voltage channel to the
> IIO device.
> 
> Only the wiring configuration where the SPI controller CS line is
> connected to the CNV pin of all of the ADCs in the chain is supported
> in this patch.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Looks good except for one minor tweak needed to ensure the allocated buffer
is zeroed as we don't necessarily overwrite the the whole thing.

Given that's all I found, I've just switched that to devm_kzalloc and
applied the series.

Applied to the togreg branch of iio.git and pushed out initially as testing
for 0-day to look at it.

Thanks,

Jonathan



>  
> +/**
> + * ad7944_chain_mode_alloc - allocate and initialize channel specs and buffers
> + *                           for daisy-chained devices
> + * @dev: The device for devm_ functions
> + * @chan_template: The channel template for the devices (array of 2 channels
> + *                 voltage and timestamp)
> + * @n_chain_dev: The number of devices in the chain
> + * @chain_chan: Pointer to receive the allocated channel specs
> + * @chain_mode_buf: Pointer to receive the allocated rx buffer
> + * @chain_scan_masks: Pointer to receive the allocated scan masks
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad7944_chain_mode_alloc(struct device *dev,
> +				   const struct iio_chan_spec *chan_template,
> +				   u32 n_chain_dev,
> +				   struct iio_chan_spec **chain_chan,
> +				   void **chain_mode_buf,
> +				   unsigned long **chain_scan_masks)
> +{
> +	struct iio_chan_spec *chan;
> +	size_t chain_mode_buf_size;
> +	unsigned long *scan_masks;
> +	void *buf;
> +	int i;
> +
> +	/* 1 channel for each device in chain plus 1 for soft timestamp */
> +
> +	chan = devm_kcalloc(dev, n_chain_dev + 1, sizeof(*chan), GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_chain_dev; i++) {
> +		chan[i] = chan_template[0];
> +
> +		if (chan_template[0].differential) {
> +			chan[i].channel = 2 * i;
> +			chan[i].channel2 = 2 * i + 1;
> +		} else {
> +			chan[i].channel = i;
> +		}
> +
> +		chan[i].scan_index = i;
> +	}
> +
> +	/* soft timestamp */
> +	chan[i] = chan_template[1];
> +	chan[i].scan_index = i;
> +
> +	*chain_chan = chan;
> +
> +	/* 1 word for each voltage channel + aligned u64 for timestamp */
> +
> +	chain_mode_buf_size = ALIGN(n_chain_dev *
> +		BITS_TO_BYTES(chan[0].scan_type.storagebits), sizeof(u64))
> +		+ sizeof(u64);
> +	buf = devm_kmalloc(dev, chain_mode_buf_size, GFP_KERNEL);

Zero it - It's not a problem to leak stale ADC data or similar
into the gap between the data and the timestamp, but it is a problem
if it's general kernel data potentially leaking.

So play it safe and devm_kzalloc()
		
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	*chain_mode_buf = buf;
> +
> +	/*
> +	 * Have to limit n_chain_dev due to current implementation of
> +	 * available_scan_masks.
> +	 */
> +	if (n_chain_dev > BITS_PER_LONG)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "chain is limited to 32 devices\n");
> +
> +	scan_masks = devm_kcalloc(dev, 2, sizeof(*scan_masks), GFP_KERNEL);
> +	if (!scan_masks)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Scan mask is needed since we always have to read all devices in the
> +	 * chain in one SPI transfer.
> +	 */
> +	scan_masks[0] = GENMASK(n_chain_dev - 1, 0);
> +
> +	*chain_scan_masks = scan_masks;
> +
> +	return 0;
> +}