Am 03.11.2011 01:58 schrieb Stefan Tauner:
The type member is enough most of the time to derive the wanted information, but
- not always (e.g. ich_set_bbar),
- only available after registration, which we want to delay till the end of init, and
- we really want to distinguish between chipset version-grained attributes which are not reflected by the registered programmer.
Hence this patch introduces a new static variable which is set up early by the init functions and allows us to get rid of all "switch (spi_programmer->type)" in ichspi.c. We reuse the enum introduced for descriptor mode for the type of the new variable.
Previously magic numbers were passed by chipset_enable wrappers. Now they use the enumeration items too. To get this working the enum definition had to be moved to programmer.h, which was fixed on the way by adding necessary includes.
Another noteworthy detail: previously we have checked for a valid programmer/ich generation all over the place. I have removed those checks and added one single check in the init method. Calling any function of a programmer without executing the init method first, is undefined behavior.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Excellent.
On Wed, 02 Nov 2011 02:44:17 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
i think it might be a good idea to get rid of the whole switch(spi_programmer->type) pattern and create a file-scope static ich_generation variable instead of using the type member and the ich_generation parameters everywhere.
Absolutely agreed. The only possible generic problem with that approach would be the registration of multiple ICH-style SPI programmers, but unless we see boards with two active ICH-style southbridges I think we can assume the static variable handles it well. (Note that boards with multiple MCP55 southbridges exist, but only one southbridge has an attached flash chip.)
and even then they would have to be from different incompatible generations... i think we are quite safe ;)
There might be an issue for those who want to use flashrom as a standalone tool (e.g. on top of libpayload) where heap allocations for static variables are unwanted, but that's a huge can of worms and I'd rather ignore that issue until either static variables work well in that environment or until someone hacks a way around static variables being a problem there.
the type member is enough most of the time to derive the wanted information, but
- not always (e.g. ich_set_bbar)
- only available after registration, which we want to delay till the end of init.
- we really want to distinguish between chipset version-grained attributes which are not reflected by the registered programmer.
Indeed. So if you could rework the patch to use your static variable suggestion, it would reduce patch size and make the code more readable.
you may want to evaluate that "patch size" argument again... *sigh*
Heh.
i have added the first applicable chipset to each default case for documentation purposes.
Good idea.
Please go ahead. Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel