New subject: [PATCH] ichspi: use a variable to distinguish ich generations instead of spi_programmer->type

6 Nov 2011

      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
-- 
http://www.hailfinger.org/

Re: [flashrom] [PATCH] ichspi: use a variable to distinguish ich generations instead of spi_programmer->type