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 <[email protected]>
Excellent. > On Wed, 02 Nov 2011 02:44:17 +0100 > Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> Regards, Carl-Daniel -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
