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 8: Code-Review+2
(4 comments)
This would not pass the Go conventions but you are probably worn out on my carping, so I leave it up to you whether you'd like to do another pass or call it a day :-)
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_part_id.go:
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 28: SPD_DIR_ARG = 1 So sorry I did not notice this. Go frowns on this type of constant. SPDDir would be fine, really. MakefileDir MempartsUsedFile
But there's a much easier way to do it.
In your main()
SPDDir, MakefileDir, MemParts := os.Args[0], os.Args[1], os.Args[2]
Then you can skip the constants, and it actually reduces the size by 2 lines :-)
Also, the convention is that acronyms are all caps ... SPD, not like UEFI Spd :-)
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 44: func checkArgs() error { Honest opinion here? I am not convinced you need this at all. in main() if len(os.Args) != 4 { log.Fatal("arg count message") }
SPDDir, MakefileDir, MemParts := os.Args[0], os.Args[1], os.Args[2]
is the os.Stat absolutely necessary? Could you just let the functions you call error out?
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 28: SPD_DIR_ARG = 1 same comment re const names
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 438: SPD_INDEX_SIZE: { constVal: SPD_VALUE_SIZE }, also you could do SPDIndexSize but OTOH maybe this is the hardware name, in which case you'd get a pass :-)