Attention is currently required from: Xiang Wang, Stefan Reinauer. Angel Pons 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 1:
(7 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/49254/comment/5407ebce_0bbc2305 PS1, Line 10: some SBC Which one have you tested this on?
File sysfsgpio.c:
https://review.coreboot.org/c/flashrom/+/49254/comment/934a4daf_654b707a PS1, Line 36: #define GPIO_DIRECTION "/sys/class/gpio/gpio%d/direction" The only definition that gets used twice is `GPIO_PATH`. The others are only used once.
https://review.coreboot.org/c/flashrom/+/49254/comment/59be022d_aa407859 PS1, Line 38: #define EXIST_PATH(path) (access((path), F_OK) == 0
cute but just inline it, the indirection is not necessary.
It's only used twice, so I don't think this provides any benefit. If anything, you could make it a static function and let the compiler decide whether to inline it or not.
https://review.coreboot.org/c/flashrom/+/49254/comment/51b24dea_824fd9f9 PS1, Line 42: int pin; Looks like the code only uses the string representation of `pin`. To reduce `snprintf` usage, how about having a `char pin_str[8];` here?
https://review.coreboot.org/c/flashrom/+/49254/comment/7030f1bb_e7019109 PS1, Line 47: static struct pin_desc pin_cs = { : .name = "cs", : .fd_direction = -1, : .fd_value = -1 : }; : : static struct pin_desc pin_sck = { : .name = "sck", : .fd_direction = -1, : .fd_value = -1 : }; : : static struct pin_desc pin_mosi = { : .name = "mosi", : .fd_direction = -1, : .fd_value = -1 : }; : : static struct pin_desc pin_miso = { : .name = "miso", : .fd_direction = -1, : .fd_value = -1 : }; I would suggest making another struct to hold all pins in a single global variable:
struct sysfsgpio_pins { struct pin_desc cs; struct pin_desc sck; struct pin_desc mosi; struct pin_desc miso; };
static struct sysfsgpio_pins pins = { .cs = { .name = "cs", .fd_direction = -1, .fd_value = -1, }, .sck = { .name = "sck", .fd_direction = -1, .fd_value = -1, }, .mosi = { .name = "mosi", .fd_direction = -1, .fd_value = -1, }, .miso = { .name = "miso", .fd_direction = -1, .fd_value = -1, }, };
https://review.coreboot.org/c/flashrom/+/49254/comment/664634ea_c98925f7 PS1, Line 80: if (ret == (int)strlen(str)) To avoid casting to int, I would test for failure here with `ret < strlen(str)`.
https://review.coreboot.org/c/flashrom/+/49254/comment/6e5477e6_6a137fc0 PS1, Line 92: snprintf(s, sizeof(s), "%d", desc->pin); If you can avoid clobbering the buffer contents with this snprintf call (see suggestion to use a `pin_str` cache in global state), you should be able to reuse the buffer contents for the direction and value files.