Patch set 1:Code-Review +2

View Change

3 comments:

• File flashrom.c:

  • Patch Set #1, Line 803:

    delete spurious \n

  • Patch Set #1, Line 811:

    	if (c == '1') {
    		*value = 1;
    		return 0;
    	}
    
    	if (c == '0') {
    		*value = 0;
    		return 0;
    	}
    

    seems like a switch would make this tidy?

• File programmer.h:

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

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