ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41612 )
Change subject: util: Add spd_tools to generate SPDs for TGL boards ......................................................................
Patch Set 7:
(6 comments)
one more round and done.
We ought to consider putting some test files into here but this is nice stuff.
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_part_id.go:
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 47: return fmt.Errorf("Incorrect number of arguments\n") The go convention is no puncutation (!) or newline characters, so probably best to remove the \n and ! below.
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 63: func readParts() ([]string, error) { for testing and other purposes, might be nicer if you could pass the file name into readParts: func readParts(name string) etc. etc.
Then you need not refer to a global in here.l Further, should you ever add flags back, os.Args would be the wrong thing to use anyway.
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 82: func readSpdManifest() (map[string]string, map[string]int, error) { same here, pass name in as a parameter.
Sorry I did not notice this last time.
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 135: spdToIndexMap map[string]int) ([]partIds, error) { up to you, but my personal preference is not to split lines this way. Go tends not to be fixated on line length :-)
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 188: return ioutil.WriteFile(filepath.Join(os.Args[MAKEFILE_DIR_ARG], makefileName), []byte(s), 0644) very nice, hope you like this too.
same caveat: pass the file name in as a parameter.
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/7/util/spd_tools/intel/lp4x/g... PS7, Line 72: 6: { unrelated note: felix held was complaining that clang-format formats superio struct inits this way, and now I see it here :-)
This is fine, I just think it's funny that none of us can agree on code formatting :-)