Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

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

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This extends the SPI framework to support hardware triggered offloading.
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_offload_prepare().
> 
> Since the hardware trigger can happen at any time, this means the SPI
> bus must be reserved for exclusive use as long as the hardware trigger
> is enabled. Since a hardware trigger could be enabled indefinitely,
> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
> otherwise this could cause deadlocks. So we introduce a new flag so that
> any attempt to lock or use the bus will fail with -EBUSY as long as the
> hardware trigger is enabled.
> 
> Peripheral drivers may need to control the trigger source as well. For
> this, we introduce a new spi_offload_hw_trigger_get_clk() function that
> can be used to get a clock trigger source. This is intended for used
> by ADC drivers that will use the clock to control the sample rate.
> Additional functions to get other types of trigger sources could be
> added in the future.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
> 
> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
> check the return value. All callers will need to be updated first before
> this can be merged.
> 
> 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".
> 
> Mark suggested that the standard SPI APIs should be aware that the
> hardware trigger is enabled. So I've added some locking for this. Nuno
> suggested that this might be overly strict though, and that we should
> let each individual controller driver decide what to do. For our use
> case though, I think we generally are going to have a single peripheral
> on the SPI bus, so this seems like a reasonable starting place anyway.
> ---

How explicitly do we want to be about returning errors? It seems that if the
trigger is enabled we can't anything else on the controller/offload_engine so we
could very well just hold the controller lock when enabling the trigger and 
release it when disabling it. Pretty much the same behavior as spi_bus_lock()...

...

> 
> +
> +/**
> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
> + * @spi: SPI device
> + * @id: Function ID if SPI device uses more than one offload or NULL.
> + *
> + * The caller is responsible for calling clk_put() on the returned clock.
> + *
> + * Return: The clock for the offload trigger, or negative error code
> + */
> +static inline
> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
> *id)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +
> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
> +}
> 

It would be nice if we could have some kind of spi abstraction...

- Nuno Sá