Attention is currently required from: Felix Singer, Thomas Heijligen, Angel Pons, qianfan, 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 4:
(11 comments)
Patchset:
PS3:
After a number of discussions in several irc channels, the conclusion is: this patch is going ahead […]
I believe I've done all those things now. Let me know if I'm still missing anything
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/70573/comment/93507707_1148e153 PS3, Line 62: /* TODO: Set this depending on the mode */
new line before
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/6d23e2c2_4b5fd952 PS3, Line 68: ch347_data->handle = NULL;
new line before
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/e5cc41ef_ff0f8c97 PS3, Line 69: free(data);
Yes please, free the data :) […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/5cca6826_fb595c44 PS3, Line 106: uint8_t *buffer = malloc(packet_len);
+1
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/e48f5f4f_6d7a846c PS3, Line 123: Could not read write response
It is confusing, given that both "read" and "write" are the operations, what does it mean "Could not […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/7b37ff3f_c0d40a97 PS3, Line 145: malloc(CH347_MAX_DATA_LEN + 3);
Same as above
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/f26aef5e_6dd69049 PS3, Line 149: command_buf[0] = CH347_CMD_SPI_IN; : command_buf[1] = 4; : command_buf[2] = 0; : command_buf[3] = readcnt & 0xFF; : command_buf[4] = (readcnt & 0xFF00) >> 8; : command_buf[5] = (readcnt & 0xFF0000) >> 16; : command_buf[6] = (readcnt & 0xFF000000) >> 24;
Use an initialiser as done in `ch347_set_cs()`?
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/75fcdd49_4180b609 PS3, Line 223: -3
nit: spacing here and in the line below
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/b568f0e4_1aabbdec PS3, Line 222: buff[0] = CH347_CMD_SPI_CFG; : buff[1] = (sizeof(buff)-3) & 0xFF; : buff[2] = ((sizeof(buff)-3) & 0xFF00) >> 8; : /* Not sure what these two bytes do, but the vendor : * drivers seem to unconditionally set these values : */ : buff[5] = 4; : buff[6] = 1; : /* Clock polarity: bit 1 */ : buff[9] = 0; : /* Clock phase: bit 0 */ : buff[11] = 0; : /* Another mystery byte */ : buff[14] = 2; : /* Clock divisor: bits 5:3 */ : buff[15] = (divisor & 0x7) << 3; : /* Bit order: bit 7, 0=MSB */ : buff[17] = 0; : /* Yet another mystery byte */ : buff[19] = 7; : /* CS polarity: bit 7 CS2, bit 6 CS1. 0 = active low */ : buff[24] = 0;
Use an initialiser as done in `ch347_set_cs()`?
Done. I had initially done it like this as I was previously using `buff` to read the original SPI configuration and then overwriting it with these values, as I thought that was a necessary step in order for the SPI_CFG command to respond. Seems like reading the initial config isn't actually necessary so I removed the initial read and initialized `buff` with the config.
https://review.coreboot.org/c/flashrom/+/70573/comment/2b8fb497_70d08469 PS3, Line 261: 64 * 1024
I was reading over some of Thomas's comments in CB:72057 (Patchset 19, lines 120 and 121) and it see […]
Done
-
Nicholas Chin (Code Review)