Re: [PATCH v6 02/17] spi: offload: add support for hardware triggers

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

On Wed, 2024-12-11 at 14:54 -0600, David Lechner wrote:
> Extend SPI offloading to support hardware triggers.
> 
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_optimize_message().
> 
> A new struct spi_offload_trigger is introduced that can be used to
> configure any type of trigger. It has a type discriminator and a union
> to allow it to be extended in the future. Two trigger types are defined
> to start with. One is a trigger that indicates that the SPI peripheral
> is ready to read or write data. The other is a periodic trigger to
> repeat a SPI message at a fixed rate.
> 
> There is also a spi_offload_hw_trigger_validate() function that works
> similar to clk_round_rate(). It basically asks the question of if we
> enabled the hardware trigger what would the actual parameters be. This
> can be used to test if the requested trigger type is actually supported
> by the hardware and for periodic triggers, it can be used to find the
> actual rate that the hardware is capable of.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
> 

One minor comment (and another suggestion) inline...

Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>

> v6 changes:
> * Updated for header file split.
> 
> v5 changes:
> * Use struct kref instead of struct dev for trigger lifetime management.
> * Don't use __free() for args.fwnode.
> * Pass *trigger instead of *priv to all callbacks.
> * Add new *spi_offload_trigger_get_priv() function.
> * Use ops instead of priv for "provider is gone" flag.
> * Combine devm_spi_offload_trigger_alloc() and
>   devm_spi_offload_trigger_register() into one function.
> * Add kernel-doc comments for public functions.
> 
> v4 changes:
> * Added new struct spi_offload_trigger that is a generic struct for any
>   hardware trigger rather than returning a struct clk.
> * Added new spi_offload_hw_trigger_validate() function.
> * Dropped extra locking since it was too restrictive.
> 
> v3 changes:
> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> * added spi_offload_hw_trigger_get_clk() function
> * fixed missing EXPORT_SYMBOL_GPL
> 
> v2 changes:
> * This is split out from "spi: add core support for controllers with
>   offload capabilities".
> * Added locking for offload trigger to claim exclusive use of the SPI
>   bus.
> ---
>  drivers/spi/spi-offload.c            | 281
> +++++++++++++++++++++++++++++++++++
>  include/linux/spi/offload/consumer.h |  12 ++
>  include/linux/spi/offload/provider.h |  28 ++++
>  include/linux/spi/offload/types.h    |  37 +++++
>  4 files changed, 358 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index
> 3a40ef30debf09c6fd7b2c14526f3e5976e2b21f..43582e50e279c4b1b958765fec556aaa9118
> 0e55 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -19,7 +19,11 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/kref.h>
> +#include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
>  #include <linux/spi/offload/consumer.h>
>  #include <linux/spi/offload/provider.h>
>  #include <linux/spi/offload/types.h>
> @@ -31,6 +35,23 @@ struct spi_controller_and_offload {
>  	struct spi_offload *offload;
>  };
>  
> +struct spi_offload_trigger {
> +	struct list_head list;
> +	struct kref ref;
> +	struct fwnode_handle *fwnode;
> +	/* synchronizes calling ops and driver registration */
> +	struct mutex lock;
> +	/*
> +	 * If the provider goes away while the consumer still has a
> reference,
> +	 * ops and priv will be set to NULL and all calls will fail with -
> ENODEV.
> +	 */
> +	const struct spi_offload_trigger_ops *ops;
> +	void *priv;
> +};
> +
> +static LIST_HEAD(spi_offload_triggers);
> +static DEFINE_MUTEX(spi_offload_triggers_lock);
> +
>  /**
>   * devm_spi_offload_alloc() - Allocate offload instance
>   * @dev: Device for devm purposes and assigned to &struct
> spi_offload.provider_dev
> @@ -112,3 +133,263 @@ struct spi_offload *devm_spi_offload_get(struct device
> *dev,
>  	return resource->offload;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> +
> +static void spi_offload_trigger_free(struct kref *ref)
> +{
> +	struct spi_offload_trigger *trigger =
> +		container_of(ref, struct spi_offload_trigger, ref);
> +
> +	mutex_destroy(&trigger->lock);
> +	fwnode_handle_put(trigger->fwnode);
> +	kfree(trigger);
> +}
> +
> +static void spi_offload_trigger_put(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	scoped_guard(mutex, &trigger->lock)
> +		if (trigger->ops && trigger->ops->release)
> +			trigger->ops->release(trigger);
> +
> +	kref_put(&trigger->ref, spi_offload_trigger_free);
> +}
> +
> +static struct spi_offload_trigger
> +*spi_offload_trigger_get(enum spi_offload_trigger_type type,
> +			 struct fwnode_reference_args *args)
> +{
> +	struct spi_offload_trigger *trigger;
> +	bool match = false;
> +	int ret;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +
> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> +		if (trigger->fwnode != args->fwnode)
> +			continue;
> +
> +		match = trigger->ops->match(trigger, type, args->args, args-
> >nargs);
> +		if (match)
> +			break;
> +	}
> +
> +	if (!match)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	guard(mutex)(&trigger->lock);
> +
> +	if (!trigger->ops)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (trigger->ops->request) {
> +		ret = trigger->ops->request(trigger, type, args->args, args-
> >nargs);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	kref_get(&trigger->ref);

maybe try_module_get() would also make sense...

> +
> +	return trigger;
> +}
> +
> +/**
> + * devm_spi_offload_trigger_get() - Get an offload trigger instance
> + * @dev: Device for devm purposes.
> + * @offload: Offload instance connected to a trigger.
> + * @type: Trigger type to get.
> + *
> + * Return: Offload trigger instance or error on failure.
> + */
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_get(struct device *dev,
> +			      struct spi_offload *offload,
> +			      enum spi_offload_trigger_type type)
> +{
> +	struct spi_offload_trigger *trigger;
> +	struct fwnode_reference_args args;
> +	int ret;
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
> >provider_dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells", 0,
> 0,
> +						 &args);

I guess at some point we can add these to fwlinks?

- Nuno Sá