Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
2 comments:
File pickit2_spi.c:
Do we need this struct? I guess it would make it easier to extend in the […]
Oh I am so happy that you asked! We haven't discussed this aspect in details, which I was slightly worried about - but now I have an excuse to write a very long comment :)
One thing I want to achieve is to hide data memory management under register_master API, so that a driver (like this one) does not need to calloc in init and free in shutdown. calloc will be in register_master, free will be in programmer_shutdown.
flashrom.c#L662
Looking closer at programmer_shutdown, it runs all shutdown functions with data, and seems like this is the last place in driver lifecycle that needs data, so I thought to free data here after corresponding shutdown function has completed.
programmer_shutdown does not care what the data is, it can run free(data) for everything. "Everything" here is all the places that do register_shutdown, and those are not only spi masters (or not only any masters). So lots of various places, lots of various datas, but for most of that free(data) is perfectly fine, exactly what's needed.
But some places have special data, which is initialised and destroyed in a special way. Like this one does libusb_init + libusb_open_device_with_vid_pid and then libusb_close + libusb_exit. programmer_shutdown has no way to detect, it only has void* as data.
So, to be able to run free(data) for all datas in programmer_shutdown I need to wrap those special datas into a struct, and then it can be allocated and freed in a standard way in the core.
Once I have all shutdown datas calloc-able and free-able, I can actually do a patch which only moves free(data) from individual drivers into programmer_shutdown. And this patch could be independent of register_master patch!
Patch Set #1, Line 343: static struct spi_master spi_master_pickit2 = {
Same and this is something Anastasia and I actively discussed. […]
>>> The per-instance `data` pointer should simply be passed to register_spi_master()
Yes I agree absolutely, you are right, and that's the goal! (I really want to fix the API as soon as it becomes possible.)
But we need data pointer to exist (to be defined) for that.
Another thing is that driver which uses global state "pretends" that it does not need data (gives NULL to register_master and register_shutdown), but it actually needs data. When I go through all of that and remove global state, create data structs, I can see all the variety of data which exists in flashrom and with all this knowledge I can be more confident and make sure new API works for all cases, including edge cases.
I also wanted to say, this has more diffs, and most of them will stay, they are useful with new API: specifically all static functions should not rely on global state, and accept data that they need as arguments.
I can even say, at the beginning I was trying to find the shortest way to fix the broken API - but after looking thought the code I realised that the shortest way is still long. This is fine, will go through everything.
Please tell me if I haven't answered your question. I am very happy to talk more!
To view, visit change 52774. To unsubscribe, or for help writing mail filters, visit settings.