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.
>
> >
> > +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.
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.
> > + xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> > +
> > + spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > + st->offload_msg.offload = st->offload;
> > +
> > + return 0;
> > +}
> > +
> > static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan,
> > unsigned int val)
> > @@ -838,6 +873,87 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
> > return ret;
> > }
>
> > static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
> > {
> > switch (trigger_sources->args[1]) {
> > + case AD4052_TRIGGER_PIN_GP0:
> > + return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_EITHER_THRESH ?
> > + 0 : -EINVAL;
> > case AD4052_TRIGGER_PIN_GP1:
> > return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
> > 0 : -EINVAL;
> > @@ -903,14 +1092,45 @@ static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
> > int ret;
> >
> > np = st->spi->dev.of_node;
> > + for (u8 i = 0; i < 2; i++) {
> > + ret = of_parse_phandle_with_args(np, "trigger-sources",
> > + "#trigger-source-cells", i,
> > + &trigger_sources);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __ad4052_validate_trigger_sources(&trigger_sources);
> > + of_node_put(trigger_sources.np);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return ret;
>
> I think this is always 0. So return 0; preferred to make that explicit.
>
Well, this whole method is deleted for v4 due to the trigger-sources
discussion. Per following David suggestion, gp0 is assumed drdy and gp1
threshold events, unless the parent (spi offload) trigger-sources says
otherwise (gp1).
Best regards,
Jorge
> > +}
> >
>
