Hello,
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
Also in attachment.
Stephan.
Signed-off-by: Stephan Guilloux mailto:stephan.guilloux@free.fr
Index: coreboot-v2/util/flashrom/board_enable.c =================================================================== --- coreboot-v2/util/flashrom/board_enable.c (révision 3860) +++ coreboot-v2/util/flashrom/board_enable.c (copie de travail) @@ -626,58 +626,369 @@ };
struct board_pciid_enable board_pciid_enables[] = { - {0x1106, 0x0571, 0x1462, 0x7120, 0x0000, 0x0000, 0x0000, 0x0000, - "msi", "kt4v", "MSI KT4V", board_msi_kt4v}, - {0x8086, 0x1a30, 0x1043, 0x8070, 0x8086, 0x244b, 0x1043, 0x8028, - NULL, NULL, "ASUS P4B266", ich2_gpio22_raise}, - {0x10de, 0x0360, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "gigabyte", "m57sli", "GIGABYTE GA-M57SLI-S4", it87xx_probe_spi_flash}, - {0x10de, 0x03e0, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "gigabyte", "m61p", "GIGABYTE GA-M61P-S3", it87xx_probe_spi_flash}, - {0x1002, 0x4398, 0x1458, 0x5004, 0x1002, 0x4385, 0x1458, 0x4385, - NULL, NULL, "GIGABYTE GA-MA78G-DS3H", it87xx_probe_spi_flash}, - {0x1039, 0x0761, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "gigabyte", "2761gxdk", "GIGABYTE GA-2761GXDK", it87xx_probe_spi_flash}, - {0x1022, 0x7468, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "iwill", "dk8_htx", "IWILL DK8-HTX", w83627hf_gpio24_raise_2e}, - {0x10de, 0x005e, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "msi", "k8n-neo3", "MSI K8N Neo3", w83627thf_gpio4_4_raise_4e}, - {0x1022, 0x746B, 0x1022, 0x36C0, 0x0000, 0x0000, 0x0000, 0x0000, - "AGAMI", "ARUMA", "agami Aruma", w83627hf_gpio24_raise_2e}, - {0x1106, 0x3177, 0x1106, 0xAA01, 0x1106, 0x3123, 0x1106, 0xAA01, - NULL, NULL, "VIA EPIA M/MII/...", board_via_epia_m}, - {0x1106, 0x3177, 0x1043, 0x80A1, 0x1106, 0x3205, 0x1043, 0x8118, - NULL, NULL, "ASUS A7V8-MX SE", board_asus_a7v8x_mx}, - {0x1106, 0x3227, 0x1106, 0xAA01, 0x1106, 0x0259, 0x1106, 0xAA01, - NULL, NULL, "VIA EPIA SP", board_via_epia_sp}, - {0x1106, 0x0314, 0x1106, 0xaa08, 0x1106, 0x3227, 0x1106, 0xAA08, - NULL, NULL, "VIA EPIA-CN", board_via_epia_sp}, - {0x8086, 0x1076, 0x8086, 0x1176, 0x1106, 0x3059, 0x10f1, 0x2498, - NULL, NULL, "Tyan Tomcat K7M", board_asus_a7v8x_mx}, - {0x10B9, 0x1541, 0x0000, 0x0000, 0x10B9, 0x1533, 0x0000, 0x0000, - "asus", "p5a", "ASUS P5A", board_asus_p5a}, - {0x1166, 0x0205, 0x1014, 0x0347, 0x0000, 0x0000, 0x0000, 0x0000, - "ibm", "x3455", "IBM x3455", board_ibm_x3455}, - {0x8086, 0x7110, 0x0000, 0x0000, 0x8086, 0x7190, 0x0000, 0x0000, - "epox", "ep-bx3", "EPoX EP-BX3", board_epox_ep_bx3}, - {0x8086, 0x1130, 0x0000, 0x0000, 0x105a, 0x0d30, 0x105a, 0x4d33, - "acorp", "6a815epd", "Acorp 6A815EPD", board_acorp_6a815epd}, - {0x1022, 0x2090, 0x0000, 0x0000, 0x1022, 0x2080, 0x0000, 0x0000, - "artecgroup", "dbe61", "Artec Group DBE61", board_artecgroup_dbe6x}, - {0x1022, 0x2090, 0x0000, 0x0000, 0x1022, 0x2080, 0x0000, 0x0000, - "artecgroup", "dbe62", "Artec Group DBE62", board_artecgroup_dbe6x}, + { + .first_vendor = 0x1106, + .first_device = 0x0571, + .first_card_vendor = 0x1462, + .first_card_device = 0x7120, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "msi", + .lb_part = "kt4v", + .name = "MSI KT4V", + .enable = board_msi_kt4v, + }, + { + .first_vendor = 0x8086, + .first_device = 0x1a30, + .first_card_vendor = 0x1043, + .first_card_device = 0x8070, + .second_vendor = 0x8086, + .second_device = 0x244b, + .second_card_vendor = 0x1043, + .second_card_device = 0x8028, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "ASUS P4B266", + .enable = ich2_gpio22_raise, + }, + { + .first_vendor = 0x10de, + .first_device = 0x0360, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "gigabyte", + .lb_part = "m57sli", + .name = "GIGABYTE GA-M57SLI-S4", + .enable = it87xx_probe_spi_flash, + }, + { + .first_vendor = 0x10de, + .first_device = 0x03e0, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "gigabyte", + .lb_part = "m61p", + .name = "GIGABYTE GA-M61P-S3", + .enable = it87xx_probe_spi_flash, + }, + { + .first_vendor = 0x1002, + .first_device = 0x4398, + .first_card_vendor = 0x1458, + .first_card_device = 0x5004, + .second_vendor = 0x1002, + .second_device = 0x4385, + .second_card_vendor = 0x1458, + .second_card_device = 0x4385, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "GIGABYTE GA-MA78G-DS3H", + .enable = it87xx_probe_spi_flash, + }, + { + .first_vendor = 0x1039, + .first_device = 0x0761, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "gigabyte", + .lb_part = "2761gxdk", + .name = "GIGABYTE GA-2761GXDK", + .enable = it87xx_probe_spi_flash, + }, + { + .first_vendor = 0x1022, + .first_device = 0x7468, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "iwill", + .lb_part = "dk8_htx", + .name = "IWILL DK8-HTX", + .enable = w83627hf_gpio24_raise_2e, + }, + { + .first_vendor = 0x10de, + .first_device = 0x005e, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "msi", + .lb_part = "k8n-neo3", + .name = "MSI K8N Neo3", + .enable = w83627thf_gpio4_4_raise_4e, + }, + { + .first_vendor = 0x1022, + .first_device = 0x746B, + .first_card_vendor = 0x1022, + .first_card_device = 0x36C0, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "AGAMI", + .lb_part = "ARUMA", + .name = "agami Aruma", + .enable = w83627hf_gpio24_raise_2e, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3177, + .first_card_vendor = 0x1106, + .first_card_device = 0xAA01, + .second_vendor = 0x1106, + .second_device = 0x3123, + .second_card_vendor = 0x1106, + .second_card_device = 0xAA01, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "VIA EPIA M/MII/...", + .enable = board_via_epia_m, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3177, + .first_card_vendor = 0x1043, + .first_card_device = 0x80A1, + .second_vendor = 0x1106, + .second_device = 0x3205, + .second_card_vendor = 0x1043, + .second_card_device = 0x8118, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "ASUS A7V8-MX SE", + .enable = board_asus_a7v8x_mx, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3227, + .first_card_vendor = 0x1106, + .first_card_device = 0xAA01, + .second_vendor = 0x1106, + .second_device = 0x0259, + .second_card_vendor = 0x1106, + .second_card_device = 0xAA01, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "VIA EPIA SP", + .enable = board_via_epia_sp, + }, + { + .first_vendor = 0x1106, + .first_device = 0x0314, + .first_card_vendor = 0x1106, + .first_card_device = 0xaa08, + .second_vendor = 0x1106, + .second_device = 0x3227, + .second_card_vendor = 0x1106, + .second_card_device = 0xAA08, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "VIA EPIA-CN", + .enable = board_via_epia_sp, + }, + { + .first_vendor = 0x8086, + .first_device = 0x1076, + .first_card_vendor = 0x8086, + .first_card_device = 0x1176, + .second_vendor = 0x1106, + .second_device = 0x3059, + .second_card_vendor = 0x10f1, + .second_card_device = 0x2498, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "Tyan Tomcat K7M", + .enable = board_asus_a7v8x_mx, + }, + { + .first_vendor = 0x10B9, + .first_device = 0x1541, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x10B9, + .second_device = 0x1533, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "asus", + .lb_part = "p5a", + .name = "ASUS P5A", + .enable = board_asus_p5a, + }, + { + .first_vendor = 0x1166, + .first_device = 0x0205, + .first_card_vendor = 0x1014, + .first_card_device = 0x0347, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "ibm", + .lb_part = "x3455", + .name = "IBM x3455", + .enable = board_ibm_x3455, + }, + { + .first_vendor = 0x8086, + .first_device = 0x7110, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x8086, + .second_device = 0x7190, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "epox", + .lb_part = "ep-bx3", + .name = "EPoX EP-BX3", + .enable = board_epox_ep_bx3, + }, + { + .first_vendor = 0x8086, + .first_device = 0x1130, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x105a, + .second_device = 0x0d30, + .second_card_vendor = 0x105a, + .second_card_device = 0x4d33, + .lb_vendor = "acorp", + .lb_part = "6a815epd", + .name = "Acorp 6A815EPD", + .enable = board_acorp_6a815epd, + }, + { + .first_vendor = 0x1022, + .first_device = 0x2090, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x1022, + .second_device = 0x2080, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "artecgroup", + .lb_part = "dbe61", + .name = "Artec Group DBE61", + .enable = board_artecgroup_dbe6x, + }, + { + .first_vendor = 0x1022, + .first_device = 0x2090, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x1022, + .second_device = 0x2080, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "artecgroup", + .lb_part = "dbe62", + .name = "Artec Group DBE62", + .enable = board_artecgroup_dbe6x, + }, /* Note: There are >= 2 version of the Kontron 986LCD-M/mITX! */ - {0x8086, 0x27b8, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, - "kontron", "986lcd-m", "Kontron 986LCD-M", board_kontron_986lcd_m}, - {0x10ec, 0x8168, 0x10ec, 0x8168, 0x104c, 0x8023, 0x104c, 0x8019, - "kontron", "986lcd-m", "Kontron 986LCD-M", board_kontron_986lcd_m}, - {0x1106, 0x3149, 0x1565, 0x3206, 0x1106, 0x3344, 0x1565, 0x1202, - NULL, NULL, "BioStar P4M80-M4", board_biostar_p4m80_m4}, - {0x1106, 0x3227, 0x1458, 0x5001, 0x10ec, 0x8139, 0x1458, 0xe000, - NULL, NULL, "GIGABYTE GA-7VT600", board_biostar_p4m80_m4}, - {0x1106, 0x3149, 0x1462, 0x7094, 0x10ec, 0x8167, 0x1462, 0x094c, - NULL, NULL, "MSI K8T Neo2", w83627thf_gpio4_4_raise_2e}, - {0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL} /* Keep this */ + { + .first_vendor = 0x8086, + .first_device = 0x27b8, + .first_card_vendor = 0x0000, + .first_card_device = 0x0000, + .second_vendor = 0x0000, + .second_device = 0x0000, + .second_card_vendor = 0x0000, + .second_card_device = 0x0000, + .lb_vendor = "kontron", + .lb_part = "986lcd-m", + .name = "Kontron 986LCD-M", + .enable = board_kontron_986lcd_m, + }, + { + .first_vendor = 0x10ec, + .first_device = 0x8168, + .first_card_vendor = 0x10ec, + .first_card_device = 0x8168, + .second_vendor = 0x104c, + .second_device = 0x8023, + .second_card_vendor = 0x104c, + .second_card_device = 0x8019, + .lb_vendor = "kontron", + .lb_part = "986lcd-m", + .name = "Kontron 986LCD-M", + .enable = board_kontron_986lcd_m, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3149, + .first_card_vendor = 0x1565, + .first_card_device = 0x3206, + .second_vendor = 0x1106, + .second_device = 0x3344, + .second_card_vendor = 0x1565, + .second_card_device = 0x1202, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "BioStar P4M80-M4", + .enable = board_biostar_p4m80_m4, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3227, + .first_card_vendor = 0x1458, + .first_card_device = 0x5001, + .second_vendor = 0x10ec, + .second_device = 0x8139, + .second_card_vendor = 0x1458, + .second_card_device = 0xe000, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "GIGABYTE GA-7VT600", + .enable = board_biostar_p4m80_m4, + }, + { + .first_vendor = 0x1106, + .first_device = 0x3149, + .first_card_vendor = 0x1462, + .first_card_device = 0x7094, + .second_vendor = 0x10ec, + .second_device = 0x8167, + .second_card_vendor = 0x1462, + .second_card_device = 0x094c, + .lb_vendor = NULL, + .lb_part = NULL, + .name = "MSI K8T Neo2", + .enable = w83627thf_gpio4_4_raise_2e, + }, + { + .first_vendor = 0, + .first_device = 0, + .first_card_vendor = 0, + .first_card_device = 0, + .second_vendor = 0, + .second_device = 0, + .second_card_vendor = 0, + .second_card_device = 0, + .lb_vendor = NULL, + .lb_part = NULL, + } /* Keep this */ };
void print_supported_boards(void)
On Tue, Jan 13, 2009 at 10:02:15PM +0100, Stephan GUILLOUX wrote:
Hello,
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
Also in attachment.
For certain versions of readable. What real problem does this solve?
Luc Verhaegen.
On Tue, Jan 13, 2009 at 3:34 PM, Luc Verhaegen libv@skynet.be wrote:
On Tue, Jan 13, 2009 at 10:02:15PM +0100, Stephan GUILLOUX wrote:
Hello,
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
Also in attachment.
For certain versions of readable. What real problem does this solve?
1. Next time someone adds a new struct member, we avoid mistakes of ordering of initializers 2. we avoid mistakes in the first place.
The .x = y stuff was added for a (good) reason, I think this is an improvement.
Acked-by: Ronald G. Minnich rminnich@gmail.com
ron minnich wrote:
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
For certain versions of readable. What real problem does this solve?
- Next time someone adds a new struct member, we avoid mistakes of
ordering of initializers 2. we avoid mistakes in the first place.
The .x = y stuff was added for a (good) reason, I think this is an improvement.
Acked-by: Ronald G. Minnich rminnich@gmail.com
Sorry, but I really do not like these at all and I would much rather see the chip table change back. There seemed to be general agreement about that on IRC the other day as well.
If a mistake sneaks into the structs it will be trivial to fix..
//Peter
On 14.01.2009 0:33 Uhr, Peter Stuge wrote:
ron minnich wrote:
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
For certain versions of readable. What real problem does this solve?
- Next time someone adds a new struct member, we avoid mistakes of
ordering of initializers 2. we avoid mistakes in the first place.
The .x = y stuff was added for a (good) reason, I think this is an improvement.
Acked-by: Ronald G. Minnich rminnich@gmail.com
Sorry, but I really do not like these at all and I would much rather see the chip table change back. There seemed to be general agreement about that on IRC the other day as well.
Why?
Stefan Reinauer wrote:
Sorry, but I really do not like these at all and I would much rather see the chip table change back. There seemed to be general agreement about that on IRC the other day as well.
Why?
I find it less readable. I find the "table" view much easier on the eyes and brain than one-field-per-line.
//Peter
On 14.01.2009 00:55, Stefan Reinauer wrote:
On 14.01.2009 0:33 Uhr, Peter Stuge wrote:
ron minnich wrote:
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
For certain versions of readable. What real problem does this solve?
- Next time someone adds a new struct member, we avoid mistakes of
ordering of initializers 2. we avoid mistakes in the first place.
The .x = y stuff was added for a (good) reason, I think this is an improvement.
Acked-by: Ronald G. Minnich rminnich@gmail.com
Sorry, but I really do not like these at all and I would much rather see the chip table change back. There seemed to be general agreement about that on IRC the other day as well.
Why?
I was unaware of the general agreement on IRC although I was there.
The current table is horrible. Two lines per entry combine the worst of both worlds.
And since I edit the wrong line every single time I touch that array, the patch is
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
On Tue, Jan 13, 2009 at 03:23:26PM -0800, ron minnich wrote:
On Tue, Jan 13, 2009 at 3:34 PM, Luc Verhaegen libv@skynet.be wrote:
On Tue, Jan 13, 2009 at 10:02:15PM +0100, Stephan GUILLOUX wrote:
Hello,
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
Also in attachment.
For certain versions of readable. What real problem does this solve?
- Next time someone adds a new struct member, we avoid mistakes of
ordering of initializers 2. we avoid mistakes in the first place.
The .x = y stuff was added for a (good) reason, I think this is an improvement.
Acked-by: Ronald G. Minnich rminnich@gmail.com
I personally prefer (very long) single lines for a table like this.
Single long lines make it easy to visually parse, provided the editor is set wide enough to show the whole table. Long lines only stop people from easily parsing this code in a 80char terminal, but the 80char boundary is being significantly broken in ther files of flashrom. And it really is the least of the two possible pains, would you rather break readability for all or just for those who know that this table should not be edited when blindfold?
Strings should be padded to a common length, so that all but the last member is fully aligned. No errors can then occur when a new member is added, as the difference between entries become immediately clear. How often have new struct members been added since this table was created close to two years ago?
Luc Verhaegen.
On 13.01.2009 22:02, Stephan GUILLOUX wrote:
Hello,
Similarly to flashchips array, this patch intends to make the table board_pciid_enables more readable.
Also in attachment.
Stephan.
Signed-off-by: Stephan Guilloux mailto:stephan.guilloux@free.fr
Thanks, r3861.
Regards, Carl-Daniel
On Thu, Jan 15, 2009 at 02:14:12AM +0100, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
r3861
..just horrible..
Acked-By Luc Verhaegen libv@skynet.be
On Thu, Jan 15, 2009 at 03:09:10AM +0100, Luc Verhaegen wrote:
On Thu, Jan 15, 2009 at 02:14:12AM +0100, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
r3861
..just horrible..
Acked-By Luc Verhaegen libv@skynet.be
Heck, i tried getting some sleep after seeing this but failed.
This change goes in against every instinct i have.
diffstat tells me this: board_enable.c | 413 +++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 362 insertions(+), 51 deletions(-)
Daniel, you just bloated an almost overseeable table 7fold. If you claim that you still have any oversight and any control, then you're lying.
Luc Verhaegen. Who just happens to be the original creator of this table.
Luc Verhaegen wrote:
Heck, i tried getting some sleep after seeing this but failed.
Hah, I don't blame you.
This change goes in against every instinct i have.
Yeah, I also feel that it is completely wrong.
Daniel, you just bloated an almost overseeable table 7fold.
I'd like to hear back from Stefan and/or Ron. If they have no comments or strong opinions in a few days or so I'd like to ask you to please revert this commit Carl-Daniel. If the formalia is important feel free to consider it naked by me by then.
//Peter
On 15.01.2009 03:44, Peter Stuge wrote:
Luc Verhaegen wrote:
Daniel, you just bloated an almost overseeable table 7fold.
I'd like to hear back from Stefan and/or Ron. If they have no comments or strong opinions in a few days or so I'd like to ask you to please revert this commit Carl-Daniel. If the formalia is important feel free to consider it naked by me by then.
Ron gave his Ack and a reason why he prefers the new format. I'll gladly forward the mail to you if you missed it.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
I'd like to hear back from Stefan and/or Ron. If they have no comments or strong opinions in a few days or so I'd like to ask you to please revert this commit Carl-Daniel.
Ron gave his Ack and a reason why he prefers the new format.
I guess I should have written "no further comments" instead. That would have been more clear.
//Peter
On 15.01.2009 03:49, Luc Verhaegen wrote:
On Thu, Jan 15, 2009 at 03:09:10AM +0100, Luc Verhaegen wrote:
On Thu, Jan 15, 2009 at 02:14:12AM +0100, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
r3861
..just horrible..
Acked-By Luc Verhaegen libv@skynet.be
Heck, i tried getting some sleep after seeing this but failed.
This change goes in against every instinct i have.
diffstat tells me this: board_enable.c | 413 +++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 362 insertions(+), 51 deletions(-)
Daniel, you just bloated an almost overseeable table 7fold. If you claim that you still have any oversight and any control, then you're lying.
Rest assured, I'll sleep well tonight.
Luc Verhaegen. Who just happens to be the original creator of this table.
The new table has a format which elicits a WTF for several entries. The reason for the WTF (content, not style) was there already, but the old style didn't make it that obvious. Making things obvious is good. Making strange code painful to look at is good.
Regards, Carl-Daniel Who thinks lots of lines with =0 or =NULL can be removed now which was impossible with the old style.
Carl-Daniel Hailfinger wrote:
Making strange code painful to look at is good.
..to ensure that noone will ever want to look at it again, so that bugs won't be found? Makes no sense.
I consider the opposite to be true, in particular code with strange logic should be easy on the eyes so that anyone who has even slight interest could actually review it and maybe even spot a bug.
If the code is painful $person will just laugh and go away, leaving bugs lingering..
//Peter
On 15.01.2009 03:55, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
Making strange code painful to look at is good.
..to ensure that noone will ever want to look at it again, so that bugs won't be found? Makes no sense.
I consider the opposite to be true, in particular code with strange logic should be easy on the eyes so that anyone who has even slight interest could actually review it and maybe even spot a bug.
It is definitely easier on the eyes if you only have to look at two places at the same time (array and board enable function) now instead of three places (flash.h and array and board enable function) before.
The pain I was referring to is in the loads of NULL/0 stuff. It can be eliminated with liberal application of grep. The real question is why there are so many NULL/0 entries in the first place.
If the code is painful $person will just laugh and go away, leaving bugs lingering..
Actually, there are bugs lingering in related code, but nobody bothered to fix it. Well, that's not entirely true. I bothered to send a patch, but iirc it was never acked. I can resend...
Anyway, this change makes it easy to add a new board without having to open two editor windows at the same time to find out the correct struct ordering. And that makes it easier for new contributors who just want to fix flashrom for their board.
The two-line layout was really painful. It was multiline (which you seem to dislike extremely) and it required anybody changing it to have flash.h open at the same time (which I think is more complicated than it should be). I consider anything to be an improvement over the old layout.
Regards, Carl-Daniel
On Thu, Jan 15, 2009 at 04:16:38AM +0100, Carl-Daniel Hailfinger wrote:
On 15.01.2009 03:55, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
Making strange code painful to look at is good.
..to ensure that noone will ever want to look at it again, so that bugs won't be found? Makes no sense.
I consider the opposite to be true, in particular code with strange logic should be easy on the eyes so that anyone who has even slight interest could actually review it and maybe even spot a bug.
It is definitely easier on the eyes if you only have to look at two places at the same time (array and board enable function) now instead of three places (flash.h and array and board enable function) before.
The pain I was referring to is in the loads of NULL/0 stuff. It can be eliminated with liberal application of grep. The real question is why there are so many NULL/0 entries in the first place.
If the code is painful $person will just laugh and go away, leaving bugs lingering..
Actually, there are bugs lingering in related code, but nobody bothered to fix it. Well, that's not entirely true. I bothered to send a patch, but iirc it was never acked. I can resend...
Anyway, this change makes it easy to add a new board without having to open two editor windows at the same time to find out the correct struct ordering. And that makes it easier for new contributors who just want to fix flashrom for their board.
The two-line layout was really painful. It was multiline (which you seem to dislike extremely) and it required anybody changing it to have flash.h open at the same time (which I think is more complicated than it should be). I consider anything to be an improvement over the old layout.
Regards, Carl-Daniel
flash.h? This table is fully file internal. The struct definition is right before the table!
My original code was single long line per entry.
Luc Verhaegen.
On 15.01.2009 04:42, Luc Verhaegen wrote:
My original code was single long line per entry.
The two-line layout change was committed in r2581 and Signed-off-by: Luc Verhaegen libv@skynet.be
So unless you want to accuse Stefan of botching the commit of your patch, there is a small problem.
Regards, Carl-Daniel
On Thu, Jan 15, 2009 at 04:45:04AM +0100, Carl-Daniel Hailfinger wrote:
On 15.01.2009 04:42, Luc Verhaegen wrote:
My original code was single long line per entry.
The two-line layout change was committed in r2581 and Signed-off-by: Luc Verhaegen libv@skynet.be
So unless you want to accuse Stefan of botching the commit of your patch, there is a small problem.
Regards, Carl-Daniel
Last patch sent on this that i can find: http://www.coreboot.org/pipermail/coreboot/2007-March/019425.html
Stefans commit message: http://www.coreboot.org/pipermail/coreboot/2007-April/019690.html
Luc Verhaegen.
Carl-Daniel Hailfinger wrote:
Making strange code painful to look at is good.
..to ensure that noone will ever want to look at it again, so that bugs won't be found? Makes no sense.
I consider the opposite to be true, in particular code with strange logic should be easy on the eyes so that anyone who has even slight interest could actually review it and maybe even spot a bug.
The pain I was referring to is in the loads of NULL/0 stuff. It can be eliminated with liberal application of grep.
I'm afraid this doesn't help me understand your point about making code painful. :\
The real question is why there are so many NULL/0 entries in the first place.
Because many boards have non-specific PCI IDs?
It is definitely easier on the eyes if you only have to look at two places at the same time (array and board enable function) now instead of three places (flash.h and array and board enable function) before.
What did you look up in flash.h? When I've poked at this table I just banged in some PCI IDs, -m strings if the IDs weren't good enough for an automatic match, the pretty name and the function that should be run. The function was often reused from another, similar, board, thus allowing me to do the change with only a single file open, not that I consider that a very important metric anyway.
If the code is painful $person will just laugh and go away, leaving bugs lingering..
Actually, there are bugs lingering in related code, but nobody bothered to fix it.
Maybe they just aren't serious enough?
Well, that's not entirely true. I bothered to send a patch, but iirc it was never acked. I can resend...
What should I search for? I can probably find the thread. Would've been real handy to have it in trac though.
..
And that makes it easier for new contributors who just want to fix flashrom for their board.
I dare say they aren't exactly the common case. Of course I would like it if they were, but for now they aren't. I don't believe this bikeshed actually plays even a small part in their (lack of) contributions anyway.
The two-line layout was really painful. It was multiline (which you seem to dislike extremely)
That's why I think the fourteen-line layout with mostly noise is bad. The 2-line layout has confused me too a few times, but it was nothing an extra carriage return while editing didn't fix immediately.
..
I consider anything to be an improvement over the old layout.
You might be able to sell me on a single line layout, but I actually think the two-line layout is as good as it gets here.
//Peter
On Thu, Jan 15, 2009 at 03:44:41AM +0100, Carl-Daniel Hailfinger wrote:
On 15.01.2009 03:49, Luc Verhaegen wrote:
Daniel, you just bloated an almost overseeable table 7fold. If you claim that you still have any oversight and any control, then you're lying.
Rest assured, I'll sleep well tonight.
Luc Verhaegen. Who just happens to be the original creator of this table.
The new table has a format which elicits a WTF for several entries. The reason for the WTF (content, not style) was there already, but the old style didn't make it that obvious. Making things obvious is good. Making strange code painful to look at is good.
Hey, if you don't understand the code, don't touch it.
And here i was thinking there was a clear comment in front of the table structure definition that explained how the different entries are used and why some entries can be nulled.
Luc Verhaegen.
On 15.01.2009 04:24, Luc Verhaegen wrote:
On Thu, Jan 15, 2009 at 03:44:41AM +0100, Carl-Daniel Hailfinger wrote:
On 15.01.2009 03:49, Luc Verhaegen wrote:
Daniel, you just bloated an almost overseeable table 7fold. If you claim that you still have any oversight and any control, then you're lying.
Rest assured, I'll sleep well tonight.
Luc Verhaegen. Who just happens to be the original creator of this table.
The new table has a format which elicits a WTF for several entries. The reason for the WTF (content, not style) was there already, but the old style didn't make it that obvious. Making things obvious is good. Making strange code painful to look at is good.
Hey, if you don't understand the code, don't touch it.
No problem. I assume you're willing to create all board enable functions and table entries for the foreseeable future for everyone who asks once we switch back to the old layout. That includes debugging and sending new patches to volunteer testers. By the way, I understand the code.
And here i was thinking there was a clear comment in front of the table structure definition that explained how the different entries are used and why some entries can be nulled.
Calling subsystem IDs card IDs is not my definition of clear.
Regards, Carl-Daniel
On Thu, Jan 15, 2009 at 04:36:54AM +0100, Carl-Daniel Hailfinger wrote:
On 15.01.2009 04:24, Luc Verhaegen wrote:
Hey, if you don't understand the code, don't touch it.
No problem. I assume you're willing to create all board enable functions and table entries for the foreseeable future for everyone who asks once we switch back to the old layout. That includes debugging and sending new patches to volunteer testers.
Sure, i'll do that next to keeping X from degrading even further.
Calling subsystem IDs card IDs is not my definition of clear.
As you well know, people use the names interchangedly, all the time.
Luc Verhaegen.
-
Carl-Daniel Hailfinger -
Luc Verhaegen -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Stephan GUILLOUX