Attention is currently required from: Peter Marheine, Simon Arlott.
Anastasia Klimchuk has posted comments on this change by Simon Arlott. ( https://review.coreboot.org/c/flashrom/+/86411?usp=email )
Change subject: spidriver: Add support for the Excamera Labs SPIDriver programmer ......................................................................
Patch Set 2:
(11 comments)
Patchset:
PS2: Simon, nice to meet you, thank you for the patch! This is really cool :)
I have added comment to the files, and I have two more comments to the patch in general:
1) We have a document where all the things that are added since the latest release, are accumulated. It helps to write release notes, and also for people who are interested. The doc is here https://www.flashrom.org/release_notes/devel.html
Could you please add a section to that doc about your new programmer? It doesn't have to be long text, and you can link manpage (especially if you do my other comment about manpage)
source code here: `/doc/release_notes/devel.rst`
2) Would you agree to be a maintainer for your programmer? This means if someone sends a patch which modifies your programmer, you are automatically added as a reviewer (you can always add more reviewers, not a problem). Duties are explained here, you will be in "flashrom reviewers" group: https://www.flashrom.org/about_flashrom/team.html
If you agree, add yourself to MAINTAINERS file, the file path is the c file of the programmer. You can include it in this patch (I mean, if you agree :) )
Thank you!
Commit Message:
https://review.coreboot.org/c/flashrom/+/86411/comment/8ddf5676_b55ef4c3?usp... : PS2, Line 9: This is a SPI hardware interface with a display (https://spidriver.com/), : connected as an FT230X USB serial device at a fixed baud rate of 460800. : : Firmware: https://github.com/jamesbowman/spidriver : Protocol: https://github.com/jamesbowman/spidriver/blob/master/protocol.md These lines would be very good to add in the beginning of manpage entry that you added.
But also keep them here too.
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/86411/comment/fc6345bb_e55af17b?usp... : PS2, Line 359: spidriver I am thinking about naming, maybe excamera_spidriver will be better? I understand that SPIDriver is literally the name of the device, but it might be too generic. Lots of other programmers names include vendor names.
What do you think? Is SPIDriver well known as a name?
File include/programmer.h:
https://review.coreboot.org/c/flashrom/+/86411/comment/feb3451c_91645dc4?usp... : PS2, Line 101: extern const struct programmer_entry programmer_spidriver; You need to swap lines 100 and 101 to be in nice alphabetical order.
(I am realising after looking through your patch that half of the places is not in the abc order :\ but at least this file is )
File spidriver.c:
https://review.coreboot.org/c/flashrom/+/86411/comment/27173a0b_d5ea5158?usp... : PS2, Line 19: */ Maybe add link to firmware here as well? (from commit message)
https://review.coreboot.org/c/flashrom/+/86411/comment/25215dca_d891decd?usp... : PS2, Line 81: { Why do you need this bracket (and closing on line 101)? Is there a reason for this block?
https://review.coreboot.org/c/flashrom/+/86411/comment/caf5e834_ef6690e2?usp... : PS2, Line 136: /* Read and write */ I am just curious, there is a way to write, or the way to read and write, but no way to only read? I see it's also in the protocol, so it's not a question to your implementation. But I am wondering why there is not read?
https://review.coreboot.org/c/flashrom/+/86411/comment/d6bc7ebd_c81f6442?usp... : PS2, Line 179: .max_data_read = MAX_DATA_READ_UNLIMITED, : .max_data_write = MAX_DATA_WRITE_UNLIMITED, The indentation here needs to be aligned with the rest of `=`
https://review.coreboot.org/c/flashrom/+/86411/comment/1abed391_c3e0ba26?usp... : PS2, Line 211: Invalid SPI mode %ld Lets add the second part to the error message: "Valid values are <...>"
https://review.coreboot.org/c/flashrom/+/86411/comment/d4d71c26_92cdf9bc?usp... : PS2, Line 212: return 1; So invalid `mode` param returns an error, but invalid values for `a` and `b` below continue running with default values. Why is this difference in behaviour? Can it be the same?
I would lean to throwing an error in case of invalid value, and only use default in param is absent completely. It feels safer, so that the user won't accidentally run when they provided invalid value and maybe they meant something else.
https://review.coreboot.org/c/flashrom/+/86411/comment/2163f0b2_a788c993?usp... : PS2, Line 309: version 1 Is it useful to print `fw_version` here instead, as an in addition to ?