Re: [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  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