On 03/20, David Lechner wrote: > On 3/19/25 9:57 AM, Marcelo Schmitt wrote: > > FPGA HDL projects can include a PWM generator in addition to SPI-Engine. > > The PWM IP is used to trigger SPI-Engine offload modules that in turn set > > SPI-Engine to execute transfers to poll data from the ADC. That allows data > > to be read at the maximum sample rates. Also, it is possible to set a > > specific sample rate by setting the proper PWM duty cycle and related state > > parameters, thus allowing an adjustable ADC sample rate when a PWM (offload > > trigger) is used in combination with SPI-Engine. > > > > Add support for SPI offload. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> > > --- > > I'm surprised I'm not on the CC for this series. scripts/get_maintainer.pl should > have picked me up due to K: spi_offload which matches this patch. get_maintainers doesn't list you if run over ad4000.c. If run over the patch file, get_maintainers lists many developers. Some of them don't seem to be related to this series, so I cropped the list to avoid churn. Will CC you on the next versions. > > > Instead of changing bits_per_word according to buffer endianness, I set > > bits_per_word for other SPI transfers and updated IIO channels to always use CPU > > endianness (the first patch in the series). With that, bits_per_word no longer > > needs be updated according to buffer endianness or, in other words, buffer > > endianness is no longer related to bits_per_word. > > As mentioned in my reply to the previous patch, this is a breaking change, so > probably not the best choice. Ack > > > > > drivers/iio/adc/Kconfig | 7 +- > > drivers/iio/adc/ad4000.c | 491 ++++++++++++++++++++++++++++++++------- > > 2 files changed, 419 insertions(+), 79 deletions(-) > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index b7ae6e0ae0df..1cfa3a32f3a7 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -25,10 +25,15 @@ config AD4000 > > tristate "Analog Devices AD4000 ADC Driver" > > depends on SPI > > select IIO_BUFFER > > + select IIO_BUFFER_DMAENGINE > > select IIO_TRIGGERED_BUFFER > > + select SPI_OFFLOAD > > help > > Say yes here to build support for Analog Devices AD4000 high speed > > - SPI analog to digital converters (ADC). > > + SPI analog to digital converters (ADC). If intended to use with > > + SPI offloading support, it is recommended to enable > > + CONFIG_SPI_AXI_SPI_ENGINE, CONFIG_PWM_AXI_PWMGEN, and > > + CONFIG_SPI_OFFLOAD_TRIGGER_PWM. > > I guess this is fine here, but people might be more likely to see it on the > ADI wiki where the HDL project that has these requirements is described. I don't see any word about that at https://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html If no one opposes to that, I'd like to keep the piece of advice. > > > > > To compile this driver as a module, choose M here: the module will be > > called ad4000. > > diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c > > index 19bc021e1b5d..b3f4d215cad4 100644 > > --- a/drivers/iio/adc/ad4000.c > > +++ b/drivers/iio/adc/ad4000.c > > @@ -21,9 +21,12 @@ > > #include <linux/iio/iio.h> > > > > #include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dmaengine.h> > > #include <linux/iio/triggered_buffer.h> > > #include <linux/iio/trigger_consumer.h> > > > > +#include <linux/spi/offload/consumer.h> > > Why not before #include <linux/spi/spi.h> ? Okay, will put it there. > ... > > > > -#define __AD4000_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access) \ > > +#define AD4000_TQUIET1_NS 190 > > +#define AD4000_TQUIET2_NS 60 > > +#define AD4000_TCONV_NS 320 > > Does this actually work for every single chip? For example, the AD7983 datasheet > give the max as 500 ns for this timing parameter. Might need to put this in the > chip_info to account for differences. > > Also AD7983 doesn't have any quite time in the datasheet, so those might not be > needed in some cases? I've tested the series with AD7687 and ADAQ4003 and got good readings. I'll check the timing specifications again and provide a delay long enough for AD7983 and any other device with longer TCONV. > ... > > -#define AD4000_DIFF_CHANNELS(_sign, _real_bits, _reg_access) \ > > +#define AD4000_DIFF_CHANNELS(_sign, _real_bits, _reg_access, _offl) \ > > We always pass 0 to _offl here, so we can leave out this parameter and just > hard-code the 0 below. We will never have a case with offload + soft timestamp. Good catch. Will apply the simplification. > ... > > -#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)\ > > +#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, \ > > + _reg_access, _offl) \ > > { \ > > .type = IIO_VOLTAGE, \ > > .indexed = 1, \ > > .channel = 0, \ > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > > BIT(IIO_CHAN_INFO_SCALE) | \ > > - BIT(IIO_CHAN_INFO_OFFSET), \ > > - .info_mask_separate_available = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\ > > + BIT(IIO_CHAN_INFO_OFFSET) | \ > > + (_offl ? BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0), \ > > + .info_mask_separate_available = (_reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0),\ > > Unrelated change (adding braces)? Oops, will undo. > > Also, could be nice to have sampling_frequency_available so that userspace can > discover the max sample rate. I don't think we should have sampling_frequency_available in this case because there is no clear list of sample frequency values nor they can always be expressed as "[min step max]" triad. https://web.git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n121 > ... > > -#define AD4000_PSEUDO_DIFF_CHANNELS(_sign, _real_bits, _reg_access) \ > > +#define AD4000_PSEUDO_DIFF_CHANNELS(_sign, _real_bits, _reg_access, _offl) \ > > ditto about hard-coding _offl here Ack > ... > > static const struct ad4000_chip_info ad7983_chip_info = { > > .dev_name = "ad7983", > > - .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0), > > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0, 0), > > + .offload_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0, 1), > > .time_spec = &ad7983_t_spec, > > + .max_rate_hz = 1330 * KILO, > > This is actually 752 ns period when 750 is allowed. Could make it 1 * MEGA + 333 > * KILO + 333 or 1333333. Ack > > > }; > > > > static const struct ad4000_chip_info ad7984_chip_info = { > > .dev_name = "ad7984", > > - .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0), > > + .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0, 0), > > + .offload_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0, 1), > > .time_spec = &ad7983_t_spec, > > + .max_rate_hz = 1330 * KILO, > > ditto Ack > ... > > +static const struct iio_buffer_setup_ops ad4000_offload_buffer_setup_ops = { > > + .postenable = &ad4000_offload_buffer_postenable, > > + .postdisable = &ad4000_offload_buffer_postdisable, > > Needs to be predisable to correctly balance postenable. Ack > ... > > +static int ad4000_prepare_offload_turbo_message(struct ad4000_state *st, > > + const struct iio_chan_spec *chan) > > +{ > > + struct spi_transfer *xfers = st->offload_xfers; > > + > > + /* Have to do a short CS toggle to trigger conversion. */ > > + xfers[0].cs_change = 1; > > + xfers[0].cs_change_delay.value = AD4000_TQUIET1_NS; > > + xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > > + xfers[0].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; > > There is no data transferred in this xfer, so we don't need to set this flag. Ack > ... > > case AD4000_SDI_VIO: > > - indio_dev->info = &ad4000_info; > > - indio_dev->channels = chip->chan_spec; > > + if (st->using_offload) { > > + indio_dev->info = &ad4000_offload_info; > > + indio_dev->channels = &chip->offload_chan_spec; > > + indio_dev->num_channels = 1; > > + > > + spi->cs_hold.value = AD4000_TCONV_NS; > > + spi->cs_hold.unit = SPI_DELAY_UNIT_NSECS; > > Modifying spi is suspicious. Normally, this would come from the devicetree. And > this isn't used by all SPI drivers - notably the AXI SPI Engine ignores it. > Better to just handle this in individual spi xfer structs. Will try that. > > > + ret = ad4000_prepare_offload_message(st, indio_dev->channels); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to optimize SPI msg\n"); > > + } else { > > + indio_dev->info = &ad4000_info; > > + indio_dev->channels = chip->chan_spec; > > + indio_dev->num_channels = ARRAY_SIZE(chip->chan_spec); > > + } > > nit: add extra blank line here Ack > > > ret = ad4000_prepare_3wire_mode_message(st, &indio_dev->channels[0]); > > if (ret) > > - return ret; > > + return dev_err_probe(dev, ret, > > + "Failed to optimize SPI msg\n"); > > > > break; >
