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

[Carl-Daniel Hailfinger]

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>

Excellent.

> 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
>>> 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 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/