[flashrom] [PATCH] Register Parallel/LPC/FWH programmers

[Carl-Daniel Hailfinger]

Am 08.09.2011 23:36 schrieb Stefan Tauner:
> On Thu, 08 Sep 2011 02:14:06 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>   
>> RFC/TODO:
>> - Should register_par_programmer(...) be called before or after setting
>>   max_rom_decode.*?
>>     
> why is that not a field in the different programmer structs (yet?)?
>   

Why do you always ask the really hard questions?

Given that the registration for parallel and SPI programmers is pretty
different but max_rom_decode.* is used from common code, it is not
immediately obvious to handle this sanely. Supply it as
register_par_programmer() parameter and extend register_spi_programmer
to accept that parameter as well?


>> - Should register_par_programmer(...) be called before or after
>>   register_shutdown()?
>>     
> like register_spi_programmer (no idea when that is, but consistency is
> the one main argument i can think of atm)
>   

They should be called after register_shutdown, otherwise there is a time
when chip access functions are callable although the programmer has
already been shut down. Will audit the code and change where appropriate.


>> - Is there a better name for register_par_programmer?
>>     
> register_parallel_programmer ofc, and imho it is not too long, because
> it is seldom used, but i don't care that much (due to the same reason).
>   

80 column limit... I think that was one of the reasons we used a shorter
name.

>> - Should max_rom_decode.* be part of the registration?
>>     
> either that or declaration, see question above. if it has to be
> modified (board enables do this it seems...), this can't be done at
> registration (only)...
>   

Same trick as buses_supported. Have a local static (not part of the
official interface) chipset_max_rom_decode variable which can be
modified by chipset/mainboard init, and then use the end result in
registration.


>> - Should map_flash_region/unmap_flash_region be part of the registration?
>>     
> no idea what that does exactly :P
>   

It makes the flash chip accessible. This is essentially physmap for
programmers with memory mapped flash and a no-op for everything else.


>> --- flashrom-register_par_programmer/cli_classic.c	(Revision 1433)
>> +++ flashrom-register_par_programmer/cli_classic.c	(Arbeitskopie)
>> […]
>> +		 flashbuses_to_text(buses_supported));
>>     
> free()!!!
>   

Argh, yes.


> sorry for the lame "review", but i thought better some feedback than
> none at all :)
>   

Thanks, it was helpful, will rework the code and audit registration
ordering.

Regards,
Carl-Daniel

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