On Mon, 22 Jul 2024 16:57:16 -0500
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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;
