Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56414 )
Change subject: dediprog.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 4:
(4 comments)
File dediprog.c:
https://review.coreboot.org/c/flashrom/+/56414/comment/3ad2c510_23880d44
PS3, Line 221: const int finish,
> Please indent at least after the opening parenthesis (or only two-tabs total, I think […]
I used "at least after the opening parenthesis" option. It is useful that you mentioned the rule, I don't remember the rule (maybe I forgot), but now I know!
dediprog_read has it not "at least after", but "at the same place" as the opening parenthesis. Let me know if dediprog_read needs to be changed as well.
https://review.coreboot.org/c/flashrom/+/56414/comment/40daeac5_bd26ba2b
PS3, Line 222: struct libusb_context *usb_ctx)
> Usually such context information is made the first parameter.
And this way it is also consistent with libusb functions, so even better. There are few functions here with usb_ctx and dediprog_handle, I put those as first parameter.
https://review.coreboot.org/c/flashrom/+/56414/comment/243067f5_213b7ce6
PS3, Line 242: size_t size,
> I don't see a need to waste one line each. For instance, cmd & value & idx and […]
Done
https://review.coreboot.org/c/flashrom/+/56414/comment/8546a920_18a3a04a
PS3, Line 1025: dp_data->dediprog_devicetype = DEV_UNKNOWN;
> This shouldn't be necessary anymore as the data is discarded.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/56414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d
Gerrit-Change-Number: 56414
Gerrit-PatchSet: 4
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 09 Sep 2021 06:59:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/56414
to look at the new patch set (#4).
Change subject: dediprog.c: Refactor singleton states into reentrant pattern
......................................................................
dediprog.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within
the spi_master data field for the life-time of the driver.
This is one of the steps on the way to move spi_master data
memory management behind the initialisation API, for more
context see other patches under the same topic "register_master_api".
BUG=b:185191942
TEST=builds and ninja test
Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M dediprog.c
1 file changed, 153 insertions(+), 115 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/56414/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/56414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d
Gerrit-Change-Number: 56414
Gerrit-PatchSet: 4
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-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/56415
to look at the new patch set (#4).
Change subject: dediprog.c: Drop dediprog_ prefix for spi data struct members
......................................................................
dediprog.c: Drop dediprog_ prefix for spi data struct members
The name of the struct type already contains dp_ prefix, so
prefix doesn't need to be repeated in members name.
BUG=b:185191942
TEST=builds and ninja test
Change-Id: I688d50926b78a6c3f1c5a8ba4ef88a0d5b495bd0
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M dediprog.c
1 file changed, 55 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/56415/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/56415
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I688d50926b78a6c3f1c5a8ba4ef88a0d5b495bd0
Gerrit-Change-Number: 56415
Gerrit-PatchSet: 4
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-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Michał Żygowski, Paul Menzel.
Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 )
Change subject: Add Tiger Lake U Premium support
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
Patchset:
PS10:
Tested on system76/darp7. I was able to read the current image and then flash a new one using the internal programmer.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55578
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd
Gerrit-Change-Number: 55578
Gerrit-PatchSet: 10
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com>
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: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Wed, 08 Sep 2021 22:39:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57437 )
Change subject: tests: Make chip definitions static global
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/57437/comment/154a5444_3e26e8f4
PS2, Line 7: static
> it isn't static though unless you use the keyword 'static' ;)
yes thank you! how did I missed this...
--
To view, visit https://review.coreboot.org/c/flashrom/+/57437
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a
Gerrit-Change-Number: 57437
Gerrit-PatchSet: 3
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-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-Comment-Date: Wed, 08 Sep 2021 21:59:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57436 )
Change subject: tests: Extract setup and teardown for chip tests
......................................................................
Patch Set 2:
(3 comments)
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/57436/comment/18597c46_62899783
PS1, Line 29: g_chip_state
> I wonder if it is time to embed this into `data` of the flashctx to avoid possible test coupling. […]
Good idea, totally makes sense! I was going to do it quickly, but then I realised I don't know how to implement it?
There is no `data` in flashctx struct, there is one in spi_master (which dummy uses). Data in spi_master is already used and owned by a programmer.
I need to think about it, I don't know how to do it quickly in this patch :\
https://review.coreboot.org/c/flashrom/+/57436/comment/ea5d6082_6c378562
PS1, Line 89: static void reset_global_chip_state()
: {
: g_chip_state.unlock_calls = 0;
: }
> Just inline this function to the one call site. […]
Done! This was actually a historical artefact from old version when there was no setup function :)
https://review.coreboot.org/c/flashrom/+/57436/comment/2d4c2250_ea6edf5d
PS1, Line 94: setup_for_chip
> +1 drop the _for_ just `setup_chip()`
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/57436
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If59315646f06344664df08b145866d9ce846d751
Gerrit-Change-Number: 57436
Gerrit-PatchSet: 2
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-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-Comment-Date: Wed, 08 Sep 2021 21:58:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/57436
to look at the new patch set (#2).
Change subject: tests: Extract setup and teardown for chip tests
......................................................................
tests: Extract setup and teardown for chip tests
Steps to setup and teardown for a chip test are repeated for every
test, so they can be extracted into their own functions.
BUG=b:181803212
TEST=builds and ninja test
Change-Id: If59315646f06344664df08b145866d9ce846d751
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/chip.c
1 file changed, 47 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/36/57436/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/57436
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If59315646f06344664df08b145866d9ce846d751
Gerrit-Change-Number: 57436
Gerrit-PatchSet: 2
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/57326
to look at the new patch set (#3).
Change subject: tests: Add tests to read from chip
......................................................................
tests: Add tests to read from chip
Two tests cover the code which performs do_read operation.
First one works with fake chip and dummy programmer. Fake chip has all
operations defined, and a buffer to emulate chip memory.
Second one uses the chip which is closer to the real one, because
read/write/unlock/erase operations are real. The tests takes the
advantage of dummyflasher's capability of emulating a W25Q128.V chip.
BUG=b:181803212
TEST=builds and ninja test
Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/chip.c
M tests/io_mock.h
M tests/meson.build
M tests/tests.c
M tests/tests.h
5 files changed, 146 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/57326/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/57326
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756
Gerrit-Change-Number: 57326
Gerrit-PatchSet: 3
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-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-MessageType: newpatchset
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/57437
to look at the new patch set (#3).
Change subject: tests: Make chip definitions static global
......................................................................
tests: Make chip definitions static global
This way chip definitions can be reused between test functions.
Existing mock chip is promoted from 8KiB to 8MiB and eraseblocks
are expanded accordingly. Old value of 8KiB was very small and it
was confusing. Mock chip looks more realistic now.
BUG=b:181803212
TEST=builds and ninja test
Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/chip.c
1 file changed, 51 insertions(+), 112 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/57437/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/57437
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a
Gerrit-Change-Number: 57437
Gerrit-PatchSet: 3
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset