On Tue, 2024-07-23 at 08:48 -0500, David Lechner wrote: > On 7/23/24 2:35 AM, Nuno Sá wrote: > > Hi David, > > > > > > I think there are things that we need to better figure but things are improving > > IMO :) > > > > I'm only doing a very superficial review since I need to better look at the > > patches... > > > > But one thing that I do want to mention is a scenario (another funny one...) > > that I've discussing and that might be a reality. Something like: > > > > +-------------------------------+ +------------------+ > > > | | | > > > SOC/FPGA | | ADC | > > > | | | > > > +---------------+ | | | > > > | SPI PS Zynq |============== SPI Bus | > > > +---------------+ | | | > > > | | | > > > +---------------------+ | | | > > > | AXI SPI Engine | | | | > > > | v================ DATA Bus | > > > | v | | | | > > > | +---------------+ | | | | > > > | | Offload 0 | | | +------------------+ > > > | | RX DATA OUT | | | > > > | | TRIGGER IN | | | > > > | +---------------+ | | > > > | > > +-------------------------------+ > > > > From the above, the spi controller for typical register access/configuration is > > not the spi_enigine and the offload core is pretty much only used for streaming > > data. So I think your current approach would not work with this usecase. In your > > first RFC you had something overly complicated (IMHO) but you already had a > > concept that maybe it's worth looking at again. I mean having a spi_offload > > object that could describe it and more importantly have a provider/consumer > > logic where a spi consumer (or maybe even something else?) can get()/put() an > > offload object to stream data. > > Although it isn't currently implemented this way in the core SPI code, I think > the DT bindings proposed in this version of the series would allow for this. > The offload provider is just the one with the #spi-offload-cells and doesn't > necessarily have to be the parent of the SPI peripheral. > I get that but the thing is that when the spi device consuming an offload service is under the same controller providing that service we have guarantees regarding lifetimes. In case the offload is another controller, those guarantees are not true anymore and we need to make sure to handle things correctly (like the controller going away under our feet). That's why a proper provider/consumer interface is needed and I think that's a significant shift from what we have now? Out of my head (the minimal interface): spi_controller_offload_register() - we may need a list of register offloads in the controller struct. (devm_)spi_offload_get() - and this one could return a nice struct spi_offload abstraction to pass to the other APIs spi_offload_put() Or for starters we may skip the registering in the core and have it per driver but if we assume more controlles will have this we might just make it common code already. Just some ideas... - Nuno Sá
