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

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Wed, 23 Oct 2024 15:59:09 -0500
David Lechner <dlechner@xxxxxxxxxxxx> 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>
A few minor additions to what has already been raised.

Jonathan

> 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>
?  If it is needed for later patch bring it in then.


> +#include <linux/spi/spi-offload.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
>
> +
> +/**
> + * 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);

Why let this return an offload that is already in use?
Maybe make that a problem for the spi controller
Seems odd to pass it spi then set it later.

I.e. have this return ERR_PTR(-EBUSY);


> +	if (IS_ERR(offload))
> +		return offload;
> +
> +	if (offload->spi)
> +		return ERR_PTR(-EBUSY);
> +
> +	offload->spi = spi;
> +	get_device(offload->provider_dev);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_put, offload);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return offload;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_get);