Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan, Anastasia Klimchuk, Alexander Goncharov.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70573 )
Change subject: ch347_spi.c: Add initial support for the WCH CH347 ......................................................................
Patch Set 5:
(10 comments)
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/70573/comment/71816cbe_1c19d45a PS4, Line 55: struct ch347_spi_data { : struct libusb_device_handle *handle; : };
Place this declaration before `devs_ch347_spi` variable.
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/ceec605b_b24bdee4 PS4, Line 62: if (ch347_data->handle == NULL) : return -1;
drop condition
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/3f282bcb_81097a46 PS4, Line 72: ch347_data->handle = NULL;
in the next line of the code, we do `free(data)`, so there's no reason to care about this pointer
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/54c69318_73970667 PS4, Line 109: uint8_t buffer[CH347_PACKET_SIZE] = {0};
How about moving this variable to the rest of data? So the code will be after the data.
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/618c99cb_60defe27 PS4, Line 265: struct libusb_device_handle *handle = NULL; : struct ch347_spi_data *ch347_data = calloc(1, sizeof(*ch347_data));
As far as you allocate `ch347_spi_data` at the beginning of initialization, you can drop `struct lib […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/8660bd82_3f76d4dd PS4, Line 269: -1
Please, use `1` value just to be consistency with other initialization functions.
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/66fb3c74_d0383595 PS4, Line 275: -1
Same here, `1`
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/892dd546_eef6c65c PS4, Line 287: handle
ch347_data->handle = ... […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/1a17d9e3_ffbd273c PS4, Line 290: -1
Same here, `1`
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/cd6a533b_bc602722 PS4, Line 335: -1
Same here, `1`
Done
-
Nicholas Chin (Code Review)