Attention is currently required from: Stefan Reinauer, Angel Pons. Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49254 )
Change subject: sysfsgpio.c implement spi interface via linux sysfs ......................................................................
Patch Set 5:
(8 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/49254/comment/16cd8007_5cc0f179 PS4, Line 800: : CONFIG_SYSFSGPIO ?= no
nit: move this above CONFIG_PRINT_WIKI
Done
File meson.build:
https://review.coreboot.org/c/flashrom/+/49254/comment/25eea8e4_98649e4c PS4, Line 71: config_sysfsgpio
We don't use this anywhere.
Done
https://review.coreboot.org/c/flashrom/+/49254/comment/2edac7d1_0382b30b PS4, Line 313: if host_machine.system() == 'linux'
Maybe it should be used here. Also, this depends on `config_bitbang_spi`. […]
Done
File sysfsgpio.c:
https://review.coreboot.org/c/flashrom/+/49254/comment/3c5cea59_69a56efe PS4, Line 93: snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/", desc->pin);
Hmmm... I think you should be able to define the common prefix in a macro and do the following: […]
Done
https://review.coreboot.org/c/flashrom/+/49254/comment/66a95047_61aecfd3 PS4, Line 111: } I want to know why not use snprintf. The changes you hope will make the code messier.
https://review.coreboot.org/c/flashrom/+/49254/comment/d910dd74_3b6bc0c9 PS4, Line 99: snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/direction", desc->pin); : desc->fd_direction = open(s, O_WRONLY); : if (desc->fd_direction < 0) { : msg_perr("Error: failed to open %s\n", s); : return -1; : } : : snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/value", desc->pin); : desc->fd_value = open(s, O_RDONLY); : if (desc->fd_value < 0) { : msg_perr("Error: failed to open %s\n", s); : return -1; : }
Here, you could do the following instead: […]
I want to know why not use snprintf. The changes you hope will make the code messier.
https://review.coreboot.org/c/flashrom/+/49254/comment/e5bf11da_b4b78c89 PS4, Line 135: desc->fd_direction = -1;
Also invalidate the pin string: […]
Done
https://review.coreboot.org/c/flashrom/+/49254/comment/4c24595a_d32f10c0 PS4, Line 204: static int parse_pins(struct sysfsgpio_pins *pins)
if (v < 0 || v > 9999999) break
99999 is a large enough number, but I don’t think there is a speed requirement here. You can hand it to export_sysfsgpio to determine whether the value of v is valid.
const long v = strtol(token, &suffix, 10);
I don’t think it is necessary to limit the use of decimal numbers
-
Xiang Wang (Code Review)