Attention is currently required from: Felix Singer, Tim Crawford, Patrick Rudolph, Jeremy Soller, Tim Wawrzynczak, Christian Walter. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60203 )
Change subject: mainboard: Fix DPTF device state in the devicetree on CFL boards ......................................................................
Patch Set 3: -Code-Review
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/60203/comment/23b9d5d1_2bd9b515 PS2, Line 14: Thus, set it to on to make the resource allocator aware of it.
Just copied the statement from below.
Ack. Technically there is nothing wrong with the statement, so I'll tag this as resolved. It's a poor commit message, though. Having the same text for rather complex and very simple cases suggests to reviewers that there is nothing to worry about, IMHO.
Changes to the devicetree always have further implications.
Yes, but sometimes they are simple, e.g. setting a device to `off` that doesn't show up anyway, and sometimes they are not.
I haven't listed each side effect in any of my other patches hooking up the FSP options to the devicetree.
Well, I can't talk about anonymous patches. Can you point me to any as subtle as this one?
Not sure why this is necessary here.
I guess you either imply that your past patches had equally complex implications or you still haven't understood the full scope of this one. Have your pick. To the former I would have to say: Having done something wrong in the past (lacking commit message) is the worst excuse to keep doing it wrong. For the latter, well, I suggest to read PCI device probing in `device/pci_device.c`. For instance, check what would happen if a device doesn't show up and is set to `on` or `off` (AFAICT, the devicetree state after probing should be the same, so no further consequences for the execution of core- boot). Then, compare that to the situation when a device does show up. These are completely different cases, yet you keep treating them the same.