Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71622 )
Change subject: dmi.c: Pass is_laptop by ref into dmi
......................................................................
Patch Set 1:
(2 comments)
File dmi.c:
https://review.coreboot.org/c/flashrom/+/71622/comment/2bfda70c_1c51374e
PS1, Line 158: *is_laptop = 2;
For another patch: This should be an enum.
https://review.coreboot.org/c/flashrom/+/71622/comment/9efed287_947fd226
PS1, Line 402: is_laptop
nit: update comment too?
--
To view, visit https://review.coreboot.org/c/flashrom/+/71622
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3d5ad6c0623269492d775a99a947fd6fe26c5f91
Gerrit-Change-Number: 71622
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 13:04:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71621 )
Change subject: internal.c: Have preinit hook do cpu and bus init
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71621/comment/bcbd22eb_8670ea2e
PS1, Line 7: cpu
Not sure why it says "Processor", but it seems inaccurate. Not this change's problem.
https://review.coreboot.org/c/flashrom/+/71621/comment/7cedb353_513ed060
PS1, Line 10: initalisation
Insert an `i` after the first 4 letters: init_i_alisation
https://review.coreboot.org/c/flashrom/+/71621/comment/1c54aa0a_2dae6194
PS1, Line 11: initalise
Insert an `i` after the first 4 letters: init_i_alise
File internal.c:
https://review.coreboot.org/c/flashrom/+/71621/comment/301c4c48_bf08dd51
PS1, Line 245: if (try_mtd(cfg) == 0) {
: ret = 0;
: goto internal_init_exit;
: }
We don't need to initialise PCI access or perform flash enables if we're using MTD, do we?
Actually, we want to run the flash enables for MTD too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71621
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia1cfd89a4fccfa07ba3c9ee1f6df1422ab95fc76
Gerrit-Change-Number: 71621
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(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-Comment-Date: Mon, 02 Jan 2023 13:02:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71620 )
Change subject: internal.c: Add preinit hook
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File internal.c:
https://review.coreboot.org/c/flashrom/+/71620/comment/bcf3b6c0_92038464
PS1, Line 223: ret = get_params(cfg,
: &force_boardenable, &force_boardmismatch,
: &force_laptop, ¬_a_laptop,
: &board_vendor, &board_model);
: if (ret)
: return ret;
It would be nice to have this done in pre-init as well. To do so, one would need to add a pointer to an opaque struct. The definition of the opaque struct would be private for each programmer, so each programmer would define its own struct to store the values of programmer-specific cfg options. The end goal would be to separate command-line parameter parsing from actual programmer init.
We can (and should) discuss this in a video call, e.g. dev meeting or the like.
(Marking as unresolved only because we need to agree on where/when to discuss this further)
--
To view, visit https://review.coreboot.org/c/flashrom/+/71620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9c978e4e74b07a2276f98dcab149ae24b8220c5f
Gerrit-Change-Number: 71620
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(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-Comment-Date: Mon, 02 Jan 2023 12:28:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71579 )
Change subject: programmer: Add pre-init hook
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71579/comment/d73452c3_8f1bb4c3
PS1, Line 9: initalisation
Insert an `i` after the first 4 letters: init_i_alisation
https://review.coreboot.org/c/flashrom/+/71579/comment/bccb40f0_d9e240ca
PS1, Line 10: either
Nit: "either" suggests mutual exclusion which conflicts with the "or both" part at the end. Maybe remove "either"?
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/71579/comment/26c88c90_4c01b8d4
PS1, Line 164: Initialising
Very pedantic, buuut... The message for init uses the US spelling of "Initializing", with a `z` instead of an `s`. It would be nice to be consistent.
File include/programmer.h:
https://review.coreboot.org/c/flashrom/+/71579/comment/c4342fbc_d8a80249
PS1, Line 51: struct programmer_cfg *cfg
The lack of `const` is intentional, right?
--
To view, visit https://review.coreboot.org/c/flashrom/+/71579
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Icb9ba758650fbe3731021d6b8f95c46ea0d24af1
Gerrit-Change-Number: 71579
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(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-Comment-Date: Mon, 02 Jan 2023 12:20:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71578 )
Change subject: internal.c: Factor out laptop alerts into helper func
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71578/comment/70c4877f_70a6da22
PS1, Line 9: unfortant
Did you mean `unfortunate`?
File internal.c:
https://review.coreboot.org/c/flashrom/+/71578/comment/c7872b7f_77acef64
PS1, Line 167:
Add "from parameters", at first it seemed like this is about the function's name.
// FIXME: remove '_' suffix from parameters once global shadowing is fixed.
https://review.coreboot.org/c/flashrom/+/71578/comment/d037c1df_35b12832
PS1, Line 170: /* Report if a non-whitelisted laptop is detected that likely uses a legacy bus. */
Maybe keep this comment on the call site
--
To view, visit https://review.coreboot.org/c/flashrom/+/71578
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8eea91012e6539b4fdf5d49a75a9cb48bb8a57ca
Gerrit-Change-Number: 71578
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 12:15:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 )
Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71576/comment/1cbfabc0_2d247888
PS1, Line 7: TL
TGL
https://review.coreboot.org/c/flashrom/+/71576/comment/2ebab355_0be9f9ad
PS1, Line 9: As listed in coreboot source under src/include/device/pci_ids.h
There's several other IDs, but they aren't documented.
https://review.coreboot.org/c/flashrom/+/71576/comment/05b4d411_3ba476dd
PS1, Line 13: TLK
TGL
--
To view, visit https://review.coreboot.org/c/flashrom/+/71576
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5
Gerrit-Change-Number: 71576
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 12:12:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71577 )
Change subject: internal.c: Move sio register to own object
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File superio.c:
https://review.coreboot.org/c/flashrom/+/71577/comment/a7b98ff0_d32c041a
PS1, Line 40: //probe_superio_smsc();
Wow, this is dead code
--
To view, visit https://review.coreboot.org/c/flashrom/+/71577
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a4c3e12bce5d22492c8d1b8f4a3f49d736dcf31
Gerrit-Change-Number: 71577
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 12:09:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Edward O'Callaghan.
Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 )
Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/71576
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5
Gerrit-Change-Number: 71576
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 07:46:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment