Re: [flashrom] [PATCH] Compile fixes

[Carl-Daniel Hailfinger]

Auf 07.03.2011 14:52, Michael Karcher schrieb:
> Am Montag, den 07.03.2011, 12:10 +0100 schrieb Carl-Daniel Hailfinger:
> > Fix compilation if CONFIG_INTERNAL=no.
> > Fix compilation if everything except CONFIG_SATAMV is no.
> > Do not compile in PCI support for wiki printing if no PCI devices are
> > supported.
>
> I assume you tested the compilation. Your changes look sensible.
> > Note: The flashrom.c hunk is ugly. Suggestions how to solve the ugliness
> > are appreciated.
>
> My suggestion: Remove the braces. Still kind-of ugly, but I also have no
> idea how to avoid that cleanly. An idea would be to define a dummy
> function physmap that does nothing in case we don't need the real
> physmap. But in fact we really want the whole if/snprintf go away, so it
> looks like we have to do the #ifdef stuff.

Done. To be honest, I don't like the "at physical address" message
anymore. We could simply remove the word "physical" and mention the
address and the programmer name. Once external LPC/FWH programmer
support is merged, we will need that anyway. And my long-term goal is to
store programmer info in struct flashchip anyway to allow handling stuff
like multiple flash chips in a Dual BIOS scenario.
> > +#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV >= 1
>
> We have this #if condition twice. Maybe we should define a
> "INCLUDE_PCIDEV" define from this condition in some header file?

Hm. NEED_PCI almost fits the mandate, except that it also is active for
CONFIG_INTERNAL. So yes, your suggestion makes sense. Given the abuse of
NEED_PCI for anything which needs I/O, we should really clean up that mess.

What about
PCIDEV for all PCI-based programmers (not internal)
HWACCESS for programmers needing memory or I/O access
PCI for all programmers using PCI in any fashion
PCIDEV selects PCI
PCI selects HWACCESS

Pick any prefix for those names, e.g. NEED_ or INCLUDE_
> > +#if CONFIG_INTERNAL == 1
> >    if (programmer_table[programmer].map_flash_region == physmap) {
> >    	snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> > } else {
> > } else
> >
> > +#endif
> > { 
> > snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
>
> Acked-by: Michael Karcher

Thanks for your review! I removed the ugliness in flashrom.c and
committed in r1278.

Regards,
Carl-Daniel