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...
-
David Hendricks (Code Review)