Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33255 )
Change subject: drivers/ipmi: Add chip ops ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c File src/drivers/ipmi/ipmi_kcs_ops.c:
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@107 PS16, Line 107: ALIGN
ALIGN_UP does the same, but is more obvious; had to look in the code to see if it does what i'd expe […]
All of the acpi code uses ALIGN. Used ALIGN_UP to clarify.
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@157 PS16, Line 157: acpigen_write_STA(0xf);
haven't looked at the details, but is this correct? didn't see the 0xf mentioned in the spec at this […]
IPMI spec chapter "C3-2 Locating IPMI System Interfaces in ACPI Name Space" recommends the usage of _STA, but it's not shown in the examples.
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@209 PS16, Line 209: 1
having this as a define would be nice, but could be done in a follow-up patch
Ack
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@230 PS16, Line 230: static void ipmi_enable_resources(struct device *dev)
is it really needed to have this empty function? also see the ops struct below
Done
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@237 PS16, Line 237: res->base = dev->path.pnp.port;
maybe check for alignment or mask the last bit?
done in enable_dev
https://review.coreboot.org/#/c/33255/16/src/drivers/ipmi/ipmi_kcs_ops.c@245 PS16, Line 245: .enable_resources = ipmi_enable_resources,
see above
Done