Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
4 comments:
Commit Message:
Patch Set #1, 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:
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:
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:
Patch Set #1, 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.
To view, visit change 55932. To unsubscribe, or for help writing mail filters, visit settings.