Re: [PATCH RFC v4 02/15] spi: add basic support for SPI offloading

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote:
> Add the basic infrastructure to support SPI offload providers and
> consumers.
> 
> SPI offloading is a feature that allows the SPI controller to perform
> transfers without any CPU intervention. This is useful, e.g. for
> high-speed data acquisition.
> 
> SPI controllers with offload support need to implement the get_offload
> callback and can use the devm_spi_offload_alloc() to allocate offload
> instances.
> 
> SPI peripheral drivers will call devm_spi_offload_get() to get a
> reference to the matching offload instance. This offload instance can
> then be attached to a SPI message to request offloading that message.
> 
> It is expected that SPI controllers with offload support will check for
> the offload instance in the SPI message in the optimize_message()
> callback and handle it accordingly.
> 
> CONFIG_SPI_OFFLOAD is intended to be a select-only option. Both
> consumer and provider drivers should `select SPI_OFFLOAD` in their
> Kconfig to ensure that the SPI core is built with offload support.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v4 changes:
> * SPI offload functions moved to a separate file instead of spi.c
>   (spi.c is already too long).
> * struct spi_offload and devm_spi_offload_get() are back, similar to
>   but improved over v1. This avoids having to pass the function ID
>   string to every function call and re-lookup the offload instance.
> * offload message prepare/unprepare functions are removed. Instead the
>   existing optimize/unoptimize functions should be used. Setting
>   spi_message::offload pointer is used as a flag to differentiate
>   between an offloaded message and a regular message.
> 
> v3 changes:
> * Minor changes to doc comments.
> * Changed to use phandle array for spi-offloads.
> * Changed id to string to make use of spi-offload-names.
> 
> v2 changes:
> * This is a rework of "spi: add core support for controllers with offload
>   capabilities" from v1.
> * The spi_offload_get() function that Nuno didn't like is gone. Instead,
>   there is now a mapping callback that uses the new generic devicetree
>   binding to request resources automatically when a SPI device is probed.
> * The spi_offload_enable/disable() functions for dealing with hardware
>   triggers are deferred to a separate patch.
> * This leaves adding spi_offload_prepare/unprepare() which have been
>   reworked to be a bit more robust.
> ---
>  drivers/spi/Kconfig             |   3 ++
>  drivers/spi/Makefile            |   1 +
>  drivers/spi/spi-offload.c       | 104 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-offload.h |  64 +++++++++++++++++++++++++
>  include/linux/spi/spi.h         |  16 +++++++
>  5 files changed, 188 insertions(+)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 823797217404..d65074b85f62 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -55,6 +55,9 @@ config SPI_MEM
>  	  This extension is meant to simplify interaction with SPI memories
>  	  by providing a high-level interface to send memory-like commands.
>  
> +config SPI_OFFLOAD
> +	bool
> +
>  comment "SPI Master Controller Drivers"
>  
>  config SPI_AIROHA_SNFI
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index a9b1bc259b68..6a470eb475a2 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -10,6 +10,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
>  obj-$(CONFIG_SPI_MASTER)		+= spi.o
>  obj-$(CONFIG_SPI_MEM)			+= spi-mem.o
>  obj-$(CONFIG_SPI_MUX)			+= spi-mux.o
> +obj-$(CONFIG_SPI_OFFLOAD)		+= spi-offload.o
>  obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
>  obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
>  
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> new file mode 100644
> index 000000000000..c344cbf50bdb
> --- /dev/null
> +++ b/drivers/spi/spi-offload.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + */
> +
> +#define DEFAULT_SYMBOL_NAMESPACE SPI_OFFLOAD
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi-offload.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +
> +/**
> + * devm_spi_offload_alloc() - Allocate offload instances
> + * @dev: Device for devm purposes
> + * @num_offloads: Number of offloads to allocate
> + * @priv_size: Size of private data to allocate for each offload
> + *
> + * Offload providers should use this to allocate offload instances.
> + *
> + * Return: Pointer to array of offloads or error on failure.
> + */
> +struct spi_offload *devm_spi_offload_alloc(struct device *dev,
> +					   size_t num_offloads,
> +					   size_t priv_size)
> +{
> +	struct spi_offload *offloads;
> +	void *privs;
> +	size_t i;
> +
> +	offloads = devm_kcalloc(dev, num_offloads, sizeof(*offloads) + priv_size,
> +				GFP_KERNEL);
> +	if (!offloads)
> +		return ERR_PTR(-ENOMEM);
> +
> +	privs = (void *)(offloads + num_offloads);
> +
> +	for (i = 0; i < num_offloads; i++) {
> +		struct spi_offload *offload = offloads + i;
> +		void *priv = privs + i * priv_size;
> +
> +		offload->provider_dev = dev;
> +		offload->priv = priv;
> +	}

Hmm looking at the spi_engine implementation, got me thinking about this. I think
like this we might mess up with natural alignments which can be pretty nasty. Maybe
do something like devres [1] (I guess we do not need the flex array though)?

Now that I also look at this better, I would not do it like this. I would keep it
simple and just allocate one spi_offload object and be done with it. In the future
and when we actually support more than one instance you could introduce a
devm_spi_offload_alloc_array() variant and I'm still not sure if it's that useful.
Anyways this is just personal preference I guess...

[1]: https://elixir.bootlin.com/linux/v6.12-rc4/source/drivers/base/devres.c#L35

- Nuno Sá