[M] Change in flashrom[main]: Bug fix: Fix CH347T PID, interface, and clock settings errors

[Nicholas Chin (Code Review)]

Attention is currently required from: ZhiYuanNJ.

Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82193?usp=email )

Change subject: Bug fix: Fix CH347T PID, interface, and clock settings errors
......................................................................

Patch Set 2:

(12 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/82193/comment/2e932e8c_c09e281c :
PS2, Line 7: Bug fix: Fix CH347T PID, interface, and clock settings errors
The typical commit message style is `<component>: Short description (up to 72 characters)` (see https://www.flashrom.org/dev_guide/development_guide.html#commit-message-1)

In this case, the component would be `ch347_spi`, and the description could be something like `Add support for QFN28 package (CH347F)`. Then the next line about `Add functionality` can be dropped. As I note later, I would move the spispeed changes to a separate patch.

https://review.coreboot.org/c/flashrom/+/82193/comment/5bfef957_13d3e12e :
PS2, Line 10: The latest package with F as 347, compared to T, does not require active mode switching and can simultaneously meet communication requirements such as USB to SPI/I2C
Please reflow to 72 characters per line

https://review.coreboot.org/c/flashrom/+/82193/comment/db3dcde1_71e49d49 :
PS2, Line 12: ?from=search&wd=eyJpdiI6Im4zdVRJdHdpNks1WUFLSUFDNnRsYnc9PSIsInZhbHVlIjoicHFsU3EwQlgzWmgzM1ZIYnBpZXFiUT09IiwibWFjIjoiNjZjNjc5ZDQ4Yzg2ZDdhNzc5YzM4OTA0MTlkM2FiYWQ1Yzg0ZTZkNWNiZDczZDM1NjM1ODIwN2NjZDhlOTNmOCJ9
Everything from the `?` onwards isn't necessary to navigate to the product page

Patchset:

PS2: 
Hello! Thanks for the patch!

File ch347_spi.c:

PS2: 
I would split the spispeed changes into a separate patch so that this one is just about adding the CH347F variant.

PS2: 
Please use the `/* comment */` comment style rather than `//coment` for consistency with the rest of the file

https://review.coreboot.org/c/flashrom/+/82193/comment/a30520cf_e5b36fc0 :
PS2, Line 10: *                                                                         
            :  *   CH347 is a high-speed USB bus converter chip that provides UART, I2C  
            :  *   and SPI synchronous serial ports and JTAG interface through USB bus.  
            :  *                                                                         
            :  *   The SPI interface by CH347 can supports transmission frequency       
            :  *   configuration up to 60MHz.                                            
            :  *                                                                         
            :  *   The USB2.0 to spi scheme based on CH347 can be used to build         
            :  *   customized USB high-speed spi debugger and other products.           
            :  *                                                                         
            :  *            _____________                                                
            :  *           |             |____SPI (MISO,MOSI,SCK,CSC0,CSC1)            
            :  *      USB__|   CH347T/F  |                                               
            :  *           |_____________|____(UART/I2C/JTAG/SWD/GPIO)             
            :  *            ______|______                                                
            :  *           |             |                                               
            :  *           | 8 MHz XTAL  |                                               
            :  *           |_____________|     
            :  * 
This should be placed after the licence header, not in the middle of it. Also, I'm not sure this text really adds any value in the first place.

https://review.coreboot.org/c/flashrom/+/82193/comment/6f0d657b_5ad5d4c6 :
PS2, Line 79: 0x55DD
This should be left at 0x55DB, which corresponds to mode 1. 0x55DD corresponds to mode 3, which does not support SPI

https://review.coreboot.org/c/flashrom/+/82193/comment/f2521d3c_87af01f6 :
PS2, Line 97: 2,       //CH347T interface number
            : 	4,       //CH347F interface number
Use the `CH347*_IFACE` defines from above? That also removes the need for the comments

https://review.coreboot.org/c/flashrom/+/82193/comment/ae96164a_82bf0b50 :
PS2, Line 344: Couldn't open device %04x:%04x.\n
This block of code would only run if flashrom has gone through all the IDs in `devs_ch347_spi` and wasn't able to open any of them. So a generic message saying it couldn't find a CH347 would probably be more appropriate compared to saying it couldn't open a particular ID (which in this case would be whatever is the last vid and pid in the list)

https://review.coreboot.org/c/flashrom/+/82193/comment/807eb57f_4a629f18 :
PS2, Line 347:  
Remove trailing whiespace

https://review.coreboot.org/c/flashrom/+/82193/comment/22f87e53_3a0f7ace :
PS2, Line 356: Failed to claim interface 2
Update this to reflect the actual interface number being used