Re: [flashrom] [PATCH] Kill globals, initialize programmer-related variables explicitly

[Michael Karcher]

Am Freitag, den 02.07.2010, 04:05 +0200 schrieb Carl-Daniel Hailfinger:
> Kill global variables, constants and functions if local scope suffices.
> Constify variables where possible.

Great!
> Some of the const pointer to const changes may be excessive. Comments
> welcome.

I don't think that they are excessive. Adding const to run-time
constants seems like a good idea to me.
> /* ichspi.c */
>  extern int ichspi_lock;
>  extern uint32_t ichspi_bbar;
> +extern void *ich_spibar;

Do we really need this global, if...
> Index: flashrom-explicit_init/chipset_enable.c
>
> --- flashrom-explicit_init/chipset_enable.c	(Revision 1066)
> +++ flashrom-explicit_init/chipset_enable.c	(Arbeitskopie)
> @@ -417,10 +417,10 @@
>    /* Do we really need no write enable? */
>    mmio_base = (pci_read_long(dev, 0xbc)) << 8;
>    msg_pdbg("MMIO base at = 0x%x\n", mmio_base);
> spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70);
>
> ich_spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70);
>
> msg_pdbg("0x6c: 0x%04x     (CLOCK/DEBUG)\n",

[yada, yada, yada]
>      mmio_readw(spibar + 0x6c));
>      mmio_readw(ich_spibar + 0x6c));
> msg_pdbg("0xB0: 0x%08x (FDOC)\n",
> 	     mmio_readl(spibar + 0xB0));
> 	     mmio_readl(ich_spibar + 0xB0));
> if (tmp2 & (1 << 15)) {
> 	msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> 	ichspi_lock = 1;

...we move all this stuff in a new, non-static function in ichspi.c?
> for (i = shutdown_fn_count - 1; i >= 0; i--)
>    	shutdown_fn[i].func(shutdown_fn[i].data);
> /* FIXME: Clear the shutdown function array on shutdown or startup? */
> return programmer_table[programmer].shutdown();
>
> }

use shutdown_fn_count as loop variable, instead of an extra i, like
while (shutdown_fn_count > 0) {
    int j = --shutdown_fn_count;
    shutdown_fn[j].func(shutdown_fn[j].data);
}

This makes sure that even if shutdown_fn[j] calls exit and we atexit()
our generic shutdown function, no function will be called twice.
> +/* programmer_param is programmer-specific, but it MUST NOT be initialized in
> programmer_init() because it is initialized in the command line parser.
> */
>
> char *programmer_param = NULL;

So why not make this a parameter of programmer_init() ?

Remaining stuff seems fine.

Regards,
  Michael Karcher