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
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
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
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
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
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
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
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
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 (#5).
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.
To test, I have diffed and inspected the doxygen documentation before
and after the change. flashrom_wp_mode is added.
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, 405 insertions(+), 438 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/63903/5
--
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: 5
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