[PATCH 0/2] iio: adc: ad4695: implement triggered buffer

[PATCH 0/2] iio: adc: ad4695: implement triggered buffer

[David Lechner]

This is a fairly simple series that adds support for triggered buffers
to the ad4695 driver.

Not directly related to this patch, but as a side discussion about
future possibilities with this chip while we are here...

The advanced sequencer on this chip can repeat the same channel multiple
times which, when combined with the autocycle feature, can be used to
create different effective sampling rates for individual channels.

For example if we set up the sequence [IN1, IN2, IN1, IN3] and the time
between each individual sample in the sequence is the same, then IN1 has
an effective sampling rate of 2x the other channels.

Have there ever been discussions before about implementing something
like this in the IIO subsystem? I didn't see anything that looked like
this already implemented in the kernel.

---
David Lechner (2):
      iio: adc: ad4695: implement triggered buffer
      doc: iio: ad4695: document buffered read

 Documentation/iio/ad4695.rst |  12 ++-
 drivers/iio/adc/ad4695.c     | 233 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 241 insertions(+), 4 deletions(-)
---
base-commit: 7cad163c39cb642ed587d3eeb37a5637ee02740f
change-id: 20240807-iio-adc-ad4695-buffered-read-f49eb511e300

[PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[David Lechner]

This implements buffered reads for the ad4695 driver using the typical
triggered buffer implementation, including adding a soft timestamp
channel.

The chip has 4 different modes for doing conversions. The driver is
using the advanced sequencer mode since that is the only mode that
allows individual configuration of all aspects each channel (e.g.
bipolar config currently and oversampling to be added in the future).

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 230 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 007ecb951bc3..a3bd5be36134 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -11,6 +11,7 @@
  * Copyright 2024 BayLibre, SAS
  */
 
+#include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/compiler.h>
@@ -18,7 +19,10 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -61,6 +65,7 @@
 #define AD4695_REG_GPIO_CTRL				0x0026
 #define AD4695_REG_GP_MODE				0x0027
 #define AD4695_REG_TEMP_CTRL				0x0029
+#define   AD4695_REG_TEMP_CTRL_TEMP_EN			  BIT(0)
 #define AD4695_REG_CONFIG_IN(n)				(0x0030 | (n))
 #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
 #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
@@ -80,13 +85,18 @@
 #define AD4695_CMD_VOLTAGE_CHAN(n)	(0x10 | (n))
 
 /* timing specs */
+#define AD4695_T_CNVL_NS		80
 #define AD4695_T_CONVERT_NS		415
 #define AD4695_T_WAKEUP_HW_MS		3
 #define AD4695_T_WAKEUP_SW_MS		3
 #define AD4695_T_REFBUF_MS		100
+#define AD4695_T_REGCONFIG_NS		20
 #define AD4695_REG_ACCESS_SCLK_HZ	(10 * MEGA)
 
+/* Max number of voltage input channels. */
 #define AD4695_MAX_CHANNELS		16
+/* Max size of 1 raw sample in bytes. */
+#define AD4695_MAX_CHANNEL_SIZE		2
 
 enum ad4695_in_pair {
 	AD4695_IN_PAIR_REFGND,
@@ -112,15 +122,21 @@ struct ad4695_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
-	struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 1];
+	/* voltages channels plus temperature and timestamp */
+	struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 2];
 	struct ad4695_channel_config channels_cfg[AD4695_MAX_CHANNELS];
 	const struct ad4695_chip_info *chip_info;
 	/* Reference voltage. */
 	unsigned int vref_mv;
 	/* Common mode input pin voltage. */
 	unsigned int com_mv;
+	/* 1 per voltage and temperature chan plus 1 xfer to trigger 1st CNV */
+	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS + 2];
+	struct spi_message buf_read_msg;
 	/* Raw conversion data received. */
-	u16 raw_data __aligned(IIO_DMA_MINALIGN);
+	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
+		     sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
+	u16 raw_data;
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;
 	u8 cnv_cmd2;
@@ -193,6 +209,9 @@ static const struct iio_chan_spec ad4695_temp_channel_template = {
 	},
 };
 
+static const struct iio_chan_spec ad4695_soft_timestamp_channel_template =
+	IIO_CHAN_SOFT_TIMESTAMP(0);
+
 static const char * const ad4695_power_supplies[] = {
 	"avdd", "vio"
 };
@@ -254,6 +273,62 @@ static int ad4695_set_single_cycle_mode(struct ad4695_state *st,
 			       AD4695_REG_SETUP_SPI_CYC_CTRL);
 }
 
+/**
+ * ad4695_enter_advanced_sequencer_mode - Put the ADC in advanced sequencer mode
+ * @st: The driver state
+ * @n: The number of slots to use - must be >= 2, <= 128
+ *
+ * As per the datasheet, to enable advanced sequencer, we need to set
+ * STD_SEQ_EN=0, NUM_SLOTS_AS=n-1 and CYC_CTRL=0 (Table 15). Setting SPI_MODE=1
+ * triggers the first conversion using the channel in AS_SLOT0.
+ *
+ * Return: 0 on success, a negative error code on failure
+ */
+static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n)
+{
+	u32 mask, val;
+	int ret;
+
+	mask = AD4695_REG_SEQ_CTRL_STD_SEQ_EN;
+	val = FIELD_PREP(AD4695_REG_SEQ_CTRL_STD_SEQ_EN, 0);
+
+	mask |= AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS;
+	val |= FIELD_PREP(AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS, n - 1);
+
+	ret = regmap_update_bits(st->regmap, AD4695_REG_SEQ_CTRL, mask, val);
+	if (ret)
+		return ret;
+
+	mask = AD4695_REG_SETUP_SPI_MODE;
+	val = FIELD_PREP(AD4695_REG_SETUP_SPI_MODE, 1);
+
+	mask |= AD4695_REG_SETUP_SPI_CYC_CTRL;
+	val |= FIELD_PREP(AD4695_REG_SETUP_SPI_CYC_CTRL, 0);
+
+	return regmap_update_bits(st->regmap, AD4695_REG_SETUP, mask, val);
+}
+
+/**
+ * ad4695_exit_conversion_mode - Exit conversion mode
+ * @st: The AD4695 state
+ *
+ * Sends SPI command to exit conversion mode.
+ *
+ * Return: 0 on success, a negative error code on failure
+ */
+static int ad4695_exit_conversion_mode(struct ad4695_state *st)
+{
+	struct spi_transfer xfer = { };
+
+	st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3;
+	xfer.tx_buf = &st->cnv_cmd2;
+	xfer.len = 1;
+	xfer.delay.value = AD4695_T_REGCONFIG_NS;
+	xfer.delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
 static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)
 {
 	u8 val;
@@ -296,6 +371,147 @@ static int ad4695_write_chn_cfg(struct ad4695_state *st,
 				  mask, val);
 }
 
+static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct ad4695_state *st = iio_priv(indio_dev);
+	struct spi_transfer *xfer;
+	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
+	bool temp_chan_en = false;
+	u32 reg, mask, val, bit, num_xfer, num_slots;
+	int ret;
+
+	/*
+	 * We are using the advanced sequencer since it is the only way to read
+	 * multiple channels that allows individual configuration of each
+	 * voltage input channel. Slot 0 in the advanced sequencer is used to
+	 * account for the gap between trigger polls - we don't read data from
+	 * this slot. Each enabled voltage channel is assigned a slot starting
+	 * with slot 1.
+	 */
+	num_slots = 1;
+
+	memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
+
+	/* First xfer is only to trigger conversion of slot 1, so no rx. */
+	xfer = &st->buf_read_xfer[0];
+	xfer->cs_change = 1;
+	xfer->delay.value = AD4695_T_CNVL_NS;
+	xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
+	xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
+	xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+	num_xfer = 1;
+
+	iio_for_each_active_channel(indio_dev, bit) {
+		xfer = &st->buf_read_xfer[num_xfer];
+		xfer->bits_per_word = 16;
+		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
+		xfer->len = 2;
+		xfer->cs_change = 1;
+		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
+		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+		if (bit == temp_chan_bit) {
+			temp_chan_en = true;
+		} else {
+			reg = AD4695_REG_AS_SLOT(num_slots);
+			val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
+
+			ret = regmap_write(st->regmap, reg, val);
+			if (ret)
+				return ret;
+
+			num_slots++;
+		}
+
+		num_xfer++;
+	}
+
+	/*
+	 * Don't keep CS asserted after last xfer. Also triggers conversion of
+	 * slot 0.
+	 */
+	xfer->cs_change = 0;
+
+	/**
+	 * The advanced sequencer requires that at least 2 slots are enabled.
+	 * Since slot 0 is always used for other purposes, we need only 1
+	 * enabled voltage channel to meet this requirement. This error will
+	 * only happen if only the temperature channel is enabled.
+	 */
+	if (num_slots < 2) {
+		dev_err_ratelimited(&indio_dev->dev,
+			"Buffered read requires at least 1 voltage channel enabled\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Temperature channel isn't included in the sequence, but rather
+	 * controlled by setting a bit in the TEMP_CTRL register.
+	 */
+
+	reg = AD4695_REG_TEMP_CTRL;
+	mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
+	val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
+
+	ret = regmap_update_bits(st->regmap, reg, mask, val);
+	if (ret)
+		return ret;
+
+	spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
+					num_xfer);
+
+	ret = spi_optimize_message(st->spi, &st->buf_read_msg);
+	if (ret)
+		return ret;
+
+	/* This triggers conversion of slot 0. */
+	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
+	if (ret) {
+		spi_unoptimize_message(&st->buf_read_msg);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ad4695_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad4695_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4695_exit_conversion_mode(st);
+	if (ret)
+		return ret;
+
+	spi_unoptimize_message(&st->buf_read_msg);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ad4695_buffer_setup_ops = {
+	.preenable = ad4695_buffer_preenable,
+	.postdisable = ad4695_buffer_postdisable,
+};
+
+static irqreturn_t ad4695_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4695_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = spi_sync(st->spi, &st->buf_read_msg);
+	if (ret)
+		goto out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buf, pf->timestamp);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * ad4695_read_one_sample - Read a single sample using single-cycle mode
  * @st: The AD4695 state
@@ -527,6 +743,10 @@ static int ad4695_parse_channel_cfg(struct ad4695_state *st)
 	/* Temperature channel must be next scan index after voltage channels. */
 	st->iio_chan[i] = ad4695_temp_channel_template;
 	st->iio_chan[i].scan_index = i;
+	i++;
+
+	st->iio_chan[i] = ad4695_soft_timestamp_channel_template;
+	st->iio_chan[i].scan_index = i;
 
 	return 0;
 }
@@ -695,7 +915,14 @@ static int ad4695_probe(struct spi_device *spi)
 	indio_dev->info = &ad4695_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = st->iio_chan;
-	indio_dev->num_channels = st->chip_info->num_voltage_inputs + 1;
+	indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad4695_trigger_handler,
+					      &ad4695_buffer_setup_ops);
+	if (ret)
+		return ret;
 
 	return devm_iio_device_register(dev, indio_dev);
 }

-- 
2.43.0

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 7cad163c39cb642ed587d3eeb37a5637ee02740f]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-implement-triggered-buffer/20240808-063333
base:   7cad163c39cb642ed587d3eeb37a5637ee02740f
patch link:    https://lore.kernel.org/r/20240807-iio-adc-ad4695-buffered-read-v1-1-bdafc39b2283%40baylibre.com
patch subject: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 408d82d352eb98e2d0a804c66d359cd7a49228fe)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408081623.ua9EBfoZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/iio/adc/ad4695.c:24:
   In file included from include/linux/iio/triggered_buffer.h:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/iio/adc/ad4695.c:24:
   In file included from include/linux/iio/triggered_buffer.h:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/iio/adc/ad4695.c:24:
   In file included from include/linux/iio/triggered_buffer.h:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   In file included from drivers/iio/adc/ad4695.c:28:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/adc/ad4695.c:454:8: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
     454 |         val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/bitfield.h:115:3: note: expanded from macro 'FIELD_PREP'
     115 |                 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/bitfield.h:72:53: note: expanded from macro '__BF_FIELD_CHECK'
      72 |                 BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
      73 |                                  __bf_cast_unsigned(_reg, ~0ull),       \
         |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      74 |                                  _pfx "type of reg too small for mask"); \
         |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   8 warnings generated.


vim +454 drivers/iio/adc/ad4695.c

   373	
   374	static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
   375	{
   376		struct ad4695_state *st = iio_priv(indio_dev);
   377		struct spi_transfer *xfer;
   378		u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
   379		bool temp_chan_en = false;
   380		u32 reg, mask, val, bit, num_xfer, num_slots;
   381		int ret;
   382	
   383		/*
   384		 * We are using the advanced sequencer since it is the only way to read
   385		 * multiple channels that allows individual configuration of each
   386		 * voltage input channel. Slot 0 in the advanced sequencer is used to
   387		 * account for the gap between trigger polls - we don't read data from
   388		 * this slot. Each enabled voltage channel is assigned a slot starting
   389		 * with slot 1.
   390		 */
   391		num_slots = 1;
   392	
   393		memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
   394	
   395		/* First xfer is only to trigger conversion of slot 1, so no rx. */
   396		xfer = &st->buf_read_xfer[0];
   397		xfer->cs_change = 1;
   398		xfer->delay.value = AD4695_T_CNVL_NS;
   399		xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
   400		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
   401		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
   402		num_xfer = 1;
   403	
   404		iio_for_each_active_channel(indio_dev, bit) {
   405			xfer = &st->buf_read_xfer[num_xfer];
   406			xfer->bits_per_word = 16;
   407			xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
   408			xfer->len = 2;
   409			xfer->cs_change = 1;
   410			xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
   411			xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
   412	
   413			if (bit == temp_chan_bit) {
   414				temp_chan_en = true;
   415			} else {
   416				reg = AD4695_REG_AS_SLOT(num_slots);
   417				val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
   418	
   419				ret = regmap_write(st->regmap, reg, val);
   420				if (ret)
   421					return ret;
   422	
   423				num_slots++;
   424			}
   425	
   426			num_xfer++;
   427		}
   428	
   429		/*
   430		 * Don't keep CS asserted after last xfer. Also triggers conversion of
   431		 * slot 0.
   432		 */
   433		xfer->cs_change = 0;
   434	
   435		/**
   436		 * The advanced sequencer requires that at least 2 slots are enabled.
   437		 * Since slot 0 is always used for other purposes, we need only 1
   438		 * enabled voltage channel to meet this requirement. This error will
   439		 * only happen if only the temperature channel is enabled.
   440		 */
   441		if (num_slots < 2) {
   442			dev_err_ratelimited(&indio_dev->dev,
   443				"Buffered read requires at least 1 voltage channel enabled\n");
   444			return -EINVAL;
   445		}
   446	
   447		/*
   448		 * Temperature channel isn't included in the sequence, but rather
   449		 * controlled by setting a bit in the TEMP_CTRL register.
   450		 */
   451	
   452		reg = AD4695_REG_TEMP_CTRL;
   453		mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
 > 454		val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
   455	
   456		ret = regmap_update_bits(st->regmap, reg, mask, val);
   457		if (ret)
   458			return ret;
   459	
   460		spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
   461						num_xfer);
   462	
   463		ret = spi_optimize_message(st->spi, &st->buf_read_msg);
   464		if (ret)
   465			return ret;
   466	
   467		/* This triggers conversion of slot 0. */
   468		ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
   469		if (ret) {
   470			spi_unoptimize_message(&st->buf_read_msg);
   471			return ret;
   472		}
   473	
   474		return 0;
   475	}
   476	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Jonathan Cameron]

On Thu, 8 Aug 2024 16:52:09 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi David,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 7cad163c39cb642ed587d3eeb37a5637ee02740f]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-implement-triggered-buffer/20240808-063333
> base:   7cad163c39cb642ed587d3eeb37a5637ee02740f
> patch link:    https://lore.kernel.org/r/20240807-iio-adc-ad4695-buffered-read-v1-1-bdafc39b2283%40baylibre.com
> patch subject: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer
> config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 408d82d352eb98e2d0a804c66d359cd7a49228fe)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408081623.ua9EBfoZ-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from drivers/iio/adc/ad4695.c:24:
>    In file included from include/linux/iio/triggered_buffer.h:6:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:14:
>    In file included from arch/hexagon/include/asm/io.h:328:
>    include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      548 |         val = __raw_readb(PCI_IOBASE + addr);
>          |                           ~~~~~~~~~~ ^
>    include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
>          |                                                         ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
>       37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>          |                                                   ^
>    In file included from drivers/iio/adc/ad4695.c:24:
>    In file included from include/linux/iio/triggered_buffer.h:6:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:14:
>    In file included from arch/hexagon/include/asm/io.h:328:
>    include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
>          |                                                         ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
>       35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
>          |                                                   ^
>    In file included from drivers/iio/adc/ad4695.c:24:
>    In file included from include/linux/iio/triggered_buffer.h:6:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:14:
>    In file included from arch/hexagon/include/asm/io.h:328:
>    include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      585 |         __raw_writeb(value, PCI_IOBASE + addr);
>          |                             ~~~~~~~~~~ ^
>    include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
>          |                                                       ~~~~~~~~~~ ^
>    include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
>          |                                                       ~~~~~~~~~~ ^
>    In file included from drivers/iio/adc/ad4695.c:28:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:21:
>    In file included from include/linux/mm.h:2228:
>    include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
>      514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
>          |                               ~~~~~~~~~~~ ^ ~~~
> >> drivers/iio/adc/ad4695.c:454:8: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]  
>      454 |         val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
>          |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/bitfield.h:115:3: note: expanded from macro 'FIELD_PREP'
>      115 |                 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/bitfield.h:72:53: note: expanded from macro '__BF_FIELD_CHECK'
>       72 |                 BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
>          |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
>       73 |                                  __bf_cast_unsigned(_reg, ~0ull),       \
>          |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       74 |                                  _pfx "type of reg too small for mask"); \
>          |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
>       39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>          |                                     ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
>    include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
>      510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>          |         ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
>      498 |         __compiletime_assert(condition, msg, prefix, suffix)
>          |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
>      490 |                 if (!(condition))                                       \
>          |                       ^~~~~~~~~
>    8 warnings generated.
> 
> 
> vim +454 drivers/iio/adc/ad4695.c
> 
>    373	
>    374	static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>    375	{
>    376		struct ad4695_state *st = iio_priv(indio_dev);
>    377		struct spi_transfer *xfer;
>    378		u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
>    379		bool temp_chan_en = false;
>    380		u32 reg, mask, val, bit, num_xfer, num_slots;
>    381		int ret;
>    382	
>    383		/*
>    384		 * We are using the advanced sequencer since it is the only way to read
>    385		 * multiple channels that allows individual configuration of each
>    386		 * voltage input channel. Slot 0 in the advanced sequencer is used to
>    387		 * account for the gap between trigger polls - we don't read data from
>    388		 * this slot. Each enabled voltage channel is assigned a slot starting
>    389		 * with slot 1.
>    390		 */
>    391		num_slots = 1;
>    392	
>    393		memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
>    394	
>    395		/* First xfer is only to trigger conversion of slot 1, so no rx. */
>    396		xfer = &st->buf_read_xfer[0];
>    397		xfer->cs_change = 1;
>    398		xfer->delay.value = AD4695_T_CNVL_NS;
>    399		xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
>    400		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>    401		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>    402		num_xfer = 1;
>    403	
>    404		iio_for_each_active_channel(indio_dev, bit) {
>    405			xfer = &st->buf_read_xfer[num_xfer];
>    406			xfer->bits_per_word = 16;
>    407			xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
>    408			xfer->len = 2;
>    409			xfer->cs_change = 1;
>    410			xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>    411			xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>    412	
>    413			if (bit == temp_chan_bit) {
>    414				temp_chan_en = true;
>    415			} else {
>    416				reg = AD4695_REG_AS_SLOT(num_slots);
>    417				val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
>    418	
>    419				ret = regmap_write(st->regmap, reg, val);
>    420				if (ret)
>    421					return ret;
>    422	
>    423				num_slots++;
>    424			}
>    425	
>    426			num_xfer++;
>    427		}
>    428	
>    429		/*
>    430		 * Don't keep CS asserted after last xfer. Also triggers conversion of
>    431		 * slot 0.
>    432		 */
>    433		xfer->cs_change = 0;
>    434	
>    435		/**
>    436		 * The advanced sequencer requires that at least 2 slots are enabled.
>    437		 * Since slot 0 is always used for other purposes, we need only 1
>    438		 * enabled voltage channel to meet this requirement. This error will
>    439		 * only happen if only the temperature channel is enabled.
>    440		 */
>    441		if (num_slots < 2) {
>    442			dev_err_ratelimited(&indio_dev->dev,
>    443				"Buffered read requires at least 1 voltage channel enabled\n");
>    444			return -EINVAL;
>    445		}
>    446	
>    447		/*
>    448		 * Temperature channel isn't included in the sequence, but rather
>    449		 * controlled by setting a bit in the TEMP_CTRL register.
>    450		 */
>    451	
>    452		reg = AD4695_REG_TEMP_CTRL;
>    453		mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
>  > 454		val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
  
Probably a case of the compiler somehow failing to squash the local variable.
I'd just go with
			val = FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN,
					 temp_chan_en ? 1 : 0);
>    455	
>    456		ret = regmap_update_bits(st->regmap, reg, mask, val);
>    457		if (ret)
>    458			return ret;
>    459	
>    460		spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
>    461						num_xfer);
>    462	
>    463		ret = spi_optimize_message(st->spi, &st->buf_read_msg);
>    464		if (ret)
>    465			return ret;
>    466	
>    467		/* This triggers conversion of slot 0. */
>    468		ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>    469		if (ret) {
>    470			spi_unoptimize_message(&st->buf_read_msg);
>    471			return ret;
>    472		}
>    473	
>    474		return 0;
>    475	}
>    476	
> 

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Nuno Sá]

On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote:
> This implements buffered reads for the ad4695 driver using the typical
> triggered buffer implementation, including adding a soft timestamp
> channel.
> 
> The chip has 4 different modes for doing conversions. The driver is
> using the advanced sequencer mode since that is the only mode that
> allows individual configuration of all aspects each channel (e.g.
> bipolar config currently and oversampling to be added in the future).
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Hi David,

Just two nit comments...

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 230 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 007ecb951bc3..a3bd5be36134 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c

...

> 
>  
> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4695_state *st = iio_priv(indio_dev);
> +	struct spi_transfer *xfer;
> +	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
> +	bool temp_chan_en = false;
> +	u32 reg, mask, val, bit, num_xfer, num_slots;
> +	int ret;
> +
> +	/*
> +	 * We are using the advanced sequencer since it is the only way to read
> +	 * multiple channels that allows individual configuration of each
> +	 * voltage input channel. Slot 0 in the advanced sequencer is used to
> +	 * account for the gap between trigger polls - we don't read data from
> +	 * this slot. Each enabled voltage channel is assigned a slot starting
> +	 * with slot 1.
> +	 */
> +	num_slots = 1;
> +
> +	memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
> +
> +	/* First xfer is only to trigger conversion of slot 1, so no rx. */
> +	xfer = &st->buf_read_xfer[0];
> +	xfer->cs_change = 1;
> +	xfer->delay.value = AD4695_T_CNVL_NS;
> +	xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
> +	xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> +	xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +	num_xfer = 1;
> +
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		xfer = &st->buf_read_xfer[num_xfer];
> +		xfer->bits_per_word = 16;
> +		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
> +		xfer->len = 2;
> +		xfer->cs_change = 1;
> +		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> +		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +		if (bit == temp_chan_bit) {
> +			temp_chan_en = true;
> +		} else {
> +			reg = AD4695_REG_AS_SLOT(num_slots);
> +			val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
> +
> +			ret = regmap_write(st->regmap, reg, val);
> +			if (ret)
> +				return ret;
> +
> +			num_slots++;
> +		}
> +
> +		num_xfer++;
> +	}
> +
> +	/*
> +	 * Don't keep CS asserted after last xfer. Also triggers conversion of
> +	 * slot 0.
> +	 */
> +	xfer->cs_change = 0;
> +
> +	/**
> +	 * The advanced sequencer requires that at least 2 slots are enabled.
> +	 * Since slot 0 is always used for other purposes, we need only 1
> +	 * enabled voltage channel to meet this requirement. This error will
> +	 * only happen if only the temperature channel is enabled.
> +	 */
> +	if (num_slots < 2) {
> +		dev_err_ratelimited(&indio_dev->dev,
> +			"Buffered read requires at least 1 voltage channel
> enabled\n");

This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ
routines where the log could be flooded.
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Temperature channel isn't included in the sequence, but rather
> +	 * controlled by setting a bit in the TEMP_CTRL register.
> +	 */
> +
> +	reg = AD4695_REG_TEMP_CTRL;
> +	mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
> +	val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
> +
> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
> +					num_xfer);
> +
> +	ret = spi_optimize_message(st->spi, &st->buf_read_msg);
> +	if (ret)
> +		return ret;
> +
> +	/* This triggers conversion of slot 0. */
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	if (ret) {
> +		spi_unoptimize_message(&st->buf_read_msg);
> +		return ret;
> +	}

Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not
return 0 on success)

ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
if (ret)
	spi_unoptimize_message(&st->buf_read_msg);

return ret;

- Nuno Sá

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[David Lechner]

On 8/9/24 9:24 AM, Nuno Sá wrote:
> On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote:
>> This implements buffered reads for the ad4695 driver using the typical
>> triggered buffer implementation, including adding a soft timestamp
>> channel.
>>
>> The chip has 4 different modes for doing conversions. The driver is
>> using the advanced sequencer mode since that is the only mode that
>> allows individual configuration of all aspects each channel (e.g.
>> bipolar config currently and oversampling to be added in the future).
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
> 
> Hi David,
> 
> Just two nit comments...
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
>>  drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 230 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>> index 007ecb951bc3..a3bd5be36134 100644
>> --- a/drivers/iio/adc/ad4695.c
>> +++ b/drivers/iio/adc/ad4695.c
> 
> ...
> 
>>
>>  
>> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4695_state *st = iio_priv(indio_dev);
>> +	struct spi_transfer *xfer;
>> +	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
>> +	bool temp_chan_en = false;
>> +	u32 reg, mask, val, bit, num_xfer, num_slots;
>> +	int ret;
>> +
>> +	/*
>> +	 * We are using the advanced sequencer since it is the only way to read
>> +	 * multiple channels that allows individual configuration of each
>> +	 * voltage input channel. Slot 0 in the advanced sequencer is used to
>> +	 * account for the gap between trigger polls - we don't read data from
>> +	 * this slot. Each enabled voltage channel is assigned a slot starting
>> +	 * with slot 1.
>> +	 */
>> +	num_slots = 1;
>> +
>> +	memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
>> +
>> +	/* First xfer is only to trigger conversion of slot 1, so no rx. */
>> +	xfer = &st->buf_read_xfer[0];
>> +	xfer->cs_change = 1;
>> +	xfer->delay.value = AD4695_T_CNVL_NS;
>> +	xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
>> +	xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +	xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +	num_xfer = 1;
>> +
>> +	iio_for_each_active_channel(indio_dev, bit) {
>> +		xfer = &st->buf_read_xfer[num_xfer];
>> +		xfer->bits_per_word = 16;
>> +		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
>> +		xfer->len = 2;
>> +		xfer->cs_change = 1;
>> +		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +
>> +		if (bit == temp_chan_bit) {
>> +			temp_chan_en = true;
>> +		} else {
>> +			reg = AD4695_REG_AS_SLOT(num_slots);
>> +			val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
>> +
>> +			ret = regmap_write(st->regmap, reg, val);
>> +			if (ret)
>> +				return ret;
>> +
>> +			num_slots++;
>> +		}
>> +
>> +		num_xfer++;
>> +	}
>> +
>> +	/*
>> +	 * Don't keep CS asserted after last xfer. Also triggers conversion of
>> +	 * slot 0.
>> +	 */
>> +	xfer->cs_change = 0;
>> +
>> +	/**
>> +	 * The advanced sequencer requires that at least 2 slots are enabled.
>> +	 * Since slot 0 is always used for other purposes, we need only 1
>> +	 * enabled voltage channel to meet this requirement. This error will
>> +	 * only happen if only the temperature channel is enabled.
>> +	 */
>> +	if (num_slots < 2) {
>> +		dev_err_ratelimited(&indio_dev->dev,
>> +			"Buffered read requires at least 1 voltage channel
>> enabled\n");
> 
> This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ
> routines where the log could be flooded.

IIO Oscilloscope does a lot of retries of buffered reads very quickly,
so was getting a minor flood (10-20 repeats). I'm not sure that
ratelimited actually helped in this case though.

I suppose we could just drop this and expect people to read the docs
if they get an EINVAL when attempting to enable the buffer. Or just
make it dev_err() since it isn't 100s of repeats.

>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Temperature channel isn't included in the sequence, but rather
>> +	 * controlled by setting a bit in the TEMP_CTRL register.
>> +	 */
>> +
>> +	reg = AD4695_REG_TEMP_CTRL;
>> +	mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
>> +	val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
>> +
>> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
>> +					num_xfer);
>> +
>> +	ret = spi_optimize_message(st->spi, &st->buf_read_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* This triggers conversion of slot 0. */
>> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>> +	if (ret) {
>> +		spi_unoptimize_message(&st->buf_read_msg);
>> +		return ret;
>> +	}
> 
> Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not
> return 0 on success)

sure

> 
> ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> if (ret)
> 	spi_unoptimize_message(&st->buf_read_msg);
> 
> return ret;
> 
> - Nuno Sá
> 

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Jonathan Cameron]

On Wed,  7 Aug 2024 15:02:10 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This implements buffered reads for the ad4695 driver using the typical
> triggered buffer implementation, including adding a soft timestamp
> channel.
> 
> The chip has 4 different modes for doing conversions. The driver is
> using the advanced sequencer mode since that is the only mode that
> allows individual configuration of all aspects each channel (e.g.
> bipolar config currently and oversampling to be added in the future).
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Main thing in here is I think you can use available_scan_masks
to avoid the need for the error path on just the temperature channel
being enabled.

Jonathan

> +/**
> + * ad4695_enter_advanced_sequencer_mode - Put the ADC in advanced sequencer mode
> + * @st: The driver state
> + * @n: The number of slots to use - must be >= 2, <= 128
> + *
> + * As per the datasheet, to enable advanced sequencer, we need to set
> + * STD_SEQ_EN=0, NUM_SLOTS_AS=n-1 and CYC_CTRL=0 (Table 15). Setting SPI_MODE=1
> + * triggers the first conversion using the channel in AS_SLOT0.
> + *
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n)
> +{
> +	u32 mask, val;
> +	int ret;
> +
> +	mask = AD4695_REG_SEQ_CTRL_STD_SEQ_EN;
> +	val = FIELD_PREP(AD4695_REG_SEQ_CTRL_STD_SEQ_EN, 0);
> +
> +	mask |= AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS;
> +	val |= FIELD_PREP(AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS, n - 1);
> +
> +	ret = regmap_update_bits(st->regmap, AD4695_REG_SEQ_CTRL, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	mask = AD4695_REG_SETUP_SPI_MODE;
> +	val = FIELD_PREP(AD4695_REG_SETUP_SPI_MODE, 1);
> +
> +	mask |= AD4695_REG_SETUP_SPI_CYC_CTRL;
> +	val |= FIELD_PREP(AD4695_REG_SETUP_SPI_CYC_CTRL, 0);

I'd just combine the two parts of mask and val.  If it were a long
complex list then fair enough to keep them as individual parts, but
not needed for 2 items.

> +
> +	return regmap_update_bits(st->regmap, AD4695_REG_SETUP, mask, val);
> +}
> +
> +/**
> + * ad4695_exit_conversion_mode - Exit conversion mode
> + * @st: The AD4695 state
> + *
> + * Sends SPI command to exit conversion mode.
> + *
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad4695_exit_conversion_mode(struct ad4695_state *st)
> +{
> +	struct spi_transfer xfer = { };
	struct spi_transfer xfer = {
		.tx_buf = &st->cnv_cmd2,
		.len = 1,
		.delay.value ...

	};

Might as well fill it in from the start.
Doesn't matter that the data is filled in just after this even if
that doesn't feel quite right, the code is so close I don't think
it will confuse readers and it's a common pattern.

> +
> +	st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3;
> +	xfer.tx_buf = &st->cnv_cmd2;
> +	xfer.len = 1;
> +	xfer.delay.value = AD4695_T_REGCONFIG_NS;
> +	xfer.delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
>  static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)
>  {
>  	u8 val;
> @@ -296,6 +371,147 @@ static int ad4695_write_chn_cfg(struct ad4695_state *st,
>  				  mask, val);
>  }
>  
> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4695_state *st = iio_priv(indio_dev);
> +	struct spi_transfer *xfer;
> +	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
> +	bool temp_chan_en = false;
> +	u32 reg, mask, val, bit, num_xfer, num_slots;
> +	int ret;
> +
> +	/*
> +	 * We are using the advanced sequencer since it is the only way to read
> +	 * multiple channels that allows individual configuration of each
> +	 * voltage input channel. Slot 0 in the advanced sequencer is used to
> +	 * account for the gap between trigger polls - we don't read data from
> +	 * this slot. Each enabled voltage channel is assigned a slot starting
> +	 * with slot 1.
> +	 */
> +	num_slots = 1;
> +
> +	memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
> +
> +	/* First xfer is only to trigger conversion of slot 1, so no rx. */
> +	xfer = &st->buf_read_xfer[0];
> +	xfer->cs_change = 1;
> +	xfer->delay.value = AD4695_T_CNVL_NS;
> +	xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
> +	xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> +	xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +	num_xfer = 1;
> +
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		xfer = &st->buf_read_xfer[num_xfer];
> +		xfer->bits_per_word = 16;
> +		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
> +		xfer->len = 2;
> +		xfer->cs_change = 1;
> +		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> +		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +		if (bit == temp_chan_bit) {
> +			temp_chan_en = true;
> +		} else {
> +			reg = AD4695_REG_AS_SLOT(num_slots);
> +			val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
> +
> +			ret = regmap_write(st->regmap, reg, val);
> +			if (ret)
> +				return ret;
> +
> +			num_slots++;
> +		}
> +
> +		num_xfer++;
> +	}
> +
> +	/*
> +	 * Don't keep CS asserted after last xfer. Also triggers conversion of
> +	 * slot 0.
> +	 */
> +	xfer->cs_change = 0;
> +
> +	/**
> +	 * The advanced sequencer requires that at least 2 slots are enabled.
> +	 * Since slot 0 is always used for other purposes, we need only 1
> +	 * enabled voltage channel to meet this requirement. This error will
> +	 * only happen if only the temperature channel is enabled.
> +	 */
> +	if (num_slots < 2) {

Can you use available_scanmasks to let the IIO core figure out it needs
to enable (and then hide) an extra channel?

Either that or spin up a channel to meet the requirement, just don't
capture it - Given this is an unlikely case, better to leave it to the
IIO core buffer demux handling than to bother handling locally.


> +		dev_err_ratelimited(&indio_dev->dev,
> +			"Buffered read requires at least 1 voltage channel enabled\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Temperature channel isn't included in the sequence, but rather
> +	 * controlled by setting a bit in the TEMP_CTRL register.
> +	 */
> +
> +	reg = AD4695_REG_TEMP_CTRL;
> +	mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
> +	val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
This is the line the bot didn't like. The local variables reg and
mask don't add anything anyway, so get rid of them and use the
values inline.

> +
> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
> +					num_xfer);
> +
> +	ret = spi_optimize_message(st->spi, &st->buf_read_msg);
> +	if (ret)
> +		return ret;
> +
> +	/* This triggers conversion of slot 0. */
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	if (ret) {
> +		spi_unoptimize_message(&st->buf_read_msg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[David Lechner]

On 8/10/24 4:35 AM, Jonathan Cameron wrote:
> On Wed,  7 Aug 2024 15:02:10 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> This implements buffered reads for the ad4695 driver using the typical
>> triggered buffer implementation, including adding a soft timestamp
>> channel.
>>
>> The chip has 4 different modes for doing conversions. The driver is
>> using the advanced sequencer mode since that is the only mode that
>> allows individual configuration of all aspects each channel (e.g.
>> bipolar config currently and oversampling to be added in the future).
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> 
> Main thing in here is I think you can use available_scan_masks
> to avoid the need for the error path on just the temperature channel
> being enabled.
> 
I had not thought about doing it that way, but now that I am
thinking about it, it seems like we would need to have a scan
mask in the list for every possible combination of channels.
This would be 10s of thousands of possible scan masks for 16
channel chips so that doesn't seem like the best way to go.

But adding some special handling to make the temperature
channel just work should be easy enough to add.

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Nuno Sá]

On Mon, 2024-08-12 at 12:03 -0500, David Lechner wrote:
> On 8/10/24 4:35 AM, Jonathan Cameron wrote:
> > On Wed,  7 Aug 2024 15:02:10 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> > 
> > > This implements buffered reads for the ad4695 driver using the typical
> > > triggered buffer implementation, including adding a soft timestamp
> > > channel.
> > > 
> > > The chip has 4 different modes for doing conversions. The driver is
> > > using the advanced sequencer mode since that is the only mode that
> > > allows individual configuration of all aspects each channel (e.g.
> > > bipolar config currently and oversampling to be added in the future).
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > 
> > Main thing in here is I think you can use available_scan_masks
> > to avoid the need for the error path on just the temperature channel
> > being enabled.
> > 
> I had not thought about doing it that way, but now that I am
> thinking about it, it seems like we would need to have a scan
> mask in the list for every possible combination of channels.
> This would be 10s of thousands of possible scan masks for 16
> channel chips so that doesn't seem like the best way to go.
> 
> But adding some special handling to make the temperature
> channel just work should be easy enough to add.
> 

Not sure if the following is meaningful to this usecase but I used to think like you
but then realized that iio_scan_mask_match() will do bitmap_subset(). So you only
need to enable a subset of the available scan mask for things to work (and with that
you should no longer need an insane number of combinations). The core will then take
care of demuxing the actual enabled channels. AFAIR, strict scan matching is only
used for HW buffering.

- Nuno Sá

Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Jonathan Cameron]

On Tue, 13 Aug 2024 09:28:06 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2024-08-12 at 12:03 -0500, David Lechner wrote:
> > On 8/10/24 4:35 AM, Jonathan Cameron wrote:  
> > > On Wed,  7 Aug 2024 15:02:10 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >   
> > > > This implements buffered reads for the ad4695 driver using the typical
> > > > triggered buffer implementation, including adding a soft timestamp
> > > > channel.
> > > > 
> > > > The chip has 4 different modes for doing conversions. The driver is
> > > > using the advanced sequencer mode since that is the only mode that
> > > > allows individual configuration of all aspects each channel (e.g.
> > > > bipolar config currently and oversampling to be added in the future).
> > > > 
> > > > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> > > 
> > > Main thing in here is I think you can use available_scan_masks
> > > to avoid the need for the error path on just the temperature channel
> > > being enabled.
> > >   
> > I had not thought about doing it that way, but now that I am
> > thinking about it, it seems like we would need to have a scan
> > mask in the list for every possible combination of channels.
> > This would be 10s of thousands of possible scan masks for 16
> > channel chips so that doesn't seem like the best way to go.

Indeed not my best suggestion.

> > 
> > But adding some special handling to make the temperature
> > channel just work should be easy enough to add.
> >   
> 
> Not sure if the following is meaningful to this usecase but I used to think like you
> but then realized that iio_scan_mask_match() will do bitmap_subset(). So you only
> need to enable a subset of the available scan mask for things to work (and with that
> you should no longer need an insane number of combinations). The core will then take
> care of demuxing the actual enabled channels. AFAIR, strict scan matching is only
> used for HW buffering.

Hmm. The validate_scan_mask callback also doesn't work as that's really about
restricting cases where we are onehot or similar (so restricting number of channels
or that sort of thing).

So, I think this is a driver problem to hide it.

Just have some logic in the driver that enables a dummy channel if only the
temperature is enabled and throw it away (probably fine to put it in the
data passed to iio_push_to_buffers() and rely on the it either being
treated as garbage, or dropped depending on whether it is in a hole, or
on the end of the scan.  That should give the intuitive interface
we expect (no restrictions to bite us that don't have to be there - see onehot
case where we have no choice) without adding too much complexity.

Jonathan

> 
> - Nuno Sá

[PATCH 2/2] doc: iio: ad4695: document buffered read

[David Lechner]

The ad4695 driver has a quirk where the temperature channel can't be
enabled on its own for buffered reads, so we should document this.

Also, since there are 4 possible modes of reading conversion data, it
is useful to know which one is actually being used, namely the advanced
sequencer mode.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/iio/ad4695.rst | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/iio/ad4695.rst b/Documentation/iio/ad4695.rst
index a33e573d61d6..af76ce2d0702 100644
--- a/Documentation/iio/ad4695.rst
+++ b/Documentation/iio/ad4695.rst
@@ -147,9 +147,19 @@ Unimplemented features
 ----------------------
 
 - Additional wiring modes
-- Buffered reads
 - Threshold events
 - Oversampling
 - Gain/offset calibration
 - GPIO support
 - CRC support
+
+Device buffers
+==============
+
+This driver supports hardware triggered buffers. This uses the "advanced
+sequencer" feature of the chip to trigger a burst of conversions.
+
+Due to hardware constraints, the temperature channel cannot be read on its own
+for buffered reads. At least one voltage channel must also be enabled.
+
+Also see :doc:`iio_devbuf` for more general information.

-- 
2.43.0

Re: [PATCH 2/2] doc: iio: ad4695: document buffered read

[Jonathan Cameron]

On Wed,  7 Aug 2024 15:02:11 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The ad4695 driver has a quirk where the temperature channel can't be
> enabled on its own for buffered reads, so we should document this.
> 
> Also, since there are 4 possible modes of reading conversion data, it
> is useful to know which one is actually being used, namely the advanced
> sequencer mode.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  Documentation/iio/ad4695.rst | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/iio/ad4695.rst b/Documentation/iio/ad4695.rst
> index a33e573d61d6..af76ce2d0702 100644
> --- a/Documentation/iio/ad4695.rst
> +++ b/Documentation/iio/ad4695.rst
> @@ -147,9 +147,19 @@ Unimplemented features
>  ----------------------
>  
>  - Additional wiring modes
> -- Buffered reads
>  - Threshold events
>  - Oversampling
>  - Gain/offset calibration
>  - GPIO support
>  - CRC support
> +
> +Device buffers
> +==============
> +
> +This driver supports hardware triggered buffers. This uses the "advanced
> +sequencer" feature of the chip to trigger a burst of conversions.
> +
> +Due to hardware constraints, the temperature channel cannot be read on its own
> +for buffered reads. At least one voltage channel must also be enabled.
As per driver review, userspace shouldn't care about that.
We can always read too much and throw the extra away :)

Jonathan

> +
> +Also see :doc:`iio_devbuf` for more general information.
>