Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/28911
to look at the new patch set (#5).
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
ft2232_spi: add an ability to use GPIO for chip selection
Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Signed-off-by: Sergey Alirzaev <zl29ah(a)gmail.com>
---
M flashrom.8.tmpl
M ft2232_spi.c
2 files changed, 26 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/28911/5
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 5
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Nico Huber has posted comments on this change. ( https://review.coreboot.org/28911 )
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
> AFAIR it's non-trivial to reset the proper CS bit while keeping the
> code clear. Also, i'm using the new parameter to choose between
> different chips while the CS pin is left floating.
Well, all the modifications of `cs_bits / pindir` happen earlier in
the code flow, so I would have thought it's enough to clear bit3 in
`pindir`. But it doesn't matter much. And I've also noticed now
that the code isn't consistent (arm-usb-ocd* seems to be confused
about the meaning of `cs_bits`; maybe `cs_idle` would describe it
better). So maybe we should clean it up later.
Noticed another nit, but it's good to go as is.
https://review.coreboot.org/#/c/28911/3/ft2232_spi.c
File ft2232_spi.c:
https://review.coreboot.org/#/c/28911/3/ft2232_spi.c@333
PS3, Line 333: unsigned
Not sure about flashrom, but in other projects (e.g. coreboot) we
have a rule now to spell it in full `unsigned int`. I have no strong
preference, though.
https://review.coreboot.org/#/c/28911/3/ft2232_spi.c@340
PS3, Line 340: unsigned
same here
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 3
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 08 Oct 2018 11:58:15 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/28911
to look at the new patch set (#3).
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
ft2232_spi: add an ability to use GPIO for chip selection
Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Signed-off-by: Sergey Alirzaev <zl29ah(a)gmail.com>
---
M flashrom.8.tmpl
M ft2232_spi.c
2 files changed, 26 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/28911/3
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 3
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/28911
to look at the new patch set (#2).
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
ft2232_spi: add an ability to use GPIO for chip selection
Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Signed-off-by: Sergey Alirzaev <zl29ah(a)gmail.com>
---
M flashrom.8.tmpl
M ft2232_spi.c
2 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/28911/2
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 2
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Sergey Alirzaev has posted comments on this change. ( https://review.coreboot.org/28911 )
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
Patch Set 1:
> Patch Set 1:
>
> (4 comments)
>
> Just some smaller remarks.
AFAIR it's non-trivial to reset the proper CS bit while keeping the code clear. Also, i'm using the new parameter to choose between different chips while the CS pin is left floating.
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 1
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Sun, 07 Oct 2018 21:33:04 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/28911 )
Change subject: ft2232_spi: add an ability to use GPIO for chip selection
......................................................................
Patch Set 1:
(4 comments)
Just some smaller remarks.
https://review.coreboot.org/#/c/28911/1/flashrom.8.tmpl
File flashrom.8.tmpl:
https://review.coreboot.org/#/c/28911/1/flashrom.8.tmpl@725
PS1, Line 725: channel/interface/port
I think "channel/interface/port" refer all to the same parameter
`port`. So the whole sentence should probably be something like:
Optional parameters specify the controller, channel/interface/
port and GPIO-based chip select to use.
https://review.coreboot.org/#/c/28911/1/ft2232_spi.c
File ft2232_spi.c:
https://review.coreboot.org/#/c/28911/1/ft2232_spi.c@331
PS1, Line 331: && strlen(arg)
I know it's a common pattern, but doesn't it prevent an error message
to be printed when somebody specifies `-p ft2232_spi:csgpiol=` without
a number?
Your call.
https://review.coreboot.org/#/c/28911/1/ft2232_spi.c@332
PS1, Line 332: = 0
Unnecessary initialization.
https://review.coreboot.org/#/c/28911/1/ft2232_spi.c@342
PS1, Line 342: cs_bits |= 1 << pin;
This keeps the original bit (0x08) to assert CS. Probably a rather
rare use-case, but what if somebody would want to use the new parameter
to choose between different chips?
--
To view, visit https://review.coreboot.org/28911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6db05619e0d69ad18549c8556ef69225337b1532
Gerrit-Change-Number: 28911
Gerrit-PatchSet: 1
Gerrit-Owner: Sergey Alirzaev <zl29ah(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Sun, 07 Oct 2018 21:20:13 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No