On Mon, 04 Sep 2023 16:14:17 +0200 Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote: > > On Thu, 31 Aug 2023 11:32:54 +0200 > > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > > On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote: > > > > On Fri, 4 Aug 2023 16:53:39 +0200 > > > > Nuno Sa <nuno.sa@xxxxxxxxxx> wrote: > > > > > > > > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx> > > > > > > > > Hi Nuno, > > > > > > > > > > Hi Jonathan, > > > > > > Thanks for the initial review... > > > > > > > > > > > One general comment is that you could have stripped this back a fair bit > > > > for ease of understanding. At this stage we don't care about things > > > > like debug or control of test patterns. Bring those in as extra patches. > > > > > > > > > > Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than > > > needed to > > > kind of showcase how we can properly configure the hdl core to support > > > things > > > (interface calibration) that were very hard to do with the current > > > implementation. > > > I'll make sure to add the minimum needed API to accommodate what we have > > > right now. > > > > > > > I haven't fully gotten my head around the ordering constraints on removal. > > > > Are there other users of the component framework that have similar > > > > problems? > > > > > > > > > > My understanding on the component API is that one should do all the tear > > > down in the > > > .unbind() callback. As usual, I can see some drivers not really doing that. > > > > > > > Also, I don't yet understand how a multiple front end, single backend > > > > setup > > > > would work. Or indeed single front end, multiple backend... Maybe we > > > > don't > > > > need those cases, but if we want this to be useful beyond adi-axi we > > > > probably at least want an outline of how they work. > > > > > > > > > > Indeed we can have multiple (and we have it out of tree) backends on one > > > frontend. > > > Think on an ADC/DAC with fairly complex data path with more than one > > > channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the > > > interface > > > will be connected to an instance of the hdl core (the backend). > > > > That might work out for your case, but not the stm32 one where I think we can > > end > > up with interleaved data from two front ends in the same buffer... > > > > Not sure I'm following this one. But wouldn't that be something specific for > each system (through devicetree)? I haven't tried but I think the same backend > could be used in different frontend devices (using the component API). That is > not really a usecase for me but definitely something that could be supported (if > we need to start doing things like keep enable/disable counters and so on) if it > is a usecase for stm32. If we are going to support both usecases, we just need to figure out what composite devices with N-M backend - frontend look like and make sure that doesn't cause problems. I'd expect the separation between backend instances might reflect data storage on capture but then again that might end up like the many IIO devices for many buffers mess we had before the multiple buffer support was added. Might need enable /disable counters as you say - I'm not quite sure without trying it! > > > > > + * than having to tear everything down in .frontend_close() and > > > > > + * .backend_close() > > > > > > > > That last bit is nasty and will be non obvious to driver authors. > > > > > > > > I wonder if we can come up with some means to make it hard to do. > > > > > > > > > > Yeah, I agree. The alternative is to always bring everything down in > > > .frontend_close() and .backend_close(). But that can also be prone to subtle > > > bugs > > > because it's easy to mess up the ordering when not using devres. > > > > > > So, at this point, I cannot really think on a perfect solution rather than > > > keeping > > > some rules like (assuming we keep the logic we have now): > > > > > > * Using devres on frontend|backend_init() only when .remove() is provided on > > > the > > > driver. > > > * No mixes of devres and .frontend|backend_close() > > > > > > But yeah, would be nice if we could come up with something to make it more > > > obvious to > > > driver authors. > > > > > > > > We might be able to detect that converter_backend_del() and > > > converter_frontend_del() > > > are under devres while no .frontend|backend_close() is being given. I guess > > > that > > > could be a valid indicator of likely misusage. > > > > > > Or even better (but I'm not sure it's doable with the current devres API), > > > detecting > > > that converter_backend_del() or converter_frontend_del() are under devres > > > while more > > > resources are also allocated in our specific opened groups. That would > > > always be a > > > problem (I think) because the only way for the _del() functions to be under > > > devres is > > > if someone added them (from .probe) with devm_add_action() which means that > > > tearing > > > down the aggregate will happen after some resources (which were allocated in > > > the > > > _init() function) are already freed (as even with new groups, devres will > > > remove > > > things on the reverse order). And that would defenitely be problematic. And, > > > in fact, > > > is the whole reason why I have the .del() functions on .remove() (so, > > > tearing down > > > the aggregate device is the first thing to happen and resources are freed in > > > the > > > reverse order they were allocated). > > > > > > > This would actually be very messy and hard to do properly. Concurrency between > the aggregate probing (at the component level) and unbinding (at driver core > level) would be very tricky if doable at all. On top that, we do have > devres_find() but no way to tell if a devres group has resources or not. It > would be easy to add one new API but likely not worth it just for this usecase. > > I also thought about an helper macro to wrap every devm_ call but your below > suggestion is way better and transparent to users. > > > I couldn't work out how to do anything easily and would need some experiments. > > Maybe some 'hidden' devres callbacks and a state flag somewhere. If we > > register > > that very late we can perhaps detect that we entered devres cleanup before > > calling > > expected manual cleanup. I'm thinking have the setup path register a flag > > checking > > callback and the cleanup path set a flag (devres now safe). Then we can at > > least > > make it scream if we end up doing things in wrong way. > > > > Hmm, that might actually be a good idea and something to try. It likely means > having a mutex (I was happy not to have one for now :) but likely inevitable > somewhere down the road) for the flag but it might work. At least we'll be able > to dump a WARN or something if we suspect something is wrong. Careful ordering an atomic might work. > > > > Other thought would be some generic helper macros to use in these type of > > > drivers so > > > a .remove() callback is always added to remove the components. > > I wondered if that could work but it's an ugly macro because needs to deal > > with > > different bus types. > > > > > > We could have a macro per bus type. I'm not really seeing anything different > than platform, spi and i2c. But even like this, it could easily start to get > ugly because of accepted parameters (and any deviation would again mean a > different version per bus type). Another thing that crossed my mind was wrappers > to module_spi_driver() and friends. Maybe that could work. Anyways, as you said > in one of your replies, this is in kernel interface that we can easily > add/change. So, I will defer this to a later point in time. Makes sense J > > - Nuno Sá >
