Attention is currently required from: ZhiYuanNJ.

View Change

12 comments:

• Commit Message:

  • Patch Set #2, 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.

  • Patch Set #2, 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

  • Patch Set #2, Line 12: ?from=search&wd=eyJpdiI6Im4zdVRJdHdpNks1WUFLSUFDNnRsYnc9PSIsInZhbHVlIjoicHFsU3EwQlgzWmgzM1ZIYnBpZXFiUT09IiwibWFjIjoiNjZjNjc5ZDQ4Yzg2ZDdhNzc5YzM4OTA0MTlkM2FiYWQ1Yzg0ZTZkNWNiZDczZDM1NjM1ODIwN2NjZDhlOTNmOCJ9

    Everything from the `?` onwards isn't necessary to navigate to the product page

• Patchset:

  • Patch Set #2:

    Hello! Thanks for the patch!

• File ch347_spi.c:

  • Patch Set #2:

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

  • Patch Set #2:

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

  • Patch Set #2, 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.

  • Patch Set #2, Line 79: 0x55DD

    This should be left at 0x55DB, which corresponds to mode 1. 0x55DD corresponds to mode 3, which does not support SPI

  • Patch Set #2, 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

  • Patch Set #2, 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)

  • Patch Set #2, Line 347:

    Remove trailing whiespace

  • Patch Set #2, Line 356: Failed to claim interface 2

    Update this to reflect the actual interface number being used

To view, visit change 82193. To unsubscribe, or for help writing mail filters, visit settings.