Attention is currently required from: Tristan Corrick, Arthur Heymans, Iru Cai (vimacs). Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30890 )
Change subject: autoport: Add support for Haswell-LynxPoint platform ......................................................................
Patch Set 32: Code-Review+1
(5 comments)
Patchset:
PS32: Thank you for this great work, but I have a few comments.
I am not an expert in the Haswell product series, and many things remain a mystery to me. Please, could you clarify some points in the code? It would be better to make more comments and use variable names that do not consist of a single character.
File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/comment/1893cf9e_8e56c838 PS32, Line 24: switch g { I would prefer to use a map here
const ( PIRQI = 0 PIRQJ = 1 ...
SOME_MAGIC_GPIO_8 = 8 ... )
func lp_gpio_to_pirq(g uint) int { var pirqmap = map[uint]int { SOME_MAGIC_GPIO_8: PIRQI, .... } pirq, valid := pirqmap[g] if valid { return pirq } return -1 }
* Please, instead of SOME_MAGIC_GPIO_8, choose a more similar meaning for this case.
https://review.coreboot.org/c/coreboot/+/30890/comment/c6b018d8_0999639e PS32, Line 170: uint16(0x100+g*8)] this is magic )) What does 0x100 mean? What does g mean?
https://review.coreboot.org/c/coreboot/+/30890/comment/aab9f7f2_3fde1942 PS32, Line 199: lvl = "LOW" I don't think we need any more variables fmt.Fprintf(gpio, "LP_GPIO_OUT_LOW\n")
https://review.coreboot.org/c/coreboot/+/30890/comment/2767657b_b70f11d0 PS32, Line 201: lvl = "HIGH" fmt.Fprintf(gpio, "LP_GPIO_OUT_HIGH\n")