From: Alexandru Tachici <[email protected]>
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
From: Alexandru Tachici <[email protected]>
Add disable_all() callback to the ad_sigma_delta_info.
Signed-off-by: Alexandru Tachici <[email protected]>
---
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
From: Alexandru Tachici <[email protected]>
Add sequencer support for AD7124.
Signed-off-by: Alexandru Tachici <[email protected]>
---
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
From: Alexandru Tachici <[email protected]>
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 <[email protected]>
---
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
From: Alexandru Tachici <[email protected]>
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 <[email protected]>
---
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
On Fri, 18 Mar 2022 18:27:15 +0200
<[email protected]> wrote:
> From: Alexandru Tachici <[email protected]>
>
> 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 <[email protected]>
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,
> },
> };
On Fri, 18 Mar 2022 18:27:19 +0200
<[email protected]> wrote:
> From: Alexandru Tachici <[email protected]>
>
> Add sequencer support for AD7124.
>
> Signed-off-by: Alexandru Tachici <[email protected]>
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;