David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42556 ) Change subject: [WIP] Add uncrustify config ...................................................................... Patch Set 1: Code-Review-2 -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Comment-Date: Fri, 19 Jun 2020 05:56:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42556 ) Change subject: [WIP] Add uncrustify config ...................................................................... Patch Set 2: Code-Review-1 (8 comments) Most of the issues are minor at this point. Overall I'm optimistic about this approach! There is a problem with empty comments in flashchips.c (L1322, for example), however the new version of gerrit seems too slow to be able to comment on that (?!). I'll look into it a bit more so we don't create superfluous newlines. https://review.coreboot.org/c/flashrom/+/42556/2/cbtable.c File cbtable.c: https://review.coreboot.org/c/flashrom/+/42556/2/cbtable.c@206 PS2, Line 206: ; This semicolon is not needed and should be removed from the original file. https://review.coreboot.org/c/flashrom/+/42556/2/hwaccess.h File hwaccess.h: https://review.coreboot.org/c/flashrom/+/42556/2/hwaccess.h@42 PS2, Line 42: #define ___constant_swab8(x) ((uint8_t)( \ These are a bit funky since the leftmost cast is on the first line. A couple of ways to make them easier for uncrustify to handle: - Put the cast on an indented newline - Make this into an inline function. IMO this is better since then we can avoid all these casts and have the compiler spit out an error if somehow the wrong type is input. https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.h File ich_descriptors.h: https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.h@184 PS2, Line 184: : 3, uncrustify does this for unnamed fields in bitfields. The easy workaround is to give each a name, e.g. rsvd1, rsvd2, etc. https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.c@217 PS2, Line 217: static const char *const freq_str[3][8] = { { The indent in this case is based on the rightmost brace (for the first array of strings). Moving it to a newline sets it to the left. https://review.coreboot.org/c/flashrom/+/42556/2/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/42556/2/jlink_spi.c@227 PS2, Line 227: if (ret != JAYLINK_OK) { : msg_perr("jaylink_parse_serial_number() failed: %s.\n", jaylink_strerror(ret)); : free(arg); : return 1; : } This seems redundant with the preceding if-statement, and should be fixed in another patch. https://review.coreboot.org/c/flashrom/+/42556/2/serial.c File serial.c: https://review.coreboot.org/c/flashrom/+/42556/2/serial.c@112 PS2, Line 112: { BAUDRATE(0) can be used to make this a single line, if desired. https://review.coreboot.org/c/flashrom/+/42556/2/spi25_statusreg.c File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/42556/2/spi25_statusreg.c@454 PS2, Line 454: msg_cdbg("no sectors are protected\n"); This is fixed by putting the case labels in parens, i.e. `case (0x0 << 2):` https://review.coreboot.org/c/flashrom/+/42556/2/uncrustify.cfg File uncrustify.cfg: https://review.coreboot.org/c/flashrom/+/42556/2/uncrustify.cfg@11 PS2, Line 11: to of -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 23 Jun 2020 06:32:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42556 ) Change subject: [WIP] Add uncrustify config ...................................................................... Patch Set 2:

Patch Set 2:

David, I'm just curious - why do you prefer uncrustify. I never heard of it, whereas clang-format seems to be a very popular mechanism for this.

That's a good question. I'm not an expert at either, but from what I've seen uncrustify has a lot more configurability that can address specific cases in flashrom that I ran across. It's been around since at least 2006 AFAICT and has fewer package dependencies. My overall impression is that clang-format does a pretty good job by default but lacks configurability that is needed to handle some cases in flashrom. For example, clang-format combines lines in array definitions in a way that impacts a lot of flashrom code in an undesirable way such as in https://review.coreboot.org/c/flashrom/+/38673/4/buspirate_spi.c#148, or https://review.coreboot.org/c/flashrom/+/38673/5/ich_descriptors.c#227. The array elements are deliberately given their own lines which IMO is a good thing in such cases we're listing legal values and is a fairly common pattern in flashrom. I looked around for a way to change that behavior, and it seems that clang-format will do that unless the column limit is set to 0 (not sure why that would affect that particular case) or if the array size is less than 5 elements, which seems like an arbitrary limit that the clang-format developers imposed. Adding a comment after each element in this case might work, but that seems like a silly thing to impose since these lists should be self-documenting. clang-format just doesn't handle these cases well from what I can tell. Another example - it aligns strings used as arguments like in msg_pdbg() in an awkward way - look around in https://review.coreboot.org/c/flashrom/+/38673/5/board_enable.c for msg_pdbg() and you'll see stuff like: msg_pdbg( "W836xx enter config mode worked or we were already in config mode. W836xx " "leave config mode had no effect.\n"); Another example is spacing in braces, i.e. { 1, 2 } vs. {1, 2}. clang-format has options for SpacesInAngles, SpacesInParentheses, SpacesInSquareBrackets, but not for braces. Or maybe I'm missing something obvious... Long story short, I think uncrustify handles flashrom's codebase better at this time. Maybe clang-format will become more configurable in the future and we can revisit this later if people have a strong reason to prefer it. -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org> Gerrit-Comment-Date: Tue, 23 Jun 2020 18:04:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/42556 to look at the new patch set (#3). Change subject: [RFC] Add uncrustify config ...................................................................... [RFC] Add uncrustify config Similar to CB:38673, this attempts to add a config file for uncrustify as an alternative to clang-format which has some limitations as mentioned in the other patch. PS1 adds a config file which is a copy of the linux.cfg file. Newer versions of this patch will tweak the config file as needed for flashrom. TODO: - subdirectories Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Signed-off-by: David Hendricks <david.hendricks@gmail.com> --- M 82802ab.c M amd_imc.c M at45db.c M atahpt.c M atapromise.c M atavia.c M bitbang_spi.c M board_enable.c M buspirate_spi.c M cbtable.c M ch341a_spi.c M chipdrivers.h M chipset_enable.c M cli_classic.c M cli_common.c M cli_output.c M coreboot_tables.h M custom_baud.c M custom_baud.h M dediprog.c M developerbox_spi.c M digilent_spi.c M dmi.c M drkaiser.c M dummyflasher.c M edi.c M edi.h M en29lv640b.c M ene.h M flash.h M flashchips.c M flashchips.h M flashrom.c M fmap.c M fmap.h M ft2232_spi.c M gfxnvidia.c M helpers.c M hwaccess.c M hwaccess.h M i2c_helper.h M i2c_helper_linux.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M internal.c M it8212.c M it85spi.c M it87spi.c M jedec.c M jlink_spi.c M layout.c M layout.h M libflashrom.c M libflashrom.h M linux_mtd.c M linux_spi.c M lspcon_i2c_spi.c M mcp6x_spi.c M mstarddc_spi.c M ni845x_spi.c M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M opaque.c M os.h M pcidev.c M physmap.c M pickit2_spi.c M platform.h M pony_spi.c M print.c M print_wiki.c M processor_enable.c M programmer.c M programmer.h M raiden_debug_spi.c M rayer_spi.c M realtek_mst_i2c_spi.c M satamv.c M satasii.c M sb600spi.c M serial.c M serprog.c M serprog.h M sfdp.c M spi.c M spi.h M spi25.c M spi25_statusreg.c M spi95.c M sst28sf040.c M sst49lfxxxc.c M sst_fwhub.c M stlinkv3_spi.c M stm50.c M udelay.c A uncrustify.cfg M usb_device.c M usb_device.h M usbblaster_spi.c M usbdev.c M w29ee011.c M w39.c M wbsio_spi.c 109 files changed, 5,306 insertions(+), 5,033 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/42556/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org> Gerrit-MessageType: newpatchset

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42556 ) Change subject: [RFC] Add uncrustify config ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/flashrom/+/42556/3/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/c/flashrom/+/42556/3/bitbang_spi.c@19 PS3, Line 19: #include <ctype.h> Uh, do we want global headers above local headers? -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org> Gerrit-Comment-Date: Wed, 24 Jun 2020 10:24:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/42556 to look at the new patch set (#4). Change subject: [RFC] Add uncrustify config ...................................................................... [RFC] Add uncrustify config Similar to CB:38673, this attempts to add a config file for uncrustify as an alternative to clang-format which has some limitations as mentioned in the other patch. PS1 adds a config file which is a copy of the linux.cfg file. Newer versions of this patch will tweak the config file as needed for flashrom. Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Signed-off-by: David Hendricks <david.hendricks@gmail.com> --- M 82802ab.c M amd_imc.c M at45db.c M atahpt.c M atapromise.c M atavia.c M bitbang_spi.c M board_enable.c M buspirate_spi.c M cbtable.c M ch341a_spi.c M chipdrivers.h M chipset_enable.c M cli_classic.c M cli_common.c M cli_output.c M coreboot_tables.h M custom_baud.c M custom_baud.h M dediprog.c M developerbox_spi.c M digilent_spi.c M dmi.c M drkaiser.c M dummyflasher.c M edi.c M edi.h M en29lv640b.c M ene.h M flash.h M flashchips.c M flashchips.h M flashrom.c M fmap.c M fmap.h M ft2232_spi.c M gfxnvidia.c M helpers.c M hwaccess.c M hwaccess.h M i2c_helper.h M i2c_helper_linux.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M internal.c M it8212.c M it85spi.c M it87spi.c M jedec.c M jlink_spi.c M layout.c M layout.h M libflashrom.c M libflashrom.h M linux_mtd.c M linux_spi.c M lspcon_i2c_spi.c M mcp6x_spi.c M mstarddc_spi.c M ni845x_spi.c M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M opaque.c M os.h M pcidev.c M physmap.c M pickit2_spi.c M platform.h M pony_spi.c M print.c M print_wiki.c M processor_enable.c M programmer.c M programmer.h M raiden_debug_spi.c M rayer_spi.c M realtek_mst_i2c_spi.c M satamv.c M satasii.c M sb600spi.c M serial.c M serprog.c M serprog.h M sfdp.c M spi.c M spi.h M spi25.c M spi25_statusreg.c M spi95.c M sst28sf040.c M sst49lfxxxc.c M sst_fwhub.c M stlinkv3_spi.c M stm50.c M udelay.c A uncrustify.cfg M usb_device.c M usb_device.h M usbblaster_spi.c M usbdev.c M w29ee011.c M w39.c M wbsio_spi.c 109 files changed, 5,222 insertions(+), 4,949 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/42556/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 4 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org> Gerrit-MessageType: newpatchset

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/42556 to look at the new patch set (#5). Change subject: [RFC] Add uncrustify config ...................................................................... [RFC] Add uncrustify config Similar to CB:38673, this attempts to add a config file for uncrustify as an alternative to clang-format which has some limitations as mentioned in the other patch. PS1 adds a config file which is a copy of the linux.cfg file. Newer versions of this patch will tweak the config file as needed for flashrom. Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Signed-off-by: David Hendricks <david.hendricks@gmail.com> --- M 82802ab.c M amd_imc.c M at45db.c M atahpt.c M atapromise.c M atavia.c M bitbang_spi.c M board_enable.c M buspirate_spi.c M cbtable.c M ch341a_spi.c M chipdrivers.h M chipset_enable.c M cli_classic.c M cli_common.c M cli_output.c M coreboot_tables.h M custom_baud.c M custom_baud.h M dediprog.c M developerbox_spi.c M digilent_spi.c M dmi.c M drkaiser.c M dummyflasher.c M edi.c M edi.h M en29lv640b.c M ene.h M flash.h M flashchips.c M flashchips.h M flashrom.c M fmap.c M fmap.h M ft2232_spi.c M gfxnvidia.c M helpers.c M hwaccess.c M hwaccess.h M i2c_helper.h M i2c_helper_linux.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M internal.c M it8212.c M it85spi.c M it87spi.c M jedec.c M jlink_spi.c M layout.c M layout.h M libflashrom.c M libflashrom.h M linux_mtd.c M linux_spi.c M lspcon_i2c_spi.c M mcp6x_spi.c M mstarddc_spi.c M ni845x_spi.c M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M opaque.c M os.h M pcidev.c M physmap.c M pickit2_spi.c M platform.h M pony_spi.c M print.c M print_wiki.c M processor_enable.c M programmer.c M programmer.h M raiden_debug_spi.c M rayer_spi.c M realtek_mst_i2c_spi.c M satamv.c M satasii.c M sb600spi.c M serial.c M serprog.c M serprog.h M sfdp.c M spi.c M spi.h M spi25.c M spi25_statusreg.c M spi95.c M sst28sf040.c M sst49lfxxxc.c M sst_fwhub.c M stlinkv3_spi.c M stm50.c M udelay.c A uncrustify.cfg M usb_device.c M usb_device.h M usbblaster_spi.c M usbdev.c M w29ee011.c M w39.c M wbsio_spi.c 109 files changed, 8,844 insertions(+), 8,508 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/42556/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/42556 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org> Gerrit-MessageType: newpatchset

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/42556?usp=email ) Change subject: [RFC] Add uncrustify config ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/42556?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ib6bd3ab49e841768f07c2705f71be66e9308bbca Gerrit-Change-Number: 42556 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Christian Walter <christian.walter@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-CC: Vadim Bendebury <vbendeb@chromium.org>