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.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

In previous versions, we locked the SPI bus when the hardware trigger
was enabled, but we found this to be too restrictive. In one use case,
to avoid a race condition, we need to enable the SPI offload via a
hardware trigger, then write a SPI message to the peripheral to place
it into a mode that will generate the trigger. If we did it the other
way around, we could miss the first trigger.

Another likely use case will be enabling two offloads/triggers at one
time on the same device, e.g. a read trigger and a write trigger. So
the exclusive bus lock for a single trigger would be too restrictive in
this case too.

So for now, I'm going with Nuno's suggestion to leave any locking up to
the individual controller driver. If we do find we need something more
generic in the future, we could add a new spi_bus_lock_exclusive() API
that causes spi_bus_lock() to fail instead of waiting and add "locked"
versions of trigger enable functions. This would allow a peripheral to
claim exclusive use of the bus indefinitely while still being able to
do any SPI messaging that it needs.

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       | 266 ++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-offload.h |  78 ++++++++++++
 2 files changed, 344 insertions(+)

diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
index c344cbf50bdb..2a1f9587f27a 100644
--- a/drivers/spi/spi-offload.c
+++ b/drivers/spi/spi-offload.c
@@ -9,12 +9,26 @@
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/property.h>
 #include <linux/spi/spi-offload.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
 
+struct spi_offload_trigger {
+	struct list_head list;
+	struct device dev;
+	/* synchronizes calling ops and driver registration */
+	struct mutex lock;
+	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 instances
  * @dev: Device for devm purposes
@@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
 	return offload;
 }
 EXPORT_SYMBOL_GPL(devm_spi_offload_get);
+
+static void spi_offload_trigger_release(void *data)
+{
+	struct spi_offload_trigger *trigger = data;
+
+	guard(mutex)(&trigger->lock);
+	if (trigger->priv && trigger->ops->release)
+		trigger->ops->release(trigger->priv);
+
+	put_device(&trigger->dev);
+}
+
+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;
+	bool match = false;
+	int ret;
+
+	ret = fwnode_property_get_reference_args(dev_fwnode(offload->provider_dev),
+						 "trigger-sources",
+						 "#trigger-source-cells", 0, 0,
+						 &args);
+	if (ret)
+		return ERR_PTR(ret);
+
+	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
+
+	guard(mutex)(&spi_offload_triggers_lock);
+
+	list_for_each_entry(trigger, &spi_offload_triggers, list) {
+		if (trigger->dev.fwnode != args.fwnode)
+			continue;
+
+		match = trigger->ops->match(trigger->priv, type, args.args, args.nargs);
+		if (match)
+			break;
+	}
+
+	if (!match)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	guard(mutex)(&trigger->lock);
+
+	if (!trigger->priv)
+		return ERR_PTR(-ENODEV);
+
+	if (trigger->ops->request) {
+		ret = trigger->ops->request(trigger->priv, type, args.args, args.nargs);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	get_device(&trigger->dev);
+
+	ret = devm_add_action_or_reset(dev, spi_offload_trigger_release, trigger);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return trigger;
+}
+EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_get);
+
+/**
+ * spi_offload_trigger_validate - Validate the requested trigger
+ * @trigger: Offload trigger instance
+ * @config: Trigger config to validate
+ *
+ * On success, @config may be modifed to reflect what the hardware can do.
+ * For example, the frequency of a periodic trigger may be adjusted to the
+ * nearest supported value.
+ *
+ * Callers will likely need to do additional validation of the modified trigger
+ * parameters.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int spi_offload_trigger_validate(struct spi_offload_trigger *trigger,
+				 struct spi_offload_trigger_config *config)
+{
+	guard(mutex)(&trigger->lock);
+
+	if (!trigger->priv)
+		return -ENODEV;
+
+	if (!trigger->ops->validate)
+		return -EOPNOTSUPP;
+
+	return trigger->ops->validate(trigger->priv, config);
+}
+EXPORT_SYMBOL_GPL(spi_offload_trigger_validate);
+
+/**
+ * spi_offload_trigger_enable - enables trigger for offload
+ * @trigger: Offload trigger instance
+ * @config: Trigger config to validate
+ *
+ * There must be a prepared offload instance with the specified ID (i.e.
+ * spi_optimize_message() was called with the same offload assigned to the
+ * message). This will also reserve the bus for exclusive use by the offload
+ * instance until the trigger is disabled. Any other attempts to send a
+ * transfer or lock the bus will fail with -EBUSY during this time.
+ *
+ * Calls must be balanced with spi_offload_trigger_disable().
+ *
+ * Context: can sleep
+ * Return: 0 on success, else a negative error code.
+ */
+int spi_offload_trigger_enable(struct spi_offload *offload,
+			       struct spi_offload_trigger *trigger,
+			       struct spi_offload_trigger_config *config)
+{
+	int ret;
+
+	guard(mutex)(&trigger->lock);
+
+	if (!trigger->priv)
+		return -ENODEV;
+
+	if (offload->ops->trigger_enable) {
+		ret = offload->ops->trigger_enable(offload);
+		if (ret)
+			return ret;
+	}
+
+	if (trigger->ops->enable) {
+		ret = trigger->ops->enable(trigger->priv, config);
+		if (ret) {
+			if (offload->ops->trigger_disable)
+				offload->ops->trigger_disable(offload);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_offload_trigger_enable);
+
+/**
+ * spi_offload_trigger_disable - disables hardware trigger for offload
+ * @offload: Offload instance
+ *
+ * Disables the hardware trigger for the offload instance with the specified ID
+ * and releases the bus for use by other clients.
+ *
+ * Context: can sleep
+ */
+void spi_offload_trigger_disable(struct spi_offload *offload,
+				 struct spi_offload_trigger *trigger)
+{
+	if (offload->ops->trigger_disable)
+		offload->ops->trigger_disable(offload);
+
+	guard(mutex)(&trigger->lock);
+
+	if (!trigger->priv)
+		return;
+
+	if (trigger->ops->disable)
+		trigger->ops->disable(trigger->priv);
+}
+EXPORT_SYMBOL_GPL(spi_offload_trigger_disable);
+
+/* Triggers providers */
+
+static void spi_offload_trigger_dev_release(struct device *dev)
+{
+	struct spi_offload_trigger *trigger =
+		container_of(dev, struct spi_offload_trigger, dev);
+
+	mutex_destroy(&trigger->lock);
+	of_node_put(trigger->dev.of_node);
+	kfree(trigger);
+}
+
+static void spi_offload_trigger_put(void *data)
+{
+	struct spi_offload_trigger *trigger = data;
+
+	put_device(&trigger->dev);
+}
+
+struct spi_offload_trigger
+*devm_spi_offload_trigger_alloc(struct device *dev,
+				struct spi_offload_trigger_info *info)
+{
+	struct spi_offload_trigger *trigger;
+	int ret;
+
+	trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
+	if (!trigger)
+		return ERR_PTR(-ENOMEM);
+
+	device_initialize(&trigger->dev);
+	trigger->dev.parent = info->parent;
+	trigger->dev.fwnode = info->fwnode;
+	trigger->dev.of_node = of_node_get(to_of_node(trigger->dev.fwnode));
+	trigger->dev.of_node_reused = true;
+	trigger->dev.release = spi_offload_trigger_dev_release;
+
+	mutex_init(&trigger->lock);
+	trigger->ops = info->ops;
+
+	ret = devm_add_action_or_reset(dev, spi_offload_trigger_put, trigger);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = dev_set_name(&trigger->dev, "%s-%d", info->name, info->id);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return trigger;
+}
+EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_alloc);
+
+static void spi_offload_trigger_unregister(void *data)
+{
+	struct spi_offload_trigger *trigger = data;
+
+	scoped_guard(mutex, &spi_offload_triggers_lock)
+		list_del(&trigger->list);
+
+	guard(mutex)(&trigger->lock);
+	trigger->priv = NULL;
+	device_del(&trigger->dev);
+}
+
+int devm_spi_offload_trigger_register(struct device *dev,
+				      struct spi_offload_trigger *trigger,
+				      void *priv)
+{
+	int ret;
+
+	ret = device_add(&trigger->dev);
+	if (ret)
+		return ret;
+
+	trigger->priv = priv;
+
+	guard(mutex)(&spi_offload_triggers_lock);
+	list_add_tail(&trigger->list, &spi_offload_triggers);
+
+	ret = devm_add_action_or_reset(dev, spi_offload_trigger_unregister, trigger);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);
diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h
index 92a557533b83..561cc1fb6f35 100644
--- a/include/linux/spi/spi-offload.h
+++ b/include/linux/spi/spi-offload.h
@@ -22,6 +22,7 @@
 MODULE_IMPORT_NS(SPI_OFFLOAD);
 
 struct device;
+struct fwnode_handle;
 struct spi_device;
 
 /* Offload can be triggered by external hardware event. */
@@ -53,6 +54,43 @@ struct spi_offload {
 	struct spi_device *spi;
 	/** @priv: provider driver private data */
 	void *priv;
+	/** @ops: callbacks for offload support */
+	const struct spi_offload_ops *ops;
+};
+
+enum spi_offload_trigger_type {
+	/* Indication from SPI peripheral that data is read to read. */
+	SPI_OFFLOAD_TRIGGER_DATA_READY,
+	/* Trigger comes from a periodic source such as a clock. */
+	SPI_OFFLOAD_TRIGGER_PERIODIC,
+};
+
+struct spi_offload_trigger_periodic {
+	u64 frequency_hz;
+};
+
+struct spi_offload_trigger_config {
+	/** @type: type discriminator for union */
+	enum spi_offload_trigger_type type;
+	union {
+		struct spi_offload_trigger_periodic periodic;
+	};
+};
+
+/**
+ * struct spi_offload_ops - callbacks implemented by offload providers
+ */
+struct spi_offload_ops {
+	/**
+	 * @trigger_enable: Optional callback to enable the trigger for the
+	 * given offload instance.
+	 */
+	int (*trigger_enable)(struct spi_offload *offload);
+	/**
+	 * @trigger_disable: Optional callback to disable the trigger for the
+	 * given offload instance.
+	 */
+	void (*trigger_disable)(struct spi_offload *offload);
 };
 
 struct spi_offload *devm_spi_offload_alloc(struct device *dev,
@@ -61,4 +99,44 @@ struct spi_offload *devm_spi_offload_alloc(struct device *dev,
 struct spi_offload *devm_spi_offload_get(struct device *dev, struct spi_device *spi,
 					 const struct spi_offload_config *config);
 
+struct spi_offload_trigger
+*devm_spi_offload_trigger_get(struct device *dev,
+			      struct spi_offload *offload,
+			      enum spi_offload_trigger_type type);
+int spi_offload_trigger_validate(struct spi_offload_trigger *trigger,
+				 struct spi_offload_trigger_config *config);
+int spi_offload_trigger_enable(struct spi_offload *offload,
+			       struct spi_offload_trigger *trigger,
+			       struct spi_offload_trigger_config *config);
+void spi_offload_trigger_disable(struct spi_offload *offload,
+				 struct spi_offload_trigger *trigger);
+
+/* Trigger providers */
+
+struct spi_offload_trigger;
+
+struct spi_offload_trigger_ops {
+	bool (*match)(void *priv, enum spi_offload_trigger_type type, u64 *args, u32 nargs);
+	int (*request)(void *priv, enum spi_offload_trigger_type type, u64 *args, u32 nargs);
+	void (*release)(void *priv);
+	int (*validate)(void *priv, struct spi_offload_trigger_config *config);
+	int (*enable)(void *priv, struct spi_offload_trigger_config *config);
+	void (*disable)(void *priv);
+};
+
+struct spi_offload_trigger_info {
+	const char *name;
+	int id;
+	struct device *parent;
+	struct fwnode_handle *fwnode;
+	const struct spi_offload_trigger_ops *ops;
+};
+
+struct spi_offload_trigger
+*devm_spi_offload_trigger_alloc(struct device *dev,
+				struct spi_offload_trigger_info *info);
+int devm_spi_offload_trigger_register(struct device *dev,
+				      struct spi_offload_trigger *trigger,
+				      void *priv);
+
 #endif /* __LINUX_SPI_OFFLOAD_H */

-- 
2.43.0

On Wed, 2024-10-23 at 15:59 -0500, 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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> In previous versions, we locked the SPI bus when the hardware trigger
> was enabled, but we found this to be too restrictive. In one use case,
> to avoid a race condition, we need to enable the SPI offload via a
> hardware trigger, then write a SPI message to the peripheral to place
> it into a mode that will generate the trigger. If we did it the other
> way around, we could miss the first trigger.
> 
> Another likely use case will be enabling two offloads/triggers at one
> time on the same device, e.g. a read trigger and a write trigger. So
> the exclusive bus lock for a single trigger would be too restrictive in
> this case too.
> 
> So for now, I'm going with Nuno's suggestion to leave any locking up to
> the individual controller driver. If we do find we need something more
> generic in the future, we could add a new spi_bus_lock_exclusive() API
> that causes spi_bus_lock() to fail instead of waiting and add "locked"
> versions of trigger enable functions. This would allow a peripheral to
> claim exclusive use of the bus indefinitely while still being able to
> do any SPI messaging that it needs.
> 
> 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       | 266 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-offload.h |  78 ++++++++++++
>  2 files changed, 344 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index c344cbf50bdb..2a1f9587f27a 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -9,12 +9,26 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/spi/spi-offload.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>  
> +struct spi_offload_trigger {
> +	struct list_head list;
> +	struct device dev;
> +	/* synchronizes calling ops and driver registration */
> +	struct mutex lock;
> +	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 instances
>   * @dev: Device for devm purposes
> @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
>  	return offload;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> +
> +static void spi_offload_trigger_release(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	guard(mutex)(&trigger->lock);
> +	if (trigger->priv && trigger->ops->release)
> +		trigger->ops->release(trigger->priv);
> +
> +	put_device(&trigger->dev);
> +}
> +
> +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;
> +	bool match = false;
> +	int ret;
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
> >provider_dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells", 0, 0,
> +						 &args);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +
> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> +		if (trigger->dev.fwnode != args.fwnode)
> +			continue;

device_match_fwnode()

- Nuno Sá

On Wed, 23 Oct 2024 15:59:10 -0500
David Lechner <dlechner@baylibre.com> 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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few generic comments inline.

Jonathan

> ---
> 
> In previous versions, we locked the SPI bus when the hardware trigger
> was enabled, but we found this to be too restrictive. In one use case,
> to avoid a race condition, we need to enable the SPI offload via a
> hardware trigger, then write a SPI message to the peripheral to place
> it into a mode that will generate the trigger. If we did it the other
> way around, we could miss the first trigger.
> 
> Another likely use case will be enabling two offloads/triggers at one
> time on the same device, e.g. a read trigger and a write trigger. So
> the exclusive bus lock for a single trigger would be too restrictive in
> this case too.
> 
> So for now, I'm going with Nuno's suggestion to leave any locking up to
> the individual controller driver. If we do find we need something more
> generic in the future, we could add a new spi_bus_lock_exclusive() API
> that causes spi_bus_lock() to fail instead of waiting and add "locked"
> versions of trigger enable functions. This would allow a peripheral to
> claim exclusive use of the bus indefinitely while still being able to
> do any SPI messaging that it needs.
> 
> 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       | 266 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-offload.h |  78 ++++++++++++
>  2 files changed, 344 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index c344cbf50bdb..2a1f9587f27a 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -9,12 +9,26 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/spi/spi-offload.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>  
> +struct spi_offload_trigger {
> +	struct list_head list;
> +	struct device dev;
> +	/* synchronizes calling ops and driver registration */
> +	struct mutex lock;
> +	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 instances
>   * @dev: Device for devm purposes
> @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
>  	return offload;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> +
> +static void spi_offload_trigger_release(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	guard(mutex)(&trigger->lock);
> +	if (trigger->priv && trigger->ops->release)
> +		trigger->ops->release(trigger->priv);
> +
> +	put_device(&trigger->dev);
> +}
> +
> +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;
> +	bool match = false;
> +	int ret;
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload->provider_dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells", 0, 0,
> +						 &args);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

> +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
I'm not fond of __free usage like this because code could get added above
this line and it wouldn't be obvious that it doesn't release the handle.

Annoying though it is, maybe just do manual release of the fwnode.  Ora
factor out the next chunk to a helper function so you can just put the fwnode after
that is called.


> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +
> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> +		if (trigger->dev.fwnode != args.fwnode)
> +			continue;
> +
> +		match = trigger->ops->match(trigger->priv, type, args.args, args.nargs);
> +		if (match)
> +			break;
> +	}
> +
> +	if (!match)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	guard(mutex)(&trigger->lock);
> +
> +	if (!trigger->priv)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (trigger->ops->request) {
> +		ret = trigger->ops->request(trigger->priv, type, args.args, args.nargs);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	get_device(&trigger->dev);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_release, trigger);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return trigger;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_get);

> +int devm_spi_offload_trigger_register(struct device *dev,
> +				      struct spi_offload_trigger *trigger,
> +				      void *priv)
> +{
> +	int ret;
> +
> +	ret = device_add(&trigger->dev);
> +	if (ret)
> +		return ret;
> +
> +	trigger->priv = priv;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +	list_add_tail(&trigger->list, &spi_offload_triggers);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_unregister, trigger);
I guess there may be more here later in the series, but if not.

	return devm_add_...

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);

On Wed, 2024-10-23 at 15:59 -0500, 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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> In previous versions, we locked the SPI bus when the hardware trigger
> was enabled, but we found this to be too restrictive. In one use case,
> to avoid a race condition, we need to enable the SPI offload via a
> hardware trigger, then write a SPI message to the peripheral to place
> it into a mode that will generate the trigger. If we did it the other
> way around, we could miss the first trigger.
> 
> Another likely use case will be enabling two offloads/triggers at one
> time on the same device, e.g. a read trigger and a write trigger. So
> the exclusive bus lock for a single trigger would be too restrictive in
> this case too.
> 
> So for now, I'm going with Nuno's suggestion to leave any locking up to
> the individual controller driver. If we do find we need something more
> generic in the future, we could add a new spi_bus_lock_exclusive() API
> that causes spi_bus_lock() to fail instead of waiting and add "locked"
> versions of trigger enable functions. This would allow a peripheral to
> claim exclusive use of the bus indefinitely while still being able to
> do any SPI messaging that it needs.
> 
> 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       | 266 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-offload.h |  78 ++++++++++++
>  2 files changed, 344 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index c344cbf50bdb..2a1f9587f27a 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -9,12 +9,26 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/spi/spi-offload.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>  
> +struct spi_offload_trigger {
> +	struct list_head list;
> +	struct device dev;
> +	/* synchronizes calling ops and driver registration */
> +	struct mutex lock;
> +	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 instances
>   * @dev: Device for devm purposes
> @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
>  	return offload;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> +
> +static void spi_offload_trigger_release(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	guard(mutex)(&trigger->lock);
> +	if (trigger->priv && trigger->ops->release)
> +		trigger->ops->release(trigger->priv);
> +
> +	put_device(&trigger->dev);
> +}
> +
> +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;
> +	bool match = false;
> +	int ret;
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
> >provider_dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells", 0, 0,
> +						 &args);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +
> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> +		if (trigger->dev.fwnode != args.fwnode)
> +			continue;
> +
> +		match = trigger->ops->match(trigger->priv, type, args.args,
> args.nargs);
> +		if (match)
> +			break;
> +	}
> +
> +	if (!match)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	guard(mutex)(&trigger->lock);
> +
> +	if (!trigger->priv)
> +		return ERR_PTR(-ENODEV);

This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I would
expect is for methods of the trigger to get a struct spi_offload_trigger opaque
pointer. Then we provide a get_private kind of API for the private data. I guess you
want to avoid that but IMO it makes for neater API instead of getting void pointers.

Another thing is, can the above actually happen? We have the
spi_offload_triggers_lock grabbed and we got a match so the trigger should not be
able to go away (should block on the same lock).

> +
> +	if (trigger->ops->request) {
> +		ret = trigger->ops->request(trigger->priv, type, args.args,
> args.nargs);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	get_device(&trigger->dev);

try_module_get() would also make sense...

...

> 
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_alloc(struct device *dev,
> +				struct spi_offload_trigger_info *info)
> +{
> +	struct spi_offload_trigger *trigger;
> +	int ret;
> +
> +	trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
> +	if (!trigger)
> +		return ERR_PTR(-ENOMEM);
> +
> +	device_initialize(&trigger->dev);

Do we really need the full struct device and the overhead of adding it to the driver
core? AFAICT, we're using it only for refcouting so we could use a plain kref for
that matter. It would make things simpler. Or do you envision an future usecase as
this might matter? Like allowing userspace to set some controls on the trigger (I
would expect to be done through consumers though)?

> +	trigger->dev.parent = info->parent;
> +	trigger->dev.fwnode = info->fwnode;
> +	trigger->dev.of_node = of_node_get(to_of_node(trigger->dev.fwnode));
> +	trigger->dev.of_node_reused = true;
> +	trigger->dev.release = spi_offload_trigger_dev_release;
> +
> +	mutex_init(&trigger->lock);
> +	trigger->ops = info->ops;
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_put, trigger);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = dev_set_name(&trigger->dev, "%s-%d", info->name, info->id);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return trigger;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_alloc);
> +
> +static void spi_offload_trigger_unregister(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	scoped_guard(mutex, &spi_offload_triggers_lock)
> +		list_del(&trigger->list);
> +
> +	guard(mutex)(&trigger->lock);
> +	trigger->priv = NULL;

nit: I guess this is a good as anything else but *ops could also be a good fit to
nullify :)

> +	device_del(&trigger->dev);
> +}
> +
> +int devm_spi_offload_trigger_register(struct device *dev,
> +				      struct spi_offload_trigger *trigger,
> +				      void *priv)
> +{
> +	int ret;
> +
> +	ret = device_add(&trigger->dev);
> +	if (ret)
> +		return ret;
> +
> +	trigger->priv = priv;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +	list_add_tail(&trigger->list, &spi_offload_triggers);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_unregister,
> trigger);
> +	if (ret)
> +		return ret;
> 

return devm_add_action_or_reset()?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);
> diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h
> index 92a557533b83..561cc1fb6f35 100644
> --- a/include/linux/spi/spi-offload.h
> +++ b/include/linux/spi/spi-offload.h
> @@ -22,6 +22,7 @@
>  MODULE_IMPORT_NS(SPI_OFFLOAD);
>  
>  struct device;
> +struct fwnode_handle;
>  struct spi_device;
>  
>  /* Offload can be triggered by external hardware event. */
> @@ -53,6 +54,43 @@ struct spi_offload {
>  	struct spi_device *spi;
>  	/** @priv: provider driver private data */
>  	void *priv;
> +	/** @ops: callbacks for offload support */
> +	const struct spi_offload_ops *ops;
> +};
> +
> +enum spi_offload_trigger_type {
> +	/* Indication from SPI peripheral that data is read to read. */
> +	SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	/* Trigger comes from a periodic source such as a clock. */
> +	SPI_OFFLOAD_TRIGGER_PERIODIC,
> +};
> +
> +struct spi_offload_trigger_periodic {
> +	u64 frequency_hz;
> +};
> +
> +struct spi_offload_trigger_config {
> +	/** @type: type discriminator for union */
> +	enum spi_offload_trigger_type type;
> +	union {
> +		struct spi_offload_trigger_periodic periodic;
> +	};
> +};
> +
> +/**
> + * struct spi_offload_ops - callbacks implemented by offload providers
> + */
> +struct spi_offload_ops {
> +	/**
> +	 * @trigger_enable: Optional callback to enable the trigger for the
> +	 * given offload instance.
> +	 */
> +	int (*trigger_enable)(struct spi_offload *offload);
> +	/**
> +	 * @trigger_disable: Optional callback to disable the trigger for the
> +	 * given offload instance.
> +	 */
> +	void (*trigger_disable)(struct spi_offload *offload);
>  };
>  
>  struct spi_offload *devm_spi_offload_alloc(struct device *dev,
> @@ -61,4 +99,44 @@ struct spi_offload *devm_spi_offload_alloc(struct device *dev,
>  struct spi_offload *devm_spi_offload_get(struct device *dev, struct spi_device
> *spi,
>  					 const struct spi_offload_config *config);
>  
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_get(struct device *dev,
> +			      struct spi_offload *offload,
> +			      enum spi_offload_trigger_type type);
> +int spi_offload_trigger_validate(struct spi_offload_trigger *trigger,
> +				 struct spi_offload_trigger_config *config);
> +int spi_offload_trigger_enable(struct spi_offload *offload,
> +			       struct spi_offload_trigger *trigger,
> +			       struct spi_offload_trigger_config *config);
> +void spi_offload_trigger_disable(struct spi_offload *offload,
> +				 struct spi_offload_trigger *trigger);
> +
> +/* Trigger providers */
> +
> +struct spi_offload_trigger;
> +
> +struct spi_offload_trigger_ops {
> +	bool (*match)(void *priv, enum spi_offload_trigger_type type, u64 *args,
> u32 nargs);
> +	int (*request)(void *priv, enum spi_offload_trigger_type type, u64 *args,
> u32 nargs);
> +	void (*release)(void *priv);
> +	int (*validate)(void *priv, struct spi_offload_trigger_config *config);
> +	int (*enable)(void *priv, struct spi_offload_trigger_config *config);
> +	void (*disable)(void *priv);
> +};

Yeah, as I said... not a fan of the void *priv thing

- Nuno Sá

On 10/24/24 9:04 AM, Nuno Sá wrote:
> On Wed, 2024-10-23 at 15:59 -0500, 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.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> In previous versions, we locked the SPI bus when the hardware trigger
>> was enabled, but we found this to be too restrictive. In one use case,
>> to avoid a race condition, we need to enable the SPI offload via a
>> hardware trigger, then write a SPI message to the peripheral to place
>> it into a mode that will generate the trigger. If we did it the other
>> way around, we could miss the first trigger.
>>
>> Another likely use case will be enabling two offloads/triggers at one
>> time on the same device, e.g. a read trigger and a write trigger. So
>> the exclusive bus lock for a single trigger would be too restrictive in
>> this case too.
>>
>> So for now, I'm going with Nuno's suggestion to leave any locking up to
>> the individual controller driver. If we do find we need something more
>> generic in the future, we could add a new spi_bus_lock_exclusive() API
>> that causes spi_bus_lock() to fail instead of waiting and add "locked"
>> versions of trigger enable functions. This would allow a peripheral to
>> claim exclusive use of the bus indefinitely while still being able to
>> do any SPI messaging that it needs.
>>
>> 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       | 266 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/spi/spi-offload.h |  78 ++++++++++++
>>  2 files changed, 344 insertions(+)
>>
>> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
>> index c344cbf50bdb..2a1f9587f27a 100644
>> --- a/drivers/spi/spi-offload.c
>> +++ b/drivers/spi/spi-offload.c
>> @@ -9,12 +9,26 @@
>>  #include <linux/cleanup.h>
>>  #include <linux/device.h>
>>  #include <linux/export.h>
>> +#include <linux/list.h>
>>  #include <linux/mutex.h>
>> +#include <linux/of.h>
>>  #include <linux/property.h>
>>  #include <linux/spi/spi-offload.h>
>>  #include <linux/spi/spi.h>
>>  #include <linux/types.h>
>>  
>> +struct spi_offload_trigger {
>> +	struct list_head list;
>> +	struct device dev;
>> +	/* synchronizes calling ops and driver registration */
>> +	struct mutex lock;
>> +	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 instances
>>   * @dev: Device for devm purposes
>> @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
>>  	return offload;
>>  }
>>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
>> +
>> +static void spi_offload_trigger_release(void *data)
>> +{
>> +	struct spi_offload_trigger *trigger = data;
>> +
>> +	guard(mutex)(&trigger->lock);
>> +	if (trigger->priv && trigger->ops->release)
>> +		trigger->ops->release(trigger->priv);
>> +
>> +	put_device(&trigger->dev);
>> +}
>> +
>> +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;
>> +	bool match = false;
>> +	int ret;
>> +
>> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
>>> provider_dev),
>> +						 "trigger-sources",
>> +						 "#trigger-source-cells", 0, 0,
>> +						 &args);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
>> +
>> +	guard(mutex)(&spi_offload_triggers_lock);
>> +
>> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
>> +		if (trigger->dev.fwnode != args.fwnode)
>> +			continue;
>> +
>> +		match = trigger->ops->match(trigger->priv, type, args.args,
>> args.nargs);
>> +		if (match)
>> +			break;
>> +	}
>> +
>> +	if (!match)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	guard(mutex)(&trigger->lock);
>> +
>> +	if (!trigger->priv)
>> +		return ERR_PTR(-ENODEV);
> 
> This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I would
> expect is for methods of the trigger to get a struct spi_offload_trigger opaque
> pointer. Then we provide a get_private kind of API for the private data. I guess you
> want to avoid that but IMO it makes for neater API instead of getting void pointers.

I was just trying to save a step of an extra call to get *priv
in each callback implementation, but yeah, no problem to change
it to something more "normal" looking.

> 
> Another thing is, can the above actually happen? We have the
> spi_offload_triggers_lock grabbed and we got a match so the trigger should not be
> able to go away (should block on the same lock).

The problem is that it could have gone away before we took the lock.

It could happen like this:

* Trigger driver registers trigger - sets *priv.
* SPI peripheral driver gets reference to trigger.
* Trigger driver unregisters trigger - removes *priv.
* SPI peripheral tries to call trigger function.

> 
>>
>> +struct spi_offload_trigger
>> +*devm_spi_offload_trigger_alloc(struct device *dev,
>> +				struct spi_offload_trigger_info *info)
>> +{
>> +	struct spi_offload_trigger *trigger;
>> +	int ret;
>> +
>> +	trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
>> +	if (!trigger)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	device_initialize(&trigger->dev);
> 
> Do we really need the full struct device and the overhead of adding it to the driver
> core? AFAICT, we're using it only for refcouting so we could use a plain kref for
> that matter. It would make things simpler. Or do you envision an future usecase as
> this might matter? Like allowing userspace to set some controls on the trigger (I
> would expect to be done through consumers though)?

Agreed. We should not need a device at this point.

> 
>> +	trigger->dev.parent = info->parent;
>> +	trigger->dev.fwnode = info->fwnode;
>> +	trigger->dev.of_node = of_node_get(to_of_node(trigger->dev.fwnode));
>> +	trigger->dev.of_node_reused = true;
>> +	trigger->dev.release = spi_offload_trigger_dev_release;
>> +
>> +	mutex_init(&trigger->lock);
>> +	trigger->ops = info->ops;
>> +
>> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_put, trigger);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	ret = dev_set_name(&trigger->dev, "%s-%d", info->name, info->id);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return trigger;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_alloc);
>> +
>> +static void spi_offload_trigger_unregister(void *data)
>> +{
>> +	struct spi_offload_trigger *trigger = data;
>> +
>> +	scoped_guard(mutex, &spi_offload_triggers_lock)
>> +		list_del(&trigger->list);
>> +
>> +	guard(mutex)(&trigger->lock);
>> +	trigger->priv = NULL;
> 
> nit: I guess this is a good as anything else but *ops could also be a good fit to
> nullify :)

I debated between the two. :-)

But if I change the priv handling like you suggest, I think ops will make
more sense here.

On Thu, 2024-10-24 at 10:02 -0500, David Lechner wrote:
> On 10/24/24 9:04 AM, Nuno Sá wrote:
> > On Wed, 2024-10-23 at 15:59 -0500, 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.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > > 
> > > In previous versions, we locked the SPI bus when the hardware trigger
> > > was enabled, but we found this to be too restrictive. In one use case,
> > > to avoid a race condition, we need to enable the SPI offload via a
> > > hardware trigger, then write a SPI message to the peripheral to place
> > > it into a mode that will generate the trigger. If we did it the other
> > > way around, we could miss the first trigger.
> > > 
> > > Another likely use case will be enabling two offloads/triggers at one
> > > time on the same device, e.g. a read trigger and a write trigger. So
> > > the exclusive bus lock for a single trigger would be too restrictive in
> > > this case too.
> > > 
> > > So for now, I'm going with Nuno's suggestion to leave any locking up to
> > > the individual controller driver. If we do find we need something more
> > > generic in the future, we could add a new spi_bus_lock_exclusive() API
> > > that causes spi_bus_lock() to fail instead of waiting and add "locked"
> > > versions of trigger enable functions. This would allow a peripheral to
> > > claim exclusive use of the bus indefinitely while still being able to
> > > do any SPI messaging that it needs.
> > > 
> > > 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       | 266 ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/spi/spi-offload.h |  78 ++++++++++++
> > >  2 files changed, 344 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> > > index c344cbf50bdb..2a1f9587f27a 100644
> > > --- a/drivers/spi/spi-offload.c
> > > +++ b/drivers/spi/spi-offload.c
> > > @@ -9,12 +9,26 @@
> > >  #include <linux/cleanup.h>
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > +#include <linux/list.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/of.h>
> > >  #include <linux/property.h>
> > >  #include <linux/spi/spi-offload.h>
> > >  #include <linux/spi/spi.h>
> > >  #include <linux/types.h>
> > >  
> > > +struct spi_offload_trigger {
> > > +	struct list_head list;
> > > +	struct device dev;
> > > +	/* synchronizes calling ops and driver registration */
> > > +	struct mutex lock;
> > > +	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 instances
> > >   * @dev: Device for devm purposes
> > > @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device
> > > *dev,
> > >  	return offload;
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> > > +
> > > +static void spi_offload_trigger_release(void *data)
> > > +{
> > > +	struct spi_offload_trigger *trigger = data;
> > > +
> > > +	guard(mutex)(&trigger->lock);
> > > +	if (trigger->priv && trigger->ops->release)
> > > +		trigger->ops->release(trigger->priv);
> > > +
> > > +	put_device(&trigger->dev);
> > > +}
> > > +
> > > +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;
> > > +	bool match = false;
> > > +	int ret;
> > > +
> > > +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
> > > > provider_dev),
> > > +						 "trigger-sources",
> > > +						 "#trigger-source-cells", 0,
> > > 0,
> > > +						 &args);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) =
> > > args.fwnode;
> > > +
> > > +	guard(mutex)(&spi_offload_triggers_lock);
> > > +
> > > +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> > > +		if (trigger->dev.fwnode != args.fwnode)
> > > +			continue;
> > > +
> > > +		match = trigger->ops->match(trigger->priv, type, args.args,
> > > args.nargs);
> > > +		if (match)
> > > +			break;
> > > +	}
> > > +
> > > +	if (!match)
> > > +		return ERR_PTR(-EPROBE_DEFER);
> > > +
> > > +	guard(mutex)(&trigger->lock);
> > > +
> > > +	if (!trigger->priv)
> > > +		return ERR_PTR(-ENODEV);
> > 
> > This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I
> > would
> > expect is for methods of the trigger to get a struct spi_offload_trigger opaque
> > pointer. Then we provide a get_private kind of API for the private data. I guess
> > you
> > want to avoid that but IMO it makes for neater API instead of getting void
> > pointers.
> 
> I was just trying to save a step of an extra call to get *priv
> in each callback implementation, but yeah, no problem to change
> it to something more "normal" looking.

Yeah, I figured that but I guess any of these paths are fastpaths anyways... 
> 
> > 
> > Another thing is, can the above actually happen? We have the
> > spi_offload_triggers_lock grabbed and we got a match so the trigger should not be
> > able to go away (should block on the same lock).
> 
> The problem is that it could have gone away before we took the lock.
> 
> It could happen like this:
> 
> * Trigger driver registers trigger - sets *priv.
> * SPI peripheral driver gets reference to trigger.
> * Trigger driver unregisters trigger - removes *priv.
> * SPI peripheral tries to call trigger function.
> 

Ah I see... we're using scoped_guard() in the unregister path.

- Nuno Sá
>