[PATCH v2 0/8] iio: adc: ad_sigma_delta: Add sequencer support

[PATCH v2 0/8] iio: adc: ad_sigma_delta: Add sequencer support

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Some sigma-delta chips support sampling of multiple
channels in continuous mode.

When the operating with more than one channel enabled,
the channel sequencer cycles through the enabled channels
in sequential order, from first channel to the last one.
If a channel is disabled, it is skipped by the sequencer.

If more than one channel is used in continuous mode,
instruct the device to append the status to the SPI transfer
(1 extra byte) every time we receive a sample.
All sigma-delta chips possessing a sampling sequencer have
this ability. Inside the status register there will be
the number of the converted channel. In this way, even
if the CPU won't keep up with the sampling rate, it won't
send to userspace wrong channel samples.

1. Removed the 1 byte .shift from channel spec in AD7124,
it confuses userspace apps (no need to shift right).

2. Add update_scan_mode to AD7124, it is required in order
to enable/disable multiple channels at once

3. Add update_scan_mode to AD7192, it is required in order
to enable/disable multiple channels at once

4. Add sequencer support for sigma_delta library.

5. Add sigma_delta_info values and callbacks for sequencer
support in AD7124.

6. Add sigma_delta_info values and callbacks for sequencer
support in AD7192.

7. Add disable_all() callback in AD7124 driver. Need this to
disable channels in ad_sd_buffer_postdisable.

8. Add disable_all() callback in AD7192 driver. Need this to
disable channels in ad_sd_buffer_postdisable.

Alexandru Tachici (8):
  iio: adc: ad7124: Remove shift from scan_type
  iio: adc: ad7124: Add update_scan_mode
  iio: adc: ad7192: Add update_scan_mode
  iio: adc: ad_sigma_delta: Add sequencer support
  iio: adc: ad7124: add sequencer support
  iio: adc: ad7192: add sequencer support
  iio: adc: ad7124: add disable_all() callback
  iio: adc: ad7192: add disable_all() callback

Changelog V1 -> V2:
  - changed commits descriptions
  - in ad_sd_buffer_postenable: switched from kzalloc to krealloc and added
  space for the timestamp too.
  - added a fast path that avoids the extra copies created during sample number tracking
  for scans with (active_slots == 1) in ad_sd_trigger_handler
  - in ad7192_update_scan_mode(), use for_each_set_bit() instead of a simple for()
  - in ad_sd_init(), initialize num_slots to 1 if set on 0.
  - added disable_all() callback in ad_sigma_delta_info that should disable all channels
  It is always called in ad_sd_buffer_postdisable
  - in ad_sd_buffer_postenable(), use ad_sigma_delta_set_channel() only for devices
  with only one sequencer slot. For the ones with .num_slots > 1, update_scan_mode
  will already have enabled the required channels
  - add checks in ad_sd_init() for disable_all() and update_scan_mode() when
  num_slots > 1 (these are needed for normal operation of the sequencer)
  - in ad7124_update_scan_mode call set_channel on each channel

 drivers/iio/adc/ad7124.c               |  62 +++++++++++-
 drivers/iio/adc/ad7192.c               |  40 +++++++-
 drivers/iio/adc/ad_sigma_delta.c       | 134 +++++++++++++++++++++++--
 include/linux/iio/adc/ad_sigma_delta.h |  33 ++++++
 4 files changed, 254 insertions(+), 15 deletions(-)

--
2.25.1

[PATCH v2 1/8] iio: adc: ad7124: Remove shift from scan_type

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

The 24 bits data is stored in 32 bits in BE. There
is no need to shift it. This confuses user-space apps.

Fixes: b3af341bbd966 ("iio: adc: Add ad7124 support")
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7124.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 998a342d51a6..7249db2c4422 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -188,7 +188,6 @@ static const struct iio_chan_spec ad7124_channel_template = {
 		.sign = 'u',
 		.realbits = 24,
 		.storagebits = 32,
-		.shift = 8,
 		.endianness = IIO_BE,
 	},
 };
-- 
2.25.1

[PATCH v2 2/8] iio: adc: ad7124: Add update_scan_mode

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

The callback .set_channel cannot be used to enable
and disable multiple channels at once as they are
presented in the active_scan_mask.

By adding an update_scan_mode callback, every time the
continuous mode is activated, channels will be enabled/disabled
accordingly.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7124.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 7249db2c4422..428ec3e257d7 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -669,11 +669,32 @@ static const struct attribute_group ad7124_attrs_group = {
 	.attrs = ad7124_attributes,
 };
 
+static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad7124_state *st = iio_priv(indio_dev);
+	bool bit_set;
+	int ret;
+	int i;
+
+	for (i = 0; i < st->num_channels; i++) {
+		bit_set = test_bit(i, scan_mask);
+		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i),
+					    AD7124_CHANNEL_EN_MSK,
+					    AD7124_CHANNEL_EN(bit_set),
+					    2);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 static const struct iio_info ad7124_info = {
 	.read_raw = ad7124_read_raw,
 	.write_raw = ad7124_write_raw,
 	.debugfs_reg_access = &ad7124_reg_access,
 	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7124_update_scan_mode,
 	.attrs = &ad7124_attrs_group,
 };
 
-- 
2.25.1

[PATCH v2 3/8] iio: adc: ad7192: Add update_scan_mode

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

The callback .set_channel cannot be used to enable
multiple channels at once, only one is allowed
simultaneously.

By adding an update_scan_mode callback, every time the
continuous mode is activated, channels will be enabled/disabled
accordingly.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7192.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 770b4e59238f..adff6472e075 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -783,6 +783,18 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ad7192_update_scan_mode(struct iio_dev *indio_dev, const unsigned long *scan_mask)
+{
+	struct ad7192_state *st = iio_priv(indio_dev);
+	int i;
+
+	st->conf &= ~AD7192_CONF_CHAN_MASK;
+	for_each_set_bit(i, scan_mask, 8)
+		st->conf |= AD7192_CONF_CHAN(i);
+
+	return ad_sd_write_reg(&st->sd, AD7192_REG_CONF, 3, st->conf);
+}
+
 static const struct iio_info ad7192_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -790,6 +802,7 @@ static const struct iio_info ad7192_info = {
 	.read_avail = ad7192_read_avail,
 	.attrs = &ad7192_attribute_group,
 	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
 };
 
 static const struct iio_info ad7195_info = {
@@ -799,6 +812,7 @@ static const struct iio_info ad7195_info = {
 	.read_avail = ad7192_read_avail,
 	.attrs = &ad7195_attribute_group,
 	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
 };
 
 #define __AD719x_CHANNEL(_si, _channel1, _channel2, _address, _extend_name, \
-- 
2.25.1

[PATCH v2 4/8] iio: adc: ad_sigma_delta: Add sequencer support

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Some sigma-delta chips support sampling of multiple
channels in continuous mode.

When the operating with more than one channel enabled,
the channel sequencer cycles through the enabled channels
in sequential order, from first channel to the last one.
If a channel is disabled, it is skipped by the sequencer.

If more than one channel is used in continuous mode,
instruct the device to append the status to the SPI transfer
(1 extra byte) every time we receive a sample.
All sigma-delta chips possessing a sampling sequencer have
this ability. Inside the status register there will be
the number of the converted channel. In this way, even
if the CPU won't keep up with the sampling rate, it won't
send to userspace wrong channel samples.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 134 +++++++++++++++++++++++--
 include/linux/iio/adc/ad_sigma_delta.h |  33 ++++++
 2 files changed, 157 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index ebcd52526cac..5d1932ad2a22 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -342,15 +342,47 @@ EXPORT_SYMBOL_NS_GPL(ad_sigma_delta_single_conversion, IIO_AD_SIGMA_DELTA);
 static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 {
 	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+	unsigned int i, slot, samples_buf_size;
 	unsigned int channel;
+	uint8_t *samples_buf;
 	int ret;
 
-	channel = find_first_bit(indio_dev->active_scan_mask,
-				 indio_dev->masklength);
-	ret = ad_sigma_delta_set_channel(sigma_delta,
-		indio_dev->channels[channel].address);
-	if (ret)
-		return ret;
+	if (sigma_delta->num_slots == 1) {
+		channel = find_first_bit(indio_dev->active_scan_mask,
+					 indio_dev->masklength);
+		ret = ad_sigma_delta_set_channel(sigma_delta,
+						 indio_dev->channels[channel].address);
+		if (ret)
+			return ret;
+		slot = 1;
+	} else {
+		/*
+		 * At this point update_scan_mode already enabled the required channels.
+		 * For sigma-delta sequencer drivers with multiple slots, an update_scan_mode
+		 * implementation is mandatory.
+		 */
+		slot = 0;
+		for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+			sigma_delta->slots[slot] = indio_dev->channels[i].address;
+			slot++;
+		}
+	}
+
+	sigma_delta->active_slots = slot;
+	sigma_delta->current_slot = 0;
+
+	if (sigma_delta->active_slots > 1) {
+		ret = ad_sigma_delta_append_status(sigma_delta, true);
+		if (ret)
+			return ret;
+	}
+
+	samples_buf_size = slot * indio_dev->channels[0].scan_type.storagebits + sizeof(int64_t);
+	samples_buf = krealloc(sigma_delta->samples_buf, samples_buf_size, GFP_KERNEL);
+	if (!samples_buf)
+		return -ENOMEM;
+
+	sigma_delta->samples_buf = samples_buf;
 
 	spi_bus_lock(sigma_delta->spi->master);
 	sigma_delta->bus_locked = true;
@@ -386,6 +418,10 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
 	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
 
+	if (sigma_delta->status_appended)
+		ad_sigma_delta_append_status(sigma_delta, false);
+
+	ad_sigma_delta_disable_all(sigma_delta);
 	sigma_delta->bus_locked = false;
 	return spi_bus_unlock(sigma_delta->spi->master);
 }
@@ -396,6 +432,10 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
 	uint8_t *data = sigma_delta->rx_buf;
+	unsigned int transfer_size;
+	unsigned int sample_size;
+	unsigned int sample_pos;
+	unsigned int status_pos;
 	unsigned int reg_size;
 	unsigned int data_reg;
 
@@ -408,21 +448,63 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 	else
 		data_reg = AD_SD_REG_DATA;
 
+	/* Status word will be appended to the sample during transfer */
+	if (sigma_delta->status_appended)
+		transfer_size = reg_size + 1;
+	else
+		transfer_size = reg_size;
+
 	switch (reg_size) {
 	case 4:
 	case 2:
 	case 1:
-		ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[0]);
+		status_pos = reg_size;
+		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[0]);
 		break;
 	case 3:
+		status_pos = reg_size + 1;
 		/* We store 24 bit samples in a 32 bit word. Keep the upper
 		 * byte set to zero. */
-		ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[1]);
+		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[1]);
 		break;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);
+	/*
+	 * For devices sampling only one channel at
+	 * once, there is no need for sample number tracking.
+	 */
+	if (sigma_delta->active_slots == 1) {
+		iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);
+		goto irq_handled;
+	}
+
+	if (sigma_delta->status_appended) {
+		u8 converted_channel;
 
+		converted_channel = data[status_pos] & sigma_delta->info->status_ch_mask;
+		if (converted_channel != sigma_delta->slots[sigma_delta->current_slot]) {
+			/*
+			 * Desync occurred during continuous sampling of multiple channels.
+			 * Drop this incomplete sample and start from first channel again.
+			 */
+
+			sigma_delta->current_slot = 0;
+			goto irq_handled;
+		}
+	}
+
+	sample_size = indio_dev->channels[0].scan_type.storagebits / 8;
+	sample_pos = sample_size * sigma_delta->current_slot;
+	memcpy(&sigma_delta->samples_buf[sample_pos], data, sample_size);
+	sigma_delta->current_slot++;
+
+	if (sigma_delta->current_slot == sigma_delta->active_slots) {
+		sigma_delta->current_slot = 0;
+		iio_push_to_buffers_with_timestamp(indio_dev, sigma_delta->samples_buf,
+						   pf->timestamp);
+	}
+
+irq_handled:
 	iio_trigger_notify_done(indio_dev->trig);
 	sigma_delta->irq_dis = false;
 	enable_irq(sigma_delta->spi->irq);
@@ -430,10 +512,17 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static bool ad_sd_validate_scan_mask(struct iio_dev *indio_dev, const unsigned long *mask)
+{
+	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+
+	return bitmap_weight(mask, indio_dev->masklength) <= sigma_delta->num_slots;
+}
+
 static const struct iio_buffer_setup_ops ad_sd_buffer_setup_ops = {
 	.postenable = &ad_sd_buffer_postenable,
 	.postdisable = &ad_sd_buffer_postdisable,
-	.validate_scan_mask = &iio_validate_scan_mask_onehot,
+	.validate_scan_mask = &ad_sd_validate_scan_mask,
 };
 
 static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
@@ -513,8 +602,14 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
  */
 int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev)
 {
+	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
 	int ret;
 
+	sigma_delta->slots = devm_kcalloc(dev, sigma_delta->num_slots,
+					  sizeof(*sigma_delta->slots), GFP_KERNEL);
+	if (!sigma_delta->slots)
+		return -ENOMEM;
+
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      &iio_pollfunc_store_time,
 					      &ad_sd_trigger_handler,
@@ -541,6 +636,25 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 {
 	sigma_delta->spi = spi;
 	sigma_delta->info = info;
+
+	/* If the field is unset in ad_sigma_delta_info, asume there can only be 1 slot. */
+	if (!info->num_slots)
+		sigma_delta->num_slots = 1;
+	else
+		sigma_delta->num_slots = info->num_slots;
+
+	if (sigma_delta->num_slots > 1) {
+		if (!indio_dev->info->update_scan_mode) {
+			dev_err(&spi->dev, "iio_dev lacks update_scan_mode().\n");
+			return -EINVAL;
+		}
+
+		if (!info->disable_all) {
+			dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_all().\n");
+			return -EINVAL;
+		}
+	}
+
 	iio_device_set_drvdata(indio_dev, sigma_delta);
 
 	return 0;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index c525fd51652f..b921ceb8adfd 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -32,26 +32,34 @@ struct iio_dev;
 /**
  * struct ad_sigma_delta_info - Sigma Delta driver specific callbacks and options
  * @set_channel: Will be called to select the current channel, may be NULL.
+ * @append_status: Will be called to enable status append at the end of the sample, may be NULL.
  * @set_mode: Will be called to select the current mode, may be NULL.
+ * @disable_all: Will be called to disable all channels, may be NULL.
  * @postprocess_sample: Is called for each sampled data word, can be used to
  *		modify or drop the sample data, it, may be NULL.
  * @has_registers: true if the device has writable and readable registers, false
  *		if there is just one read-only sample data shift register.
  * @addr_shift: Shift of the register address in the communications register.
  * @read_mask: Mask for the communications register having the read bit set.
+ * @status_ch_mask: Mask for the channel number stored in status register.
  * @data_reg: Address of the data register, if 0 the default address of 0x3 will
  *   be used.
  * @irq_flags: flags for the interrupt used by the triggered buffer
+ * @num_slots: Number of sequencer slots
  */
 struct ad_sigma_delta_info {
 	int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
+	int (*append_status)(struct ad_sigma_delta *, bool append);
+	int (*disable_all)(struct ad_sigma_delta *);
 	int (*set_mode)(struct ad_sigma_delta *, enum ad_sigma_delta_mode mode);
 	int (*postprocess_sample)(struct ad_sigma_delta *, unsigned int raw_sample);
 	bool has_registers;
 	unsigned int addr_shift;
 	unsigned int read_mask;
+	unsigned int status_ch_mask;
 	unsigned int data_reg;
 	unsigned long irq_flags;
+	unsigned int num_slots;
 };
 
 /**
@@ -76,6 +84,13 @@ struct ad_sigma_delta {
 	uint8_t			comm;
 
 	const struct ad_sigma_delta_info *info;
+	unsigned int		active_slots;
+	unsigned int		current_slot;
+	unsigned int		num_slots;
+	bool			status_appended;
+	/* map slots to channels in order to know what to expect from devices */
+	unsigned int		*slots;
+	uint8_t			*samples_buf;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -97,6 +112,24 @@ static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,
 	return 0;
 }
 
+static inline int ad_sigma_delta_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	if (sd->info->append_status) {
+		sd->status_appended = append;
+		return sd->info->append_status(sd, append);
+	}
+
+	return 0;
+}
+
+static inline int ad_sigma_delta_disable_all(struct ad_sigma_delta *sd)
+{
+	if (sd->info->disable_all)
+		return sd->info->disable_all(sd);
+
+	return 0;
+}
+
 static inline int ad_sigma_delta_set_mode(struct ad_sigma_delta *sd,
 	unsigned int mode)
 {
-- 
2.25.1

[PATCH v2 5/8] iio: adc: ad7124: add sequencer support

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Add sequencer support for AD7124.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7124.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 428ec3e257d7..782b7cdd8ebe 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -43,6 +43,8 @@
 #define AD7124_STATUS_POR_FLAG_MSK	BIT(4)
 
 /* AD7124_ADC_CONTROL */
+#define AD7124_ADC_STATUS_EN_MSK	BIT(10)
+#define AD7124_ADC_STATUS_EN(x)		FIELD_PREP(AD7124_ADC_STATUS_EN_MSK, x)
 #define AD7124_ADC_CTRL_REF_EN_MSK	BIT(8)
 #define AD7124_ADC_CTRL_REF_EN(x)	FIELD_PREP(AD7124_ADC_CTRL_REF_EN_MSK, x)
 #define AD7124_ADC_CTRL_PWR_MSK	GENMASK(7, 6)
@@ -512,14 +514,27 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
 	return ret;
 }
 
+static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
+
+	st->adc_control &= ~AD7124_ADC_STATUS_EN_MSK;
+	st->adc_control |= AD7124_ADC_STATUS_EN(append);
+
+	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
+}
+
 static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.set_channel = ad7124_set_channel,
+	.append_status = ad7124_append_status,
 	.set_mode = ad7124_set_mode,
 	.has_registers = true,
 	.addr_shift = 0,
 	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
 	.data_reg = AD7124_DATA,
-	.irq_flags = IRQF_TRIGGER_FALLING
+	.num_slots = 8,
+	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
 static int ad7124_read_raw(struct iio_dev *indio_dev,
@@ -679,10 +694,11 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 
 	for (i = 0; i < st->num_channels; i++) {
 		bit_set = test_bit(i, scan_mask);
-		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i),
-					    AD7124_CHANNEL_EN_MSK,
-					    AD7124_CHANNEL_EN(bit_set),
-					    2);
+		if (bit_set)
+			ret = ad7124_set_channel(&st->sd, i);
+		else
+			ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK,
+						    0, 2);
 		if (ret < 0)
 			return ret;
 	}
@@ -906,12 +922,14 @@ static int ad7124_probe(struct spi_device *spi)
 
 	st->chip_info = info;
 
-	ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
-
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ad7124_info;
 
+	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
+	if (ret < 0)
+		return ret;
+
 	ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node);
 	if (ret < 0)
 		return ret;
-- 
2.25.1

[PATCH v2 6/8] iio: adc: ad7192: add sequencer support

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Add sequencer support for AD7192.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7192.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index adff6472e075..e59753c61274 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -58,7 +58,8 @@
 /* Mode Register Bit Designations (AD7192_REG_MODE) */
 #define AD7192_MODE_SEL(x)	(((x) & 0x7) << 21) /* Operation Mode Select */
 #define AD7192_MODE_SEL_MASK	(0x7 << 21) /* Operation Mode Select Mask */
-#define AD7192_MODE_DAT_STA	BIT(20) /* Status Register transmission */
+#define AD7192_MODE_STA(x)	(((x) & 0x1) << 20) /* Status Register transmission */
+#define AD7192_MODE_STA_MASK	BIT(20) /* Status Register transmission Mask */
 #define AD7192_MODE_CLKSRC(x)	(((x) & 0x3) << 18) /* Clock Source Select */
 #define AD7192_MODE_SINC3	BIT(15) /* SINC3 Filter Select */
 #define AD7192_MODE_ACX		BIT(14) /* AC excitation enable(AD7195 only)*/
@@ -288,12 +289,25 @@ static int ad7192_set_mode(struct ad_sigma_delta *sd,
 	return ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
 }
 
+static int ad7192_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	struct ad7192_state *st = ad_sigma_delta_to_ad7192(sd);
+
+	st->mode &= ~AD7192_MODE_STA_MASK;
+	st->mode |= AD7192_MODE_STA(append);
+
+	return ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+}
+
 static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
 	.set_channel = ad7192_set_channel,
+	.append_status = ad7192_append_status,
 	.set_mode = ad7192_set_mode,
 	.has_registers = true,
 	.addr_shift = 3,
 	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
+	.num_slots = 4,
 	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
-- 
2.25.1

[PATCH v2 7/8] iio: adc: ad7124: add disable_all() callback

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Add the disable all callback to the ad_sigma_delta_info struct.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7124.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 782b7cdd8ebe..3214079ff05f 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -524,9 +524,25 @@ static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
 	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 }
 
+static int ad7124_disable_all(struct ad_sigma_delta *sd)
+{
+	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
+	int ret;
+	int i;
+
+	for (i = 0; i < st->num_channels; i++) {
+		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK, 0, 2);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.set_channel = ad7124_set_channel,
 	.append_status = ad7124_append_status,
+	.disable_all = ad7124_disable_all,
 	.set_mode = ad7124_set_mode,
 	.has_registers = true,
 	.addr_shift = 0,
-- 
2.25.1

[PATCH v2 8/8] iio: adc: ad7192: add disable_all() callback

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Add disable_all() callback to the ad_sigma_delta_info.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7192.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index e59753c61274..5bc929b5367a 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -299,9 +299,19 @@ static int ad7192_append_status(struct ad_sigma_delta *sd, bool append)
 	return ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
 }
 
+static int ad7192_disable_all(struct ad_sigma_delta *sd)
+{
+	struct ad7192_state *st = ad_sigma_delta_to_ad7192(sd);
+
+	st->conf &= ~AD7192_CONF_CHAN_MASK;
+
+	return ad_sd_write_reg(&st->sd, AD7192_REG_CONF, 3, st->conf);
+}
+
 static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
 	.set_channel = ad7192_set_channel,
 	.append_status = ad7192_append_status,
+	.disable_all = ad7192_disable_all,
 	.set_mode = ad7192_set_mode,
 	.has_registers = true,
 	.addr_shift = 3,
-- 
2.25.1

Re: [PATCH v2 1/8] iio: adc: ad7124: Remove shift from scan_type

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:15 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> The 24 bits data is stored in 32 bits in BE. There
> is no need to shift it. This confuses user-space apps.
> 
> Fixes: b3af341bbd966 ("iio: adc: Add ad7124 support")
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
Hi Alexandru,

Just to confirm my understanding (which gets a bit messy when endian
conversions are involved - and it occurs to me that our docs
are not great on how to handle endian conversions with shifts).

With a little endian cpu:
After userspace performs the 32bit big endian to little endian conversion
the value the shift would have previously dropped the bottom 8 bits
of the channel reading?

Looking at what ad_sigma_delta is doing it's documented as
leaving the upper 8 bits as 0 so this would make sense.

Have I understood the issue correctly?

I'll need to hold this one for now as I'll need to rebase the
fixes-togreg branch of iio.git after rc1 is available.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/ad7124.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 998a342d51a6..7249db2c4422 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -188,7 +188,6 @@ static const struct iio_chan_spec ad7124_channel_template = {
>  		.sign = 'u',
>  		.realbits = 24,
>  		.storagebits = 32,
> -		.shift = 8,
>  		.endianness = IIO_BE,
>  	},
>  };

Re: [PATCH v2 2/8] iio: adc: ad7124: Add update_scan_mode

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:16 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> The callback .set_channel cannot be used to enable
> and disable multiple channels at once as they are
> presented in the active_scan_mask.
Trivial: Aim to wrap commit messages around 70ish chars
and whatever you pick be consistent.

Patch looks fine to me otherwise so if nothing else comes up, I
may rewrap these patch descriptions whilst applying.

Jonathan

> 
> By adding an update_scan_mode callback, every time the
> continuous mode is activated, channels will be enabled/disabled
> accordingly.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/iio/adc/ad7124.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 7249db2c4422..428ec3e257d7 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -669,11 +669,32 @@ static const struct attribute_group ad7124_attrs_group = {
>  	.attrs = ad7124_attributes,
>  };
>  
> +static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7124_state *st = iio_priv(indio_dev);
> +	bool bit_set;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < st->num_channels; i++) {
> +		bit_set = test_bit(i, scan_mask);
> +		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i),
> +					    AD7124_CHANNEL_EN_MSK,
> +					    AD7124_CHANNEL_EN(bit_set),
> +					    2);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  static const struct iio_info ad7124_info = {
>  	.read_raw = ad7124_read_raw,
>  	.write_raw = ad7124_write_raw,
>  	.debugfs_reg_access = &ad7124_reg_access,
>  	.validate_trigger = ad_sd_validate_trigger,
> +	.update_scan_mode = ad7124_update_scan_mode,
>  	.attrs = &ad7124_attrs_group,
>  };
>  

Re: [PATCH v2 4/8] iio: adc: ad_sigma_delta: Add sequencer support

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:18 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>

If this is originally Lars' patch, the From should match
Lars.  If you've taken if further than appropriate
co-developed tag is needed.

> 
> Some sigma-delta chips support sampling of multiple
> channels in continuous mode.
> 
> When the operating with more than one channel enabled,
> the channel sequencer cycles through the enabled channels
> in sequential order, from first channel to the last one.
> If a channel is disabled, it is skipped by the sequencer.
> 
> If more than one channel is used in continuous mode,
> instruct the device to append the status to the SPI transfer
> (1 extra byte) every time we receive a sample.
> All sigma-delta chips possessing a sampling sequencer have
> this ability. Inside the status register there will be
> the number of the converted channel. In this way, even
> if the CPU won't keep up with the sampling rate, it won't
> send to userspace wrong channel samples.

It would be good to say what it does do if this happens as well
as what it doesn't do.  You have it well commented below, but
I think you should also mention it here.

A few comments inline but the basic approach looks good to me.

Thanks,

Jonathan


> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 134 +++++++++++++++++++++++--
>  include/linux/iio/adc/ad_sigma_delta.h |  33 ++++++
>  2 files changed, 157 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index ebcd52526cac..5d1932ad2a22 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -342,15 +342,47 @@ EXPORT_SYMBOL_NS_GPL(ad_sigma_delta_single_conversion, IIO_AD_SIGMA_DELTA);
>  static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +	unsigned int i, slot, samples_buf_size;
>  	unsigned int channel;
> +	uint8_t *samples_buf;
>  	int ret;
>  
> -	channel = find_first_bit(indio_dev->active_scan_mask,
> -				 indio_dev->masklength);
> -	ret = ad_sigma_delta_set_channel(sigma_delta,
> -		indio_dev->channels[channel].address);
> -	if (ret)
> -		return ret;
> +	if (sigma_delta->num_slots == 1) {
> +		channel = find_first_bit(indio_dev->active_scan_mask,
> +					 indio_dev->masklength);
> +		ret = ad_sigma_delta_set_channel(sigma_delta,
> +						 indio_dev->channels[channel].address);
> +		if (ret)
> +			return ret;
> +		slot = 1;
> +	} else {
> +		/*
> +		 * At this point update_scan_mode already enabled the required channels.
> +		 * For sigma-delta sequencer drivers with multiple slots, an update_scan_mode
> +		 * implementation is mandatory.
> +		 */
> +		slot = 0;
> +		for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +			sigma_delta->slots[slot] = indio_dev->channels[i].address;
> +			slot++;
> +		}
> +	}
> +
> +	sigma_delta->active_slots = slot;
> +	sigma_delta->current_slot = 0;
> +
> +	if (sigma_delta->active_slots > 1) {
> +		ret = ad_sigma_delta_append_status(sigma_delta, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	samples_buf_size = slot * indio_dev->channels[0].scan_type.storagebits + sizeof(int64_t);

In general slot might be such that this size is not a multiple of 8 bytes so the timestamp
would end up unaligned (it'll just go off the end currently) so you need an ALIGN() to
force the size of slog * indio_dev->channels[0].scan_type.storagebits up to a multiple of 8.

> +	samples_buf = krealloc(sigma_delta->samples_buf, samples_buf_size, GFP_KERNEL);
> +	if (!samples_buf)
> +		return -ENOMEM;

Where is this freed?  Probably a case for a devm_krealloc()

> +
> +	sigma_delta->samples_buf = samples_buf;
>  
>  	spi_bus_lock(sigma_delta->spi->master);
>  	sigma_delta->bus_locked = true;
> @@ -386,6 +418,10 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
>  
> +	if (sigma_delta->status_appended)
> +		ad_sigma_delta_append_status(sigma_delta, false);
> +
> +	ad_sigma_delta_disable_all(sigma_delta);
>  	sigma_delta->bus_locked = false;
>  	return spi_bus_unlock(sigma_delta->spi->master);
>  }
> @@ -396,6 +432,10 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
>  	uint8_t *data = sigma_delta->rx_buf;
> +	unsigned int transfer_size;
> +	unsigned int sample_size;
> +	unsigned int sample_pos;
> +	unsigned int status_pos;
>  	unsigned int reg_size;
>  	unsigned int data_reg;
>  
> @@ -408,21 +448,63 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  	else
>  		data_reg = AD_SD_REG_DATA;
>  
> +	/* Status word will be appended to the sample during transfer */
> +	if (sigma_delta->status_appended)
> +		transfer_size = reg_size + 1;
> +	else
> +		transfer_size = reg_size;
> +
>  	switch (reg_size) {
>  	case 4:
>  	case 2:
>  	case 1:
> -		ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[0]);
> +		status_pos = reg_size;
> +		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[0]);
>  		break;
>  	case 3:
> +		status_pos = reg_size + 1;

This would benefit from a comment.  Why is it not just reg_size?

>  		/* We store 24 bit samples in a 32 bit word. Keep the upper
>  		 * byte set to zero. */
> -		ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[1]);
> +		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[1]);
>  		break;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);
> +	/*
> +	 * For devices sampling only one channel at
> +	 * once, there is no need for sample number tracking.
> +	 */
> +	if (sigma_delta->active_slots == 1) {
> +		iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);
> +		goto irq_handled;
> +	}
> +
> +	if (sigma_delta->status_appended) {
> +		u8 converted_channel;
>  
> +		converted_channel = data[status_pos] & sigma_delta->info->status_ch_mask;
> +		if (converted_channel != sigma_delta->slots[sigma_delta->current_slot]) {
> +			/*
> +			 * Desync occurred during continuous sampling of multiple channels.
> +			 * Drop this incomplete sample and start from first channel again.
> +			 */

Good.  Add this detail to the commit message as well.

> +
> +			sigma_delta->current_slot = 0;
> +			goto irq_handled;
> +		}
> +	}
> +
> +	sample_size = indio_dev->channels[0].scan_type.storagebits / 8;
> +	sample_pos = sample_size * sigma_delta->current_slot;
> +	memcpy(&sigma_delta->samples_buf[sample_pos], data, sample_size);
> +	sigma_delta->current_slot++;
> +
> +	if (sigma_delta->current_slot == sigma_delta->active_slots) {
> +		sigma_delta->current_slot = 0;
> +		iio_push_to_buffers_with_timestamp(indio_dev, sigma_delta->samples_buf,
> +						   pf->timestamp);
> +	}
> +
> +irq_handled:
>  	iio_trigger_notify_done(indio_dev->trig);
>  	sigma_delta->irq_dis = false;
>  	enable_irq(sigma_delta->spi->irq);
> @@ -430,10 +512,17 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static bool ad_sd_validate_scan_mask(struct iio_dev *indio_dev, const unsigned long *mask)
> +{
> +	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +
> +	return bitmap_weight(mask, indio_dev->masklength) <= sigma_delta->num_slots;
> +}
> +
>  static const struct iio_buffer_setup_ops ad_sd_buffer_setup_ops = {
>  	.postenable = &ad_sd_buffer_postenable,
>  	.postdisable = &ad_sd_buffer_postdisable,
> -	.validate_scan_mask = &iio_validate_scan_mask_onehot,
> +	.validate_scan_mask = &ad_sd_validate_scan_mask,
>  };
>  
>  static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
> @@ -513,8 +602,14 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
>   */
>  int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev)
>  {
> +	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
>  	int ret;
>  
> +	sigma_delta->slots = devm_kcalloc(dev, sigma_delta->num_slots,
> +					  sizeof(*sigma_delta->slots), GFP_KERNEL);
> +	if (!sigma_delta->slots)
> +		return -ENOMEM;
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      &iio_pollfunc_store_time,
>  					      &ad_sd_trigger_handler,
> @@ -541,6 +636,25 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  {
>  	sigma_delta->spi = spi;
>  	sigma_delta->info = info;
> +
> +	/* If the field is unset in ad_sigma_delta_info, asume there can only be 1 slot. */
> +	if (!info->num_slots)
> +		sigma_delta->num_slots = 1;
> +	else
> +		sigma_delta->num_slots = info->num_slots;
> +
> +	if (sigma_delta->num_slots > 1) {
> +		if (!indio_dev->info->update_scan_mode) {
> +			dev_err(&spi->dev, "iio_dev lacks update_scan_mode().\n");
> +			return -EINVAL;
> +		}
> +
> +		if (!info->disable_all) {
> +			dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_all().\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	iio_device_set_drvdata(indio_dev, sigma_delta);
>  
>  	return 0;
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index c525fd51652f..b921ceb8adfd 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -32,26 +32,34 @@ struct iio_dev;
>  /**
>   * struct ad_sigma_delta_info - Sigma Delta driver specific callbacks and options
>   * @set_channel: Will be called to select the current channel, may be NULL.
> + * @append_status: Will be called to enable status append at the end of the sample, may be NULL.
>   * @set_mode: Will be called to select the current mode, may be NULL.
> + * @disable_all: Will be called to disable all channels, may be NULL.

Not in the same order as the parameters below.

>   * @postprocess_sample: Is called for each sampled data word, can be used to
>   *		modify or drop the sample data, it, may be NULL.
>   * @has_registers: true if the device has writable and readable registers, false
>   *		if there is just one read-only sample data shift register.
>   * @addr_shift: Shift of the register address in the communications register.
>   * @read_mask: Mask for the communications register having the read bit set.
> + * @status_ch_mask: Mask for the channel number stored in status register.
>   * @data_reg: Address of the data register, if 0 the default address of 0x3 will
>   *   be used.
>   * @irq_flags: flags for the interrupt used by the triggered buffer
> + * @num_slots: Number of sequencer slots
>   */
>  struct ad_sigma_delta_info {
>  	int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
> +	int (*append_status)(struct ad_sigma_delta *, bool append);
> +	int (*disable_all)(struct ad_sigma_delta *);
>  	int (*set_mode)(struct ad_sigma_delta *, enum ad_sigma_delta_mode mode);
>  	int (*postprocess_sample)(struct ad_sigma_delta *, unsigned int raw_sample);
>  	bool has_registers;
>  	unsigned int addr_shift;
>  	unsigned int read_mask;
> +	unsigned int status_ch_mask;
>  	unsigned int data_reg;
>  	unsigned long irq_flags;
> +	unsigned int num_slots;
>  };
>  
>  /**
> @@ -76,6 +84,13 @@ struct ad_sigma_delta {
>  	uint8_t			comm;
>  
>  	const struct ad_sigma_delta_info *info;
> +	unsigned int		active_slots;
> +	unsigned int		current_slot;
> +	unsigned int		num_slots;
> +	bool			status_appended;
> +	/* map slots to channels in order to know what to expect from devices */
> +	unsigned int		*slots;
> +	uint8_t			*samples_buf;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> @@ -97,6 +112,24 @@ static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,
>  	return 0;
>  }
>  
> +static inline int ad_sigma_delta_append_status(struct ad_sigma_delta *sd, bool append)
> +{
> +	if (sd->info->append_status) {
> +		sd->status_appended = append;
> +		return sd->info->append_status(sd, append);

Minor but usually a better idea to only set the cached state if
the calls succeeds. So check return value of the call before updating
sd->status_appended.  In some error cases you won't know whether it's
set or not, but meh, we can't handle everything :)

> +	}
> +
> +	return 0;
> +}
> +
> +static inline int ad_sigma_delta_disable_all(struct ad_sigma_delta *sd)
> +{
> +	if (sd->info->disable_all)
> +		return sd->info->disable_all(sd);
> +
> +	return 0;
> +}
> +
>  static inline int ad_sigma_delta_set_mode(struct ad_sigma_delta *sd,
>  	unsigned int mode)
>  {

Re: [PATCH v2 5/8] iio: adc: ad7124: add sequencer support

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:19 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Add sequencer support for AD7124.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
Hi Alexandru,

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7124.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 428ec3e257d7..782b7cdd8ebe 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -43,6 +43,8 @@
>  #define AD7124_STATUS_POR_FLAG_MSK	BIT(4)
>  
>  /* AD7124_ADC_CONTROL */
> +#define AD7124_ADC_STATUS_EN_MSK	BIT(10)
> +#define AD7124_ADC_STATUS_EN(x)		FIELD_PREP(AD7124_ADC_STATUS_EN_MSK, x)
>  #define AD7124_ADC_CTRL_REF_EN_MSK	BIT(8)
>  #define AD7124_ADC_CTRL_REF_EN(x)	FIELD_PREP(AD7124_ADC_CTRL_REF_EN_MSK, x)
>  #define AD7124_ADC_CTRL_PWR_MSK	GENMASK(7, 6)
> @@ -512,14 +514,27 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
>  	return ret;
>  }
>  
> +static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
> +{
> +	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> +
> +	st->adc_control &= ~AD7124_ADC_STATUS_EN_MSK;
> +	st->adc_control |= AD7124_ADC_STATUS_EN(append);

Generally avoid updating cached state until you know the write succeeded.
So I would operate on a local variable and if ad_sd_write_reg() succeeds
copy that back into st->adc_control.

Obviously on error it 'might' have been updated successfully and something failed
after the write, but it is probably more likely the write failed.

> +
> +	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
> +}
> +
>  static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
>  	.set_channel = ad7124_set_channel,
> +	.append_status = ad7124_append_status,
>  	.set_mode = ad7124_set_mode,
>  	.has_registers = true,
>  	.addr_shift = 0,
>  	.read_mask = BIT(6),
> +	.status_ch_mask = GENMASK(3, 0),
>  	.data_reg = AD7124_DATA,
> -	.irq_flags = IRQF_TRIGGER_FALLING
> +	.num_slots = 8,
> +	.irq_flags = IRQF_TRIGGER_FALLING,
>  };
>  
>  static int ad7124_read_raw(struct iio_dev *indio_dev,
> @@ -679,10 +694,11 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
>  
>  	for (i = 0; i < st->num_channels; i++) {
>  		bit_set = test_bit(i, scan_mask);
> -		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i),
> -					    AD7124_CHANNEL_EN_MSK,
> -					    AD7124_CHANNEL_EN(bit_set),
> -					    2);
> +		if (bit_set)
> +			ret = ad7124_set_channel(&st->sd, i);

This is going to repeatedly take an release the cfg mutex. Perhaps it's worth
introducing a __ad7124_set_channel() that doesn't take the lock then take it around
this loop instead?

> +		else
> +			ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK,
> +						    0, 2);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -906,12 +922,14 @@ static int ad7124_probe(struct spi_device *spi)
>  
>  	st->chip_info = info;
>  
> -	ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
> -
>  	indio_dev->name = st->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &ad7124_info;
>  
> +	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node);
>  	if (ret < 0)
>  		return ret;

Re: [PATCH v2 7/8] iio: adc: ad7124: add disable_all() callback

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:21 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Add the disable all callback to the ad_sigma_delta_info struct.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>

I think patch 5 leaves a non working driver as this will be required
for slots > 1.  So squash this with patch 5.

> ---
>  drivers/iio/adc/ad7124.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 782b7cdd8ebe..3214079ff05f 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -524,9 +524,25 @@ static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
>  	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
>  }
>  
> +static int ad7124_disable_all(struct ad_sigma_delta *sd)
> +{
> +	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < st->num_channels; i++) {
> +		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK, 0, 2);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
>  	.set_channel = ad7124_set_channel,
>  	.append_status = ad7124_append_status,
> +	.disable_all = ad7124_disable_all,
>  	.set_mode = ad7124_set_mode,
>  	.has_registers = true,
>  	.addr_shift = 0,

Re: [PATCH v2 6/8] iio: adc: ad7192: add sequencer support

[Jonathan Cameron]

On Fri, 18 Mar 2022 18:27:20 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Add sequencer support for AD7192.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/iio/adc/ad7192.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index adff6472e075..e59753c61274 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -58,7 +58,8 @@
>  /* Mode Register Bit Designations (AD7192_REG_MODE) */
>  #define AD7192_MODE_SEL(x)	(((x) & 0x7) << 21) /* Operation Mode Select */
>  #define AD7192_MODE_SEL_MASK	(0x7 << 21) /* Operation Mode Select Mask */
> -#define AD7192_MODE_DAT_STA	BIT(20) /* Status Register transmission */
> +#define AD7192_MODE_STA(x)	(((x) & 0x1) << 20) /* Status Register transmission */
> +#define AD7192_MODE_STA_MASK	BIT(20) /* Status Register transmission Mask */
>  #define AD7192_MODE_CLKSRC(x)	(((x) & 0x3) << 18) /* Clock Source Select */
>  #define AD7192_MODE_SINC3	BIT(15) /* SINC3 Filter Select */
>  #define AD7192_MODE_ACX		BIT(14) /* AC excitation enable(AD7195 only)*/
> @@ -288,12 +289,25 @@ static int ad7192_set_mode(struct ad_sigma_delta *sd,
>  	return ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
>  }
>  
> +static int ad7192_append_status(struct ad_sigma_delta *sd, bool append)
> +{
> +	struct ad7192_state *st = ad_sigma_delta_to_ad7192(sd);
> +
> +	st->mode &= ~AD7192_MODE_STA_MASK;
> +	st->mode |= AD7192_MODE_STA(append);
> +
Another case where I would prefer the state cache be updated after the write
succeeds unless there is some reason that doesn't make sense.

+ Swash patch 8 into this one so we don't leave a broken driver inbetween.

Thanks,

Jonathan

> +	return ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +}
> +
>  static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
>  	.set_channel = ad7192_set_channel,
> +	.append_status = ad7192_append_status,
>  	.set_mode = ad7192_set_mode,
>  	.has_registers = true,
>  	.addr_shift = 3,
>  	.read_mask = BIT(6),
> +	.status_ch_mask = GENMASK(3, 0),
> +	.num_slots = 4,
>  	.irq_flags = IRQF_TRIGGER_FALLING,
>  };
>