Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46717 )
Change subject: Introduce `extract_programmer_param_toggle` helper ......................................................................
Patch Set 1: Code-Review+2
(3 comments)
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c File flashrom.c:
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@803 PS1, Line 803: delete spurious \n
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@811 PS1, Line 811: if (c == '1') { : *value = 1; : return 0; : } : : if (c == '0') { : *value = 0; : return 0; : } seems like a switch would make this tidy?
https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h File programmer.h:
https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h@615 PS1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value); I like this but I feel flashrom has suffered from many half done solutions in the past. So I feel like we should switch all users in one go. It's a little more work but I think your solution can get its benefits fully realized with the effort. mstarddc_spi.c is one example candidate, digilent_spi.c is another. Maybe buspirate_spi.c ?
I would very much like to see a unit-test written, do you have any thoughts and suggestions about that?
Bigger vision here: Another step would be to perhaps have a struct type defined that contains the param type and a union between of the primitive types so that common parse logic could be co-located out of the spi masters here. This could then become part of the libflashrom api as well.