Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 )
Change subject: pcidev: Move pci_get_dev() logic into canonical place ......................................................................
Patch Set 4:
(1 comment)
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/59276/comment/63c8f76a_99505f4f PS3, Line 1097: dev = pcidev_clonedev(dev);
Yes `pacc` is a handle scoped to libpci and so lexically scope to pcidev.c which is what this change is doing.
I see what your code is doing, but I don't know your reasoning. Are you just following design patterns you have learned or did you think it through? how should I know? That's why we have a mailing list where we can discuss such design choices. If we don't discuss such things and the reasons are not 100% obvious, another developer with a different opinion might accidently work on reverting your changes in the future. I've seen this a lot in coreboot btw. it's not just a figment.
Currently `pacc` is global yes. But as you well know we want to get rid of global state. So eventually, `pacc` might just become another, locally known parameter, like `dev` is to pci_read_long() etc. And I don't see (yet), that we'd be wrapping every single libpci function in `pcidev.c`. That's not saying that it's wrong to do so for some functions, I just think our reasoning should be more subtle.
Also, after reading above paragraphs together again: Can we tell that this change makes replacing the global `pacc` easier or harder? That's all things a reviewer must consider when the direction is unclear.