Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 13:
(2 comments)
I have posted some comments on the bug. I don't think adding sample code is the right thing to do here. It doesn't really help any partner devices. Please see my proposal on how we can make use of SSDT and devicetree configs to generate the required code at runtime.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1
No, It should be a dummy function and if any board needs to adjust their setting for the tablet mode […]
I don't understand what you mean by having a dummy function. How would a variant use this? What is it expected to do in these calls?
Also, since you have already provided implementations of POF1 and POF0 below, if any variant adds these methods, it is going to result in ACPI errors complaining about duplicate methods. How do you plan to handle that?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX
will not call this method. it's a sample name.
We don't really want to keep dead code which isn't really called/used. What is the intent of this function? Are variants expected to make a copy of this?