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

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

v5 changes:
* Don't include linux/property.h (moved to later patch).
* Only allocate single offload instance instead of array.
* Allocate *priv separately to avoid alignment issues.
* Add put_offload() callback instead of assuming devm semantics.
* Drop struct spi_offload::spi. It was only being used as a flag.
* Don't get/put struct spi_offload::provider_dev.
* Add MAINTAINERS entry for me as reviewer for anything related to
  SPI offload.

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.
---
 MAINTAINERS                     |   6 +++
 drivers/spi/Kconfig             |   3 ++
 drivers/spi/Makefile            |   1 +
 drivers/spi/spi-offload.c       | 103 ++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-offload.h |  60 +++++++++++++++++++++++
 include/linux/spi/spi.h         |  18 +++++++
 6 files changed, 191 insertions(+)

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

On 11/24/24 10:32 AM, Jonathan Cameron wrote:
> 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.
>>

...

>> +	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);
>> +	}

Hmm... maybe somewhere along the way ret was being checked again
after this, but doesn't to be the case anymore.

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

Yes, it was Uwe that suggested that I put it in the header. :-)

Are there any unwanted side effects of having it in the header?

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

On Sun, 24 Nov 2024 12:01:23 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/24/24 10:32 AM, Jonathan Cameron wrote:
> > 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.
> >>  
> 
> ...
> 
> >> +	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);
> >> +	}  
> 
> Hmm... maybe somewhere along the way ret was being checked again
> after this, but doesn't to be the case anymore.
> 
> >> +
> >> +	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.  
> 
> Yes, it was Uwe that suggested that I put it in the header. :-)
> 
> Are there any unwanted side effects of having it in the header?
Reviewer surprise?
:)

Up to Mark as he gets to enjoy this code for ever.

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