October 2022 - flashrom-gerrit

[M] Change in flashrom[master]: ft2232_spi.c: Split out most programmer param parsing
by Felix Singer (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Thomas Heijligen, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67908 ) Change subject: ft2232_spi.c: Split out most programmer param parsing ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Sry, this should have gone to the first patch Then also see CB:67886 ;) Though, patches need to be squashed and I need to push the rest rest of the programmers I have only locally. -- To view, visit https://review.coreboot.org/c/flashrom/+/67908 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I25faac1060fc2bcd6042e34802e5bc493c936377 Gerrit-Change-Number: 67908 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 02 Oct 2022 06:38:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ft2232_spi.c: Split out most programmer param parsing
by Thomas Heijligen (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Angel Pons. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67908 ) Change subject: ft2232_spi.c: Split out most programmer param parsing ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > On the first view, still from a mobile device, I like this patch. […] Sry, this should have gone to the first patch -- To view, visit https://review.coreboot.org/c/flashrom/+/67908 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I25faac1060fc2bcd6042e34802e5bc493c936377 Gerrit-Change-Number: 67908 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 02 Oct 2022 06:21:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ft2232_spi.c: Split out most programmer param parsing
by Thomas Heijligen (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Angel Pons. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67908 ) Change subject: ft2232_spi.c: Split out most programmer param parsing ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: On the first view, still from a mobile device, I like this patch. what do you think of creating an const instance of ft2232_parameters with the default values instead of passing them in the code? So you have `struct ft2232_parameters programmer_parameters = ft2232_default_parameters;` -- To view, visit https://review.coreboot.org/c/flashrom/+/67908 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I25faac1060fc2bcd6042e34802e5bc493c936377 Gerrit-Change-Number: 67908 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 02 Oct 2022 06:21:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: add basic lifecycle test for ch341a_spi
by Felix Singer (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk, Alexander Goncharov. Felix Singer 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: (1 comment) File tests/libusb_wraps.c: PS5: > Why? In all previous tests, wraps were added in the same patch with the test that needed those wraps […] Commits should only do one thing and not multiple things. I think this commit does two things: * Introducing lots of functions in common code * Introducing tests which are based on the new common code Speaking generally, I think it's a bad practice to submit a commit of this kind, because it's not atomic. The commit title doesn't even give me an idea that new common code is introduced, but many comments are related to the common code. Assuming that future commits get merged, which depend on the newly introduced common code, and we decide or we would like to revert this commit (for whatever reason, everything can happen), then we also have to revert all others that depend on it. Don't get me wrong. I'm not saying that this will be the case for this commit. I just would like to move to this practice, since it gives us more possibilities in the future when we try or even have to fix something. Like a revert might be faster and easier than trying to fix the specific issue. Other advantages: People are able to concentrate on the specific changes during the review and the comment history stays focused. -- 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Sat, 01 Oct 2022 23:31:46 +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> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: stlinkv3_spi: Bug fix, Initialize uninitialized pointers
by Thomas Heijligen (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Felix Singer, Shreeyash . Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66225 ) Change subject: stlinkv3_spi: Bug fix, Initialize uninitialized pointers ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Yes... I can't seem to understand the next step? […] This patch addresses the symptoms of two issues. One, with the libusb_context, was caused by a wrong use of the libusb api. This part I've fixed. The second part, the libusb_device_handle, is an false positive of the analyzers. It is still open and now handled by multiple open patches. I'm sorry if this was not clear. File stlinkv3_spi.c: https://review.coreboot.org/c/flashrom/+/66225/comment/b75aaa93_5514ef4b PS1, Line 501: stlinkv3_handle = usb_dev_get_by_vid_pid_serial(usb_ctx, > So I think this is a false positive. […] Sorry, this has gone under. But it's important to make clear that the NULL is for a false positive in the analyser not a big in flashrom -- To view, visit https://review.coreboot.org/c/flashrom/+/66225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ce156a9f7455434ea461560320c84660c93c02f Gerrit-Change-Number: 66225 Gerrit-PatchSet: 2 Gerrit-Owner: Shreeyash <shreeyash335(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Shreeyash <shreeyash335(a)gmail.com> Gerrit-Comment-Date: Sat, 01 Oct 2022 20:29:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Shreeyash <shreeyash335(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: stlinkv3_spi: Bug fix, Initialize uninitialized pointers
by Shreeyash (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Felix Singer. Shreeyash has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66225 ) Change subject: stlinkv3_spi: Bug fix, Initialize uninitialized pointers ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > @Shreeyash Are you still interested in getting this done? Yes... I can't seem to understand the next step? My change, I believe was overruled by another change suggested by Thomas that I agree with. What do I have to do? -- To view, visit https://review.coreboot.org/c/flashrom/+/66225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ce156a9f7455434ea461560320c84660c93c02f Gerrit-Change-Number: 66225 Gerrit-PatchSet: 2 Gerrit-Owner: Shreeyash <shreeyash335(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Sat, 01 Oct 2022 13:50:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Nico Huber (Code Review) Oct. 1, 2022

Oct. 1, 2022

Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 ) Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... Patch Set 9: Code-Review+1 (1 comment) Patchset: PS9: Looks good, no more comments from me :) -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 9 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Sat, 01 Oct 2022 12:40:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0