Re: [PATCH RFC v3 9/9] iio: adc: ad7944: add support for SPI offload

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 22 Jul 2024 16:57:16 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds support for SPI offload to the ad7944 driver. This allows
> reading data at the max sample rate of 2.5 MSPS.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
I think you can easily make the sampling frequence attribute disappear for
cases where we can't provide a real value back for a read.

> ---
> 
> Note: in v2 we discussed if we should make the SPI offload use buffer1
> instead of buffer0 as to not break userspace. I'm still on the fence
> about if we should do that or not. Mainly because many userspace tools
> aren't aware of multiple buffers yet, so would make it harder to
> use the driver. And technically, the way it is implemented right now
> is not going to change anything for existing users since they won't
> be using the offload feature. So the argument could be made that this
> isn't really breaking userspace after all.
> 
> v3 changes:
> * Finished TODOs.
> * Adapted to changes in other patches.
> 
> v2 changes:
> 
> In the previous version, there was a new separate driver for the PWM
> trigger and DMA hardware buffer. This was deemed too complex so they
> are moved into the ad7944 driver.
> 
> It has also been reworked to accommodate for the changes described in
> the other patches.
> ---
>  drivers/iio/adc/ad7944.c | 173 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 166 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index 0f36138a7144..43674ff439d2 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -9,6 +9,7 @@
>  #include <linux/align.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -21,6 +22,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer-dmaengine.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> @@ -54,6 +56,8 @@ struct ad7944_adc {
>  	enum ad7944_spi_mode spi_mode;
>  	struct spi_transfer xfers[3];
>  	struct spi_message msg;
> +	struct spi_transfer offload_xfers[3];
> +	struct spi_message offload_msg;
>  	void *chain_mode_buf;
>  	/* Chip-specific timing specifications. */
>  	const struct ad7944_timing_spec *timing_spec;
> @@ -65,6 +69,8 @@ struct ad7944_adc {
>  	bool always_turbo;
>  	/* Reference voltage (millivolts). */
>  	unsigned int ref_mv;
> +	/* Clock that triggers SPI offload. */
> +	struct clk *trigger_clk;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> @@ -81,6 +87,8 @@ struct ad7944_adc {
>  
>  /* quite time before CNV rising edge */
>  #define T_QUIET_NS	20

I'd prefer these prefixed as AD7944_T_QUIET_NS etc

> +/* minimum CNV high time to trigger conversion */
> +#define T_CNVH_NS	20
>  
>  static const struct ad7944_timing_spec ad7944_timing_spec = {
>  	.conv_ns = 420,
> @@ -123,6 +131,7 @@ static const struct ad7944_chip_info _name##_chip_info = {		\
>  			.scan_type.endianness = IIO_CPU,		\
>  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	\
>  					| BIT(IIO_CHAN_INFO_SCALE),	\
> +			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\

So this gets registered for all cases, but may return -EIOPNOTSUPPORTED?

That's not ideal.  If we can we should hide this file if we aren't going
to be able to read it.
Whilst it may seem overkill that's a separate iio_chan_spec array etc
that is picked based on whether we are using spi offloading or not.

>  		},							\
>  		IIO_CHAN_SOFT_TIMESTAMP(1),				\
>  	},								\
> @@ -236,6 +245,54 @@ static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc
>  	return devm_spi_optimize_message(dev, adc->spi, &adc->msg);
>  }
>  
> +static void ad7944_offload_unprepare(void *p)
> +{
> +	struct ad7944_adc *adc = p;
> +
> +	spi_offload_unprepare(adc->spi, NULL, &adc->offload_msg);
> +}

>  static int ad7944_probe(struct spi_device *spi)
>  {
>  	const struct ad7944_chip_info *chip_info;
> @@ -554,13 +682,11 @@ static int ad7944_probe(struct spi_device *spi)
>  		ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		if (ret)
>  			return ret;	
> -
>  		break;
>  	case AD7944_SPI_MODE_SINGLE:
>  		ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		if (ret)
>  			return ret;
> -
Don't edit unrelated white space in a patch that is doing more important things.
Just noise...
>  		break;