- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: (1 comment) Patchset: PS6: > > Therefore, 'pullups=on' inherently works as an open collector. […] Is it correct to say that if pullups=on then the value of hiz doesn't matter? exactly, I will return an error with pullups=on + hiz=off -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:45:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: (1 comment) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79299/comment/5ac3eeb8_ed7c3007 : PS7, Line 709: (Open-Drain mode) > Is this always true? If both params are set, `pullup == true && hiz == true`, from you explanation I […] pullups == true means open-drain mode + enable internal pull-ups hi-z == true mean open drain mode So yes, pullup or hiz means always: Open-Drain mode -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:32:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:31:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: all done -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:26:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

David Reguera Garcia has uploaded a new patch set (#2). ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... buspirate_spi: return init error for invalid values of pullups, hiz, psus Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/79407/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: (6 comments) Patchset: PS6: > Therefore, 'pullups=on' inherently works as an open collector. Thus, setting both 'pullups=on' and 'hiz=on' is redundant but not incorrect. My main idea was: if this is a valid combination, it would be great to document it. Is it correct to say that if `pullups=on` then the value of `hiz` doesn't matter? From the user point of view, is there any difference between running with: 1) pullups=on (hiz not set) 2) pullups=on,hiz=on 3) pullups=on,hiz=off I think it would be useful to add one more sentence to the man page, clarifying what happens when various combinations of two params are used. If any of 1,2,3 is invalid combinations, you can catch that and return, as in the other patch. Patchset: PS7: David, thank you so much for your patches! I definitely want them both to get submitted. I went through the diffs in more detail and added a few comments. This is a very normal process of code review, you would typically get a few comments, fix/respond to them, and upload new patchset to the same patch. If all is good, all comments resolved, the patch will be approved. File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79299/comment/7a94f6d6_c3364bd1 : PS7, Line 709: (Open-Drain mode) Is this always true? If both params are set, `pullup == true && hiz == true`, from you explanation I understand that will be open collector? I just want to make sure the message is correct. This branch is executed if `pullup || hiz` which includes the cases one of the params true, or they both true. File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/79299/comment/2ccf5c95_7e04852a : PS7, Line 766: trailing whitespace, but you don't need a new line here, you can make new line after the end of sentence `resistors.`) https://review.coreboot.org/c/flashrom/+/79299/comment/6c7e6899_f5b4116d : PS7, Line 765: : When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. Could you swap this new paragraph about `hiz` with existing about `pullups` param? It seems more logical, first there will be information on `pullups` param, and then further explanation for the case `pullups` param is not enough and you need to use `hiz`. https://review.coreboot.org/c/flashrom/+/79299/comment/d681e339_4ed2da4a : PS7, Line 767: trailing whitespace -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:23:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Reguera Garcia <dreg(a)rootkit.es> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: Return init error for invalid values of pullups, hiz, psus
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

David Reguera Garcia has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: Return init error for invalid values of pullups, hiz, psus ...................................................................... Return init error for invalid values of pullups, hiz, psus Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/79407/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index 7436d80..bcc2964 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -375,8 +375,11 @@ hiz = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid hiz state, not using them.\n"); + else { + msg_perr("Invalid hiz state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); @@ -386,8 +389,11 @@ pullup = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid pullups state, not using them.\n"); + else { + msg_perr("Invalid pullups state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); @@ -397,8 +403,11 @@ psu = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid psus state, not enabling.\n"); + else { + msg_perr("Invalid psus state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... Patch Set 1: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/79405/comment/3ba96206_92280869 : PS1, Line 7: Stop execution when invalid params Return init error for invalid values of pullups, hiz, psus https://review.coreboot.org/c/flashrom/+/79405/comment/ff7636da_68ef2884 : PS1, Line 8: Maybe you can add a little clarifying paragraph here in commit message, for example: > Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. `pullups=1` and expected pullups to be on, while the value `1` was handled as invalid and off. File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79405/comment/e7e1a03a_cc598b9d : PS1, Line 380: return You need to `free(tmp)` before returning (and same for two other places below) -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 00:52:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by David Reguera Garcia (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Anastasia Klimchuk. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 06 Dec 2023 19:30:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by David Reguera Garcia (Code Review) 06 Dec '23

06 Dec '23

David Reguera Garcia has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... buspirate_spi: Stop execution when invalid params Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 12 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/79405/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index 7436d80..e09cea5 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -375,8 +375,10 @@ hiz = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid hiz state, not using them.\n"); + else { + msg_perr("Invalid hiz state. Use on/off.\n"); + return 1; + } } free(tmp); @@ -386,8 +388,10 @@ pullup = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid pullups state, not using them.\n"); + else { + msg_perr("Invalid pullups state. Use on/off.\n"); + return 1; + } } free(tmp); @@ -397,8 +401,10 @@ psu = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid psus state, not enabling.\n"); + else { + msg_perr("Invalid psus state. Use on/off.\n"); + return 1; + } } free(tmp); -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newchange

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit