Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 )
Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ......................................................................
Patch Set 1:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55932/comment/b8cca1cc_04e78950 PS1, Line 14: adding another argument to register_spi_master. The new version seems better! If the shutdown function can be decided at compile time, just like any other master function, this makes things much cleaner.
Patchset:
PS1:
I wasn't sure which one of the structs is the right place for the shutdown function, and even more, why there are two structs describing the same thing?
I guess `master` is the right place as that is what the `data` is tied to. They are not the same thing. And there is even a 1:n relation. A programmer driver can register multiple masters. A most intricate example is the `internal` programmer. It can add multiple masters for the chipset alone; because there are different buses where the BIOS flash could be connected. It could even be indirectly connected e.g. through a super-i/o chip on the LPC bus, so `internal` might add a master for the super-i/o as well. `linux_mtd` is also considered part of the `internal` programmer on plat- forms where that combination is possible. If one thinks it through, even the EC masters should be part of it (something that was missed in the chromium fork, it seems).
Especially now, when both structs are in the same file. Both in the same file, however they are separate and cannot share information between each other.
Only in the same file for the "few" masters that you have been looking at so far ;)
Programmer_entry has init function, so I was thinking of adding a shutdown there as well. But then I cannot access it from register_spi_master. Alternatively, I thought to add `register_shutdown(programmer->shutdown, data)` after flashrom.c#159, but data is not available on that level.
It's a hierarchy. In fact, we already have a programmer shutdown. Just the same for all programmers which simply calls all the master's shutdown functions. This may change in the future. Same as it was for the masters, there is global state cluttered over programmers. If that is to change, we'll need per-programmer shutdown pointers as well. More work in that direction will not cease to pop up soon.
The libflashrom API provides a hint: `struct flashrom_programmer`. Where is that defined? Ooops, nobody did it (yet) ;) This should probably con- tain a pointer to the `struct programmer_entry` and one to its runtime `data`.
And if that would ever be done: There is also a library-level context missing. That's not even declared yet (API needs to change /o).
Data is needed for shutdown, but also data is something that is re-created for every new run. With that, maybe adding shutdown into spi_master is ok? Data is there already.
+1
Another thing I don't know, why data is added into spi/par/opaque master struct (3 times), and not in registered_master struct. I added shutdown function where the data is.
Now that we pass `data` to the registration functions, it might be easy to move it to `registered_master`.
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/55932/comment/461477af_084711c5 PS1, Line 243: if (register_spi_master(&spi_master_linux, spi_data)) : return 1; /* core does cleanup */ : return 0; : Or just
return register_spi_master(&spi_master_linux, spi_data);
File programmer.h:
https://review.coreboot.org/c/flashrom/+/55932/comment/59c7b8fb_8388a9aa PS1, Line 359: shutdown
I am not sure how others feel but I would vote for this to be ideally called `deinit` and logically […]
I think we'll need shutdown functions for both eventually. Starting with the masters seems like a good next step.
-
Nico Huber (Code Review)