Hi,
the whitespace and coding-style fixes in r1397 changed a few places which should have been left untouched or changed differently for various reasons: - Mixing uninitialized and initialized local variables leads to confusion. - ft2232_spi error cases should have gotten some error handling, and that's the reason the curly braces were there. - Depending on whom you ask about the C99 standard, a comma after the last element of an array initializer will cause another empty array element to be allocated, and that increases array size by 1 and may break some code which assumes certain fields to be nonzero. - Sometimes the introduced linebreaks look worse than before. - Fixing typos/wording in some places would have been nice given that those places were touched anyway. - If we insist on spelling "ASUS" all uppercase because the vendor said so, we have to spell "LPC Bridge" with an uppercase B because the datasheet said so. - #if/#else/#endif are sometimes hard to follow, and a comment at the #endif can improve readability a lot.
Comments welcome.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-revert_r1397_partially/it87spi.c =================================================================== --- flashrom-revert_r1397_partially/it87spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/it87spi.c (Arbeitskopie) @@ -203,7 +203,8 @@
int init_superio_ite(void) { - int i, ret = 0; + int i; + int ret = 0;
for (i = 0; i < superio_count; i++) { if (superios[i].vendor != SUPERIO_VENDOR_ITE) Index: flashrom-revert_r1397_partially/cli_classic.c =================================================================== --- flashrom-revert_r1397_partially/cli_classic.c (Revision 1402) +++ flashrom-revert_r1397_partially/cli_classic.c (Arbeitskopie) @@ -104,13 +104,14 @@ struct flashchip flashes[3]; struct flashchip *fill_flash; const char *name; - int startchip = 0, chipcount = 0, namelen, opt, option_index = 0; - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; - int dont_verify_it = 0, list_supported = 0, force = 0; + int namelen, opt, i; + int startchip = 0, chipcount = 0, option_index = 0, force = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int operation_specified = 0, i, ret = 0; + int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + int dont_verify_it = 0, list_supported = 0, operation_specified = 0; + int ret = 0;
static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; static const struct option long_options[] = { Index: flashrom-revert_r1397_partially/dmi.c =================================================================== --- flashrom-revert_r1397_partially/dmi.c (Revision 1402) +++ flashrom-revert_r1397_partially/dmi.c (Arbeitskopie) @@ -196,7 +196,8 @@ */ static int dmi_compare(const char *value, const char *pattern) { - int anchored = 0, patternlen; + int anchored = 0; + int patternlen;
msg_pspew("matching %s against %s\n", value, pattern); /* The empty string is part of all strings! */ Index: flashrom-revert_r1397_partially/dediprog.c =================================================================== --- flashrom-revert_r1397_partially/dediprog.c (Revision 1402) +++ flashrom-revert_r1397_partially/dediprog.c (Arbeitskopie) @@ -496,7 +496,8 @@ static int parse_voltage(char *voltage) { char *tmp = NULL; - int i, millivolt, fraction = 0; + int i; + int millivolt = 0, fraction = 0;
if (!voltage || !strlen(voltage)) { msg_perr("Empty voltage= specified.\n"); @@ -574,7 +575,8 @@ { struct usb_device *dev; char *voltage; - int millivolt = 3500, ret; + int millivolt = 3500; + int ret;
msg_pspew("%s\n", __func__);
Index: flashrom-revert_r1397_partially/it85spi.c =================================================================== --- flashrom-revert_r1397_partially/it85spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/it85spi.c (Arbeitskopie) @@ -84,15 +84,15 @@ static int it85xx_scratch_rom_reenter = 0;
/* This function will poll the keyboard status register until either - * an expected value shows up, or - * timeout reaches. + * an expected value shows up, or the timeout is reached. + * timeout is in usec. * - * Returns: 0 -- the expected value has shown. - * 1 -- timeout reached. + * Returns: 0 -- the expected value showed up. + * 1 -- timeout. */ static int wait_for(const unsigned int mask, const unsigned int expected_value, - const int timeout /* in usec */, const char *error_message, - const char *function_name, const int lineno) + const int timeout, const char * error_message, + const char * function_name, const int lineno) { int time_passed;
@@ -317,8 +317,9 @@ int i;
it85xx_enter_scratch_rom(); - /* exit scratch ROM ONLY when programmer shuts down. Otherwise, the - * temporary flash state may halt EC. */ + /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the + * temporary flash state may halt the EC. + */
#ifdef LPC_IO INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); Index: flashrom-revert_r1397_partially/buspirate_spi.c =================================================================== --- flashrom-revert_r1397_partially/buspirate_spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/buspirate_spi.c (Arbeitskopie) @@ -109,7 +109,7 @@ {"2.6M", 0x5}, {"4M", 0x6}, {"8M", 0x7}, - {NULL, 0x0}, + {NULL, 0x0} };
static int buspirate_spi_shutdown(void *data) @@ -150,9 +150,11 @@ int buspirate_spi_init(void) { unsigned char buf[512]; - int ret = 0, i, spispeed = 0x7; char *dev = NULL; char *speed = NULL; + int spispeed = 0x7; + int ret = 0; + int i;
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { Index: flashrom-revert_r1397_partially/physmap.c =================================================================== --- flashrom-revert_r1397_partially/physmap.c (Revision 1402) +++ flashrom-revert_r1397_partially/physmap.c (Arbeitskopie) @@ -76,7 +76,7 @@
mi.address = phys_addr; mi.size = len; - ret = __dpmi_physical_address_mapping (&mi); + ret = __dpmi_physical_address_mapping(&mi);
if (ret != 0) return ERROR_PTR; Index: flashrom-revert_r1397_partially/ft2232_spi.c =================================================================== --- flashrom-revert_r1397_partially/ft2232_spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/ft2232_spi.c (Arbeitskopie) @@ -246,17 +246,21 @@ ftdic->error_str); }
- if (ftdi_usb_reset(ftdic) < 0) + if (ftdi_usb_reset(ftdic) < 0) { msg_perr("Unable to reset FTDI device\n"); + }
- if (ftdi_set_latency_timer(ftdic, 2) < 0) + if (ftdi_set_latency_timer(ftdic, 2) < 0) { msg_perr("Unable to set latency timer\n"); + }
- if (ftdi_write_data_set_chunksize(ftdic, 256)) + if (ftdi_write_data_set_chunksize(ftdic, 256)) { msg_perr("Unable to set chunk size\n"); + }
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) + if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { msg_perr("Unable to set bitmode to SPI\n"); + }
if (clock_5x) { msg_pdbg("Disable divide-by-5 front stage\n"); @@ -323,7 +327,8 @@ struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */ - int i = 0, ret = 0, failed = 0, bufsize; + int i = 0, ret = 0, failed = 0; + int bufsize; static int oldbufsize = 0;
if (writecnt > 65536 || readcnt > 65536) Index: flashrom-revert_r1397_partially/chipset_enable.c =================================================================== --- flashrom-revert_r1397_partially/chipset_enable.c (Revision 1402) +++ flashrom-revert_r1397_partially/chipset_enable.c (Arbeitskopie) @@ -309,8 +309,9 @@ static int enable_flash_ich_dc(struct pci_dev *dev, const char *name) { uint32_t fwh_conf; + int i, tmp; char *idsel = NULL; - int i, tmp, max_decode_fwh_idsel = 0, max_decode_fwh_decode = 0; + int max_decode_fwh_idsel = 0, max_decode_fwh_decode = 0; int contiguous = 1;
idsel = extract_programmer_param("fwh_idsel"); @@ -1028,8 +1029,8 @@ flashbase = parx << 12; } } else { - msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. " - "Assuming flash at 4G\n"); + msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. Assuming " + "flash at 4G.\n"); }
/* 4. Clean up */ Index: flashrom-revert_r1397_partially/flashrom.c =================================================================== --- flashrom-revert_r1397_partially/flashrom.c (Revision 1402) +++ flashrom-revert_r1397_partially/flashrom.c (Arbeitskopie) @@ -600,8 +600,7 @@ size_t size = flash->total_size * 1024; /* Flash registers live 4 MByte below the flash. */ /* FIXME: This is incorrect for nonstandard flashbase. */ - flash->virtual_registers = (chipaddr)programmer_map_flash_region( - "flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); + flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); }
int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len) @@ -753,8 +752,9 @@ int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, const char *message) { - int i, ret = 0, failcount = 0; + int i; uint8_t *readbuf = malloc(len); + int ret = 0, failcount = 0;
if (!len) goto out_free; @@ -832,7 +832,8 @@ */ int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran) { - int result = 0, i, j, limit; + int result = 0; + int i, j, limit;
switch (gran) { case write_gran_1bit: @@ -898,7 +899,8 @@ static int get_next_write(uint8_t *have, uint8_t *want, int len, int *first_start, enum write_granularity gran) { - int need_write = 0, rel_start = 0, first_len = 0, i, limit, stride; + int need_write = 0, rel_start = 0, first_len = 0; + int i, limit, stride;
switch (gran) { case write_gran_1bit: @@ -1326,7 +1328,8 @@ */ static int selfcheck_eraseblocks(const struct flashchip *flash) { - int i, j, k, ret = 0; + int i, j, k; + int ret = 0;
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { unsigned int done = 0; @@ -1451,7 +1454,8 @@ void *param1, void *param2) { int i, j; - unsigned int start = 0, len; + unsigned int start = 0; + unsigned int len; struct block_eraser eraser = flash->block_erasers[erasefunction];
for (i = 0; i < NUM_ERASEREGIONS; i++) { @@ -1603,8 +1607,10 @@ void list_programmers_linebreak(int startcol, int cols, int paren) { const char *pname; - int pnamelen, remaining = 0, firstline = 1, i; + int pnamelen; + int remaining = 0, firstline = 1; enum programmer p; + int i;
for (p = 0; p < PROGRAMMER_INVALID; p++) { pname = programmer_table[p].name; @@ -1737,7 +1743,7 @@ msg_gerr("Known laptops table does not exist!\n"); ret = 1; } -#endif +#endif // CONFIG_INTERNAL == 1 return ret; }
Index: flashrom-revert_r1397_partially/board_enable.c =================================================================== --- flashrom-revert_r1397_partially/board_enable.c (Revision 1402) +++ flashrom-revert_r1397_partially/board_enable.c (Arbeitskopie) @@ -179,7 +179,7 @@ static const struct winbond_port w83627hf[3] = { UNIMPLEMENTED_PORT, {w83627hf_port2_mux, 0x08, 0, 0xF0}, - UNIMPLEMENTED_PORT, + UNIMPLEMENTED_PORT };
static const struct winbond_mux w83627ehf_port2_mux[8] = { @@ -190,7 +190,7 @@ {0x2A, 0x01, 0x01}, /* or keyboard/mouse interface */ {0x2A, 0x01, 0x01}, {0x2A, 0x01, 0x01}, - {0x2A, 0x01, 0x01}, + {0x2A, 0x01, 0x01} };
static const struct winbond_port w83627ehf[6] = { @@ -199,7 +199,7 @@ UNIMPLEMENTED_PORT, UNIMPLEMENTED_PORT, UNIMPLEMENTED_PORT, - UNIMPLEMENTED_PORT, + UNIMPLEMENTED_PORT };
static const struct winbond_mux w83627thf_port4_mux[8] = { @@ -210,7 +210,7 @@ {0x2D, 0x10, 0x10}, /* or PWROK */ {0x2D, 0x20, 0x20}, /* or suspend LED */ {0x2D, 0x40, 0x40}, /* or panel switch input */ - {0x2D, 0x80, 0x80}, /* or panel switch output */ + {0x2D, 0x80, 0x80} /* or panel switch output */ };
static const struct winbond_port w83627thf[5] = { @@ -218,7 +218,7 @@ UNIMPLEMENTED_PORT, /* GPIO2 */ UNIMPLEMENTED_PORT, /* GPIO3 */ {w83627thf_port4_mux, 0x09, 1, 0xF4}, - UNIMPLEMENTED_PORT, /* GPIO5 */ + UNIMPLEMENTED_PORT /* GPIO5 */ };
static const struct winbond_chip winbond_chips[] = { @@ -812,7 +812,7 @@ { struct pci_dev *dev;
- dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA bridge. */ + dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA Bridge. */ if (!dev) { msg_perr("\nERROR: NVIDIA nForce4 ISA bridge not found.\n"); return -1; @@ -860,7 +860,7 @@ return -1; }
- /* First, check the ISA bridge */ + /* First, check the ISA Bridge */ dev = pci_dev_find_vendorclass(0x10DE, 0x0601); switch (dev->device_id) { case 0x0030: /* CK804 */ @@ -1101,7 +1101,7 @@ struct pci_dev *dev; uint32_t reg;
- dev = pci_dev_find(0x1002, 0x4372); /* AMD SMBus controller */ + dev = pci_dev_find(0x1002, 0x4372); /* AMD SMBus Controller */ if (!dev) { msg_perr("\nERROR: AMD SMBus Controller (0x4372) not found.\n"); return -1; @@ -1128,8 +1128,8 @@ struct pci_dev *dev; uint32_t tmp, base;
- /* GPPO {0,8,27,28,30} are always available */ - static const uint32_t nonmuxed_gpos = 0x58000101; + /* GPO{0,8,27,28,30} are always available. */ + static const uint32_t nonmuxed_gpos = 0x58000101;
static const struct {unsigned int reg, mask, value; } piix4_gpo[] = { {0}, @@ -1178,10 +1178,9 @@ }
if ((((1 << gpo) & nonmuxed_gpos) == 0) && - (pci_read_word(dev, piix4_gpo[gpo].reg) - & piix4_gpo[gpo].mask) != piix4_gpo[gpo].value) { - msg_perr("\nERROR: PIIX4 GPO%d not programmed for output.\n", - gpo); + ((pci_read_word(dev, piix4_gpo[gpo].reg) & piix4_gpo[gpo].mask) != + piix4_gpo[gpo].value)) { + msg_perr("\nERROR: PIIX4 GPO%d not programmed for output.\n", gpo); return -1; }
@@ -1317,7 +1316,7 @@ /* libpci before version 2.2.4 does not store class info. */ device_class = pci_read_word(dev, PCI_CLASS_DEVICE); if ((dev->vendor_id == 0x8086) && - (device_class == 0x0601)) { /* ISA bridge */ + (device_class == 0x0601)) { /* ISA Bridge */ /* Is this device in our list? */ for (i = 0; intel_ich_gpio_table[i].id; i++) if (dev->device_id == intel_ich_gpio_table[i].id) @@ -1329,7 +1328,7 @@ }
if (!dev) { - msg_perr("\nERROR: No known Intel LPC bridge found.\n"); + msg_perr("\nERROR: No Known Intel LPC Bridge found.\n"); return -1; }
@@ -1349,12 +1348,12 @@ allowed = (intel_ich_gpio_table[i].bank2 >> (gpio - 64)) & 0x01;
if (!allowed) { - msg_perr("\nERROR: This Intel LPC bridge does not allow" - " setting GPIO%02d\n", gpio); + msg_perr("\nERROR: This Intel LPC Bridge does not allow" + " setting GPIO%02d\n", gpio); return -1; }
- msg_pdbg("\nIntel ICH LPC bridge: %sing GPIO%02d.\n", + msg_pdbg("\nIntel ICH LPC Bridge: %sing GPIO%02d.\n", raise ? "Rais" : "Dropp", gpio);
if (gpio < 32) { @@ -1373,7 +1372,7 @@ if (dev->device_id > 0x2800) { tmp = INL(base); if (!(tmp & (1 << gpio))) { - msg_perr("\nERROR: This Intel LPC bridge" + msg_perr("\nERROR: This Intel LPC Bridge" " does not allow setting GPIO%02d\n", gpio); return -1; @@ -1405,7 +1404,7 @@ if (dev->device_id > 0x2800) { tmp = INL(base + 30); if (!(tmp & (1 << gpio))) { - msg_perr("\nERROR: This Intel LPC bridge" + msg_perr("\nERROR: This Intel LPC Bridge" " does not allow setting GPIO%02d\n", gpio + 32); return -1; @@ -1434,7 +1433,7 @@
tmp = INL(base + 40); if (!(tmp & (1 << gpio))) { - msg_perr("\nERROR: This Intel LPC bridge does " + msg_perr("\nERROR: This Intel LPC Bridge does " "not allow setting GPIO%02d\n", gpio + 64); return -1; } @@ -2091,8 +2090,8 @@ * Match boards on coreboot table gathered vendor and part name. * Require main PCI IDs to match too as extra safety. */ -static const struct board_pciid_enable *board_match_coreboot_name( - const char *vendor, const char *part) +static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor, + const char *part) { const struct board_pciid_enable *board = board_pciid_enables; const struct board_pciid_enable *partmatch = NULL; @@ -2144,8 +2143,7 @@ * Match boards on PCI IDs and subsystem IDs. * Second set of IDs can be main only or missing completely. */ -const static struct board_pciid_enable *board_match_pci_card_ids( - enum board_match_phase phase) +const static struct board_pciid_enable *board_match_pci_card_ids(enum board_match_phase phase) { const struct board_pciid_enable *board = board_pciid_enables;
On Wed, 03 Aug 2011 00:07:14 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Index: flashrom-revert_r1397_partially/ft2232_spi.c
--- flashrom-revert_r1397_partially/ft2232_spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/ft2232_spi.c (Arbeitskopie) @@ -246,17 +246,21 @@ ftdic->error_str); }
- if (ftdi_usb_reset(ftdic) < 0)
- if (ftdi_usb_reset(ftdic) < 0) { msg_perr("Unable to reset FTDI device\n");
- }
- if (ftdi_set_latency_timer(ftdic, 2) < 0)
- if (ftdi_set_latency_timer(ftdic, 2) < 0) { msg_perr("Unable to set latency timer\n");
- }
- if (ftdi_write_data_set_chunksize(ftdic, 256))
- if (ftdi_write_data_set_chunksize(ftdic, 256)) { msg_perr("Unable to set chunk size\n");
- }
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0)
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { msg_perr("Unable to set bitmode to SPI\n");
- }
those could be skipped until we really need them. but since you have done the work already i think it is ok.
Index: flashrom-revert_r1397_partially/chipset_enable.c […] @@ -1028,8 +1029,8 @@ flashbase = parx << 12; } } else {
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. ""Assuming flash at 4G\n");
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. Assuming ""flash at 4G.\n");
i also like breaking full sentences like it was done here previously. filling the whole line just because we can does not mean it is useful. reading one sentence per line is much more natural and as long as the line count does not change i prefer it.
Index: flashrom-revert_r1397_partially/board_enable.c […]
- dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA bridge. */
- dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA Bridge. */
imo it should be bridge. there is all kind of crap in datasheets. this includes orthographic nonsense like this. but we have our own quirks too... so i dont really care :)
if (!dev) { msg_perr("\nERROR: NVIDIA nForce4 ISA bridge not found.\n"); return -1; @@ -860,7 +860,7 @@ return -1; }
- /* First, check the ISA bridge */
- /* First, check the ISA Bridge */
First, check (or look) _for_ the ISA [Bb]ridge?
[…] if (!dev) {
msg_perr("\nERROR: No known Intel LPC bridge found.\n");
msg_perr("\nERROR: No Known Intel LPC Bridge found.\n");
well we can discuss (or not) about [Bb]ridge, but that 'Known' slipped in i hope? :)
[…]
-static const struct board_pciid_enable *board_match_coreboot_name(
const char *vendor, const char *part)+static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor,
const char *part)
both ugly :)
some comments apply of course to all other instances of the discussed problem too.
Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Am 03.08.2011 01:27 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 00:07:14 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Index: flashrom-revert_r1397_partially/ft2232_spi.c
--- flashrom-revert_r1397_partially/ft2232_spi.c (Revision 1402) +++ flashrom-revert_r1397_partially/ft2232_spi.c (Arbeitskopie) @@ -246,17 +246,21 @@ ftdic->error_str); }
- if (ftdi_usb_reset(ftdic) < 0)
- if (ftdi_usb_reset(ftdic) < 0) { msg_perr("Unable to reset FTDI device\n");
- }
- if (ftdi_set_latency_timer(ftdic, 2) < 0)
- if (ftdi_set_latency_timer(ftdic, 2) < 0) { msg_perr("Unable to set latency timer\n");
- }
- if (ftdi_write_data_set_chunksize(ftdic, 256))
- if (ftdi_write_data_set_chunksize(ftdic, 256)) { msg_perr("Unable to set chunk size\n");
- }
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0)
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { msg_perr("Unable to set bitmode to SPI\n");
- }
those could be skipped until we really need them. but since you have done the work already i think it is ok.
Index: flashrom-revert_r1397_partially/chipset_enable.c […] @@ -1028,8 +1029,8 @@ flashbase = parx << 12; } } else {
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. ""Assuming flash at 4G\n");
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. Assuming ""flash at 4G.\n");i also like breaking full sentences like it was done here previously. filling the whole line just because we can does not mean it is useful. reading one sentence per line is much more natural and as long as the line count does not change i prefer it.
OK. My point was about adding a . at the end of the second sentence.
Index: flashrom-revert_r1397_partially/board_enable.c […]
- dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA bridge. */
- dev = pci_dev_find(0x10DE, 0x0050); /* NVIDIA CK804 ISA Bridge. */
imo it should be bridge. there is all kind of crap in datasheets. this includes orthographic nonsense like this. but we have our own quirks too... so i dont really care :)
I think Uwe was the one who wanted "ASUS" instead of "Asus", so I thought the "Bridge"->"bridge" conversion was an unintended mistake on his side. Then again, I don't really care about capitalization here.
Uwe?
if (!dev) { msg_perr("\nERROR: NVIDIA nForce4 ISA bridge not found.\n"); return -1; @@ -860,7 +860,7 @@ return -1; }
- /* First, check the ISA bridge */
- /* First, check the ISA Bridge */
First, check (or look) _for_ the ISA [Bb]ridge?
/* Check for the ISA bridge first. */
[…] if (!dev) {
msg_perr("\nERROR: No known Intel LPC bridge found.\n");
msg_perr("\nERROR: No Known Intel LPC Bridge found.\n");well we can discuss (or not) about [Bb]ridge, but that 'Known' slipped in i hope? :)
I was reading though the reversed patch and missed that, thanks for the hint.
[…]
-static const struct board_pciid_enable *board_match_coreboot_name(
const char *vendor, const char *part)+static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor,
const char *part)both ugly :)
Indeed. Unfortunately I didn't find any beautiful solution which fit in 80 columns. The second line should be moved a bit to the right, but then we violate the 80 column limit even more. Ideas anyone?
some comments apply of course to all other instances of the discussed problem too.
Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Thanks for the review.
I'l wait a bit so Uwe has a chance to comment as well.
Regards, Carl-Daniel
On Wed, 03 Aug 2011 01:51:37 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 03.08.2011 01:27 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 00:07:14 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Index: flashrom-revert_r1397_partially/chipset_enable.c […] @@ -1028,8 +1029,8 @@ flashbase = parx << 12; } } else {
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. ""Assuming flash at 4G\n");
msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. Assuming ""flash at 4G.\n");i also like breaking full sentences like it was done here previously. filling the whole line just because we can does not mean it is useful. reading one sentence per line is much more natural and as long as the line count does not change i prefer it.
OK. My point was about adding a . at the end of the second sentence.
ah! missed that completely :)
-static const struct board_pciid_enable *board_match_coreboot_name(
const char *vendor, const char *part)+static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor,
const char *part)both ugly :)
Indeed. Unfortunately I didn't find any beautiful solution which fit in 80 columns. The second line should be moved a bit to the right, but then we violate the 80 column limit even more. Ideas anyone?
yes, violate the §$&§$ 80 column limit (scnr :) the function could be renamed... and board_pciid_enable is also a very long name for a struct that's used a lot.
Am 03.08.2011 02:27 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 01:51:37 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 03.08.2011 01:27 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 00:07:14 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
-static const struct board_pciid_enable *board_match_coreboot_name(
const char *vendor, const char *part)+static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor,
const char *part)both ugly :)
Indeed. Unfortunately I didn't find any beautiful solution which fit in 80 columns. The second line should be moved a bit to the right, but then we violate the 80 column limit even more. Ideas anyone?
yes, violate the §$&§$ 80 column limit (scnr :) the function could be renamed... and board_pciid_enable is also a very long name for a struct that's used a lot.
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
Regards, Carl-Daniel
On Wed, 03 Aug 2011 09:02:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
looks fine imo. board_match_pci_card_ids could also be renamed to board_match_pci_ids and if you are at it please change the comment of it: /* * Match boards on PCI IDs and subsystem IDs. * Second set of IDs can be main only or missing completely. */ const static struct board_pciid_enable *board_match_pci_card_ids(enum board_match_phase phase)
- * Second set of IDs can be main only or missing completely. + * Second set of IDs can contain primary IDs only or be missing completely.
aaaand if we change board_match_pci_card_ids we should also change pci_card_find to pci_dev_find or something like that... :)
and now i better stop looking for such stuff.
New stripped down version.
Turns out that trailing commas in initializers are allowed by C99: http://c0x.coding-guidelines.com/6.7.8.html
Fixup of r1397: - Mixing uninitialized and initialized local variables leads to confusion. - ft2232_spi error cases should have gotten some error handling, and that's the reason the curly braces were there. - Fixing typos/wording in some places would have been nice given that those places were touched anyway.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Index: flashrom-revert_r1397_partially/it87spi.c =================================================================== --- flashrom-revert_r1397_partially/it87spi.c (Revision 1412) +++ flashrom-revert_r1397_partially/it87spi.c (Arbeitskopie) @@ -203,7 +203,8 @@
int init_superio_ite(void) { - int i, ret = 0; + int i; + int ret = 0;
for (i = 0; i < superio_count; i++) { if (superios[i].vendor != SUPERIO_VENDOR_ITE) Index: flashrom-revert_r1397_partially/cli_classic.c =================================================================== --- flashrom-revert_r1397_partially/cli_classic.c (Revision 1412) +++ flashrom-revert_r1397_partially/cli_classic.c (Arbeitskopie) @@ -104,13 +104,14 @@ struct flashchip flashes[3]; struct flashchip *fill_flash; const char *name; - int startchip = 0, chipcount = 0, namelen, opt, option_index = 0; - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; - int dont_verify_it = 0, list_supported = 0, force = 0; + int namelen, opt, i; + int startchip = 0, chipcount = 0, option_index = 0, force = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int operation_specified = 0, i, ret = 0; + int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + int dont_verify_it = 0, list_supported = 0, operation_specified = 0; + int ret = 0;
static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; static const struct option long_options[] = { Index: flashrom-revert_r1397_partially/dmi.c =================================================================== --- flashrom-revert_r1397_partially/dmi.c (Revision 1412) +++ flashrom-revert_r1397_partially/dmi.c (Arbeitskopie) @@ -196,7 +196,8 @@ */ static int dmi_compare(const char *value, const char *pattern) { - int anchored = 0, patternlen; + int anchored = 0; + int patternlen;
msg_pspew("matching %s against %s\n", value, pattern); /* The empty string is part of all strings! */ Index: flashrom-revert_r1397_partially/dediprog.c =================================================================== --- flashrom-revert_r1397_partially/dediprog.c (Revision 1412) +++ flashrom-revert_r1397_partially/dediprog.c (Arbeitskopie) @@ -496,7 +496,8 @@ static int parse_voltage(char *voltage) { char *tmp = NULL; - int i, millivolt, fraction = 0; + int i; + int millivolt = 0, fraction = 0;
if (!voltage || !strlen(voltage)) { msg_perr("Empty voltage= specified.\n"); @@ -574,7 +575,8 @@ { struct usb_device *dev; char *voltage; - int millivolt = 3500, ret; + int millivolt = 3500; + int ret;
msg_pspew("%s\n", __func__);
Index: flashrom-revert_r1397_partially/it85spi.c =================================================================== --- flashrom-revert_r1397_partially/it85spi.c (Revision 1412) +++ flashrom-revert_r1397_partially/it85spi.c (Arbeitskopie) @@ -84,15 +84,15 @@ static int it85xx_scratch_rom_reenter = 0;
/* This function will poll the keyboard status register until either - * an expected value shows up, or - * timeout reaches. + * an expected value shows up, or the timeout is reached. + * timeout is in usec. * - * Returns: 0 -- the expected value has shown. - * 1 -- timeout reached. + * Returns: 0 -- the expected value showed up. + * 1 -- timeout. */ static int wait_for(const unsigned int mask, const unsigned int expected_value, - const int timeout /* in usec */, const char *error_message, - const char *function_name, const int lineno) + const int timeout, const char * error_message, + const char * function_name, const int lineno) { int time_passed;
@@ -317,8 +317,9 @@ int i;
it85xx_enter_scratch_rom(); - /* exit scratch ROM ONLY when programmer shuts down. Otherwise, the - * temporary flash state may halt EC. */ + /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the + * temporary flash state may halt the EC. + */
#ifdef LPC_IO INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); Index: flashrom-revert_r1397_partially/buspirate_spi.c =================================================================== --- flashrom-revert_r1397_partially/buspirate_spi.c (Revision 1412) +++ flashrom-revert_r1397_partially/buspirate_spi.c (Arbeitskopie) @@ -150,9 +150,11 @@ int buspirate_spi_init(void) { unsigned char buf[512]; - int ret = 0, i, spispeed = 0x7; char *dev = NULL; char *speed = NULL; + int spispeed = 0x7; + int ret = 0; + int i;
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { Index: flashrom-revert_r1397_partially/physmap.c =================================================================== --- flashrom-revert_r1397_partially/physmap.c (Revision 1412) +++ flashrom-revert_r1397_partially/physmap.c (Arbeitskopie) @@ -76,7 +76,7 @@
mi.address = phys_addr; mi.size = len; - ret = __dpmi_physical_address_mapping (&mi); + ret = __dpmi_physical_address_mapping(&mi);
if (ret != 0) return ERROR_PTR; Index: flashrom-revert_r1397_partially/ft2232_spi.c =================================================================== --- flashrom-revert_r1397_partially/ft2232_spi.c (Revision 1412) +++ flashrom-revert_r1397_partially/ft2232_spi.c (Arbeitskopie) @@ -252,17 +252,21 @@ ftdic->error_str); }
- if (ftdi_usb_reset(ftdic) < 0) + if (ftdi_usb_reset(ftdic) < 0) { msg_perr("Unable to reset FTDI device\n"); + }
- if (ftdi_set_latency_timer(ftdic, 2) < 0) + if (ftdi_set_latency_timer(ftdic, 2) < 0) { msg_perr("Unable to set latency timer\n"); + }
- if (ftdi_write_data_set_chunksize(ftdic, 256)) + if (ftdi_write_data_set_chunksize(ftdic, 256)) { msg_perr("Unable to set chunk size\n"); + }
- if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) + if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { msg_perr("Unable to set bitmode to SPI\n"); + }
if (clock_5x) { msg_pdbg("Disable divide-by-5 front stage\n"); @@ -329,7 +333,8 @@ struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */ - int i = 0, ret = 0, failed = 0, bufsize; + int i = 0, ret = 0, failed = 0; + int bufsize; static int oldbufsize = 0;
if (writecnt > 65536 || readcnt > 65536) Index: flashrom-revert_r1397_partially/chipset_enable.c =================================================================== --- flashrom-revert_r1397_partially/chipset_enable.c (Revision 1412) +++ flashrom-revert_r1397_partially/chipset_enable.c (Arbeitskopie) @@ -309,8 +309,9 @@ static int enable_flash_ich_dc(struct pci_dev *dev, const char *name) { uint32_t fwh_conf; + int i, tmp; char *idsel = NULL; - int i, tmp, max_decode_fwh_idsel = 0, max_decode_fwh_decode = 0; + int max_decode_fwh_idsel = 0, max_decode_fwh_decode = 0; int contiguous = 1;
idsel = extract_programmer_param("fwh_idsel"); @@ -1035,7 +1036,7 @@ } } else { msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. " - "Assuming flash at 4G\n"); + "Assuming flash at 4G.\n"); }
/* 4. Clean up */ Index: flashrom-revert_r1397_partially/flashrom.c =================================================================== --- flashrom-revert_r1397_partially/flashrom.c (Revision 1412) +++ flashrom-revert_r1397_partially/flashrom.c (Arbeitskopie) @@ -600,8 +600,7 @@ size_t size = flash->total_size * 1024; /* Flash registers live 4 MByte below the flash. */ /* FIXME: This is incorrect for nonstandard flashbase. */ - flash->virtual_registers = (chipaddr)programmer_map_flash_region( - "flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); + flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); }
int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len) @@ -753,8 +752,9 @@ int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, const char *message) { - int i, ret = 0, failcount = 0; + int i; uint8_t *readbuf = malloc(len); + int ret = 0, failcount = 0;
if (!len) goto out_free; @@ -832,7 +832,8 @@ */ int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran) { - int result = 0, i, j, limit; + int result = 0; + int i, j, limit;
switch (gran) { case write_gran_1bit: @@ -898,7 +899,8 @@ static int get_next_write(uint8_t *have, uint8_t *want, int len, int *first_start, enum write_granularity gran) { - int need_write = 0, rel_start = 0, first_len = 0, i, limit, stride; + int need_write = 0, rel_start = 0, first_len = 0; + int i, limit, stride;
switch (gran) { case write_gran_1bit: @@ -1326,7 +1328,8 @@ */ static int selfcheck_eraseblocks(const struct flashchip *flash) { - int i, j, k, ret = 0; + int i, j, k; + int ret = 0;
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { unsigned int done = 0; @@ -1451,7 +1454,8 @@ void *param1, void *param2) { int i, j; - unsigned int start = 0, len; + unsigned int start = 0; + unsigned int len; struct block_eraser eraser = flash->block_erasers[erasefunction];
for (i = 0; i < NUM_ERASEREGIONS; i++) { @@ -1603,8 +1607,10 @@ void list_programmers_linebreak(int startcol, int cols, int paren) { const char *pname; - int pnamelen, remaining = 0, firstline = 1, i; + int pnamelen; + int remaining = 0, firstline = 1; enum programmer p; + int i;
for (p = 0; p < PROGRAMMER_INVALID; p++) { pname = programmer_table[p].name; Index: flashrom-revert_r1397_partially/board_enable.c =================================================================== --- flashrom-revert_r1397_partially/board_enable.c (Revision 1412) +++ flashrom-revert_r1397_partially/board_enable.c (Arbeitskopie) @@ -860,7 +860,7 @@ return -1; }
- /* First, check the ISA bridge */ + /* Check for the ISA bridge first. */ dev = pci_dev_find_vendorclass(0x10DE, 0x0601); switch (dev->device_id) { case 0x0030: /* CK804 */ @@ -1129,8 +1129,8 @@ struct pci_dev *dev; uint32_t tmp, base;
- /* GPPO {0,8,27,28,30} are always available */ - static const uint32_t nonmuxed_gpos = 0x58000101; + /* GPO{0,8,27,28,30} are always available. */ + static const uint32_t nonmuxed_gpos = 0x58000101;
static const struct {unsigned int reg, mask, value; } piix4_gpo[] = { {0}, @@ -1179,10 +1179,9 @@ }
if ((((1 << gpo) & nonmuxed_gpos) == 0) && - (pci_read_word(dev, piix4_gpo[gpo].reg) - & piix4_gpo[gpo].mask) != piix4_gpo[gpo].value) { - msg_perr("\nERROR: PIIX4 GPO%d not programmed for output.\n", - gpo); + ((pci_read_word(dev, piix4_gpo[gpo].reg) & piix4_gpo[gpo].mask) != + piix4_gpo[gpo].value)) { + msg_perr("\nERROR: PIIX4 GPO%d not programmed for output.\n", gpo); return -1; }
Am 15.08.2011 21:53 schrieb Carl-Daniel Hailfinger:
Fixup of r1397:
- Mixing uninitialized and initialized local variables leads to confusion.
- ft2232_spi error cases should have gotten some error handling, and
that's the reason the curly braces were there.
- Fixing typos/wording in some places would have been nice given that
those places were touched anyway.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Committed in r1413.
Regards, Carl-Daniel
Am 03.08.2011 13:40 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 09:02:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
looks fine imo. board_match_pci_card_ids could also be renamed to board_match_pci_ids and if you are at it please change the comment of it:
Done.
/*
- Match boards on PCI IDs and subsystem IDs.
- Second set of IDs can be main only or missing completely.
*/ const static struct board_pciid_enable *board_match_pci_card_ids(enum board_match_phase phase)
- Second set of IDs can be main only or missing completely.
- Second set of IDs can contain primary IDs only or be missing completely.
I tried to find an alternate wording.
aaaand if we change board_match_pci_card_ids we should also change pci_card_find to pci_dev_find or something like that... :)
Well, if we rename that one, we'd have to call it pci_dev_subsys_find, and that's a net loss from the 80 column perspective, but it may indeed clarify the code. Further input is appreciated.
Shorten some board enable related function names
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c (Arbeitskopie) @@ -125,7 +125,7 @@ int num_notes = 0; char *notes = calloc(1, 1); char tmp[900 + 1]; - const struct board_pciid_enable *b = board_pciid_enables; + const struct board_match *b = board_matches;
for (i = 0; boards[i].vendor != NULL; i++) { if (boards[i].working) Index: flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c (Arbeitskopie) @@ -1731,7 +1731,7 @@ msg_gerr("Chipset enables table does not exist!\n"); ret = 1; } - if (board_pciid_enables == NULL) { + if (board_matches == NULL) { msg_gerr("Board enables table does not exist!\n"); ret = 1; } Index: flashrom-cosmetic_shorten_boardenable_function_names/programmer.h =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/programmer.h (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/programmer.h (Arbeitskopie) @@ -158,7 +158,7 @@ P3 };
-struct board_pciid_enable { +struct board_match { /* Any device, but make it sensible, like the ISA bridge. */ uint16_t first_vendor; uint16_t first_device; @@ -190,7 +190,7 @@ int (*enable) (void); /* May be NULL. */ };
-extern const struct board_pciid_enable board_pciid_enables[]; +extern const struct board_match board_matches[];
struct board_info { const char *vendor; Index: flashrom-cosmetic_shorten_boardenable_function_names/print.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/print.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/print.c (Arbeitskopie) @@ -204,7 +204,7 @@ const char *devicetype) { int i, boardcount_good = 0, boardcount_bad = 0; - const struct board_pciid_enable *e = board_pciid_enables; + const struct board_match *e = board_matches; const struct board_info *b = boards; int maxvendorlen = strlen("Vendor") + 1; int maxboardlen = strlen("Board") + 1; @@ -242,7 +242,7 @@ msg_ginfo(" "); msg_ginfo((b->working) ? "OK " : "BAD ");
- for (e = board_pciid_enables; e->vendor_name != NULL; e++) { + for (e = board_matches; e->vendor_name != NULL; e++) { if (strcmp(e->vendor_name, b->vendor) || strcmp(e->board_name, b->name)) continue; Index: flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c (Arbeitskopie) @@ -1971,7 +1971,7 @@ */
/* Please keep this list alphabetically ordered by vendor/board name. */ -const struct board_pciid_enable board_pciid_enables[] = { +const struct board_match board_matches[] = {
/* first pci-id set [4], second pci-id set [4], dmi identifier, coreboot id [2], phase, vendor name, board name max_rom_... OK? flash enable */ #if defined(__i386__) || defined(__x86_64__) @@ -2097,11 +2097,11 @@ * Match boards on coreboot table gathered vendor and part name. * Require main PCI IDs to match too as extra safety. */ -static const struct board_pciid_enable *board_match_coreboot_name( - const char *vendor, const char *part) +static const struct board_match *board_match_cbname(const char *vendor, + const char *part) { - const struct board_pciid_enable *board = board_pciid_enables; - const struct board_pciid_enable *partmatch = NULL; + const struct board_match *board = board_matches; + const struct board_match *partmatch = NULL;
for (; board->vendor_name; board++) { if (vendor && (!board->lb_vendor @@ -2148,12 +2148,11 @@
/* * Match boards on PCI IDs and subsystem IDs. - * Second set of IDs can be main only or missing completely. + * Second set of IDs can be either main+subsystem IDs, main IDs or no IDs. */ -const static struct board_pciid_enable *board_match_pci_card_ids( - enum board_match_phase phase) +const static struct board_match *board_match_pci_ids(enum board_match_phase phase) { - const struct board_pciid_enable *board = board_pciid_enables; + const struct board_match *board = board_matches;
for (; board->vendor_name; board++) { if ((!board->first_card_vendor || !board->first_card_device) && @@ -2199,7 +2198,7 @@ return NULL; }
-static int unsafe_board_handler(const struct board_pciid_enable *board) +static int unsafe_board_handler(const struct board_match *board) { if (!board) return 1; @@ -2226,9 +2225,9 @@ /* FIXME: Should this be identical to board_flash_enable? */ static int board_handle_phase(enum board_match_phase phase) { - const struct board_pciid_enable *board = NULL; + const struct board_match *board = NULL;
- board = board_match_pci_card_ids(phase); + board = board_match_pci_ids(phase);
if (unsafe_board_handler(board)) board = NULL; @@ -2257,14 +2256,14 @@
int board_flash_enable(const char *vendor, const char *part) { - const struct board_pciid_enable *board = NULL; + const struct board_match *board = NULL; int ret = 0;
if (part) - board = board_match_coreboot_name(vendor, part); + board = board_match_cbname(vendor, part);
if (!board) - board = board_match_pci_card_ids(P3); + board = board_match_pci_ids(P3);
if (unsafe_board_handler(board)) board = NULL;
On Mon, 15 Aug 2011 22:40:58 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 03.08.2011 13:40 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 09:02:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
looks fine imo. board_match_pci_card_ids could also be renamed to board_match_pci_ids and if you are at it please change the comment of it:
Done.
oh my. now that i see the code, i think renaming the board enables to board_match is really a bad idea. mainly because we use "match" as verb in other places, but here as substantive. e.g. static const struct board_match *board_match_cbname a function that matches boards according to their cbname should return a board, not a "board match". it is just a question of taste, but i find it awful :)
the wiki table is named board_info hmhm maybe board_detail, but that's long.. :/ what about just "board"? another way to mitigate "my" problem would be to no use match as a verb for the method names, but using "get" or "find" instead.
/*
- Match boards on PCI IDs and subsystem IDs.
- Second set of IDs can be main only or missing completely.
*/ const static struct board_pciid_enable *board_match_pci_card_ids(enum board_match_phase phase)
- Second set of IDs can be main only or missing completely.
- Second set of IDs can contain primary IDs only or be missing completely.
I tried to find an alternate wording.
that's good, because i obviously missed the point of the whole sentence/method ;)
aaaand if we change board_match_pci_card_ids we should also change pci_card_find to pci_dev_find or something like that... :)
Well, if we rename that one, we'd have to call it pci_dev_subsys_find, and that's a net loss from the 80 column perspective, but it may indeed clarify the code. Further input is appreciated.
why do we have to? we find pci devs, not pci dev subsystems ;) hm maybe it should be find_pci_dev? or maybe no... still dizzy from sleeping sorry :)
Am 15.08.2011 23:42 schrieb Stefan Tauner:
On Mon, 15 Aug 2011 22:40:58 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 03.08.2011 13:40 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 09:02:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
looks fine imo. board_match_pci_card_ids could also be renamed to board_match_pci_ids and if you are at it please change the comment of it:
Done.
oh my. now that i see the code, i think renaming the board enables to board_match is really a bad idea. mainly because we use "match" as verb in other places, but here as substantive. e.g. static const struct board_match *board_match_cbname a function that matches boards according to their cbname should return a board, not a "board match". it is just a question of taste, but i find it awful :)
It's a trap! The struct is there to match a board, so struct board_match is exactly the right name for the _type_. The name of the function is another thing, you could probably call it match_board_match_cbname, but that would be silly, so we can either call it match_board_cbname or board_match_cbname. And the name of the variable which stores the result of the function call is even another thing, and there the name board might be appropriate.
the wiki table is named board_info hmhm maybe board_detail, but that's long.. :/ what about just "board"? another way to mitigate "my" problem would be to no use match as a verb for the method names, but using "get" or "find" instead.
"get" has the wrong semantic implication for the cbname matching, and "find" as in board_find_cbname suffers from the same problem because we're NOT interested in finding the cbname of the board, but rather in looking for a board which matches the supplied cbname.
aaaand if we change board_match_pci_card_ids we should also change pci_card_find to pci_dev_find or something like that... :)
Well, if we rename that one, we'd have to call it pci_dev_subsys_find, and that's a net loss from the 80 column perspective, but it may indeed clarify the code. Further input is appreciated.
why do we have to? we find pci devs, not pci dev subsystems ;) hm maybe it should be find_pci_dev?
pci_dev_find() looks for a device matching the supplied vendor+dev IDs. pci_card_find() looks for a device matching the supplied vendor+dev+subvendor+subdev IDs.
The name "card" was picked to allow differentiating between PCI devices (cards) which share vendor+dev ID, but have different subvendor+subdev IDs. That's pretty common for network cards where dozens of vendors use the same network chip (and thus fixed vendor+dev ID), but completely different PCBs.
Regards, Carl-Daniel
On Tue, 16 Aug 2011 13:38:05 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 15.08.2011 23:42 schrieb Stefan Tauner:
On Mon, 15 Aug 2011 22:40:58 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 03.08.2011 13:40 schrieb Stefan Tauner:
On Wed, 03 Aug 2011 09:02:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Suggestion: struct board_pciid_enable -> struct board_match board_match_coreboot_name() -> board_match_cbname()
What do you think?
looks fine imo. board_match_pci_card_ids could also be renamed to board_match_pci_ids and if you are at it please change the comment of it:
Done.
oh my. now that i see the code, i think renaming the board enables to board_match is really a bad idea. mainly because we use "match" as verb in other places, but here as substantive. e.g. static const struct board_match *board_match_cbname a function that matches boards according to their cbname should return a board, not a "board match". it is just a question of taste, but i find it awful :)
It's a trap! The struct is there to match a board, so struct board_match is exactly the right name for the _type_.
if that would be its only purpose, it would contain identification attributes only. last time i looked it had general info about boards (vendor and board name) and board enable specific fields (phase, max_rom etc). its previous name also suggests it is not just for matching. ;)
The name of the function is another thing, you could probably call it match_board_match_cbname, but that would be silly, so we can either call it match_board_cbname or board_match_cbname. And the name of the variable which stores the result of the function call is even another thing, and there the name board might be appropriate.
the wiki table is named board_info hmhm maybe board_detail, but that's long.. :/ what about just "board"? another way to mitigate "my" problem would be to no use match as a verb for the method names, but using "get" or "find" instead.
"get" has the wrong semantic implication for the cbname matching, and "find" as in board_find_cbname suffers from the same problem because we're NOT interested in finding the cbname of the board, but rather in looking for a board which matches the supplied cbname.
i think the reason for our difference of opinion is how we view the table. for me it is a kind of database or container class. and we want to query it for objects with certain properties.
in sane languages (supporting polymorphy) you could write that as <container variable>.find_board(cbname). of course "match" would work too, but as explained above i would like to use different terms just to have a (mental) distinction.
aaaand if we change board_match_pci_card_ids we should also change pci_card_find to pci_dev_find or something like that... :)
Well, if we rename that one, we'd have to call it pci_dev_subsys_find, and that's a net loss from the 80 column perspective, but it may indeed clarify the code. Further input is appreciated.
why do we have to? we find pci devs, not pci dev subsystems ;) hm maybe it should be find_pci_dev?
pci_dev_find() looks for a device matching the supplied vendor+dev IDs. pci_card_find() looks for a device matching the supplied vendor+dev+subvendor+subdev IDs.
The name "card" was picked to allow differentiating between PCI devices (cards) which share vendor+dev ID, but have different subvendor+subdev IDs. That's pretty common for network cards where dozens of vendors use the same network chip (and thus fixed vendor+dev ID), but completely different PCBs.
hm, ok
well... it is certainly an improvement over the current state and this discussion wont lead to something more sane i feel, so think of it as Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Am 21.08.2011 21:50 schrieb Stefan Tauner:
On Tue, 16 Aug 2011 13:38:05 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 15.08.2011 23:42 schrieb Stefan Tauner:
oh my. now that i see the code, i think renaming the board enables to board_match is really a bad idea. mainly because we use "match" as verb in other places, but here as substantive. e.g. static const struct board_match *board_match_cbname a function that matches boards according to their cbname should return a board, not a "board match". it is just a question of taste, but i find it awful :)
It's a trap! The struct is there to match a board, so struct board_match is exactly the right name for the _type_.
if that would be its only purpose, it would contain identification attributes only. last time i looked it had general info about boards (vendor and board name) and board enable specific fields (phase, max_rom etc). its previous name also suggests it is not just for matching. ;)
Hm right.
the wiki table is named board_info hmhm maybe board_detail, but that's long.. :/ what about just "board"? another way to mitigate "my" problem would be to no use match as a verb for the method names, but using "get" or "find" instead.
"get" has the wrong semantic implication for the cbname matching, and "find" as in board_find_cbname suffers from the same problem because we're NOT interested in finding the cbname of the board, but rather in looking for a board which matches the supplied cbname.
i think the reason for our difference of opinion is how we view the table. for me it is a kind of database or container class. and we want to query it for objects with certain properties.
in sane languages (supporting polymorphy) you could write that as <container variable>.find_board(cbname). of course "match" would work too, but as explained above i would like to use different terms just to have a (mental) distinction.
Understood, your view makes sense. To be honest, my approach to renaming functions is "would I misunderstand the purpose of the function?"
pci_dev_find() looks for a device matching the supplied vendor+dev IDs. pci_card_find() looks for a device matching the supplied vendor+dev+subvendor+subdev IDs.
The name "card" was picked to allow differentiating between PCI devices (cards) which share vendor+dev ID, but have different subvendor+subdev IDs. That's pretty common for network cards where dozens of vendors use the same network chip (and thus fixed vendor+dev ID), but completely different PCBs.
hm, ok
well... it is certainly an improvement over the current state and this discussion wont lead to something more sane i feel, so think of it as Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Thanks, committed in r1424.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Tauner