Nico Huber has posted comments on this change. ( https://review.coreboot.org/22344 )
Change subject: linux_spi: Dynamically detect max buffer size ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/22344/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/22344/3//COMMIT_MSG@9 PS3, Line 9: The max buffer size of the linux kernel's SPI device is a compile-time : parameter. That's not true, and I can say that from this patch: If we read it from a module parameter representation in sysfs it's obviously not compile-time.
https://review.coreboot.org/#/c/22344/1/linux_spi.c File linux_spi.c:
https://review.coreboot.org/#/c/22344/1/linux_spi.c@141 PS1, Line 141: if (param_fd != -1) : close(param_fd);
May as well put this in the else clause above.
No, the else path is not hit if open() succeeds but read() fails.
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: int param_fd = open("/sys/module/spidev/parameters/bufsiz", O_RDONLY); Isn't there a way to query this over the device interface?
https://review.coreboot.org/#/c/22344/3/linux_spi.c@135 PS3, Line 135: if (param_fd == -1 || read(param_fd, &buf, sizeof(buf) - 1) == -1) { What if read() returns a short byte count? What if it gets inter- rupted (I have to admit, unlikely on sysfs). Generally, using the low level file functions is a bad idea if you can use stream func- tions (e.g. fread()) instead.
-
Nico Huber (Code Review)