November 2021 - flashrom-gerrit

Change in flashrom[master]: flashchips,writeprotect_ranges: add range decoding function
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58480 ) Change subject: flashchips,writeprotect_ranges: add range decoding function ...................................................................... Patch Set 10: (4 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58480/comment/66cd25fd_b76a7afb PS10, Line 202: range_decode_fn_t In this typedef wp_ prefix would be informative. And to save some line length, you can omit cfg and range param names, they are self-explanatory from param types. https://review.coreboot.org/c/flashrom/+/58480/comment/43c08639_b507f3e0 PS10, Line 319: decode_range If typedef gets wp_ prefix, this name can be decode_wp_range. File writeprotect_ranges.c: https://review.coreboot.org/c/flashrom/+/58480/comment/1c0641a9_b19275a1 PS10, Line 19: ithat typo https://review.coreboot.org/c/flashrom/+/58480/comment/8c98bccb_c410ecfa PS10, Line 40: range->chip_len; Same as Sergii said, chip_len doesn't seem to belong to range. It should be possible to have it as decode_range_w25 function argument, right? (I still think chip pointer is not needed in wp_chip_config) -- To view, visit https://review.coreboot.org/c/flashrom/+/58480 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id163ed80938a946a502ed116e48e8236e36eb203 Gerrit-Change-Number: 58480 Gerrit-PatchSet: 10 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 05:34:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add support for reading/writing WP configs
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add support for reading/writing WP configs ...................................................................... Patch Set 9: (7 comments) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/8a6c2b8e_8dbbc368 PS9, Line 131: int flashrom_wp_read_chip_config(const struct flashrom_flashctx *flash, struct flashrom_wp_chip_config *cfg); : int flashrom_wp_write_chip_config(struct flashrom_flashctx *flash, const struct flashrom_wp_chip_config *cfg); : int flashrom_wp_compare_chip_configs(const struct flashrom_wp_chip_config *cfga, const struct flashrom_wp_chip_config *cfgb); You can omit parameter names if they are self-explanatory from parameter type, which all of these seems to be (see existing declarations in this header). File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/328d703b_795e062b PS9, Line 4: 2010 Does anyone know, should we add 2021 here? Does it even matter? The file is completely re-written. https://review.coreboot.org/c/flashrom/+/58479/comment/f81f657d_7873ed6a PS9, Line 27: #define wp_chip_config flashrom_wp_chip_config : #define wp_range flashrom_wp_range : #define wp_mode flashrom_wp_mode Oh, so why this prefix was added from the very beginning? ;) https://review.coreboot.org/c/flashrom/+/58479/comment/d3162d71_fbdc6f4c PS9, Line 39: if (spi_read_register(flash, bit.reg, value)) : return 1; Let's propagate return value, ret = spi_read_register if (ret) return ret Maybe now it's always 1 on error, but who knows, may change in future. https://review.coreboot.org/c/flashrom/+/58479/comment/35188478_988d0c0d PS9, Line 128: return ret; This should be on its own line (and same below). https://review.coreboot.org/c/flashrom/+/58479/comment/0ea74958_2a7a3bdf PS9, Line 158: if (ord == 0) ord = cfga->srp[i] - cfgb->srp[i]; How does it work? If ord becomes non-0 in the first loop cycle, then the rest of (ord == 0) are skipped? is it the same as having `else return ord`? https://review.coreboot.org/c/flashrom/+/58479/comment/8ae08f38_3df56450 PS9, Line 161: ord = cfga->cmp - cfgb->cmp; Please make it on its own line, and the same for other conditionals below (including ones inside for loops). -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 05:06:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flash: add structure to represent chip wp configuration
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58478 ) Change subject: flash: add structure to represent chip wp configuration ...................................................................... Patch Set 9: (2 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58478/comment/c56968e4_a9ca7d59 PS9, Line 203: flashrom_ I am thinking... do we really need "flashrom" prefix in this struct type name? If "flash" is an important piece of info (I don't think so, but what if) maybe we can have `wp_flashchip_config` as a name? https://review.coreboot.org/c/flashrom/+/58478/comment/6668f3cd_59cae7e8 PS9, Line 219: /* Used for intrepreting configuration bits above */ : const struct flashchip *chip; A question: is interpreting config bits implemented in CB:58479 or there are other patches? In CB:58479 all the places where chip config is used also have flash context available, and flashcontext has chip already. So just for CB:58479 this pointer is not needed? -- To view, visit https://review.coreboot.org/c/flashrom/+/58478 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809 Gerrit-Change-Number: 58478 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 04:33:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: add writeprotect bit layout map to chips
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58477 ) Change subject: flashchips: add writeprotect bit layout map to chips ...................................................................... Patch Set 9: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/58477/comment/cb17cd3f_aaa6e900 PS9, Line 6760: {STATUS1, 5, RW} The same entry repeats for .bp and .tb , what does it mean? the same bit in the same register is responsible for two different things? -- To view, visit https://review.coreboot.org/c/flashrom/+/58477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284 Gerrit-Change-Number: 58477 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 04:15:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: make register read/write functions generic
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 ) Change subject: spi25_statusreg: make register read/write functions generic ...................................................................... Patch Set 9: (1 comment) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/58475/comment/27334e8c_cfda5f3d PS9, Line 120: msg_cerr("Cannot read register: not supported by chip\n"); > Do we need the same error for spi_write_register? Just to clarify: here the error comes in else clause, which I understand. In spi_write_register there is no else clause to register condition. -- To view, visit https://review.coreboot.org/c/flashrom/+/58475 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7 Gerrit-Change-Number: 58475 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:15:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg,flashchips: add SR2 read/write support
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58570 ) Change subject: spi25_statusreg,flashchips: add SR2 read/write support ...................................................................... Patch Set 7: (1 comment) Patchset: PS7: Is this the latest and up-to-date version of CB:58476 ? If yes, maybe the latter can be abandoned? -- To view, visit https://review.coreboot.org/c/flashrom/+/58570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db Gerrit-Change-Number: 58570 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:13:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg,flashchips: add SR2 read/write support
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58570 ) Change subject: spi25_statusreg,flashchips: add SR2 read/write support ...................................................................... Patch Set 7: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/58570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db Gerrit-Change-Number: 58570 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:12:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: make register read/write functions generic
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 ) Change subject: spi25_statusreg: make register read/write functions generic ...................................................................... Patch Set 9: (4 comments) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/58475/comment/2847260c_cd1346f9 PS9, Line 31: If you add new line it looks nicer, and easier to read (and I think consistent with most of long comments in codebase). https://review.coreboot.org/c/flashrom/+/58475/comment/fb4750f0_7b2fb994 PS9, Line 32: Is this one extra space? probably not needed? https://review.coreboot.org/c/flashrom/+/58475/comment/b83ee274_67635b15 PS9, Line 120: msg_cerr("Cannot read register: not supported by chip\n"); Do we need the same error for spi_write_register? https://review.coreboot.org/c/flashrom/+/58475/comment/35630664_591f8c6b PS9, Line 143: uint8_t status; Is it 0 initialised? -- To view, visit https://review.coreboot.org/c/flashrom/+/58475 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7 Gerrit-Change-Number: 58475 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:06:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect, cli_classic: delete old writeprotect code
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58474 ) Change subject: writeprotect, cli_classic: delete old writeprotect code ...................................................................... Patch Set 9: (3 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/58474/comment/5ef3da0b_34f46a4c PS9, Line 159: int set_wp_enable, : int set_wp_disable, : int wp_status, : int set_wp_range, : int wp_list, These all are commands/actions? I noticed some of them start with verb "set" and others start with a noun "wp". It would be great to have consistent names for all actions. Not sure whether this better be in this patch or later. This chunk of code is new so can be renamed without consequences... on the other hand renaming args here would make them inconsistent with existing variables in cli_classic. https://review.coreboot.org/c/flashrom/+/58474/comment/957cb995_739a5865 PS9, Line 786: set_wp_range || (set_wp_region && wp_region) A tiny bit confusing, because function param is named simply `set_wp_range` however actual argument is more than that. Maybe add a comment? What is the difference between wp range and wp region? File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58474/comment/82a1ca93_29fb627a PS9, Line 4: 2010 It is 2021 now ;) -- To view, visit https://review.coreboot.org/c/flashrom/+/58474 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67e9b31f86465e5a8f7d3def637198671ee818a8 Gerrit-Change-Number: 58474 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 01:48:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Split very long init function into two
by Anastasia Klimchuk (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 ) Change subject: ichspi: Split very long init function into two ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I want to talk more about this patch. Yes it splits init function into two: init_ich7_spi and init_ich_default. However another thing here is: initialisation of hwseq and swseq that used to happen in the beginning of init function now moved to init_ich_default. I did this after staring at the code for a while, the code looks like hwseq and swseq are only used for chipsets served by init_ich_default. I could make a mistake, because both hwseq and swseq are global state (global state that I hope to remove one day), global state makes it harder to read the code and understand what is used where. This chain is hanging for so long (sorry) because I am trying to find devices to test older chipsets which are served by init_ich7_spi. If anyone here knows that hwseq and swseq are actually used for chipsets on init_ich7_spi, please tell me (thanks!!), this would mean I made a mistake and need to fix this patch. Now that I look at my own comment, I see that commit message should be updated. Leaving this comment unresolved for this. -- To view, visit https://review.coreboot.org/c/flashrom/+/58735 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b Gerrit-Change-Number: 58735 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 16 Nov 2021 01:05:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2021