AD4030 and similar devices can read common-mode voltage together with
ADC sample data. When enabled, common-mode voltage data is provided in a
separate IIO channel since it measures something other than the primary
ADC input signal and requires separate scaling to convert to voltage
units. The initial SPI offload support patch for AD4030 only provided
differential channels. Now, extend the AD4030 driver to also provide
common-mode IIO channels when setup with SPI offloading capability.
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
New patch.
I hope this works for ADCs with two channels. It's not clear if works as
expected with current HDL and single-channel ADCs (like ADAQ4216).
The ad4630_fmc HDL project was designed for ADCs with two channels and
always streams two data channels to DMA (even when the ADC has only one
physical channel). Though, if the ADC has only one physical channel, the
data that would come from the second ADC channel comes in as noise and
would have to be discarded. Because of that, when using single-channel
ADCs, the ADC driver would need to use a special DMA buffer to filter out
half of the data that reaches DMA memory. With that, the ADC sample data
could be delivered to user space without any noise being added to the IIO
buffer. I have implemented a prototype of such specialized buffer
(industrialio-buffer-dmaengine-filtered), but it is awful and only worked
with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual
differential channel data is also affected by the extra 0xFFFFFFFF data
pushed to DMA. Though, for the differential channel, it's easier to see it
shall work for two-channel ADCs (the sine wave appears "filled" in
iio-oscilloscope).
So, I sign this, but don't guarantee it to work.
drivers/iio/adc/ad4030.c | 49 ++++++++++++++++++++++++++++++++--------
1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index 5f968f2a2b3c..74ff35742cda 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -192,7 +192,7 @@ struct ad4030_state {
unsigned int avg_log2;
enum ad4030_out_mode mode;
/* Offload sampling */
- struct spi_transfer offload_xfer;
+ struct spi_transfer offload_xfer[2];
struct spi_message offload_msg;
struct spi_offload *offload;
struct spi_offload_trigger *offload_trigger;
@@ -237,7 +237,7 @@ struct ad4030_state {
* - _idx - _ch * 2 + _ch gives the channel number for this specific common-mode
* channel
*/
-#define AD4030_CHAN_CMO(_idx, _ch) { \
+#define __AD4030_CHAN_CMO(_idx, _ch, _offload) { \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
.type = IIO_VOLTAGE, \
@@ -247,12 +247,18 @@ struct ad4030_state {
.scan_index = (_idx), \
.scan_type = { \
.sign = 'u', \
- .storagebits = 8, \
+ .storagebits = (_offload ? 32 : 8), \
.realbits = 8, \
- .endianness = IIO_BE, \
+ .endianness = (_offload ? IIO_CPU : IIO_BE), \
}, \
}
+#define AD4030_CHAN_CMO(_idx, _ch) \
+ __AD4030_CHAN_CMO(_idx, _ch, 0)
+
+#define AD4030_OFFLOAD_CHAN_CMO(_idx, _ch) \
+ __AD4030_CHAN_CMO(_idx, _ch, 1)
+
/*
* For a chip with 2 hardware channel this will be used to create 2 differential
* channels:
@@ -1180,6 +1186,7 @@ static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
static void ad4030_prepare_offload_msg(struct iio_dev *indio_dev)
{
struct ad4030_state *st = iio_priv(indio_dev);
+ bool common_mode;
u8 offload_bpw;
if (st->mode == AD4030_OUT_DATA_MD_30_AVERAGED_DIFF)
@@ -1187,10 +1194,22 @@ static void ad4030_prepare_offload_msg(struct iio_dev *indio_dev)
else
offload_bpw = st->chip->precision_bits;
- st->offload_xfer.bits_per_word = offload_bpw;
- st->offload_xfer.len = spi_bpw_to_bytes(offload_bpw);
- st->offload_xfer.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
- spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
+ st->offload_xfer[0].bits_per_word = offload_bpw;
+ st->offload_xfer[0].len = spi_bpw_to_bytes(offload_bpw);
+ st->offload_xfer[0].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+
+ common_mode = st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
+ st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM;
+
+ if (common_mode) {
+ offload_bpw = 8;
+ st->offload_xfer[1].bits_per_word = offload_bpw;
+ st->offload_xfer[1].len = spi_bpw_to_bytes(offload_bpw);
+ st->offload_xfer[1].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+ }
+
+ spi_message_init_with_transfers(&st->offload_msg, st->offload_xfer,
+ common_mode ? 2 : 1);
}
static int ad4030_offload_buffer_postenable(struct iio_dev *indio_dev)
@@ -1273,6 +1292,7 @@ static int ad4030_offload_buffer_predisable(struct iio_dev *indio_dev)
static const struct iio_buffer_setup_ops ad4030_offload_buffer_setup_ops = {
.postenable = &ad4030_offload_buffer_postenable,
.predisable = &ad4030_offload_buffer_predisable,
+ .validate_scan_mask = ad4030_validate_scan_mask,
};
static int ad4030_regulators_get(struct ad4030_state *st)
@@ -1524,7 +1544,7 @@ static int ad4030_probe(struct spi_device *spi)
* Offloaded SPI transfers can't support software timestamp so
* no additional timestamp channel is added.
*/
- indio_dev->num_channels = st->chip->num_voltage_inputs;
+ indio_dev->num_channels = 2 * st->chip->num_voltage_inputs;
indio_dev->channels = st->chip->offload_channels;
ret = ad4030_spi_offload_setup(indio_dev, st);
if (ret)
@@ -1645,6 +1665,7 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
},
.offload_channels = {
AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_24_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(1, 0),
},
.grade = AD4030_REG_CHIP_GRADE_AD4030_24_GRADE,
.precision_bits = 24,
@@ -1666,6 +1687,8 @@ static const struct ad4030_chip_info ad4630_16_chip_info = {
.offload_channels = {
AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_16_offload_scan_types),
AD4030_OFFLOAD_CHAN_DIFF(1, ad4030_16_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(2, 0),
+ AD4030_OFFLOAD_CHAN_CMO(3, 1),
},
.grade = AD4030_REG_CHIP_GRADE_AD4630_16_GRADE,
.precision_bits = 16,
@@ -1687,6 +1710,8 @@ static const struct ad4030_chip_info ad4630_24_chip_info = {
.offload_channels = {
AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_24_offload_scan_types),
AD4030_OFFLOAD_CHAN_DIFF(1, ad4030_24_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(2, 0),
+ AD4030_OFFLOAD_CHAN_CMO(3, 1),
},
.grade = AD4030_REG_CHIP_GRADE_AD4630_24_GRADE,
.precision_bits = 24,
@@ -1708,6 +1733,8 @@ static const struct ad4030_chip_info ad4632_16_chip_info = {
.offload_channels = {
AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_16_offload_scan_types),
AD4030_OFFLOAD_CHAN_DIFF(1, ad4030_16_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(2, 0),
+ AD4030_OFFLOAD_CHAN_CMO(3, 1),
},
.grade = AD4030_REG_CHIP_GRADE_AD4632_16_GRADE,
.precision_bits = 16,
@@ -1729,6 +1756,8 @@ static const struct ad4030_chip_info ad4632_24_chip_info = {
.offload_channels = {
AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_24_offload_scan_types),
AD4030_OFFLOAD_CHAN_DIFF(1, ad4030_24_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(2, 0),
+ AD4030_OFFLOAD_CHAN_CMO(3, 1),
},
.grade = AD4030_REG_CHIP_GRADE_AD4632_24_GRADE,
.precision_bits = 24,
@@ -1747,6 +1776,7 @@ static const struct ad4030_chip_info adaq4216_chip_info = {
},
.offload_channels = {
ADAQ4216_OFFLOAD_CHAN_DIFF(0, ad4030_16_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(1, 0),
},
.grade = AD4030_REG_CHIP_GRADE_ADAQ4216_GRADE,
.precision_bits = 16,
@@ -1766,6 +1796,7 @@ static const struct ad4030_chip_info adaq4224_chip_info = {
},
.offload_channels = {
ADAQ4216_OFFLOAD_CHAN_DIFF(0, ad4030_24_offload_scan_types),
+ AD4030_OFFLOAD_CHAN_CMO(1, 0),
},
.grade = AD4030_REG_CHIP_GRADE_ADAQ4224_GRADE,
.precision_bits = 24,
--
2.39.2On Mon, 20 Oct 2025 16:15:39 -0300 Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > AD4030 and similar devices can read common-mode voltage together with > ADC sample data. When enabled, common-mode voltage data is provided in a > separate IIO channel since it measures something other than the primary > ADC input signal and requires separate scaling to convert to voltage > units. The initial SPI offload support patch for AD4030 only provided > differential channels. Now, extend the AD4030 driver to also provide > common-mode IIO channels when setup with SPI offloading capability. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > New patch. > I hope this works for ADCs with two channels. It's not clear if works as > expected with current HDL and single-channel ADCs (like ADAQ4216). > > The ad4630_fmc HDL project was designed for ADCs with two channels and > always streams two data channels to DMA (even when the ADC has only one > physical channel). Though, if the ADC has only one physical channel, the > data that would come from the second ADC channel comes in as noise and > would have to be discarded. Because of that, when using single-channel > ADCs, the ADC driver would need to use a special DMA buffer to filter out > half of the data that reaches DMA memory. With that, the ADC sample data > could be delivered to user space without any noise being added to the IIO > buffer. I have implemented a prototype of such specialized buffer > (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > differential channel data is also affected by the extra 0xFFFFFFFF data > pushed to DMA. Though, for the differential channel, it's easier to see it > shall work for two-channel ADCs (the sine wave appears "filled" in > iio-oscilloscope). > > So, I sign this, but don't guarantee it to work. So what's the path to resolve this? Waiting on HDL changes or not support those devices until we have a clean solution? Also, just to check, is this only an issue with the additional stuff this patch adds or do we have a problem with SPI offload in general (+ this IP) and those single channel devices? Jonathan
On 10/27, Jonathan Cameron wrote: > On Mon, 20 Oct 2025 16:15:39 -0300 > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > AD4030 and similar devices can read common-mode voltage together with > > ADC sample data. When enabled, common-mode voltage data is provided in a > > separate IIO channel since it measures something other than the primary > > ADC input signal and requires separate scaling to convert to voltage > > units. The initial SPI offload support patch for AD4030 only provided > > differential channels. Now, extend the AD4030 driver to also provide > > common-mode IIO channels when setup with SPI offloading capability. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > New patch. > > I hope this works for ADCs with two channels. It's not clear if works as > > expected with current HDL and single-channel ADCs (like ADAQ4216). > > > > The ad4630_fmc HDL project was designed for ADCs with two channels and > > always streams two data channels to DMA (even when the ADC has only one > > physical channel). Though, if the ADC has only one physical channel, the > > data that would come from the second ADC channel comes in as noise and > > would have to be discarded. Because of that, when using single-channel > > ADCs, the ADC driver would need to use a special DMA buffer to filter out > > half of the data that reaches DMA memory. With that, the ADC sample data > > could be delivered to user space without any noise being added to the IIO > > buffer. I have implemented a prototype of such specialized buffer > > (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > > with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > > differential channel data is also affected by the extra 0xFFFFFFFF data > > pushed to DMA. Though, for the differential channel, it's easier to see it > > shall work for two-channel ADCs (the sine wave appears "filled" in > > iio-oscilloscope). > > > > So, I sign this, but don't guarantee it to work. > > So what's the path to resolve this? Waiting on HDL changes or not support > those devices until we have a clean solution? Waiting for HDL to get updated I'd say. > > Also, just to check, is this only an issue with the additional stuff this > patch adds or do we have a problem with SPI offload in general (+ this > IP) and those single channel devices? IMO, one solution would be to update the HDL project for AD4630 and similar ADCs to not send data from channel 2 to DMA memory when single-channel ADCs are connected. Another possibility would be to intercept and filter out the extra data before pushing it to user space. My first attempt of doing that didn't work out with upstream kernel but I may revisit that. We could maybe split the driver into two. One for supporting two-channel ADCs and one for single-channel. Though, we would fall into the same issue when handling offloaded data for the single-channel driver.
On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: > On 10/27, Jonathan Cameron wrote: > > On Mon, 20 Oct 2025 16:15:39 -0300 > > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > > > AD4030 and similar devices can read common-mode voltage together with > > > ADC sample data. When enabled, common-mode voltage data is provided in a > > > separate IIO channel since it measures something other than the primary > > > ADC input signal and requires separate scaling to convert to voltage > > > units. The initial SPI offload support patch for AD4030 only provided > > > differential channels. Now, extend the AD4030 driver to also provide > > > common-mode IIO channels when setup with SPI offloading capability. > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > > --- > > > New patch. > > > I hope this works for ADCs with two channels. It's not clear if works as > > > expected with current HDL and single-channel ADCs (like ADAQ4216). > > > > > > The ad4630_fmc HDL project was designed for ADCs with two channels and > > > always streams two data channels to DMA (even when the ADC has only one > > > physical channel). Though, if the ADC has only one physical channel, the > > > data that would come from the second ADC channel comes in as noise and > > > would have to be discarded. Because of that, when using single-channel > > > ADCs, the ADC driver would need to use a special DMA buffer to filter out > > > half of the data that reaches DMA memory. With that, the ADC sample data > > > could be delivered to user space without any noise being added to the IIO > > > buffer. I have implemented a prototype of such specialized buffer > > > (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > > > with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > > > differential channel data is also affected by the extra 0xFFFFFFFF data > > > pushed to DMA. Though, for the differential channel, it's easier to see it > > > shall work for two-channel ADCs (the sine wave appears "filled" in > > > iio-oscilloscope). > > > > > > So, I sign this, but don't guarantee it to work. > > > > So what's the path to resolve this? Waiting on HDL changes or not support > > those devices until we have a clean solution? > > Waiting for HDL to get updated I'd say. Agree. We kind of control the IP here so why should we do awful tricks in SW right :)? At the very least I would expect hdl to be capable to discard the data in HW. > > > > > Also, just to check, is this only an issue with the additional stuff this > > patch adds or do we have a problem with SPI offload in general (+ this > > IP) and those single channel devices? > > IMO, one solution would be to update the HDL project for AD4630 and similar ADCs > to not send data from channel 2 to DMA memory when single-channel ADCs are > connected. Another possibility would be to intercept and filter out the extra > data before pushing it to user space. My first attempt of doing that didn't > work out with upstream kernel but I may revisit that. I'm also confused. Is this also an issue with the current series without common mode? If I'm getting things right, one channel ADCs pretty much do not work right now with spi offload? If the above is correct I would just not support it for 1 channel ADCs. - Nuno Sá
On 10/30, Nuno Sá wrote: > On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: > > On 10/27, Jonathan Cameron wrote: > > > On Mon, 20 Oct 2025 16:15:39 -0300 > > > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > > > > > AD4030 and similar devices can read common-mode voltage together with > > > > ADC sample data. When enabled, common-mode voltage data is provided in a > > > > separate IIO channel since it measures something other than the primary > > > > ADC input signal and requires separate scaling to convert to voltage > > > > units. The initial SPI offload support patch for AD4030 only provided > > > > differential channels. Now, extend the AD4030 driver to also provide > > > > common-mode IIO channels when setup with SPI offloading capability. > > > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > > > --- > > > > New patch. > > > > I hope this works for ADCs with two channels. It's not clear if works as > > > > expected with current HDL and single-channel ADCs (like ADAQ4216). > > > > > > > > The ad4630_fmc HDL project was designed for ADCs with two channels and > > > > always streams two data channels to DMA (even when the ADC has only one > > > > physical channel). Though, if the ADC has only one physical channel, the > > > > data that would come from the second ADC channel comes in as noise and > > > > would have to be discarded. Because of that, when using single-channel > > > > ADCs, the ADC driver would need to use a special DMA buffer to filter out > > > > half of the data that reaches DMA memory. With that, the ADC sample data > > > > could be delivered to user space without any noise being added to the IIO > > > > buffer. I have implemented a prototype of such specialized buffer > > > > (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > > > > with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > > > > differential channel data is also affected by the extra 0xFFFFFFFF data > > > > pushed to DMA. Though, for the differential channel, it's easier to see it > > > > shall work for two-channel ADCs (the sine wave appears "filled" in > > > > iio-oscilloscope). > > > > > > > > So, I sign this, but don't guarantee it to work. > > > > > > So what's the path to resolve this? Waiting on HDL changes or not support > > > those devices until we have a clean solution? > > > > Waiting for HDL to get updated I'd say. > > Agree. We kind of control the IP here so why should we do awful tricks in > SW right :)? At the very least I would expect hdl to be capable to discard the > data in HW. > > > > > > > > > Also, just to check, is this only an issue with the additional stuff this > > > patch adds or do we have a problem with SPI offload in general (+ this > > > IP) and those single channel devices? > > > > IMO, one solution would be to update the HDL project for AD4630 and similar ADCs > > to not send data from channel 2 to DMA memory when single-channel ADCs are > > connected. Another possibility would be to intercept and filter out the extra > > data before pushing it to user space. My first attempt of doing that didn't > > work out with upstream kernel but I may revisit that. > > I'm also confused. Is this also an issue with the current series without common mode? > > If I'm getting things right, one channel ADCs pretty much do not work right now with > spi offload? Yes, that's correct. It kind of works for single-channel ADCs, but half of the data we see in user space is valid and the other half is not. For two-channel ADCs, everything should be fine. > > If the above is correct I would just not support it for 1 channel ADCs. Currently, it's just one part that is single-channel (AD4030). If patches 6 and 7 were accepted, it would be 3 single-channel parts supported. I can add an `if` somewhere to check the number of channel, but it will eventually have to be removed when HDL gets fixed. Or, if HDL can't be fixed, then we'll need the `if` now and something else latter to filter out extra data before pushing to IIO buffers as mentioned above. Though, this scenario seems odd to me as I think the HDL wouldn't be 100% compatible with single-channel AD4030-like parts. We would be writing code to support AD4030 _and_ a peculiar data stream from this specific HDL project? My suggestion is to apply all patches except patch 8. IMHO, SPI offload single-channel ADC support is broken due to HDL IP data stream not being compatible with single-channel parts. That's not a Linux driver issue. Patch 8 (common-mode for single-channel) may be fine, but I haven't been able to validate that since I only have remote access to a single-channel ADC that has already required a buffer work around to test.
On Mon, 2025-11-03 at 10:22 -0300, Marcelo Schmitt wrote: > On 10/30, Nuno Sá wrote: > > On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: > > > On 10/27, Jonathan Cameron wrote: > > > > On Mon, 20 Oct 2025 16:15:39 -0300 > > > > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > > > > > > > AD4030 and similar devices can read common-mode voltage together with > > > > > ADC sample data. When enabled, common-mode voltage data is provided in a > > > > > separate IIO channel since it measures something other than the primary > > > > > ADC input signal and requires separate scaling to convert to voltage > > > > > units. The initial SPI offload support patch for AD4030 only provided > > > > > differential channels. Now, extend the AD4030 driver to also provide > > > > > common-mode IIO channels when setup with SPI offloading capability. > > > > > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > > > > --- > > > > > New patch. > > > > > I hope this works for ADCs with two channels. It's not clear if works as > > > > > expected with current HDL and single-channel ADCs (like ADAQ4216). > > > > > > > > > > The ad4630_fmc HDL project was designed for ADCs with two channels and > > > > > always streams two data channels to DMA (even when the ADC has only one > > > > > physical channel). Though, if the ADC has only one physical channel, the > > > > > data that would come from the second ADC channel comes in as noise and > > > > > would have to be discarded. Because of that, when using single-channel > > > > > ADCs, the ADC driver would need to use a special DMA buffer to filter out > > > > > half of the data that reaches DMA memory. With that, the ADC sample data > > > > > could be delivered to user space without any noise being added to the IIO > > > > > buffer. I have implemented a prototype of such specialized buffer > > > > > (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > > > > > with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > > > > > differential channel data is also affected by the extra 0xFFFFFFFF data > > > > > pushed to DMA. Though, for the differential channel, it's easier to see it > > > > > shall work for two-channel ADCs (the sine wave appears "filled" in > > > > > iio-oscilloscope). > > > > > > > > > > So, I sign this, but don't guarantee it to work. > > > > > > > > So what's the path to resolve this? Waiting on HDL changes or not support > > > > those devices until we have a clean solution? > > > > > > Waiting for HDL to get updated I'd say. > > > > Agree. We kind of control the IP here so why should we do awful tricks in > > SW right :)? At the very least I would expect hdl to be capable to discard the > > data in HW. > > > > > > > > > > > > > Also, just to check, is this only an issue with the additional stuff this > > > > patch adds or do we have a problem with SPI offload in general (+ this > > > > IP) and those single channel devices? > > > > > > IMO, one solution would be to update the HDL project for AD4630 and similar ADCs > > > to not send data from channel 2 to DMA memory when single-channel ADCs are > > > connected. Another possibility would be to intercept and filter out the extra > > > data before pushing it to user space. My first attempt of doing that didn't > > > work out with upstream kernel but I may revisit that. > > > > I'm also confused. Is this also an issue with the current series without common mode? > > > > If I'm getting things right, one channel ADCs pretty much do not work right now with > > spi offload? > > Yes, that's correct. It kind of works for single-channel ADCs, but half of the > data we see in user space is valid and the other half is not. For two-channel > ADCs, everything should be fine. To me that is something that does not work eheheh :). I mean, going with all this trouble to sample as fast as we can just so we have to discard (or mask out) half of every sample in userspace (even though I can imagine we still get better performance vs non offload case). > > > > > If the above is correct I would just not support it for 1 channel ADCs. > > Currently, it's just one part that is single-channel (AD4030). If patches 6 and > 7 were accepted, it would be 3 single-channel parts supported. I can add an `if` > somewhere to check the number of channel, but it will eventually have to be > removed when HDL gets fixed. I would probably do the above or maybe we just need to push for an hdl fix or some final conclusion (like if they cannot fix it for some reason) and act accordingly. > > Or, if HDL can't be fixed, then we'll need the `if` now and something else > latter to filter out extra data before pushing to IIO buffers as mentioned > above. Though, this scenario seems odd to me as I think the HDL wouldn't be 100% > compatible with single-channel AD4030-like parts. We would be writing code to > support AD4030 _and_ a peculiar data stream from this specific HDL project? > > My suggestion is to apply all patches except patch 8. IMHO, SPI offload > single-channel ADC support is broken due to HDL IP data stream not being > compatible with single-channel parts. That's not a Linux driver issue. Well, it's not a SW issue but we are driving the HW and we know it's broken so I don't see a point in having something that does not work. Given that this is so connected to the HDL part of it I'm not sure it's fine to ignore that offload does not work for 1 channel parts. Anyways, it's odd to me but ultimately if Jonathan is fine with it, I won't object :) - Nuno Sá
On 11/3/25 8:30 AM, Nuno Sá wrote: > On Mon, 2025-11-03 at 10:22 -0300, Marcelo Schmitt wrote: >> On 10/30, Nuno Sá wrote: >>> On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: >>>> On 10/27, Jonathan Cameron wrote: >>>>> On Mon, 20 Oct 2025 16:15:39 -0300 >>>>> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: >>>>> >>>>>> AD4030 and similar devices can read common-mode voltage together with >>>>>> ADC sample data. When enabled, common-mode voltage data is provided in a >>>>>> separate IIO channel since it measures something other than the primary >>>>>> ADC input signal and requires separate scaling to convert to voltage >>>>>> units. The initial SPI offload support patch for AD4030 only provided >>>>>> differential channels. Now, extend the AD4030 driver to also provide >>>>>> common-mode IIO channels when setup with SPI offloading capability. >>>>>> >>>>>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> >>>>>> --- >>>>>> New patch. >>>>>> I hope this works for ADCs with two channels. It's not clear if works as >>>>>> expected with current HDL and single-channel ADCs (like ADAQ4216). >>>>>> >>>>>> The ad4630_fmc HDL project was designed for ADCs with two channels and >>>>>> always streams two data channels to DMA (even when the ADC has only one >>>>>> physical channel). Though, if the ADC has only one physical channel, the >>>>>> data that would come from the second ADC channel comes in as noise and >>>>>> would have to be discarded. Because of that, when using single-channel >>>>>> ADCs, the ADC driver would need to use a special DMA buffer to filter out >>>>>> half of the data that reaches DMA memory. With that, the ADC sample data >>>>>> could be delivered to user space without any noise being added to the IIO >>>>>> buffer. I have implemented a prototype of such specialized buffer >>>>>> (industrialio-buffer-dmaengine-filtered), but it is awful and only worked >>>>>> with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual >>>>>> differential channel data is also affected by the extra 0xFFFFFFFF data >>>>>> pushed to DMA. Though, for the differential channel, it's easier to see it >>>>>> shall work for two-channel ADCs (the sine wave appears "filled" in >>>>>> iio-oscilloscope). >>>>>> >>>>>> So, I sign this, but don't guarantee it to work. >>>>> >>>>> So what's the path to resolve this? Waiting on HDL changes or not support >>>>> those devices until we have a clean solution? >>>> >>>> Waiting for HDL to get updated I'd say. >>> >>> Agree. We kind of control the IP here so why should we do awful tricks in >>> SW right :)? At the very least I would expect hdl to be capable to discard the >>> data in HW. >>> >>>> >>>>> >>>>> Also, just to check, is this only an issue with the additional stuff this >>>>> patch adds or do we have a problem with SPI offload in general (+ this >>>>> IP) and those single channel devices? >>>> >>>> IMO, one solution would be to update the HDL project for AD4630 and similar ADCs >>>> to not send data from channel 2 to DMA memory when single-channel ADCs are >>>> connected. Another possibility would be to intercept and filter out the extra >>>> data before pushing it to user space. My first attempt of doing that didn't >>>> work out with upstream kernel but I may revisit that. >>> >>> I'm also confused. Is this also an issue with the current series without common mode? >>> >>> If I'm getting things right, one channel ADCs pretty much do not work right now with >>> spi offload? >> >> Yes, that's correct. It kind of works for single-channel ADCs, but half of the >> data we see in user space is valid and the other half is not. For two-channel >> ADCs, everything should be fine. > > To me that is something that does not work eheheh :). I mean, going with all this trouble > to sample as fast as we can just so we have to discard (or mask out) half of every sample > in userspace (even though I can imagine we still get better performance vs non offload case). If we are getting extra data to userspace, then either we aren't creating the SPI message correctly and telling the controller to read too much data or the HDL is broken. > >> >>> >>> If the above is correct I would just not support it for 1 channel ADCs. >> >> Currently, it's just one part that is single-channel (AD4030). If patches 6 and >> 7 were accepted, it would be 3 single-channel parts supported. I can add an `if` >> somewhere to check the number of channel, but it will eventually have to be >> removed when HDL gets fixed. > > I would probably do the above or maybe we just need to push for an hdl fix or some > final conclusion (like if they cannot fix it for some reason) and act accordingly. > >> >> Or, if HDL can't be fixed, then we'll need the `if` now and something else >> latter to filter out extra data before pushing to IIO buffers as mentioned >> above. Though, this scenario seems odd to me as I think the HDL wouldn't be 100% >> compatible with single-channel AD4030-like parts. We would be writing code to >> support AD4030 _and_ a peculiar data stream from this specific HDL project? >> >> My suggestion is to apply all patches except patch 8. IMHO, SPI offload >> single-channel ADC support is broken due to HDL IP data stream not being >> compatible with single-channel parts. That's not a Linux driver issue. > > Well, it's not a SW issue but we are driving the HW and we know it's broken so I > don't see a point in having something that does not work. Given that this is so > connected to the HDL part of it I'm not sure it's fine to ignore that offload does > not work for 1 channel parts. > > Anyways, it's odd to me but ultimately if Jonathan is fine with it, I won't object :) > > > - Nuno Sá If single-channel parts currently don't work and two-channel parts need [1] or a hardware descrambler to work with a single data line, then it sounds like we are blocked here until the HDL is improved or [1] is merged. [1]: https://lore.kernel.org/linux-iio/20251014-spi-add-multi-bus-support-v1-0-2098c12d6f5f@baylibre.com/
On 11/03, David Lechner wrote: > On 11/3/25 8:30 AM, Nuno Sá wrote: > > On Mon, 2025-11-03 at 10:22 -0300, Marcelo Schmitt wrote: > >> On 10/30, Nuno Sá wrote: > >>> On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: > >>>> On 10/27, Jonathan Cameron wrote: > >>>>> On Mon, 20 Oct 2025 16:15:39 -0300 > >>>>> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > >>>>> > >>>>>> AD4030 and similar devices can read common-mode voltage together with > >>>>>> ADC sample data. When enabled, common-mode voltage data is provided in a > >>>>>> separate IIO channel since it measures something other than the primary > >>>>>> ADC input signal and requires separate scaling to convert to voltage > >>>>>> units. The initial SPI offload support patch for AD4030 only provided > >>>>>> differential channels. Now, extend the AD4030 driver to also provide > >>>>>> common-mode IIO channels when setup with SPI offloading capability. > >>>>>> > >>>>>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > >>>>>> --- > >>>>>> New patch. > >>>>>> I hope this works for ADCs with two channels. It's not clear if works as > >>>>>> expected with current HDL and single-channel ADCs (like ADAQ4216). > >>>>>> > >>>>>> The ad4630_fmc HDL project was designed for ADCs with two channels and > >>>>>> always streams two data channels to DMA (even when the ADC has only one > >>>>>> physical channel). Though, if the ADC has only one physical channel, the > >>>>>> data that would come from the second ADC channel comes in as noise and > >>>>>> would have to be discarded. Because of that, when using single-channel > >>>>>> ADCs, the ADC driver would need to use a special DMA buffer to filter out > >>>>>> half of the data that reaches DMA memory. With that, the ADC sample data > >>>>>> could be delivered to user space without any noise being added to the IIO > >>>>>> buffer. I have implemented a prototype of such specialized buffer > >>>>>> (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > >>>>>> with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > >>>>>> differential channel data is also affected by the extra 0xFFFFFFFF data > >>>>>> pushed to DMA. Though, for the differential channel, it's easier to see it > >>>>>> shall work for two-channel ADCs (the sine wave appears "filled" in > >>>>>> iio-oscilloscope). > >>>>>> > >>>>>> So, I sign this, but don't guarantee it to work. > >>>>> > >>>>> So what's the path to resolve this? Waiting on HDL changes or not support > >>>>> those devices until we have a clean solution? > >>>> > >>>> Waiting for HDL to get updated I'd say. > >>> > >>> Agree. We kind of control the IP here so why should we do awful tricks in > >>> SW right :)? At the very least I would expect hdl to be capable to discard the > >>> data in HW. > >>> > >>>> > >>>>> > >>>>> Also, just to check, is this only an issue with the additional stuff this > >>>>> patch adds or do we have a problem with SPI offload in general (+ this > >>>>> IP) and those single channel devices? > >>>> > >>>> IMO, one solution would be to update the HDL project for AD4630 and similar ADCs > >>>> to not send data from channel 2 to DMA memory when single-channel ADCs are > >>>> connected. Another possibility would be to intercept and filter out the extra > >>>> data before pushing it to user space. My first attempt of doing that didn't > >>>> work out with upstream kernel but I may revisit that. > >>> > >>> I'm also confused. Is this also an issue with the current series without common mode? > >>> > >>> If I'm getting things right, one channel ADCs pretty much do not work right now with > >>> spi offload? > >> > >> Yes, that's correct. It kind of works for single-channel ADCs, but half of the > >> data we see in user space is valid and the other half is not. For two-channel > >> ADCs, everything should be fine. > > > > To me that is something that does not work eheheh :). Well, yeah, I tend to agree with that 😅 > > I mean, going with all this trouble > > to sample as fast as we can just so we have to discard (or mask out) half of every sample > > in userspace (even though I can imagine we still get better performance vs non offload case). > > If we are getting extra data to userspace, then either we aren't creating the > SPI message correctly and telling the controller to read too much data or > the HDL is broken. The current patch set version (v6) only asks for the amount of ADC precision bits in each transfer when offloading messages. I can't see how that would work but okay, I'll test it with smaller xfer length. > > > > >> > >>> > >>> If the above is correct I would just not support it for 1 channel ADCs. > >> > >> Currently, it's just one part that is single-channel (AD4030). If patches 6 and > >> 7 were accepted, it would be 3 single-channel parts supported. I can add an `if` > >> somewhere to check the number of channel, but it will eventually have to be > >> removed when HDL gets fixed. > > > > I would probably do the above or maybe we just need to push for an hdl fix or some > > final conclusion (like if they cannot fix it for some reason) and act accordingly. > > > >> > >> Or, if HDL can't be fixed, then we'll need the `if` now and something else > >> latter to filter out extra data before pushing to IIO buffers as mentioned > >> above. Though, this scenario seems odd to me as I think the HDL wouldn't be 100% > >> compatible with single-channel AD4030-like parts. We would be writing code to > >> support AD4030 _and_ a peculiar data stream from this specific HDL project? > >> > >> My suggestion is to apply all patches except patch 8. IMHO, SPI offload > >> single-channel ADC support is broken due to HDL IP data stream not being > >> compatible with single-channel parts. That's not a Linux driver issue. > > > > Well, it's not a SW issue but we are driving the HW and we know it's broken so I > > don't see a point in having something that does not work. Given that this is so > > connected to the HDL part of it I'm not sure it's fine to ignore that offload does > > not work for 1 channel parts. > > > > Anyways, it's odd to me but ultimately if Jonathan is fine with it, I won't object :) > > > > > > - Nuno Sá > > If single-channel parts currently don't work and two-channel parts need [1] or > a hardware descrambler to work with a single data line, then it sounds like we > are blocked here until the HDL is improved or [1] is merged. > > [1]: https://lore.kernel.org/linux-iio/20251014-spi-add-multi-bus-support-v1-0-2098c12d6f5f@baylibre.com/ Ack, I think so.
On Mon, 3 Nov 2025 18:56:21 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 11/03, David Lechner wrote: > > On 11/3/25 8:30 AM, Nuno Sá wrote: > > > On Mon, 2025-11-03 at 10:22 -0300, Marcelo Schmitt wrote: > > >> On 10/30, Nuno Sá wrote: > > >>> On Wed, 2025-10-29 at 15:11 -0300, Marcelo Schmitt wrote: > > >>>> On 10/27, Jonathan Cameron wrote: > > >>>>> On Mon, 20 Oct 2025 16:15:39 -0300 > > >>>>> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > >>>>> > > >>>>>> AD4030 and similar devices can read common-mode voltage together with > > >>>>>> ADC sample data. When enabled, common-mode voltage data is provided in a > > >>>>>> separate IIO channel since it measures something other than the primary > > >>>>>> ADC input signal and requires separate scaling to convert to voltage > > >>>>>> units. The initial SPI offload support patch for AD4030 only provided > > >>>>>> differential channels. Now, extend the AD4030 driver to also provide > > >>>>>> common-mode IIO channels when setup with SPI offloading capability. > > >>>>>> > > >>>>>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > >>>>>> --- > > >>>>>> New patch. > > >>>>>> I hope this works for ADCs with two channels. It's not clear if works as > > >>>>>> expected with current HDL and single-channel ADCs (like ADAQ4216). > > >>>>>> > > >>>>>> The ad4630_fmc HDL project was designed for ADCs with two channels and > > >>>>>> always streams two data channels to DMA (even when the ADC has only one > > >>>>>> physical channel). Though, if the ADC has only one physical channel, the > > >>>>>> data that would come from the second ADC channel comes in as noise and > > >>>>>> would have to be discarded. Because of that, when using single-channel > > >>>>>> ADCs, the ADC driver would need to use a special DMA buffer to filter out > > >>>>>> half of the data that reaches DMA memory. With that, the ADC sample data > > >>>>>> could be delivered to user space without any noise being added to the IIO > > >>>>>> buffer. I have implemented a prototype of such specialized buffer > > >>>>>> (industrialio-buffer-dmaengine-filtered), but it is awful and only worked > > >>>>>> with CONFIG_IIO_DMA_BUF_MMAP_LEGACY (only present in ADI Linux tree). Usual > > >>>>>> differential channel data is also affected by the extra 0xFFFFFFFF data > > >>>>>> pushed to DMA. Though, for the differential channel, it's easier to see it > > >>>>>> shall work for two-channel ADCs (the sine wave appears "filled" in > > >>>>>> iio-oscilloscope). > > >>>>>> > > >>>>>> So, I sign this, but don't guarantee it to work. > > >>>>> > > >>>>> So what's the path to resolve this? Waiting on HDL changes or not support > > >>>>> those devices until we have a clean solution? > > >>>> > > >>>> Waiting for HDL to get updated I'd say. > > >>> > > >>> Agree. We kind of control the IP here so why should we do awful tricks in > > >>> SW right :)? At the very least I would expect hdl to be capable to discard the > > >>> data in HW. > > >>> > > >>>> > > >>>>> > > >>>>> Also, just to check, is this only an issue with the additional stuff this > > >>>>> patch adds or do we have a problem with SPI offload in general (+ this > > >>>>> IP) and those single channel devices? > > >>>> > > >>>> IMO, one solution would be to update the HDL project for AD4630 and similar ADCs > > >>>> to not send data from channel 2 to DMA memory when single-channel ADCs are > > >>>> connected. Another possibility would be to intercept and filter out the extra > > >>>> data before pushing it to user space. My first attempt of doing that didn't > > >>>> work out with upstream kernel but I may revisit that. > > >>> > > >>> I'm also confused. Is this also an issue with the current series without common mode? > > >>> > > >>> If I'm getting things right, one channel ADCs pretty much do not work right now with > > >>> spi offload? > > >> > > >> Yes, that's correct. It kind of works for single-channel ADCs, but half of the > > >> data we see in user space is valid and the other half is not. For two-channel > > >> ADCs, everything should be fine. > > > > > > To me that is something that does not work eheheh :). > Well, yeah, I tend to agree with that 😅 > > > > I mean, going with all this trouble > > > to sample as fast as we can just so we have to discard (or mask out) half of every sample > > > in userspace (even though I can imagine we still get better performance vs non offload case). > > > > If we are getting extra data to userspace, then either we aren't creating the > > SPI message correctly and telling the controller to read too much data or > > the HDL is broken. > > The current patch set version (v6) only asks for the amount of ADC precision > bits in each transfer when offloading messages. I can't see how that would work > but okay, I'll test it with smaller xfer length. > > > > > > > > >> > > >>> > > >>> If the above is correct I would just not support it for 1 channel ADCs. > > >> > > >> Currently, it's just one part that is single-channel (AD4030). If patches 6 and > > >> 7 were accepted, it would be 3 single-channel parts supported. I can add an `if` > > >> somewhere to check the number of channel, but it will eventually have to be > > >> removed when HDL gets fixed. > > > > > > I would probably do the above or maybe we just need to push for an hdl fix or some > > > final conclusion (like if they cannot fix it for some reason) and act accordingly. > > > > > >> > > >> Or, if HDL can't be fixed, then we'll need the `if` now and something else > > >> latter to filter out extra data before pushing to IIO buffers as mentioned > > >> above. Though, this scenario seems odd to me as I think the HDL wouldn't be 100% > > >> compatible with single-channel AD4030-like parts. We would be writing code to > > >> support AD4030 _and_ a peculiar data stream from this specific HDL project? > > >> > > >> My suggestion is to apply all patches except patch 8. IMHO, SPI offload > > >> single-channel ADC support is broken due to HDL IP data stream not being > > >> compatible with single-channel parts. That's not a Linux driver issue. > > > > > > Well, it's not a SW issue but we are driving the HW and we know it's broken so I > > > don't see a point in having something that does not work. Given that this is so > > > connected to the HDL part of it I'm not sure it's fine to ignore that offload does > > > not work for 1 channel parts. > > > > > > Anyways, it's odd to me but ultimately if Jonathan is fine with it, I won't object :) > > > > > > > > > - Nuno Sá > > > > If single-channel parts currently don't work and two-channel parts need [1] or > > a hardware descrambler to work with a single data line, then it sounds like we > > are blocked here until the HDL is improved or [1] is merged. > > > > [1]: https://lore.kernel.org/linux-iio/20251014-spi-add-multi-bus-support-v1-0-2098c12d6f5f@baylibre.com/ > > Ack, I think so. OK. So let me know (send a new version) when we have something we can move forwards with. Looks to me like we should rule out single channel parts + spi offload for now. I'll take a look at [1] later today. Jonathan
© 2016 - 2025 Red Hat, Inc.