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