Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
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 3:
(6 comments)
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/70573/comment/7bde1c88_74ca3b6b PS2, Line 24: #define CH347_CMD_SPI_CFG 0xC0 : #define CH347_CMD_SPI_CS_CTRL 0xC1 : #define CH347_CMD_SPI_OUT_IN 0xC2 : #define CH347_CMD_SPI_IN 0xC3 : #define CH347_CMD_SPI_OUT 0xC4
Suggestion: use tabs to align the hex values? […]
Done. I used mixed spaces and tabs to get it to align properly even when using different tab widths in a text editor, not sure if that's the best way to do it.
https://review.coreboot.org/c/flashrom/+/70573/comment/13686991_a4e02a57 PS2, Line 48: static struct libusb_device_handle *handle = NULL;
There have been changes to other programmers to pass this kind of data around through the flashctx. […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/b0a2bac7_b9e3ef53 PS2, Line 51: 1
Do you need to specify the array size? Or is this hardcoded because the code below only tries openin […]
Done. Removed the array size and added a terminating {0} entry which other programmers include.
https://review.coreboot.org/c/flashrom/+/70573/comment/4ee00a36_504b7a33 PS2, Line 71: uint8_t cmd[13] = {0}; : cmd[0] = CH347_CMD_SPI_CS_CTRL; : /* payload length, uint16 LSB: 10 */ : cmd[1] = 10; : : cmd[3] = cs1; : cmd[8] = cs2;
How about: […]
Done. I forgot about this syntax, thanks!
https://review.coreboot.org/c/flashrom/+/70573/comment/8516c7c7_03fec2d2 PS2, Line 221:
nit: trailing space
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/ce33b794_89ee63d2 PS2, Line 287: uint16_t vid = devs_ch347_spi[0].vendor_id; : uint16_t pid = devs_ch347_spi[0].device_id; : handle = libusb_open_device_with_vid_pid(NULL, vid, pid); : if (handle == NULL) { : msg_perr("Couldn't open device %04x:%04x.\n", vid, pid); : return -1; : }
Does the CH341A code do the same? It should be fine as long as there's only one VID/DID in the list.
Yeah, it does. I took a look at other libusb programmers and most of them do basically the same thing as most only match a single VID/DID. It looks like stlinkv3_spi is the only libusb programmer which matches multiple IDs. I'll take a look at how that works when I add support for the CH347 HID mode which uses a different DID.