Am 08.09.2011 23:36 schrieb Stefan Tauner: > On Thu, 08 Sep 2011 02:14:06 +0200 > Carl-Daniel Hailfinger <[email protected]> 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/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
