Luc Verhaegen has posted comments on this change. ( https://review.coreboot.org/29078 )
Change subject: add newer pci infrastructure ......................................................................
Patch Set 2:
(15 comments)
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@9 PS2, Line 9: This code does not negatively affect existing pci support and existing : pci devices,
This reads like "This can't hurt, please wave through.", please […]
Has nothing to do with waving through, but everything to do with "will not touch nor break support based on the other infrastructure". No existing drivers need to be force ported.
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@16 PS2, Line 16: but means that support for other operating systems still : needs to be cobbled together.
Why not fall back to the existing dev-mem code on other OS'?
The base device detection struct has changed. This is central to the existing /dev/mem code.
As it stands now, it is limited to linux and to just the one programmer that uses it. Anyone can come along and fix either. We have not broken existing programmers, and we should not have broken support for other platforms.
Attempting to do a more thorough job of this quickly dissolves into a combination of: * a porting nightmare of existing programmers * a porting nightmare to all supported platforms Which in turn would completely deadlock this and future infrastructural changes.
This way, we get to add a sensible ati_spi programmer with 240 supported pci devices (today), in a reasonable timeframe with reasonable infrastructure. Users of other platforms and/or other programmers can follow as and when they have the time.
https://review.coreboot.org/#/c/29078/2/pcidev.c File pcidev.c:
https://review.coreboot.org/#/c/29078/2/pcidev.c@354 PS2, Line 354: */
What is with these empty comments? Please remove (or fill).
I have been using this like forever to both have a visual marker for function start, and to attempt to help me fill it in in future. Can be removed as stated.
https://review.coreboot.org/#/c/29078/2/pcidev.c@356 PS2, Line 356: void *
This makes C's type checking even weaker. Why not declare the parameter […]
To be able to use register_shutdown() on this directly.
compiler messages: error: passing argument 1 of 'xxx_shutdown' from incompatible pointer type [-Werror] note: expected 'int (*)(void *)' but argument is of type 'int (*)(struct flashrom_pci_device *)'
https://review.coreboot.org/#/c/29078/2/pcidev.c@361 PS2, Line 361: cleanup
`clean up`
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@367 PS2, Line 367: device->sysfs_path = NULL;
What for? we release it before it could be dereferenced again.
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@404 PS2, Line 404: return NULL;
leaking `pcidev_bdf`
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@410 PS2, Line 410: for (dev = pacc->devices; dev; dev = dev->next)
Please always use braces if multiple lines follow.
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@411 PS2, Line 411: if (pci_filter_match(&filter, dev)) {
Please turn into […]
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@412 PS2, Line 412:
no space before semicolon
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@424 PS2, Line 424: if (name)
Please use a conditional expression or just: […]
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@452 PS2, Line 452: card
s/card/device/ ? it doesn't have to be a "card"
Direct copy-paste from above.
https://review.coreboot.org/#/c/29078/2/pcidev.c@465 PS2, Line 465: - 1,
`- 1` is not necessary
Ack
https://review.coreboot.org/#/c/29078/2/pcidev.c@467 PS2, Line 467: 0
found_dev->domain ?
Ack
https://review.coreboot.org/#/c/29078/2/programmer.h File programmer.h:
https://review.coreboot.org/#/c/29078/2/programmer.h@852 PS2, Line 852: #if NEED_PCI == 1
Why guard? are there conflicting alternative definitions?
Iirc, struct pci_dev only exists when NEED_PCI is defined. Not sure whether there is a placeholder for that outside of NEED_PCI.