Re: [PATCH v3 6/8] iio: adc: Add offload support for ad4052

[Jonathan Cameron]

On Fri, 20 Jun 2025 20:52:10 +0200
Jorge Marques <gastmaier@xxxxxxxxx> wrote:

> On Sat, Jun 14, 2025 at 11:20:22AM +0100, Jonathan Cameron wrote:
> > On Tue, 10 Jun 2025 09:34:39 +0200
> > Jorge Marques <jorge.marques@xxxxxxxxxx> wrote:
> >   
> > > Support SPI offload with appropriate FPGA firmware. Since the SPI-Engine
> > > offload module always sends 32-bit data to the DMA engine, the
> > > scantype.storagebytes is set to 32-bit and the SPI transfer length is
> > > based on the scantype.realbits. This combination allows to optimize the
> > > SPI to transfer only 2 or 3 bytes (depending on the granularity and
> > > mode), while the number of samples are computed correctly by tools on
> > > top of the iio scantype.
> > > 
> > > Signed-off-by: Jorge Marques <jorge.marques@xxxxxxxxxx>  
> > Minor comments inline.  I think they are all follow up from comments on
> > earlier patches that apply here as well.
> >   
> > > ---
> > >  drivers/iio/adc/ad4052.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 242 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> > > index 842f5972a1c58701addf5243e7b87da9c26c773f..7d32dc4701ddb0204b5505a650ce7caafc2cb5ed 100644
> > > --- a/drivers/iio/adc/ad4052.c
> > > +++ b/drivers/iio/adc/ad4052.c
> > > @@ -11,6 +11,8 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/err.h>
> > >  #include <linux/gpio/consumer.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/buffer-dmaengine.h>
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > >  #include <linux/interrupt.h>
> > > @@ -23,6 +25,8 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/spi/spi.h>
> > > +#include <linux/spi/offload/consumer.h>
> > > +#include <linux/spi/offload/provider.h>
> > >  #include <linux/string.h>
> > >  #include <linux/types.h>
> > >  #include <linux/units.h>
> > > @@ -111,6 +115,7 @@ enum ad4052_interrupt_en {
> > >  
> > >  struct ad4052_chip_info {
> > >  	const struct iio_chan_spec channels[1];
> > > +	const struct iio_chan_spec offload_channels[1];  
> > 
> > If there is only ever one of these drop the array.
> >   
> Hi Jonathan,
> 
> It is hard to predict if no other similar device will have only two
> channels. But I would say most drivers end-up having more channels.

Ok. I don't mind that much, but it does feel like planning for a future
that might or might not come.   Easy enough to refactor later.

> >   
> > >  
> > > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> > > +				      struct iio_chan_spec const *chan)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +	const struct iio_scan_type *scan_type;
> > > +	struct spi_transfer *xfer = &st->offload_xfer;
> > > +
> > > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > > +	if (IS_ERR(scan_type))
> > > +		return PTR_ERR(scan_type);
> > > +
> > > +	xfer->bits_per_word = scan_type->realbits;
> > > +	xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> > > +	xfer->len = scan_type->realbits == 24 ? 4 : 2;  
> > 
> > Same question on length vs bits_per_word applies here as in the earlier
> > patch.
> >   
> To be able to optimize the SPI message, len must be a multiple of 16
> bits. To achieve maximum throughput, no extra bits (and therefore SCLK
> clock cycles) must be transferred during the SPI transfer. This is set
> by bits_per_word, 24-bits means 24 SCLK.

I got that intention, what I wasn't sure on was what the spi subsystem
would do with this case.

I checked the docs and this case is called out though only in the
spi_device docs for bits_per_word (not mentioned in the spi_transfer
docs) so fair enough.  Just seemed strange!
 
> 
> Finally, storagebits is the number of bits actually used to store the
> reading, and for the offload channel is the DMA width, always 32-bits.
> An abstraction to obtain the DMA width should be created, so the 32-bits
> value is not hard-coded into the driver, still, for this series, it is.
> 
Thanks

Jonathan