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
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
