Attention is currently required from: Peter Marheine.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52890 )
Change subject: CHROMIUM: cli_classic.c: Remove deprecated options
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> It's kind of weird that this doesn't include any diff on cli_classic. […]
Wouldn't that make it hard to link the Original CL to this one? I guess I could also add a BACKPORT: tag if that is the convention here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib329f3adb59ce1848d1540844d64b968f49eb22a
Gerrit-Change-Number: 52890
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:45:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Daniel Campello.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52890 )
Change subject: CHROMIUM: cli_classic.c: Remove deprecated options
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
It's kind of weird that this doesn't include any diff on cli_classic.c when the commit message says it changes that file- I guess that's because the options were already removed or never present in this tree? Would it make sense to change the commit message?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib329f3adb59ce1848d1540844d64b968f49eb22a
Gerrit-Change-Number: 52890
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:13:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Daniel Campello, Peter Marheine.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52890 )
Change subject: CHROMIUM: cli_classic.c: Remove deprecated options
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib329f3adb59ce1848d1540844d64b968f49eb22a
Gerrit-Change-Number: 52890
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:11:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Daniel Campello.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52889 )
Change subject: CHROMIUM: flashrom_tester: Drop nix dependency
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52889
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iea61c65efb04da9cd0bc0bd85a34fc10912ea87b
Gerrit-Change-Number: 52889
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:11:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52889 )
Change subject: CHROMIUM: flashrom_tester: Drop nix dependency
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/52889
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iea61c65efb04da9cd0bc0bd85a34fc10912ea87b
Gerrit-Change-Number: 52889
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:10:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Daniel Campello.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52892 )
Change subject: flashrom.8.tmpl: Add man entry for --extract
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52892
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Icc068f5545b6f30ac390b7b815a31e2d61bf4789
Gerrit-Change-Number: 52892
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 May 2021 00:03:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/b11241b2_4b231f69
PS10, Line 53: MEC1308_SIO_PORT1
> Nico, Simon, thank you so much for this conversation, really useful for me to read! […]
Well that's fine, you are the one doing the work, after all.
Yes working in untested code is not a lot of fun. Hopefully you don't get too many war stories from it!
--
To view, visit https://review.coreboot.org/c/flashrom/+/51487
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc
Gerrit-Change-Number: 51487
Gerrit-PatchSet: 10
Gerrit-Owner: 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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 23:45:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Simon Glass.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
......................................................................
Patch Set 10:
(1 comment)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/d0246473_9c4eb258
PS10, Line 53: MEC1308_SIO_PORT1
> OK. […]
Nico, Simon, thank you so much for this conversation, really useful for me to read!
I can understand both approaches, but I am taking the side of leaving #defines in the driver code where they are (especially because I was on that side from the beginning).
This point I think is really important:
>>> Pulling such
>>> definitions out of their compilation unit makes it easier to write
>>> tests I suppose. But it also makes it harder to read/review the code.
I don't want to make it harder to read/review the code.
For tests, I can address read/review with comments like
0x20 /* MEC1308_DEVICE_ID_REG */
Also, how often those values change, for a given chip? Is it ~never? Please correct me if I am wrong.
Another thing I want to say about
>>> I don't think we should be afraid to modify the code as needed for tests.
Not afraid, but need to be very very careful. It is like walking on ice...
--
To view, visit https://review.coreboot.org/c/flashrom/+/51487
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc
Gerrit-Change-Number: 51487
Gerrit-PatchSet: 10
Gerrit-Owner: 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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Simon Glass <sjg(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 23:23:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Richard Hughes, Angel Pons, Patrick Rudolph.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 )
Change subject: libflashrom: Return progress state to the library user
......................................................................
Patch Set 6:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/49643/comment/c325d5b5_1fa33580
PS6, Line 9: Projects using libflashrom like fwupd expect the user to wait for the operation
: to complete. To avoid the user thinking the process has "hung" or "got stuck"
: report back the progress complete of the erase, write and read operations
That's good, thank you! One thing left, limit for commit message lines is 72 chars, look like this one is over the limit and lines are broken.
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/49643/comment/aa2abe69_510ddc5a
PS4, Line 106: void *
> Is there a reason why last argument doesn't have a name here? all other arguments have names.
Done
File tests/spi25.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/3e8f7ef7_67640bba
PS4, Line 54: size_t current,
> Thank you for the test! Is there any way to assert what current value is? it should change (unlike t […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/49643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Gerrit-Change-Number: 49643
Gerrit-PatchSet: 6
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Richard Hughes <hughsient(a)gmail.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 04 May 2021 22:50:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment