Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52365 )
Change subject: flashrom.c: Fixup stale FIXME comment when doit() was removed
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52365
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641
Gerrit-Change-Number: 52365
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Thu, 15 Apr 2021 12:23:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
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/+/52364 )
Change subject: programmer.h,chipset_enable.c: Make penable cb more descript
......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52364/comment/b9912f28_0edb9a88
PS1, Line 7: Make penable cb more descript
Maybe it's just me, but `cb` resolves to `coreboot`. How about:
Rename penable callback
https://review.coreboot.org/c/flashrom/+/52364/comment/537cdc12_f7bb2356
PS1, Line 15: use
typo use*d*
File programmer.h:
https://review.coreboot.org/c/flashrom/+/52364/comment/87bff6ba_275817c9
PS1, Line 228: enable_flash_xxx
> What does xxx mean here? Maybe there is the reason for xxx? Is it possible to have the function name […]
I'd prefer `enable_flash` too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52364
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d
Gerrit-Change-Number: 52364
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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: Thu, 15 Apr 2021 12:20:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Walker.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52310 )
Change subject: flashchips: Add Spansion/Cypress S25FL256L
......................................................................
Patch Set 3: Code-Review+1
(2 comments)
Patchset:
PS1:
> Thank you, I'm pretty new to contributing to public repositories, and this review process is a littl […]
No worries. Mistakes are a part of the learning process 😊
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/52310/comment/ef4207c9_555870cb
PS3, Line 16380:
nit: tabs here too (see other chips)
--
To view, visit https://review.coreboot.org/c/flashrom/+/52310
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4e8cba554d0590c94dac92aa91e9ab400efca281
Gerrit-Change-Number: 52310
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 12:09:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Thomas Walker <thh.walker(a)gmail.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52365 )
Change subject: flashrom.c: Fixup stale FIXME comment when doit() was removed
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52365/comment/5c7ceed3_e4e66068
PS1, Line 7: Fixup
Nit: The verb is spelled with a space: Fix up
--
To view, visit https://review.coreboot.org/c/flashrom/+/52365
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641
Gerrit-Change-Number: 52365
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 08:15:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52364 )
Change subject: programmer.h,chipset_enable.c: Make penable cb more descript
......................................................................
Patch Set 1:
(1 comment)
File programmer.h:
https://review.coreboot.org/c/flashrom/+/52364/comment/a5bce4ad_6b3ce4f3
PS1, Line 228: enable_flash_xxx
What does xxx mean here? Maybe there is the reason for xxx? Is it possible to have the function name just "enable_flash"?
Thank you for renaming doit.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52364
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d
Gerrit-Change-Number: 52364
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 05:09:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Daniel Campello, Angel Pons.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 )
Change subject: cli_classic: Make -r/-w/-v argument optional
......................................................................
Patch Set 1:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/52362/comment/04d0f4b0_7b0681d5
PS1, Line 46:
I'm not too familiar with what the design considerations are here, so take this with a grain of salt, but I wonder if there is a way to make the flags more orthogonal. Right now, these two invocations would be equivalent:
-r some_file.bin -i some_region
-r -i some_region:some_file.bin
One option is to get rid of "-i" completely and make -r/-w/-v repeated with optional region parameters, e.g. something like
-r some_file.bin
-r some_file.bin:some_region
-r some_file.bin:some_region -r some_other_file.bin:some_other_region
But if we need to keep "-i" around, then everything here seems reasonable.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52362
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41
Gerrit-Change-Number: 52362
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:48:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52285 )
Change subject: linux_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52285/comment/7e10acea_861a7dd8
PS2, Line 178: size_t max_kernel_buf_size;
> s/these lines/this line/
I introduced this line here, because I remove global state in this patch, and in previous patches max_kernel_buf_size is still global and don't need local definition in init function.
What do you think?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078
Gerrit-Change-Number: 52285
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:47:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52283 )
Change subject: linux_spi.c: Extract get_max_kernel_buf_size() as a function
......................................................................
Patch Set 1:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52283/comment/478435d1_24c5304e
PS1, Line 169: const uint8_t bits = 8;
> missing the line: […]
Oh, I can explain why this line is not here. At this patch, max_kernel_buf_size is still a global state (together with fd), they are defined in lines 47, 49. I am not removing global state in this patch, so I left it as is, and since max_kernel_buf_size is global I don't need to define it locally in init function.
I do remove global state later in https://review.coreboot.org/c/flashrom/+/52285 , but not here.
What do you think about it? I was trying to keep the purpose for each patch very focused.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52283
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4b8c5775fb8f4b0dff702fcc0fb258221254c659
Gerrit-Change-Number: 52283
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:44:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment