Re: [PATCH 12/13] iio: offload: add new PWM triggered DMA buffer driver

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Fri, 2024-01-12 at 12:51 +0000, Jonathan Cameron wrote:
> On Thu, 11 Jan 2024 15:59:02 +0100
> Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> 
> > On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote:
> > > This adds a new driver for handling SPI offloading using a PWM as the
> > > trigger and DMA for the received data. This will be used by ADCs in
> > > conjunction with SPI controllers with offloading support to be able
> > > to sample at high rates without CPU intervention.
> > > 
> > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > > ---
> > >  drivers/iio/Kconfig                                |   1 +
> > >  drivers/iio/Makefile                               |   1 +
> > >  .../iio/buffer/industrialio-hw-triggered-buffer.c  |   1 +
> > >  drivers/iio/offload/Kconfig                        |  21 ++
> > >  drivers/iio/offload/Makefile                       |   2 +
> > >  drivers/iio/offload/iio-pwm-triggered-dma-buffer.c | 212
> > > +++++++++++++++++++++
> > >  6 files changed, 238 insertions(+)
> > > 
> > > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> > > index 52eb46ef84c1..56738282d82f 100644
> > > --- a/drivers/iio/Kconfig
> > > +++ b/drivers/iio/Kconfig
> > > @@ -90,6 +90,7 @@ source "drivers/iio/imu/Kconfig"
> > >  source "drivers/iio/light/Kconfig"
> > >  source "drivers/iio/magnetometer/Kconfig"
> > >  source "drivers/iio/multiplexer/Kconfig"
> > > +source "drivers/iio/offload/Kconfig"
> > >  source "drivers/iio/orientation/Kconfig"
> > >  source "drivers/iio/test/Kconfig"
> > >  if IIO_TRIGGER
> > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > index 9622347a1c1b..20acf5e1a4a7 100644
> > > --- a/drivers/iio/Makefile
> > > +++ b/drivers/iio/Makefile
> > > @@ -34,6 +34,7 @@ obj-y += imu/
> > >  obj-y += light/
> > >  obj-y += magnetometer/
> > >  obj-y += multiplexer/
> > > +obj-y += offload/
> > >  obj-y += orientation/
> > >  obj-y += position/
> > >  obj-y += potentiometer/
> > > diff --git a/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > index 7a8a71066b0e..a2fae6059616 100644
> > > --- a/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > +++ b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > @@ -86,6 +86,7 @@ static int iio_hw_trigger_buffer_probe(struct
> > > auxiliary_device *adev,
> > >  }
> > >  
> > >  static const struct auxiliary_device_id iio_hw_trigger_buffer_id_table[] = {
> > > +	{ .name = "pwm-triggered-dma-buffer.triggered-buffer" },
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(auxiliary, iio_hw_trigger_buffer_id_table);
> > > diff --git a/drivers/iio/offload/Kconfig b/drivers/iio/offload/Kconfig
> > > new file mode 100644
> > > index 000000000000..760c0cfe0e9c
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/Kconfig
> > > @@ -0,0 +1,21 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# SPI offload handlers for Industrial I/O
> > > +#
> > > +# When adding new entries keep the list in alphabetical order
> > > +
> > > +menu "SPI offload handlers"
> > > +
> > > +config IIO_PWM_TRIGGERED_DMA_BUFFER
> > > +	tristate "PWM trigger and DMA buffer connected to SPI offload"
> > > +	select AUXILIARY_BUS
> > > +	select IIO_BUFFER_DMAENGINE
> > > +	help
> > > +	  Provides a periodic hardware trigger via a PWM connected to the
> > > +	  trigger input of a SPI offload and a hardware buffer implemented
> > > +	  via DMA connected to the data output stream the a SPI offload.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called "iio-pwm-triggered-dma-buffer".
> > > +
> > > +endmenu
> > > diff --git a/drivers/iio/offload/Makefile b/drivers/iio/offload/Makefile
> > > new file mode 100644
> > > index 000000000000..7300ce82f066
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/Makefile
> > > @@ -0,0 +1,2 @@
> > > +
> > > +obj-$(CONFIG_IIO_PWM_TRIGGERED_DMA_BUFFER) := iio-pwm-triggered-dma-buffer.o
> > > diff --git a/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > b/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > new file mode 100644
> > > index 000000000000..970ea82316f6
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > @@ -0,0 +1,212 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Platform driver for a PWM trigger and DMA buffer connected to a SPI
> > > + * controller offload instance implementing the iio-hw-triggered-buffer
> > > + * interface.
> > > + *
> > > + * Copyright (C) 2023 Analog Devices, Inc.
> > > + * Copyright (C) 2023 BayLibre, SAS
> > > + */
> > > +
> > > +#include <linux/auxiliary_bus.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/hw_triggered_buffer_impl.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/buffer-dmaengine.h>
> > > +#include <linux/sysfs.h>
> > > +
> > > +struct iio_pwm_triggered_dma_buffer {
> > > +	struct iio_hw_triggered_buffer_device hw;
> > > +	struct pwm_device *pwm;
> > > +};
> > > +
> > > +static const struct iio_trigger_ops iio_pwm_triggered_dma_buffer_ops;
> > > +
> > > +static int iio_pwm_triggered_dma_buffer_set_state(struct iio_trigger *trig,
> > > bool state)
> > > +{
> > > +	struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > +
> > > +	if (state)
> > > +		return pwm_enable(st->pwm);
> > > +
> > > +	pwm_disable(st->pwm);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int iio_pwm_triggered_dma_buffer_validate_device(struct iio_trigger
> > > *trig,
> > > +							struct iio_dev
> > > *indio_dev)
> > > +{
> > > +	/* Don't allow assigning trigger via sysfs. */
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops iio_pwm_triggered_dma_buffer_ops = {
> > > +	.set_trigger_state = iio_pwm_triggered_dma_buffer_set_state,
> > > +	.validate_device = iio_pwm_triggered_dma_buffer_validate_device,
> > > +};
> > > +
> > > +static u32 axi_spi_engine_offload_pwm_trigger_get_rate(struct iio_trigger
> > > *trig)
> > > +{
> > > +	struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > +	u64 period_ns = pwm_get_period(st->pwm);
> > > +
> > > +	if (period_ns)
> > > +		return DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, period_ns);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +axi_spi_engine_offload_set_samp_freq(struct iio_pwm_triggered_dma_buffer *st,
> > > +				     u32 requested_hz)
> > > +{
> > > +	int period_ns;
> > > +
> > > +	if (requested_hz == 0)
> > > +		return -EINVAL;
> > > +
> > > +	period_ns = DIV_ROUND_UP(NSEC_PER_SEC, requested_hz);
> > > +
> > > +	/*
> > > +	 * FIXME: We really just need a clock, not a PWM. The current duty
> > > cycle
> > > +	 * value is a hack to work around the edge vs. level offload trigger
> > > +	 * issue in the ADI AXI SPI Engine firmware.
> > > +	 */
> > > +	return pwm_config(st->pwm, 10, period_ns);
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_show(struct device *dev,
> > > +				       struct device_attribute *attr, char
> > > *buf)
> > > +{
> > > +	struct iio_trigger *trig = to_iio_trigger(dev);
> > > +
> > > +	return sysfs_emit(buf, "%u\n",
> > > +			  axi_spi_engine_offload_pwm_trigger_get_rate(trig));
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_store(struct device *dev,
> > > +					struct device_attribute *attr,
> > > +					const char *buf, size_t len)
> > > +{
> > > +	struct iio_trigger *trig = to_iio_trigger(dev);
> > > +	struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > +	int ret;
> > > +	u32 val;
> > > +
> > > +	ret = kstrtou32(buf, 10, &val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = axi_spi_engine_offload_set_samp_freq(st, val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return len;
> > > +}
> > > +  
> > 
> > This is also something that made me wonder... In the end of the day, the
> > frequency at which we want to sample the data depends on the converter device.
> > This completely detached interface makes it more easy for the user to screw up
> > things, right?
> It's easy to do that anyway :) If you think of a normal SPI attached device that
> has it's own internal clocking then it's usually easy to set the device to grab
> data faster than than the spi bus can drain it.  We drop samples.

> Here it might make sense to add some bounds I guess as it's all in hardware
> - either have the ADC provide them or push it to DT,

Indeed... DT would likely be simpler

> 
> > 
> > Things might even become more complicated in usecases where we have an
> > additional pwm (on top of the data fetch trigger one) for triggering conversions
> > in the converter and we might need to properly control the phase between those
> > two signals. So, how would we handle it in the current form? We have one pwm
> > belonging to the offload engine and one belonging to the converter but they need
> > to cope together...
> 
> Groan loudly? 
> If someone wants careful sync between a trigger for the ADC and the read back
> triggering
> they should do it in hardware. Sure we can do it if the pwm is sophisticated enough
> but the mess of layering etc needed to make it work is just nasty.
> 
> I guess you make a custom trigger that bakes in your requirements.
> 

Yeah, we have yet another FPGA soft core implementing a pwm controller (also to be
upstreamed soon) which can output multiple signals at different phases, frequency,
etc...

- Nuno Sá