[flashrom] [PATCH] ichspi: use a variable to distinguish ich generations instead of spi_programmer->type
c-d.hailfinger.devel.2006 at gmx.net
Sun Nov 6 23:56:42 CET 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 at student.tuwien.ac.at>
> On Wed, 02 Nov 2011 02:44:17 +0100
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>>> i think it might be a good idea to get rid of the whole
>>> 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*
> i have added the first applicable chipset to each default case for
> documentation purposes.
Please go ahead.
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
More information about the flashrom