All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support
@ 2021-11-10 11:17 alexandru.tachici
  2021-11-10 11:17 ` [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode alexandru.tachici
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, 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. Fix bug in AD7124 where channels stayed enabled
even when they are not supposed to, in continuous mode.

2. Fix bug in AD7192 where channels stayed enabled
even when they are not supposed to, in continuous mode.

3. Add sequencer support for sigma_delta library.

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

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

Alexandru Tachici (5):
  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

 drivers/iio/adc/ad7124.c               |  38 ++++++++-
 drivers/iio/adc/ad7192.c               |  32 +++++++-
 drivers/iio/adc/ad_sigma_delta.c       | 106 ++++++++++++++++++++-----
 include/linux/iio/adc/ad_sigma_delta.h |  22 +++++
 4 files changed, 178 insertions(+), 20 deletions(-)

--
2.25.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode
  2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
@ 2021-11-10 11:17 ` alexandru.tachici
  2021-11-12 16:54   ` Jonathan Cameron
  2021-11-10 11:17 ` [PATCH 2/5] iio: adc: ad7192: " alexandru.tachici
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, Alexandru Tachici

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

In continuous mode neither sigma_delta.c nor ad7124.c
will disable previously enabled channels.

Before this patch a channel stayed enabled indefinetly,
even when one another one was supposed to be sampled.
This causes mixed samples in continuous mode to be delivered
to the host.

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

Fixes: b3af341bbd966 ("iio: adc: Add ad7124 support")
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 11ce6a3729a5..30299b899799 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/5] iio: adc: ad7192: Add update_scan_mode
  2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
  2021-11-10 11:17 ` [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode alexandru.tachici
@ 2021-11-10 11:17 ` alexandru.tachici
  2021-11-12 16:57   ` Jonathan Cameron
  2021-11-10 11:17 ` [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, Alexandru Tachici

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

In continuous mode neither sigma_delta.c nor ad7192.c
will disable previously enabled channels.

Before this patch a channel stayed enabled indefinetly,
even when one another one was supposed to be sampled.
This causes mixed samples in continuous mode to be delivered
to the host.

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

Fixes: 3f7c3306cf38 ("staging:iio:ad7192: Use common Sigma Delta library")
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7192.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 2121a812b0c3..1fc0f4eb858e 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -782,6 +782,20 @@ 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 (i = 0; i < 8; i++) {
+		if (test_bit(i, scan_mask))
+			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,
@@ -789,6 +803,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 = {
@@ -798,6 +813,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
  2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
  2021-11-10 11:17 ` [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode alexandru.tachici
  2021-11-10 11:17 ` [PATCH 2/5] iio: adc: ad7192: " alexandru.tachici
@ 2021-11-10 11:17 ` alexandru.tachici
  2021-11-12 17:14   ` Jonathan Cameron
  2021-11-10 11:17 ` [PATCH 4/5] iio: adc: ad7124: add " alexandru.tachici
  2021-11-10 11:17 ` [PATCH 5/5] iio: adc: ad7192: " alexandru.tachici
  4 siblings, 1 reply; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, Alexandru Tachici, Lars-Peter Clausen

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       | 106 ++++++++++++++++++++-----
 include/linux/iio/adc/ad_sigma_delta.h |  22 +++++
 2 files changed, 110 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 1d652d9b2f5c..d595e7972975 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -342,33 +342,48 @@ EXPORT_SYMBOL_GPL(ad_sigma_delta_single_conversion);
 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 channel;
+	unsigned int i, slot;
 	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;
+	slot = 0;
+	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+		sigma_delta->slots[slot] = indio_dev->channels[i].address;
+		ret = ad_sigma_delta_set_channel(sigma_delta, indio_dev->channels[i].address);
+		if (ret)
+			return ret;
+		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;
+	}
+
+	kfree(sigma_delta->samples_buf);
+	sigma_delta->samples_buf = kzalloc(slot * indio_dev->channels[0].scan_type.storagebits,
+					   GFP_KERNEL);
+	if (!sigma_delta->samples_buf)
+		return -ENOMEM;
 
 	spi_bus_lock(sigma_delta->spi->master);
 	sigma_delta->bus_locked = true;
 	sigma_delta->keep_cs_asserted = true;
 
 	ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
-	if (ret)
-		goto err_unlock;
+	if (ret) {
+		kfree(sigma_delta->samples_buf);
+		spi_bus_unlock(sigma_delta->spi->master);
+		return ret;
+	}
 
 	sigma_delta->irq_dis = false;
 	enable_irq(sigma_delta->spi->irq);
 
 	return 0;
-
-err_unlock:
-	spi_bus_unlock(sigma_delta->spi->master);
-
-	return ret;
 }
 
 static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
@@ -386,6 +401,9 @@ 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);
+
 	sigma_delta->bus_locked = false;
 	return spi_bus_unlock(sigma_delta->spi->master);
 }
@@ -396,6 +414,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,20 +430,55 @@ 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);
+	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]) {
+			/* Desynq occurred during continuous sampling of multiple channels.
+			 * Drop this incomplete sample and start from first channel again.
+			 */
+
+			sigma_delta->current_slot = 0;
+			iio_trigger_notify_done(indio_dev->trig);
+			sigma_delta->irq_dis = false;
+			enable_irq(sigma_delta->spi->irq);
+
+			return 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);
+	}
 
 	iio_trigger_notify_done(indio_dev->trig);
 	sigma_delta->irq_dis = false;
@@ -430,10 +487,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->info->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)
@@ -517,8 +581,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->info->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,
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index c525fd51652f..758ff25590c3 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -32,6 +32,7 @@ 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.
  * @postprocess_sample: Is called for each sampled data word, can be used to
  *		modify or drop the sample data, it, may be NULL.
@@ -39,19 +40,24 @@ struct iio_dev;
  *		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 (*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 +82,12 @@ struct ad_sigma_delta {
 	uint8_t			comm;
 
 	const struct ad_sigma_delta_info *info;
+	unsigned int		active_slots;
+	unsigned int		current_slot;
+	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 +109,16 @@ 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_set_mode(struct ad_sigma_delta *sd,
 	unsigned int mode)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/5] iio: adc: ad7124: add sequencer support
  2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
                   ` (2 preceding siblings ...)
  2021-11-10 11:17 ` [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
@ 2021-11-10 11:17 ` alexandru.tachici
  2021-11-10 11:17 ` [PATCH 5/5] iio: adc: ad7192: " alexandru.tachici
  4 siblings, 0 replies; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, 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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 30299b899799..5760a222df5f 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,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/5] iio: adc: ad7192: add sequencer support
  2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
                   ` (3 preceding siblings ...)
  2021-11-10 11:17 ` [PATCH 4/5] iio: adc: ad7124: add " alexandru.tachici
@ 2021-11-10 11:17 ` alexandru.tachici
  4 siblings, 0 replies; 12+ messages in thread
From: alexandru.tachici @ 2021-11-10 11:17 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, 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 1fc0f4eb858e..e70f86d9a219 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)*/
@@ -287,12 +288,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode
  2021-11-10 11:17 ` [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode alexandru.tachici
@ 2021-11-12 16:54   ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-11-12 16:54 UTC (permalink / raw)
  To: alexandru.tachici; +Cc: linux-iio, linux-kernel

On Wed, 10 Nov 2021 13:17:46 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> In continuous mode neither sigma_delta.c nor ad7124.c
> will disable previously enabled channels.
> 
> Before this patch a channel stayed enabled indefinetly,
> even when one another one was supposed to be sampled.
> This causes mixed samples in continuous mode to be delivered
> to the host.
> 
> By adding an update_scan_mode callback, every time the
> continuous mode is activated, channels will be enabled/disabled
> accordingly.
> 
> Fixes: b3af341bbd966 ("iio: adc: Add ad7124 support")
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>

I'm a little confused about which paths this bug affects.

If we have done a single channel read, then the channel is always
disabled afterwards so we won't leave one on in that path.

So the remaining path is on a previous buffer setup. In that path
set_channel is called as part the post_enable callback and will
enable the channel.

Currently nothing disables it again, so I guess if you then
change the enabled channels and restart the buffer you see the
condition you are covering here.

However, as I read the docs, this should also affect any
single reads of a later channel than the one that was originally enabled.
e.g. buffered capture enabled for channel 0 then disabled.  Later read of
channel 1 may get the data from channel 0.

The fix you have here won't close that path because we don't
call update_scan_mode() in the buffer disable path (we have considered
it a few times, but so many devices don't have to do an explicit disable there
that we never put it in). Perhaps with hindsight we should have always
called it in the disable path, but maybe with a parameter to allow devices
to opt out of doing anything.

We could add core callback called something like disable_all() which
is called in the remove path. Alternatively the issue can be closed
in the ad_sigma_delta core by adding a callback to disable channels
in predisable().

Jonathan
 

> ---
>  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 11ce6a3729a5..30299b899799 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,
>  };
>  


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] iio: adc: ad7192: Add update_scan_mode
  2021-11-10 11:17 ` [PATCH 2/5] iio: adc: ad7192: " alexandru.tachici
@ 2021-11-12 16:57   ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-11-12 16:57 UTC (permalink / raw)
  To: alexandru.tachici; +Cc: linux-iio, linux-kernel

On Wed, 10 Nov 2021 13:17:47 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> In continuous mode neither sigma_delta.c nor ad7192.c
> will disable previously enabled channels.
> 
> Before this patch a channel stayed enabled indefinetly,
> even when one another one was supposed to be sampled.
> This causes mixed samples in continuous mode to be delivered
> to the host.

Can you expand a bit on the path that leads to this.  As far as I can tell
in both continuous mode enable and single channel reading set_channel()
callback is called and will overwrite the channels enabled previously.

Perhaps I'm missing a path in which that call isn't made?
> 
> By adding an update_scan_mode callback, every time the
> continuous mode is activated, channels will be enabled/disabled
> accordingly.
> 
> Fixes: 3f7c3306cf38 ("staging:iio:ad7192: Use common Sigma Delta library")
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/iio/adc/ad7192.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 2121a812b0c3..1fc0f4eb858e 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -782,6 +782,20 @@ 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 (i = 0; i < 8; i++) {
> +		if (test_bit(i, scan_mask))

for_each_set_bit()

> +			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,
> @@ -789,6 +803,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 = {
> @@ -798,6 +813,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, \


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
  2021-11-10 11:17 ` [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
@ 2021-11-12 17:14   ` Jonathan Cameron
  2021-11-13 17:00     ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2021-11-12 17:14 UTC (permalink / raw)
  To: alexandru.tachici; +Cc: linux-iio, linux-kernel, Lars-Peter Clausen

On Wed, 10 Nov 2021 13:17:48 +0200
<alexandru.tachici@analog.com> wrote:

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

Hi Alexandru,

A few comments inline. Approach looks good.

Jonathan

> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 106 ++++++++++++++++++++-----
>  include/linux/iio/adc/ad_sigma_delta.h |  22 +++++
>  2 files changed, 110 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 1d652d9b2f5c..d595e7972975 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -342,33 +342,48 @@ EXPORT_SYMBOL_GPL(ad_sigma_delta_single_conversion);
>  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 channel;
> +	unsigned int i, slot;
>  	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;
> +	slot = 0;
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		sigma_delta->slots[slot] = indio_dev->channels[i].address;
> +		ret = ad_sigma_delta_set_channel(sigma_delta, indio_dev->channels[i].address);
> +		if (ret)
> +			return ret;
> +		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;
> +	}
> +
> +	kfree(sigma_delta->samples_buf);

krealloc() preferred.   It might not be necessary to actually do an allocation after all
if we happen to have one we can already use.

> +	sigma_delta->samples_buf = kzalloc(slot * indio_dev->channels[0].scan_type.storagebits,
> +					   GFP_KERNEL);

I think this needs space for an aligned timestamp give you pass it directly
to iio_push_to_buffer_with_timestamp();

I haven't checked, but I wonder if the fact you are no longer directly using rx_buf
would allow you to relax the alignment constraint on that. (see below though as I
think you should consider keeping the original path to avoid a copy for the simple
devices).


> +	if (!sigma_delta->samples_buf)
> +		return -ENOMEM;
>  
>  	spi_bus_lock(sigma_delta->spi->master);
>  	sigma_delta->bus_locked = true;
>  	sigma_delta->keep_cs_asserted = true;
>  
>  	ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
> -	if (ret)
> -		goto err_unlock;
> +	if (ret) {
> +		kfree(sigma_delta->samples_buf);
> +		spi_bus_unlock(sigma_delta->spi->master);

From style point of view preferred to keep to the goto style previously used.
Also, unlock then kfree() buffer + given you pass samples_buf to kfree() on next
run of this function you need to set it to NULL to avoid a double free.


> +		return ret;
> +	}
>  
>  	sigma_delta->irq_dis = false;
>  	enable_irq(sigma_delta->spi->irq);
>  
>  	return 0;
> -
> -err_unlock:
> -	spi_bus_unlock(sigma_delta->spi->master);
> -
> -	return ret;
>  }
>  
>  static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
> @@ -386,6 +401,9 @@ 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);
> +
>  	sigma_delta->bus_locked = false;
>  	return spi_bus_unlock(sigma_delta->spi->master);
>  }
> @@ -396,6 +414,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,20 +430,55 @@ 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);

Given lots of drivers don't support scan mode, perhaps you want to keep a fast path that
avoids the extra copies used here? i.e. keep old code in place if active_slots == 1

> +	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]) {
> +			/* Desynq occurred during continuous sampling of multiple channels.
/*
 * Desync

preferred though i see we have some other comments in here not using that style.

> +			 * Drop this incomplete sample and start from first channel again.
> +			 */
> +
> +			sigma_delta->current_slot = 0;
> +			iio_trigger_notify_done(indio_dev->trig);
> +			sigma_delta->irq_dis = false;
> +			enable_irq(sigma_delta->spi->irq);
> +
> +			return 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);
> +	}
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>  	sigma_delta->irq_dis = false;
> @@ -430,10 +487,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->info->num_slots;

I think this requires all existing drivers to have num_slots == 1 to do the same as
iio_validate_scan_mask_onehot. I'm not seeing that change in this patch, or any setting
of the default to 1.

> +}
> +
>  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)
> @@ -517,8 +581,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->info->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,
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index c525fd51652f..758ff25590c3 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -32,6 +32,7 @@ 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.
>   * @postprocess_sample: Is called for each sampled data word, can be used to
>   *		modify or drop the sample data, it, may be NULL.
> @@ -39,19 +40,24 @@ struct iio_dev;
>   *		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 (*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 +82,12 @@ struct ad_sigma_delta {
>  	uint8_t			comm;
>  
>  	const struct ad_sigma_delta_info *info;
> +	unsigned int		active_slots;
> +	unsigned int		current_slot;
> +	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 +109,16 @@ 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_set_mode(struct ad_sigma_delta *sd,
>  	unsigned int mode)
>  {


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
  2021-11-12 17:14   ` Jonathan Cameron
@ 2021-11-13 17:00     ` Andy Shevchenko
  2021-11-13 18:23       ` Jonathan Cameron
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2021-11-13 17:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Alexandru Tachici, linux-iio, Linux Kernel Mailing List,
	Lars-Peter Clausen

On Fri, Nov 12, 2021 at 7:10 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 10 Nov 2021 13:17:48 +0200
> <alexandru.tachici@analog.com> wrote:


> > +     kfree(sigma_delta->samples_buf);
>
> krealloc() preferred.   It might not be necessary to actually do an allocation after all
> if we happen to have one we can already use.

Looking at below, shouldn't it be krealloc_array()?

> > +     sigma_delta->samples_buf = kzalloc(slot * indio_dev->channels[0].scan_type.storagebits,
> > +                                        GFP_KERNEL);


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
  2021-11-13 17:00     ` Andy Shevchenko
@ 2021-11-13 18:23       ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-11-13 18:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alexandru Tachici, linux-iio, Linux Kernel Mailing List,
	Lars-Peter Clausen

On Sat, 13 Nov 2021 19:00:11 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Nov 12, 2021 at 7:10 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Wed, 10 Nov 2021 13:17:48 +0200
> > <alexandru.tachici@analog.com> wrote:  
> 
> 
> > > +     kfree(sigma_delta->samples_buf);  
> >
> > krealloc() preferred.   It might not be necessary to actually do an allocation after all
> > if we happen to have one we can already use.  
> 
> Looking at below, shouldn't it be krealloc_array()?

True

> 
> > > +     sigma_delta->samples_buf = kzalloc(slot * indio_dev->channels[0].scan_type.storagebits,
> > > +                                        GFP_KERNEL);  
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
@ 2021-11-19  4:59 kernel test robot
  0 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2021-11-19  4:59 UTC (permalink / raw)
  To: kbuild

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211110111750.27263-4-alexandru.tachici@analog.com>
References: <20211110111750.27263-4-alexandru.tachici@analog.com>
TO: alexandru.tachici(a)analog.com
TO: linux-iio(a)vger.kernel.org
TO: linux-kernel(a)vger.kernel.org
CC: jic23(a)kernel.org
CC: Alexandru Tachici <alexandru.tachici@analog.com>
CC: "Lars-Peter Clausen" <lars@metafoo.de>

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linux/master linus/master v5.16-rc1 next-20211118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/alexandru-tachici-analog-com/iio-adc-ad_sigma_delta-Add-sequencer-support/20211110-190925
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
:::::: branch date: 9 days ago
:::::: commit date: 9 days ago
config: i386-randconfig-m021-20211115 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/iio/adc/ad_sigma_delta.c:457 ad_sd_trigger_handler() error: uninitialized symbol 'status_pos'.

vim +/status_pos +457 drivers/iio/adc/ad_sigma_delta.c

af3008485ea037 Lars-Peter Clausen 2012-08-10  410  
af3008485ea037 Lars-Peter Clausen 2012-08-10  411  static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
af3008485ea037 Lars-Peter Clausen 2012-08-10  412  {
af3008485ea037 Lars-Peter Clausen 2012-08-10  413  	struct iio_poll_func *pf = p;
af3008485ea037 Lars-Peter Clausen 2012-08-10  414  	struct iio_dev *indio_dev = pf->indio_dev;
af3008485ea037 Lars-Peter Clausen 2012-08-10  415  	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
0fb6ee8d0b5e90 Lars-Peter Clausen 2020-11-24  416  	uint8_t *data = sigma_delta->rx_buf;
1d2220fad25a99 Alexandru Tachici  2021-11-10  417  	unsigned int transfer_size;
1d2220fad25a99 Alexandru Tachici  2021-11-10  418  	unsigned int sample_size;
1d2220fad25a99 Alexandru Tachici  2021-11-10  419  	unsigned int sample_pos;
1d2220fad25a99 Alexandru Tachici  2021-11-10  420  	unsigned int status_pos;
af3008485ea037 Lars-Peter Clausen 2012-08-10  421  	unsigned int reg_size;
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  422  	unsigned int data_reg;
af3008485ea037 Lars-Peter Clausen 2012-08-10  423  
af3008485ea037 Lars-Peter Clausen 2012-08-10  424  	reg_size = indio_dev->channels[0].scan_type.realbits +
af3008485ea037 Lars-Peter Clausen 2012-08-10  425  			indio_dev->channels[0].scan_type.shift;
af3008485ea037 Lars-Peter Clausen 2012-08-10  426  	reg_size = DIV_ROUND_UP(reg_size, 8);
af3008485ea037 Lars-Peter Clausen 2012-08-10  427  
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  428  	if (sigma_delta->info->data_reg != 0)
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  429  		data_reg = sigma_delta->info->data_reg;
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  430  	else
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  431  		data_reg = AD_SD_REG_DATA;
f0aef2d0186431 Lars-Peter Clausen 2018-11-13  432  
1d2220fad25a99 Alexandru Tachici  2021-11-10  433  	/* Status word will be appended to the sample during transfer */
1d2220fad25a99 Alexandru Tachici  2021-11-10  434  	if (sigma_delta->status_appended)
1d2220fad25a99 Alexandru Tachici  2021-11-10  435  		transfer_size = reg_size + 1;
1d2220fad25a99 Alexandru Tachici  2021-11-10  436  	else
1d2220fad25a99 Alexandru Tachici  2021-11-10  437  		transfer_size = reg_size;
1d2220fad25a99 Alexandru Tachici  2021-11-10  438  
af3008485ea037 Lars-Peter Clausen 2012-08-10  439  	switch (reg_size) {
af3008485ea037 Lars-Peter Clausen 2012-08-10  440  	case 4:
af3008485ea037 Lars-Peter Clausen 2012-08-10  441  	case 2:
af3008485ea037 Lars-Peter Clausen 2012-08-10  442  	case 1:
1d2220fad25a99 Alexandru Tachici  2021-11-10  443  		status_pos = reg_size;
1d2220fad25a99 Alexandru Tachici  2021-11-10  444  		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[0]);
af3008485ea037 Lars-Peter Clausen 2012-08-10  445  		break;
af3008485ea037 Lars-Peter Clausen 2012-08-10  446  	case 3:
1d2220fad25a99 Alexandru Tachici  2021-11-10  447  		status_pos = reg_size + 1;
af3008485ea037 Lars-Peter Clausen 2012-08-10  448  		/* We store 24 bit samples in a 32 bit word. Keep the upper
af3008485ea037 Lars-Peter Clausen 2012-08-10  449  		 * byte set to zero. */
1d2220fad25a99 Alexandru Tachici  2021-11-10  450  		ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[1]);
af3008485ea037 Lars-Peter Clausen 2012-08-10  451  		break;
af3008485ea037 Lars-Peter Clausen 2012-08-10  452  	}
af3008485ea037 Lars-Peter Clausen 2012-08-10  453  
1d2220fad25a99 Alexandru Tachici  2021-11-10  454  	if (sigma_delta->status_appended) {
1d2220fad25a99 Alexandru Tachici  2021-11-10  455  		u8 converted_channel;
1d2220fad25a99 Alexandru Tachici  2021-11-10  456  
1d2220fad25a99 Alexandru Tachici  2021-11-10 @457  		converted_channel = data[status_pos] & sigma_delta->info->status_ch_mask;
1d2220fad25a99 Alexandru Tachici  2021-11-10  458  		if (converted_channel != sigma_delta->slots[sigma_delta->current_slot]) {
1d2220fad25a99 Alexandru Tachici  2021-11-10  459  			/* Desynq occurred during continuous sampling of multiple channels.
1d2220fad25a99 Alexandru Tachici  2021-11-10  460  			 * Drop this incomplete sample and start from first channel again.
1d2220fad25a99 Alexandru Tachici  2021-11-10  461  			 */
1d2220fad25a99 Alexandru Tachici  2021-11-10  462  
1d2220fad25a99 Alexandru Tachici  2021-11-10  463  			sigma_delta->current_slot = 0;
1d2220fad25a99 Alexandru Tachici  2021-11-10  464  			iio_trigger_notify_done(indio_dev->trig);
1d2220fad25a99 Alexandru Tachici  2021-11-10  465  			sigma_delta->irq_dis = false;
1d2220fad25a99 Alexandru Tachici  2021-11-10  466  			enable_irq(sigma_delta->spi->irq);
1d2220fad25a99 Alexandru Tachici  2021-11-10  467  
1d2220fad25a99 Alexandru Tachici  2021-11-10  468  			return IRQ_HANDLED;
1d2220fad25a99 Alexandru Tachici  2021-11-10  469  		}
1d2220fad25a99 Alexandru Tachici  2021-11-10  470  	}
1d2220fad25a99 Alexandru Tachici  2021-11-10  471  
1d2220fad25a99 Alexandru Tachici  2021-11-10  472  	sample_size = indio_dev->channels[0].scan_type.storagebits / 8;
1d2220fad25a99 Alexandru Tachici  2021-11-10  473  	sample_pos = sample_size * sigma_delta->current_slot;
1d2220fad25a99 Alexandru Tachici  2021-11-10  474  	memcpy(&sigma_delta->samples_buf[sample_pos], data, sample_size);
1d2220fad25a99 Alexandru Tachici  2021-11-10  475  	sigma_delta->current_slot++;
1d2220fad25a99 Alexandru Tachici  2021-11-10  476  
1d2220fad25a99 Alexandru Tachici  2021-11-10  477  	if (sigma_delta->current_slot == sigma_delta->active_slots) {
1d2220fad25a99 Alexandru Tachici  2021-11-10  478  		sigma_delta->current_slot = 0;
1d2220fad25a99 Alexandru Tachici  2021-11-10  479  		iio_push_to_buffers_with_timestamp(indio_dev, sigma_delta->samples_buf,
1d2220fad25a99 Alexandru Tachici  2021-11-10  480  						   pf->timestamp);
1d2220fad25a99 Alexandru Tachici  2021-11-10  481  	}
af3008485ea037 Lars-Peter Clausen 2012-08-10  482  
af3008485ea037 Lars-Peter Clausen 2012-08-10  483  	iio_trigger_notify_done(indio_dev->trig);
af3008485ea037 Lars-Peter Clausen 2012-08-10  484  	sigma_delta->irq_dis = false;
af3008485ea037 Lars-Peter Clausen 2012-08-10  485  	enable_irq(sigma_delta->spi->irq);
af3008485ea037 Lars-Peter Clausen 2012-08-10  486  
af3008485ea037 Lars-Peter Clausen 2012-08-10  487  	return IRQ_HANDLED;
af3008485ea037 Lars-Peter Clausen 2012-08-10  488  }
af3008485ea037 Lars-Peter Clausen 2012-08-10  489  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38013 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-11-19  4:59 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 11:17 [PATCH 0/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
2021-11-10 11:17 ` [PATCH 1/5] iio: adc: ad7124: Add update_scan_mode alexandru.tachici
2021-11-12 16:54   ` Jonathan Cameron
2021-11-10 11:17 ` [PATCH 2/5] iio: adc: ad7192: " alexandru.tachici
2021-11-12 16:57   ` Jonathan Cameron
2021-11-10 11:17 ` [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support alexandru.tachici
2021-11-12 17:14   ` Jonathan Cameron
2021-11-13 17:00     ` Andy Shevchenko
2021-11-13 18:23       ` Jonathan Cameron
2021-11-10 11:17 ` [PATCH 4/5] iio: adc: ad7124: add " alexandru.tachici
2021-11-10 11:17 ` [PATCH 5/5] iio: adc: ad7192: " alexandru.tachici
2021-11-19  4:59 [PATCH 3/5] iio: adc: ad_sigma_delta: Add " kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.