Re: [PATCH v5 01/16] spi: add basic support for SPI offloading

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 Nov 2024 14:18:40 -0600
David Lechner <dlechner@baylibre.com> 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
> and put_offload callbacks 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 ctlr->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>

A few additional comments, but basically fine.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> new file mode 100644
> index 000000000000..5ded7aecf9fc
> --- /dev/null
> +++ b/drivers/spi/spi-offload.c

...

> +/**
> + * 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_controller_and_offload *resource;
> +	int ret;
> +
> +	if (!spi || !config)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!spi->controller->get_offload)
> +		return ERR_PTR(-ENODEV);
> +
> +	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> +	if (!resource)
> +		return ERR_PTR(-ENOMEM);
> +
> +	resource->controller = spi->controller;
> +	resource->offload = spi->controller->get_offload(spi, config);
> +	ret = PTR_ERR_OR_ZERO(resource->offload);
> +	if (ret) {
Why not simply
	if (IS_ERR(resource->offload) {
		kfree(resource);
		return resource->offload;
	}
> +		kfree(resource);
> +		return ERR_PTR(ret);
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_put, resource);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return resource->offload;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_get);

> diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h
> new file mode 100644
> index 000000000000..81b115fc89bf
> --- /dev/null
> +++ b/include/linux/spi/spi-offload.h

> +
> +MODULE_IMPORT_NS(SPI_OFFLOAD);

This is rarely done in headers. (only pwm.h does it I think)
I'd push it down into code that uses this.

It might be worth splitting the header into a spi-offload-provider.h
and spi-offload-consumer.h with a common spi-offload-types.h included
by both.


> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 8497f4747e24..c2b24a0909ea 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -31,6 +31,9 @@ struct spi_transfer;
>  struct spi_controller_mem_ops;
>  struct spi_controller_mem_caps;
>  struct spi_message;
> +struct spi_controller_offload_ops;

Not used so introduce it only when needed (I guess in a later patch).