[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