Paul Kocialkowski has posted comments on this change. ( https://review.coreboot.org/23262 )
Change subject: Add a SPI command class to `struct flashchip` ......................................................................
Patch Set 4:
(1 comment)
Note that naming the field spi_command_set breaks the current tab alignment in the chips declaration, e.g.: .spi_command_set = EDI, .probe = edi_probe_kb9012,
I think naming it "command_set" would be enough since SPI25 already has SPI in the name and we could generalize the issue with other protoocls anyway.
Also, I think it would be more convenient to be able to specify the command set to use from the command line instead of specifying the exact chip. One might want to just hook EDI to the keyboard pins for reflashing the EC and let flashrom detect which ENE chip it is.
If you want to pick up that idea, that'd be great, otherwise I'll probably end up writing and submitting something in this direction.
https://review.coreboot.org/#/c/23262/4/flash.h File flash.h:
https://review.coreboot.org/#/c/23262/4/flash.h@186 PS4, Line 186: /* By the way, it looks like the dummy structure commented out in the flashchips struct array declaration is out of sync with the structure (missing fields). This adds another field there.
Since it serves as documentation, we might want to keep it in sync with the structure definition, or just get rid of it altogether. Both options are fine with me.
What do you think?
-
Paul Kocialkowski (Code Review)