Attention is currently required from: Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66754 )
Change subject: writeprotect.c: skip unnecessary writes
......................................................................
Patch Set 2:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/66754/comment/f91e7f1e_f0399f94
PS1, Line 7: writeprotect
> Done
Git log takes care about the actual files touched. The prefix,…
[View More] as it was
introduced in coreboot and later here in flashrom, should just provide a
topic. So that people can filter patches / emails quickly. There is no
need to make it a file name.
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/66754/comment/b5fa02e7_74d7ed4a
PS1, Line 159: new
> Done
I don't quite follow, how can it be expected? Should the flash chip
predict what we want it to be?
--
To view, visit https://review.coreboot.org/c/flashrom/+/66754
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I81d2d3fc0a103ee00ced78838d77fe33a9d3056a
Gerrit-Change-Number: 66754
Gerrit-PatchSet: 2
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 07 Oct 2022 12:17:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Felix Singer, Nico Huber, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 )
Change subject: tree/: Move programmer_delay() out of programmer state machine
......................................................................
Patch Set 4:
(1 comment)
File serprog.c:
https://review.coreboot.org/c/flashrom/+/67393/comment/bac64ef1_bad7fa69
PS4, Line 451: };
> Actually I …
[View More]looked at this again, and yes this probably needs delay too. […]
Leaving as unresolved, thanks for spotting that Nico. Fixed locally, will push a rebased update once CB:66373 lands.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67393
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Gerrit-Change-Number: 67393
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 07 Oct 2022 10:58:36 +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>
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67664 )
Change subject: tests: add basic lifecycle test for ch341a_spi
......................................................................
Patch Set 6:
(3 comments)
File tests/ch341a_spi.c:
https://review.coreboot.org/c/flashrom/+/67664/comment/8f8a5a24_6a9fa196
PS5, Line 22: transfer->status = …
[View More]LIBUSB_TRANSFER_COMPLETED;
: transfer->actual_length = transfer->length;
: transfer->callback(transfer);
> *TL;DR* Yeah, it's important step. Without it the code go to an infinite loop. […]
Thank you so much for the detailed explanation, I appreciate a lot!
It is so amazing, you digging deep to find the answer and explain to others!
I understand about callback now, and seems like you found a nice trick to invoke static functions in test (I mean `cb_in` and `cb_out`).
The only thing left is to add a comment in the beginning of `ch341a_libusb_submit_transfer` explaining (in short) that the code would normally be executing as a part of handle_events_timeout, and the test needs to mock that to be able to invoke callback from the programmer and indicate that transfer completed. Something like that.
File tests/libusb_wraps.c:
PS5:
> Commits should only do one thing and not multiple things. I think this commit does two things: […]
Okay I agree on that. Let's say we are introducing this as a default approach from now on.
Joursoir, looks like you need to split the patch into 2. First one will just wraps, which means specifically: io_mock.h, libusb_wraps.h, libusb_wraps.c. And the second patch adding the new test and implementing custom mocks.
You can leave the splitting until the end, when all other comments are resolved. It is easier to modify one large patch locally.
File tests/usb_unittests.h:
https://review.coreboot.org/c/flashrom/+/67664/comment/7aebe72a_1ff9d03c
PS5, Line 58: struct libusb_transfer;
> Typedefs above are exciting in libusb code. But there's no typedef for `struct libusb_transfer`. […]
Okay I agree, resolved.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67664
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If28fbe09ad685082152aa3a7e8d5a150169aee9e
Gerrit-Change-Number: 67664
Gerrit-PatchSet: 6
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Fri, 07 Oct 2022 05:27:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
[View Less]
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68163 )
Change subject: meson.build: Fix indentation of mstarddc_spi definition
......................................................................
Patch Set 1:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68163
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: …
[View More]master
Gerrit-Change-Id: I064b50f87760fd7ad40b3629b3fa68552c8ddb46
Gerrit-Change-Number: 68163
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 06 Oct 2022 23:36:04 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[View Less]