Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 22:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div
How about: […]
I can do that. I'll leave the formula comment as well so who ever visits the file later can see that we just pulled the 1/8 out of the denominator.
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 184: core_freq
Can you just return 0?
Yea
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 234: current_value
Can you rename this to current_value_amps or something like it. […]
Will do.
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 305: static
Does this need to be static?
Definitely not. I was going to define it as a global, but then decided I liked being able to see the values here in the function without scrolling too far. I'll remove the static.