On Tue, 2024-07-23 at 09:30 -0500, David Lechner wrote: > On 7/23/24 2:53 AM, Nuno Sá wrote: > > 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()... > > The problem I see with using spi_bus_lock() in it's current form is that > SPI offload triggers could be enabled indefinitely. So any other devices > on the bus that tried to use the bus and take the lock would essentially > deadlock waiting for the offload user to release the lock. This is why > I added the -BUSY return, to avoid this deadlock. > If someone does not disable the trigger at some point that's a bug. If I understood things correctly, even if someone tries to access the bus will get -EBUSY. But yeah, arguably it's better to get a valid error rather than blocking/sleeping > > > > ... > > > > > > > > + > > > +/** > > > + * 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... > > After reading your other replies, I think I understand what you mean here. > > Are you thinking some kind of `struct spi_offload_trigger` that could be > any kind of trigger (clk, gpio, etc.)? > Yeah, something in that direction... - Nuno Sá