June 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
by Michał Żygowski (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Nico Huber. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55647 ) Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: I guess we just need a rebase here to resolve conflicts -- To view, visit https://review.coreboot.org/c/flashrom/+/55647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Gerrit-Change-Number: 55647 Gerrit-PatchSet: 4 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.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-Comment-Date: Wed, 30 Jun 2021 12:19:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Nico Huber (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: I wanted to say: let's not bikeshed, we won't need it for long. But then again, do we need it at all? All the candidates for this function will likely get a shutdown NULL pointer once the allocation moves into common code. No matter if we introduce default_shutdown() or not, right? -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 09:42:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Nico Huber (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 3: (2 comments) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/bca06c00_85043fb4 PS3, Line 34: /* Define libusb symbols to avoid dependency on libusb.h */ Now that I realized that we don't mock `libusb.h` for the code under test. I wonder why we should avoid it here? It doesn't hurt so I'm ok with it. Just wondering. (I guess it wouldn't be hard to build the tests without libusb, but that can still be done if anyone ever needs it.) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/3c357fa5_e8df9617 PS2, Line 52: */ > Done, it works! but I'm not sure I understand how it works... […] Oh, I missed earlier that you avoid the libusb dependency here while it's still needed for the tests to build (includes of the code under test). There are some explanations why this works. Here is why this is legal in C: * The typedef is somewhat transparent. You can repeat it whereever you want, as long as it's the same type. * Just saying `struct libusb_device_handle;` without a member list makes it an incomplete type. That's good enough to declare pointers as long as the compiler doesn't need to know the size of the type, i.e. we don't try to dereference anything. Strictly speaking, the type we use here is not `libusb_device_handle`, it is `pointer to libusb_device_handle`. * Naturally, if incomplete types are allowed, it must be allowed to mix them with the actual (complete) type in another compilation unit. Otherwise they would be rather useless. (NB. Incomplete types are a means of information hiding in C.) Here is my favorite explanation: It all doesn't matter ;) because the compiler only looks at compilation units individually. And the linker who stitches the compilation results of the various compilation units together doesn't know C and its types. It doesn't even have to know parameters. There's a call to a symbol, it replaces the symbol with an address. That's about it. To get a working program out of it, the compiler needs to know the size of the parameters, and the size needs to match across compilation units. All pointers have the same size in C which makes it work even if we'd use `int *` here. -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 08:53:21 +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

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Angel Pons (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 08:36:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
by Nico Huber (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > thank you, this is actually very useful! (as usual!) […] The "few" wasn't meant literally :) About other masters, well, it's not very obvious indeed. As always it's hard to see/show absence of something. And it turns out that the `internal` programer might actually be the only case of this: Let's see what files have a `struct programmer_entry`: $ git grep -l '^const struct programmer_entry ' *.c and which call some `register_*master()`: $ git grep -l '\<register_$[a-z_]*$master(' *.c and compare the two: $ diff -u <(git grep -l '^const struct programmer_entry ' *.c) <(git grep -l '\<register_$[a-z_]*$master(' *.c) Some of it is noise (opaque.c, bitbang_spi.c, programmer.c are just infrastructure). But there are some masters without a programmer entry: * ichspi.c * it85spi.c * it87spi.c * mcp6x_spi.c * sb600spi.c * wbsio_spi.c All files not touched by commit 4f5169df. And I assume all part of the `internal` programmer. -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 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: 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: Thomas Heijligen <src(a)posteo.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: Wed, 30 Jun 2021 08:19:30 +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

1 0

Change in flashrom[master]: dediprog: Separate shutdown from failed init cleanup
by Angel Pons (Code Review) June 30, 2021

June 30, 2021

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55887 ) Change subject: dediprog: Separate shutdown from failed init cleanup ...................................................................... dediprog: Separate shutdown from failed init cleanup Shutdown function was covering two different jobs here: 1) the actual shutdown which is run at the end of the driver's lifecycle and 2) cleanup in cases when initialisation failed. Now, shutdown is only doing its main job (#1), and the driver itself is doing cleanup when init fails (#2). The good thing is that now resources are released/closed immediately in cases when init fails (vs shutdown function which was run at some point later), and the driver leaves clean space after itself if init fails. And very importantly this unlocks API change which plans to move register_shutdown inside register master API, see https://review.coreboot.org/c/flashrom/+/51761 BUG=b:185191942 TEST=builds Change-Id: I3273da907614a042d50090338c337dfd64695354 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55887 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c 1 file changed, 12 insertions(+), 8 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved diff --git a/dediprog.c b/dediprog.c index 6a2d1f2..b47e8e9 100644 --- a/dediprog.c +++ b/dediprog.c @@ -1227,16 +1227,13 @@ msg_pinfo("Using dediprog id SF%06d.\n", found_id); } - if (register_shutdown(dediprog_shutdown, NULL)) - return 1; - /* Try reading the devicestring. If that fails and the device is old (FW < 6.0.0, which we can not know) * then we need to try the "set voltage" command and then attempt to read the devicestring again. */ if (dediprog_check_devicestring()) { if (dediprog_set_voltage()) - return 1; + goto init_err_cleanup_exit; if (dediprog_check_devicestring()) - return 1; + goto init_err_cleanup_exit; } /* SF100/SF200 uses one in/out endpoint, SF600 uses separate in/out endpoints */ @@ -1261,11 +1258,11 @@ dediprog_set_spi_speed(spispeed_idx) || dediprog_set_spi_voltage(millivolt)) { dediprog_set_leds(LED_ERROR); - return 1; + goto init_err_cleanup_exit; } if (dediprog_standalone_mode()) - return 1; + goto init_err_cleanup_exit; if ((dediprog_devicetype == DEV_SF100) || (dediprog_devicetype == DEV_SF600 && protocol() == PROTOCOL_V3)) @@ -1274,10 +1271,17 @@ if (protocol() >= PROTOCOL_V2) spi_master_dediprog.features |= SPI_MASTER_4BA; + if (register_shutdown(dediprog_shutdown, NULL)) + goto init_err_cleanup_exit; + if (register_spi_master(&spi_master_dediprog, NULL) || dediprog_set_leds(LED_NONE)) - return 1; + return 1; /* shutdown function does cleanup */ return 0; + +init_err_cleanup_exit: + dediprog_shutdown(NULL); + return 1; } const struct programmer_entry programmer_dediprog = { -- To view, visit https://review.coreboot.org/c/flashrom/+/55887 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3273da907614a042d50090338c337dfd64695354 Gerrit-Change-Number: 55887 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-MessageType: merged

1 0

Change in flashrom[master]: buspirate_spi: Use non-zero return value from buspirate_sendrecv
by Angel Pons (Code Review) June 30, 2021

June 30, 2021

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55818 ) Change subject: buspirate_spi: Use non-zero return value from buspirate_sendrecv ...................................................................... buspirate_spi: Use non-zero return value from buspirate_sendrecv Return values for buspirate_sendrecv come from serialport_write and serialport_read, and those return 1s for any error. No need to explicity assign ret = 1, because it is already 1 for error. Follow up on commit 751afa88a7f22487069665c5d206ae490f158231 where this idea was suggested. BUG=b:185191942 TEST=builds Change-Id: I31fd70f607dc965d5cac1cd0116faa447dbc177a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55818 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M buspirate_spi.c 1 file changed, 5 insertions(+), 15 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/buspirate_spi.c b/buspirate_spi.c index bfd0e05..b56d422 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -629,10 +629,8 @@ /* Enter raw SPI mode */ bp_commbuf[0] = 0x01; ret = buspirate_sendrecv(bp_commbuf, 1, 0); - if (ret) { - ret = 1; + if (ret) goto init_err_cleanup_exit; - } if ((ret = buspirate_wait_for_string(bp_commbuf, "SPI"))) goto init_err_cleanup_exit; if ((ret = buspirate_sendrecv(bp_commbuf, 0, 1))) @@ -655,10 +653,8 @@ msg_pdbg("Enabling PSUs.\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) { - ret = 1; + if (ret) goto init_err_cleanup_exit; - } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting power/CS/AUX(/Pull-up resistors)!\n"); ret = 1; @@ -668,10 +664,8 @@ /* Set SPI speed */ bp_commbuf[0] = 0x60 | spispeed; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) { - ret = 1; + if (ret) goto init_err_cleanup_exit; - } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI speed!\n"); ret = 1; @@ -685,10 +679,8 @@ msg_pdbg("Pull-ups enabled, so using HiZ pin output! (Open-Drain mode)\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) { - ret = 1; + if (ret) goto init_err_cleanup_exit; - } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI config!\n"); ret = 1; @@ -698,10 +690,8 @@ /* De-assert CS# */ bp_commbuf[0] = 0x03; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) { - ret = 1; + if (ret) goto init_err_cleanup_exit; - } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); ret = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/55818 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31fd70f607dc965d5cac1cd0116faa447dbc177a Gerrit-Change-Number: 55818 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-MessageType: merged

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Angel Pons (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 3: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55934/comment/2d78b895_95715f3a PS3, Line 9: This patch adds mocks for libusb functions. To avoid dependency on I expect that some of the code could be reused to test other libusb programmers. Would it make sense to have libusb mocks in a separate compilation unit? File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55934/comment/710bfe4a_1e00f15d PS3, Line 98: /* dediprog_cmds CMD_READ_PROG_INFO */ Hmmm, how about making a patch that moves the dediprog definitions the tests need into a header? File tests/tests.c: https://review.coreboot.org/c/flashrom/+/55934/comment/2454c11f_9a054ff2 PS3, Line 140: 2021 Is this magic number the current year? Or does it have another meaning? -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 08:11:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests.c: Move opening brace on new line for function body
by Angel Pons (Code Review) June 30, 2021

June 30, 2021

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55974 ) Change subject: tests.c: Move opening brace on new line for function body ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55974 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia1b00bb68ec2426161bfc183d93d2c0719a949ec Gerrit-Change-Number: 55974 Gerrit-PatchSet: 1 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 08:05:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
by Anastasia Klimchuk (Code Review) June 29, 2021

June 29, 2021

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ...................................................................... Patch Set 2: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55932/comment/1744ba4e_7fb50a4d PS1, Line 14: adding another argument to register_spi_master. > The new version seems better! If the shutdown function can be […] I am so happy to hear this! Patchset: PS1: > > I wasn't sure which one of the structs is the right place for the shutdown function, and even more […] thank you, this is actually very useful! (as usual!) If master is a right place, than the whole change can be done in 3 chunks spi/par/opaque (like we did with the data). Then moving data and shutdown into registered_master can be next step? But that would be internal to the core, callers won't even notice (or shouldn't notice to be precise). >>> Only in the same file for the "few" masters that you have been looking at so far ;) I had commit 4f5169df5ff2b63a40d654ab133b8a3108b6a599 in mind when I said "in the same file", looks like lots of masters, but maybe I misunderstand something? File linux_spi.c: https://review.coreboot.org/c/flashrom/+/55932/comment/57cd61d5_24cfa2c0 PS1, Line 243: if (register_spi_master(&spi_master_linux, spi_data)) : return 1; /* core does cleanup */ : return 0; : > Or just […] Done File programmer.h: https://review.coreboot.org/c/flashrom/+/55932/comment/097e9372_b256c172 PS1, Line 359: shutdown > I think we'll need shutdown functions for both eventually. Starting with […] >>>for this to be ideally called `deinit` and logically one level up in `struct programmer_entry` There is no data one level up, but shutdown function needs data. I started thinking about programmer_entry, but then returned back to master struct, because it has data. Maybe deinit is a different thing to what is currently called "shutdown function"? -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 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: 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 30 Jun 2021 06:48:41 +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> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0