October 2017 - flashrom-gerrit

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 03 Oct '17

03 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21828 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: > (1 comment) > > > ... Hence I want to have such rather benign things in > > it if possible. > > Yeah, that's ok, I guess. We are already making exceptions for > the Git conversion commit anyway. Generally, I'd prefer only > changes that fix semantical bugs in the code to be fixups, i.e. > what you really need to make it "stable". Instead of stable or "random" we might better call it subjectively_perfect or something ;) Staging was meant to iron out the most obvious problems and I would then work on the minor details before merging them to stable. The current process (including the lag of stable) is not as it was envisaged... we will see how the next weeks will work out, then I'll write something down/propose a more strictly ruled process. > > The comment in question documents intent to make this clear for > > reviewers as contributors (while the sh shebang should be a clear > > hint I think the comment is still warranted due to the local > > restriction and its outspokenness). There was no strict review > for > > this I guess but a) I always intended it to be that way (but of > > course there might be bugs... e.g., I wasn't aware local is not > > POSIX before you brought it up), b) you had a look for that > > obviously in the first review. > > Not sure if that was me. At least I don't recall really checking > the whole file. Maybe just rephrase (see inline comment), I'd > like to avoid compatibility claims if we are not 100% sure. (It's > my code-comment allergy. I've seen too many misleading comments > over the years. So I prefer to write code that explains itself.) > Also, who'd maintain the comment if anybody puts another non- > POSIX construct in this file? Adding the comment was meant to exactly reduce the chance of somebody adding non-POSIX code without himself or a reviewer noticing it. The point is that we want the script to work on all possible build systems of course... and there we get away with little violations. We just need to keep it in mind. I like your suggestion of damping down the authority of the comment very much (see next PS). -- To view, visit https://review.coreboot.org/21828 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ia60186f783067ba084439a8ef701dc8f4c0072f0 Gerrit-Change-Number: 21828 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 04 Oct 2017 00:37:04 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 03 Oct '17

03 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21827 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: > (1 comment) > > Don't see that the hidden behavioural change deserves a fixup. Let > me know if you have strong feelings about that. <the usual initial git commit argument blabla> The commit message tries to give some motivations on why these are good ideas. Less interdependence between (the code in) the makefile, the actual vcs used, and getrevision.sh; and it's less but more readable code too :) -- To view, visit https://review.coreboot.org/21827 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iedc9df4c033a70447b8b1b65c83764c769b02c3f Gerrit-Change-Number: 21827 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 04 Oct 2017 00:25:14 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 03 Oct '17

03 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21825 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: > I have a strong preference, indeed. But I can't force anybody as > there > is no written rule. My general impression is: When somebody makes > the > visible width (end minus indentation) of a text block wider than 80 > chars, he doesn't care if somebody will read it. If he makes it > wider > than 100 chars, he doesn't want it to be read. > > The typesetting rule is to break around 60~70 chars (going back to > typesetting research done some centuries ago, iirc). I don't really disagree with you on the readability of shorter lines in general... I write almost everything I publish for others to read in Latex-based PDFs for a reason after all ;) However, the cited 60-70 cpl are for continuous justified text printed out on paper. For source code where fixed-point fonts are used in very short paragraphs these results are not necessarily true and there are studies that disprove the theory that e.g. 100 cpl is less legible than shorter lines on average when reading from screens as well. That said, I would not mind a shorter maximum length of comment lines if we agree on some rule (e.g. 75 cpl hard limit) although it would be a bit of a nuisance to comply with that (I know of no editor that provides such an dynamic guide easily). If you want such a rule please post a mail on the mailing list with your proposition so that others can chime in and we have something to refer to in the future if a consensus is reached. Until then I will continue using the space to the maximum column limit of 112. With the help of git hooks we might check for these things automatically though... if someone wants to hack on that ;) -- To view, visit https://review.coreboot.org/21825 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iff4021be49a9fab208b619c555b9f9e81f671ab8 Gerrit-Change-Number: 21825 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 04 Oct 2017 00:05:21 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 03 Oct '17

03 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21834 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push@34 PS1, Line 34: $\([0-9]\+\.$\+$[0-9]\+$\)$ > Outer parentheses: because that's needed when using the matching expression Same paragraph mentions ^ is implicit and unclear what happens if you use it. So better leave it out. Shorter alternative: $\([0-9]\+\.$\{2,\}x\)$ -- To view, visit https://review.coreboot.org/21834 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I1d4b4a7ef2673cabee980ec4a7d7d5fbebdcaed1 Gerrit-Change-Number: 21834 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 03 Oct 2017 13:08:11 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! nicintel_eeprom: Support for I210 emulated EEprom
by David Hendricks (Code Review) 02 Oct '17

02 Oct '17

David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/21702 ) Change subject: fixup! nicintel_eeprom: Support for I210 emulated EEprom ...................................................................... fixup! nicintel_eeprom: Support for I210 emulated EEprom A couple of C99-style variable declarations within loops are causing compilation failures on some systems (gcc 4.9.2-10 on Raspbian). This moves them to make gcc happy. Change-Id: Ib7ad5a69244e462f84eae93df9e841716e089b31 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-on: https://review.coreboot.org/21702 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M nicintel_eeprom.c 1 file changed, 4 insertions(+), 2 deletions(-) Approvals: Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/nicintel_eeprom.c b/nicintel_eeprom.c index e2fb584..e4a91ef 100644 --- a/nicintel_eeprom.c +++ b/nicintel_eeprom.c @@ -211,7 +211,8 @@ pci_mmio_writel(eewr, nicintel_eebar + EEWR); programmer_delay(5); - for (int i = 0; i < MAX_ATTEMPTS; i++) + int i; + for (i = 0; i < MAX_ATTEMPTS; i++) if (pci_mmio_readl(nicintel_eebar + EEWR) & BIT(EEWR_DONE)) return 0; return -1; @@ -419,7 +420,8 @@ flup |= BIT(EE_FLUPD); pci_mmio_writel(flup, nicintel_eebar + EEC); - for (int i = 0; i < MAX_ATTEMPTS; i++) + int i; + for (i = 0; i < MAX_ATTEMPTS; i++) if (pci_mmio_readl(nicintel_eebar + EEC) & BIT(EE_FLUDONE)) return 0; -- To view, visit https://review.coreboot.org/21702 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Ib7ad5a69244e462f84eae93df9e841716e089b31 Gerrit-Change-Number: 21702 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 02 Oct '17

02 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21834 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push@34 PS1, Line 34: $\([0-9]\+\.$\+$[0-9]\+$\)$ > Why the outer-most parentheses? Why not start with ^ ? and didn't Outer parentheses: because that's needed when using the matching expression of expr: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/expr.html#tag_20_… ^ is a good idea, no idea why it's not already there. Not sure about the last question... that 'x' is a literal one i presume. Works for me... so this would be '$^\([0-9]\+\.$\+$[0-9]\+$\.x\)$' https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push@51 PS1, Line 51: echo "Neither \"Signed-off-by\" nor \"Acked-by\" were found in commit $local_sha in " \ > Both Signed-off-by and Acked-by have to be present, don't they? this Right. Or to be more precise: this is a wrong intermediate wording from an earlier patch set of David. I should have left his last version as is, sorry. -- To view, visit https://review.coreboot.org/21834 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I1d4b4a7ef2673cabee980ec4a7d7d5fbebdcaed1 Gerrit-Change-Number: 21834 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 23:59:15 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! nicintel_eeprom: Support for I210 emulated EEprom
by build bot (Jenkins) (Code Review) 02 Oct '17

02 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21702 ) Change subject: fixup! nicintel_eeprom: Support for I210 emulated EEprom ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/640/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/696/ : SUCCESS -- To view, visit https://review.coreboot.org/21702 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ib7ad5a69244e462f84eae93df9e841716e089b31 Gerrit-Change-Number: 21702 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 23:05:39 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: flashchips: Add ISSI IS25WP128
by build bot (Jenkins) (Code Review) 02 Oct '17

02 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19532 ) Change subject: flashchips: Add ISSI IS25WP128 ...................................................................... Patch Set 1: Build Failed https://qa.coreboot.org/job/flashrom_gerrit/642/ : FAILURE https://qa.coreboot.org/job/flashrom-customrules/698/ : SUCCESS -- To view, visit https://review.coreboot.org/19532 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ia31764f0bf3ba1ed609f83b95e21343d7c23a508 Gerrit-Change-Number: 19532 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 23:05:39 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Add initial J-Link SPI programmer
by build bot (Jenkins) (Code Review) 02 Oct '17

02 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19994 ) Change subject: Add initial J-Link SPI programmer ...................................................................... Patch Set 4: Build Failed https://qa.coreboot.org/job/flashrom_gerrit/641/ : FAILURE https://qa.coreboot.org/job/flashrom-customrules/697/ : SUCCESS -- To view, visit https://review.coreboot.org/19994 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 19994 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 23:05:38 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[stable]: util: Add CH341a to z60_flashrom.rules
by build bot (Jenkins) (Code Review) 02 Oct '17

02 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21778 ) Change subject: util: Add CH341a to z60_flashrom.rules ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/639/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/695/ : SUCCESS -- To view, visit https://review.coreboot.org/21778 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: comment Gerrit-Change-Id: Id1ceebbf406f81a28bd6dae44acd6e569e0c834c Gerrit-Change-Number: 21778 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 23:05:38 +0000 Gerrit-HasComments: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2017