- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Anastasia Klimchuk (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Simon Buhrow, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 34: Code-Review+1 (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/61b67166_866b9f73 PS34, Line 283: const size_t cmd_len = 3; /* same length for any ft2232 command */ If it is always the same, maybe it can be #define above? -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 34 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 23:56:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Move test environment header files into tests directory
by Anastasia Klimchuk (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55408 ) Change subject: tests: Move test environment header files into tests directory ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55408/comment/5af51e06_9e96f100 PS1, Line 8: > I agree with the change, but I'd appreciate if you could add a sentence to explain why this is a goo […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/55408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Gerrit-Change-Number: 55408 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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 23:39:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Move test environment header files into tests directory
by Anastasia Klimchuk (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: 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/+/55408 to look at the new patch set (#2). Change subject: tests: Move test environment header files into tests directory ...................................................................... tests: Move test environment header files into tests directory Both of these headers are only used in test builds, so they should live in tests/ directory. No changes to meson.build and tests/meson.build files are needed because tests/meson.build adds current directory to search for include files. BUG=b:181803212 TEST=ninja test -> all tests pass nm builddir/tests/flashrom_unit_tests.p/.._it85spi.c.o -> has symbols _test_calloc, _test_free, test_inb, test_outb nm builddir/flashrom.p/it85spi.c.o -> has symbols calloc free inb outb Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- R tests/hwaccess_x86_io_unittest.h R tests/unittest_env.h 2 files changed, 0 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/55408/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Gerrit-Change-Number: 55408 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: layout: Use linked list for `struct romentry`
by Peter Marheine (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33521 ) Change subject: layout: Use linked list for `struct romentry` ...................................................................... Patch Set 8: (1 comment) File layout.c: https://review.coreboot.org/c/flashrom/+/33521/comment/c5c597f6_0ec69e1d PS4, Line 418: entry->next = layout->head; > Ping Ack; makes sense looking at it now. -- To view, visit https://review.coreboot.org/c/flashrom/+/33521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60a0aa1007ebcd5eb401db116f835d129b3e9732 Gerrit-Change-Number: 33521 Gerrit-PatchSet: 8 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.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: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 20 Jun 2021 23:33:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Revise comments about output pin states
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... Patch Set 1: (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/55695/comment/8b2cc93a_66910bc7 PS1, Line 98: * TCK/SK is bit 0. : * TDI/DO is bit 1. : * TDO/DI is bit 2. : * TMS/CS is bit 3. : * GPIOL0 is bit 4. : * GPIOL1 is bit 5. : * GPIOL2 is bit 6. : * GPIOL3 is bit 7. > I was thinking of using these in the code where we handle the `type` programmer param. […] For the GPIOL stuff, one could use something like this: #define PIN_GPIOL(x) (1 << (4 + (x))) -- To view, visit https://review.coreboot.org/c/flashrom/+/55695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Gerrit-Change-Number: 55695 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 15:12:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Revise comments about output pin states
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... Patch Set 1: (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/55695/comment/ebf7e590_7901d2a1 PS1, Line 98: * TCK/SK is bit 0. : * TDI/DO is bit 1. : * TDO/DI is bit 2. : * TMS/CS is bit 3. : * GPIOL0 is bit 4. : * GPIOL1 is bit 5. : * GPIOL2 is bit 6. : * GPIOL3 is bit 7. > I don't know, the identifiers wouldn't be used in the code. It's mostly […] I was thinking of using these in the code where we handle the `type` programmer param. Still, it's something that can be discussed later. -- To view, visit https://review.coreboot.org/c/flashrom/+/55695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Gerrit-Change-Number: 55695 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 15:11:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Revise comments about output pin states
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Angel Pons, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... Patch Set 1: (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/55695/comment/949fe212_6a244b8b PS1, Line 98: * TCK/SK is bit 0. : * TDI/DO is bit 1. : * TDO/DI is bit 2. : * TMS/CS is bit 3. : * GPIOL0 is bit 4. : * GPIOL1 is bit 5. : * GPIOL2 is bit 6. : * GPIOL3 is bit 7. > Shouldn't this be code, e.g. macros or an enum? […] I don't know, the identifiers wouldn't be used in the code. It's mostly reading a number from the UI and shifting that into a register value. Or we could write less condensed code, e.g. switch (input) { case 0: cs_bits |= PIN_GPIOL0; ... break; or const int bits[] = { PIN_GPIOL0, PIN_GPIOL1, PIN_GPIOL2, PIN_GPIOL3 }; cs_bits |= bits[input]; ... Not sure if I'd want to write it either way. What do you think? -- To view, visit https://review.coreboot.org/c/flashrom/+/55695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Gerrit-Change-Number: 55695 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 15:08:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Normalize error paths in ft2232_shutdown()
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55696 ) Change subject: ft2232_spi: Normalize error paths in ft2232_shutdown() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55696 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5158d06127a9a8934b083e48b69d29c4d5a11831 Gerrit-Change-Number: 55696 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 20 Jun 2021 15:05:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Revise error message
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55697 ) Change subject: ft2232_spi: Revise error message ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55697 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I125ae9ec0d5487fc26d588a7fd6c54da4ebd0d70 Gerrit-Change-Number: 55697 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 20 Jun 2021 15:04:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Normalize error paths in ft2232_shutdown()
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Anastasia Klimchuk. Hello Alan Green, Anastasia Klimchuk, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55696 to review the following change. Change subject: ft2232_spi: Normalize error paths in ft2232_shutdown() ...................................................................... ft2232_spi: Normalize error paths in ft2232_shutdown() We missed to `free(spi_data)` on one path. It also seems odd to leak the return code of a locally used library into our common infrastruc- ture, so normalize all error paths to return 1. Change-Id: I5158d06127a9a8934b083e48b69d29c4d5a11831 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M ft2232_spi.c 1 file changed, 7 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/55696/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index e546f6c..fbea0f9 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -170,10 +170,10 @@ static int ft2232_shutdown(void *data) { - int f; struct ft2232_data *spi_data = (struct ft2232_data *) data; struct ftdi_context *ftdic = &spi_data->ftdic_context; unsigned char buf[3]; + int ret = 0; msg_pdbg("Releasing I/Os\n"); buf[0] = SET_BITS_LOW; @@ -182,16 +182,18 @@ if (send_buf(ftdic, buf, 3)) { msg_perr("Unable to set pins back inputs: (%s)\n", ftdi_get_error_string(ftdic)); + ret = 1; } - if ((f = ftdi_usb_close(ftdic)) < 0) { - msg_perr("Unable to close FTDI device: %d (%s)\n", f, + const int close_ret = ftdi_usb_close(ftdic); + if (close_ret < 0) { + msg_perr("Unable to close FTDI device: %d (%s)\n", close_ret, ftdi_get_error_string(ftdic)); - return f; + ret = 1; } free(spi_data); - return 0; + return ret; } static bool ft2232_spi_command_fits(const struct spi_command *cmd, size_t buffer_size) -- To view, visit https://review.coreboot.org/c/flashrom/+/55696 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5158d06127a9a8934b083e48b69d29c4d5a11831 Gerrit-Change-Number: 55696 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit