David Hendricks has posted comments on this change. ( https://review.coreboot.org/22344 )
Change subject: linux_spi: Dynamically detect max buffer size
......................................................................
Patch Set 4:
(5 comments)
Thanks for the quick (and thorough) review as always.
I ended up re-writing a significant portion of this patch. Assuming the changes look good, what's the "right" thing to do to get things merged? Merge everything in this patch, or split my updates into a follow-up patch?
https://review.coreboot.org/#/c/22344/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/#/c/22344/3//COMMIT_MSG@9
PS3, Line 9: Read max buffer size from sysfs if available.
:
> That's not true, and I can say that from this patch: If we
I guess the original author meant that it's a compile-time constant in the kernel? In any case, I went ahead and simplified this further to remove any possible confusion.
https://review.coreboot.org/#/c/22344/3/linux_spi.c
File linux_spi.c:
https://review.coreboot.org/#/c/22344/3/linux_spi.c@134
PS3, Line 134: fp = fopen(BUF_SIZE_FROM_SYSFS, "r");
> Isn't there a way to query this over the device interface?
Not sure... I don't see any other obvious way of getting at it. There's no ioctl for this AFAICT by looking thru the spidev documentation. The only clue seems to be:
- There's a limit on the number of bytes each I/O request can transfer to the SPI device. It defaults to one page, but that can be changed using a module parameter.
So either read the module parameter via sysfs or default to using getpagesize() which the code currently does.
https://review.coreboot.org/#/c/22344/3/linux_spi.c@135
PS3, Line 135: if (!fp) {
> What if read() returns a short byte count? What if it gets inter-
Done.
https://review.coreboot.org/#/c/22344/3/linux_spi.c@199
PS3, Line 199: The implementation currently does not support requests that
> Why? rx/tx buffers seem to be handled separately.
Re "why": As to not rely on the driver to split large transactions? I'm not if this is a big deal for spidev, but I have had issues with MTD in the past (https://chromium-review.googlesource.com/505409).
Re rx/tx being handled separately: Are you referring to the current (old) code? That may be an error, I think Keno was correct to handle read and write buffers the same and account for command size.
https://review.coreboot.org/#/c/22344/4/linux_spi.c
File linux_spi.c:
https://review.coreboot.org/#/c/22344/4/linux_spi.c@222
PS4, Line 222: *
oops, i'll fix this...
--
To view, visit https://review.coreboot.org/22344
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064
Gerrit-Change-Number: 22344
Gerrit-PatchSet: 4
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: david.hendricks(a)gmail.com
Gerrit-Comment-Date: Sun, 05 Nov 2017 21:38:37 +0000
Gerrit-HasComments: Yes
David Hendricks has uploaded this change for review. ( https://review.coreboot.org/22353
Change subject: this is a test, please ignore
......................................................................
this is a test, please ignore
Change-Id: I9aa1dab92b6c69b27a2cb90e9f6154cb73214c26
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
A test.txt
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/22353/1
diff --git a/test.txt b/test.txt
new file mode 100644
index 0000000..3b18e51
--- /dev/null
+++ b/test.txt
@@ -0,0 +1 @@
+hello world
--
To view, visit https://review.coreboot.org/22353
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9aa1dab92b6c69b27a2cb90e9f6154cb73214c26
Gerrit-Change-Number: 22353
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>