Attention is currently required from: Angel Pons, Nicholas Chin.
qianfan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70529 )
Change subject: Add initial CH347T SPI programmer ......................................................................
Patch Set 5:
(4 comments)
File ch347t_spi.c:
https://review.coreboot.org/c/flashrom/+/70529/comment/4c87c78f_8af12ac2 PS5, Line 94: typedef uint32_t __le32;
Why the typedefs?
all the data structure transfed to ch347 should be little endian. create this typedef can remind me doing a cpu_to_le32 before any transfer.
https://review.coreboot.org/c/flashrom/+/70529/comment/cbe771db_1f389490 PS5, Line 110: } __attribute__((packed));
This doesn't seem to be portable.
It's OK for gcc based compiler. Or we can create a portable marco such as '__packed' to replace all the "__attribute__((packed))" in the source code.
https://review.coreboot.org/c/flashrom/+/70529/comment/04beb50a_e4d2a86b PS5, Line 169: if (n > CH347_MAX_DATA_WRITE) : n = CH347_MAX_DATA_WRITE;
This doesn't seem to be a good idea...
I can't find a better idea, could you please give me some advices?
https://review.coreboot.org/c/flashrom/+/70529/comment/42db8761_ad22492a PS5, Line 279: memset(&csctrl, 0, sizeof(csctrl));
Why not use an initialiser? […]
OK
-
qianfan (Code Review)