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

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

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@xxxxxxxxxxxx>
> ---

Hi David,

Just one minor comment...

> 
> 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

Cool, was not aware of this :)
> +
> +#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;
> +	}
> +
> +	return offloads;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_alloc);
> +
> +static void spi_offload_put(void *data)
> +{
> +	struct spi_offload *offload = data;
> +
> +	offload->spi = NULL;
> +	put_device(offload->provider_dev);
> +}
> +
> +/**
> + * devm_spi_offload_get() - Get an offload instance
> + * @dev: Device for devm purposes
> + * @spi: SPI device to use for the transfers
> + * @config: Offload configuration
> + *
> + * Peripheral drivers call this function to get an offload instance that meets
> + * the requirements specified in @config. If no suitable offload instance is
> + * available, -ENODEV is returned.
> + *
> + * Return: Offload instance or error on failure.
> + */
> +struct spi_offload *devm_spi_offload_get(struct device *dev,
> +					 struct spi_device *spi,
> +					 const struct spi_offload_config *config)
> +{
> +	struct spi_offload *offload;
> +	int ret;
> +
> +	if (!spi || !config)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!spi->controller->get_offload)
> +		return ERR_PTR(-ENODEV);
> +
> +	offload = spi->controller->get_offload(spi, config);
> +	if (IS_ERR(offload))
> +		return offload;
> +
> +	if (offload->spi)
> +		return ERR_PTR(-EBUSY);
> +
> +	offload->spi = spi;
> +	get_device(offload->provider_dev);

Isn't this redundant? From what I can tell, we're assuming that the spi controller
(of the spi device) is the offload provider. Therefore, getting an extra reference
for it does not really seems necessary. The device cannot go away without under the
spi_device feet. If that could happen, then we would also need to take care about
callback access and things like that. Going this way, it would also be arguable to
have a try_module_get().

- Nuno Sá