May 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: libflashrom: Move documentation to header
by Angel Pons (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Evan Benn. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63903 ) Change subject: libflashrom: Move documentation to header ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/63903 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I856b74d5bfea13722539be15496755a95e701eea Gerrit-Change-Number: 63903 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 02 May 2022 14:39:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Move documentation to header
by Angel Pons (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Evan Benn. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63903 ) Change subject: libflashrom: Move documentation to header ...................................................................... Patch Set 6: (10 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63903/comment/6d157803_6df90e63 PS6, Line 10: FFI What's this? https://review.coreboot.org/c/flashrom/+/63903/comment/c6439a62_1535d331 PS6, Line 16: and after the change. And what's the result of the test? File include/libflashrom.h: https://review.coreboot.org/c/flashrom/+/63903/comment/80ae6f83_e7982ad1 PS6, Line 65: file a file I'm not sure if the examples are needed. https://review.coreboot.org/c/flashrom/+/63903/comment/3dc6d0b6_95569b72 PS6, Line 118: /** : * @brief Returns list of supported flash chips : * @return List of supported flash chips, or NULL if an error occurred : */ : struct flashrom_flashchip_info *flashrom_supported_flash_chips(void); : /** : * @brief Returns list of supported mainboards : * @return List of supported mainboards, or NULL if an error occurred : */ : struct flashrom_board_info *flashrom_supported_boards(void); : /** : * @brief Returns list of supported chipsets : * @return List of supported chipsets, or NULL if an error occurred : */ : struct flashrom_chipset_info *flashrom_supported_chipsets(void); Unrelated question, but how does one know the length of the returned list? https://review.coreboot.org/c/flashrom/+/63903/comment/67b4e967_0a4d0374 PS6, Line 262: short small https://review.coreboot.org/c/flashrom/+/63903/comment/5143f914_6cb7e45d PS6, Line 356: buffer a buffer https://review.coreboot.org/c/flashrom/+/63903/comment/aaa7232b_6d9fc741 PS6, Line 372: another I'd use `a` to be more precise, as `flashrom_layout_new()` returns layouts with no regions. https://review.coreboot.org/c/flashrom/+/63903/comment/30327834_ed495b52 PS6, Line 398: * @param start The start address to be written. : * @param len The length of the region to be written. Maybe @param[out] ? https://review.coreboot.org/c/flashrom/+/63903/comment/132288c4_2f501720 PS6, Line 416: he I'd avoid using personal pronouns in documentation, the caller isn't necessarily a "he". How about: The caller must not release the layout as long as it is used through the given flash context. Here, "it" refers to "the layout". https://review.coreboot.org/c/flashrom/+/63903/comment/496a5c4f_902d2778 PS6, Line 463: [out] Why is this an out parameter? -- To view, visit https://review.coreboot.org/c/flashrom/+/63903 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I856b74d5bfea13722539be15496755a95e701eea Gerrit-Change-Number: 63903 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 02 May 2022 14:39:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: util/flashrom_tester: Update sys-info crate to version 0.9
by Peter Marheine (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Jack Rosenthal, Tim Wawrzynczak, Edward O'Callaghan, Angel Pons. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63975 ) Change subject: util/flashrom_tester: Update sys-info crate to version 0.9 ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/63975 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3b6b21e830ff3107860f7bcbfe2d58b29efe0c12 Gerrit-Change-Number: 63975 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 02 May 2022 05:34:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Move documentation to header
by Peter Marheine (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Evan Benn. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63903 ) Change subject: libflashrom: Move documentation to header ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/63903 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I856b74d5bfea13722539be15496755a95e701eea Gerrit-Change-Number: 63903 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 02 May 2022 05:33:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: outsource platform specific code to `platform/meson.build`
by Felix Singer (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Thomas Heijligen. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63826 ) Change subject: meson: outsource platform specific code to `platform/meson.build` ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63826/comment/396c85cd_133db9bf PS3, Line 7: outsource I would use "move" https://review.coreboot.org/c/flashrom/+/63826/comment/289aa82d_f7c0b698 PS3, Line 8: Please state the reason for this change. What does it improve? -- To view, visit https://review.coreboot.org/c/flashrom/+/63826 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88044a3f903f316138483dd872a6d95f8686ae69 Gerrit-Change-Number: 63826 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 02 May 2022 05:18:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: add option to disable tests
by Felix Singer (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63832 ) Change subject: meson: add option to disable tests ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63832/comment/b8941aa3_d89cd985 PS4, Line 9: usefull useful https://review.coreboot.org/c/flashrom/+/63832/comment/ce5e5c25_b77464eb PS4, Line 10: build built -- To view, visit https://review.coreboot.org/c/flashrom/+/63832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I384c904c577b265dfe36bf46bf07c641bc29de9b Gerrit-Change-Number: 63832 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 02 May 2022 05:12:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [WIP] meson: rework the programmer selection
by Anastasia Klimchuk (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63724 ) Change subject: [WIP] meson: rework the programmer selection ...................................................................... Patch Set 11: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63724/comment/d7eed736_13c1897c PS11, Line 10: has some known and unknown bugs. Speaking about bugs, and especially unknown bugs, can we collectively try detect them? Is this patch ready to be downloaded locally and built with various combinations of options? File meson.build: https://review.coreboot.org/c/flashrom/+/63724/comment/4164d880_ce8cc1e6 PS7, Line 212: 'internal' : { : 'systems' : systems_raw_hwaccess + ['linux'], : 'cpu_families' : (host_machine.system() == 'linux' ? [host_machine.cpu_family()] : ['x86', 'x86_64']), : 'deps' : [ libpci ], : 'groups' : [ ], : 'srcs' : (host_machine.cpu_family() in ['x86', 'x86_64'] ? files( : 'processor_enable.c', : 'chipset_enable.c', : 'board_enable.c', : 'cbtable.c', : 'internal.c', : 'it87spi.c', : 'it85spi.c', : 'sb600spi.c', : 'amd_imc.c', : 'wbsio_spi.c', : 'mcp6x_spi.c', : 'ichspi.c', : 'dmi.c', : 'pcidev.c', : ) : files( : 'board_enable.c', : 'cbtable.c', : 'chipset_enable.c', : 'internal.c', : 'processor_enable.c', : 'pcidev.c', : )), : 'flags' : [ : '-DCONFIG_INTERNAL=1', : '-DCONFIG_INTERNAL_DMI=' + (get_option('use_internal_dmi') ? '1' : '0'), : ] : }, > This should work. I only discovered this possibility while working on the code. […] Thank you, good to know that refactoring internal is not blocking further work on build system. I agree this needs to be done, but good to know it's not blocking. -- To view, visit https://review.coreboot.org/c/flashrom/+/63724 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib44b26e3748fc71f116184082b4aed0bb208b4c1 Gerrit-Change-Number: 63724 Gerrit-PatchSet: 11 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 02 May 2022 05:07:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: add option to disable tests
by Anastasia Klimchuk (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63832 ) Change subject: meson: add option to disable tests ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63832/comment/d38c8be5_5e8e6a67 PS1, Line 10: Run `meson -Dtests=true` to enable tests. (default) : Run `meson -Dtests=false` to disable tests. > I've updated the commit message. Now it should be mode clear what happen. […] Done Commit Message: https://review.coreboot.org/c/flashrom/+/63832/comment/e3d3fce5_c0c2964d PS4, Line 10: musst musst -> must (typo) File meson.build: https://review.coreboot.org/c/flashrom/+/63832/comment/f3dd066b_8e81e81a PS1, Line 522: if cmocka_dep.found() > cmocka is now a required dependency for tests. Therefore the `if cmocka_dep. […] I found what I was remembering, this commit 16c62a791d0a311bb040812b03c33881ab641a8e That was handling of the case when tests are enabled but libcmocka not available: in this case flashrom will still build just without tests. We need to keep this behaviour. -- To view, visit https://review.coreboot.org/c/flashrom/+/63832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I384c904c577b265dfe36bf46bf07c641bc29de9b Gerrit-Change-Number: 63832 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 02 May 2022 04:55:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Move documentation to header
by Evan Benn (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63903 ) Change subject: libflashrom: Move documentation to header ...................................................................... Patch Set 6: (1 comment) Patchset: PS4: > Something in the rebase has made this not work any more. I am taking a look. This change: https://review.coreboot.org/c/flashrom/+/58622 moved the header but not the Doxyfile path to the header. that is now fixed. I removed the other files as they are not needed as of this patch. PTAL -- To view, visit https://review.coreboot.org/c/flashrom/+/63903 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I856b74d5bfea13722539be15496755a95e701eea Gerrit-Change-Number: 63903 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 02 May 2022 04:49:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Move documentation to header
by Evan Benn (Code Review) May 2, 2022

May 2, 2022

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine. Hello build bot (Jenkins), Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/63903 to look at the new patch set (#6). Change subject: libflashrom: Move documentation to header ...................................................................... libflashrom: Move documentation to header The doxygen documentation was in the libflashrom.c file. Move the documentation to the libflashrom.h file. This allows FFI binding generators that operate on the .h file to generate documentation for the target language. Some doxygen errors were also corrected, mostly undocumented or wrongly labeled parameters. To test, I have diffed and inspected the doxygen documentation before and after the change. Change-Id: I856b74d5bfea13722539be15496755a95e701eea Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Doxyfile M flashrom.c M include/libflashrom.h M layout.c M libflashrom.c M writeprotect.c 6 files changed, 440 insertions(+), 473 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/63903/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/63903 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I856b74d5bfea13722539be15496755a95e701eea Gerrit-Change-Number: 63903 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0