ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41612 )
Change subject: util/mb/google: Add spd_tools to generate SPDs for TGL boards ......................................................................
Patch Set 3:
(18 comments)
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/README.md:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 134: ```
If possible, just indent code blocks by four spaces instead of using ```.
yeah, but this is JSON, so does that apply?
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/gen_part_id.go:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 5: import "encoding/csv" Go ahead and do this: import ( "encoding/csv" etc. etc. )
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 25: var spdDirPtr = flag.String("spd_dir", "", "Directory containing SPD files and manifest") it's also common here var ( spdDirPtr = whatever memPartListPart = whatever )
and so on. Thing of all the space you save :-)
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 35: if *spdDirPtr == "" { if a flag is not optional, I think it's not really a flag. It might be cleaner just to require it as a positional parameter.
calling log.Fatal everywhere is frowned upon. usually you return an error to main. Why? testing.
I'm hardly a paragon of virtue when it comes to testing, but that said, I think some tests would be good here.
I'm not even sure you absolutely need these stats here; is there harm done if you read the first two files and the last fails? probably not. Just wondering.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 64: func readParts() []string { so here, for example: func readParts() ([]string, error)
if err != nil { return nil, err }
return parts, nil
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 83: func readSpdManifest() (map[string]string, map[string]int) { func readSpdManifest() (map[string]string, map[string]int, error)
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 95: for true { you can just say for https://play.golang.org/p/OqptsHybDRP
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 145: for i := 0; i < len(parts); i++ { for i := range parts
or, better, since you can avoid parts[i] everywhere for _, p := range parts { if p == "" { } etc.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 151: if ok == false { if ! ok { }
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 181: filePath := *makefileDirPtr + "/" + makefileName The Go convention is all errors should be checked, and you are missing many here.
Suggestion: build a string via fmt.Sprintf: var s string s += fmt.Sprint(etc. etc.)
and then the last step if _, err := ioutil.WriteFile(filePath, []byte(string)) { return err }
this lets you blow off the create, the defer, and gets all your writes into one place in a way that's easy to check.
So the overall pattern
func genMakefile(partIdList []partIds) error { s := fmt.Sprintf("%s\n\n", generatedCodeLicense) s += fmt.Sprintf("SPD_SOURCES =\n")
for i := 0; i < len(partIdList); i++ { s += fmt.Sprintf("SPD_SOURCES += %s ", partIdList[i].spdFileName) s += fmt.Sprintf(" # ID = %d(0b%04b) ", i, int64(i)) s += fmt.Sprintf(" Parts = %04s\n", partIdList[i].memParts) }
return ioutil.WriteFile(filepath.Join(*makefileDirPtr, makefileName), []byte(s)) }
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 5: import "bufio" same here, fix imports
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 63: func encodeSpdSize(memAttribs *memAttributes) byte { can't these just be const?
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 510: filePath := *spdDirPtr + "/" + spdManifestFileName always use filepath.Join
Consider using the pattern I showed for writing in that previous file.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 546: filePath := *spdDirPtr + "/" + fileName always use filepath.Join
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 592: partsFile, err := os.Open(*memPartListPtr) you could, also, just use ioutil.ReadFile func ReadFile(filename string) ([]byte, error)
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 605: func validateDensity(densityPerChannelGb int) { return an error, then writing tests is easier
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 770: if *spdDirPtr == "" { I'd still argue fore positional arguments but...
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 794: readMemoryParts(&memParts) here is where you'd have if err := readMemoryParts(&memParts); err != nil { log.Fatal(err) }
note that the file read and write functions return errs with filename embedded: https://play.golang.org/p/N1LDiSghWy0