View Change

15 comments:

• Commit Message:

  • Patch Set #2, 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.

  • Patch Set #2, 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.

• File pcidev.c:

  • Patch Set #2, 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.

  • Patch Set #2, 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 *)'

  • Patch Set #2, Line 361: cleanup

    `clean up`

    Ack

  • Patch Set #2, Line 367: device->sysfs_path = NULL;

    What for? we release it before it could be dereferenced again.

    Ack

  • Patch Set #2, Line 404: return NULL;

    leaking `pcidev_bdf`

    Ack

  • Patch Set #2, Line 410: for (dev = pacc->devices; dev; dev = dev->next)

    Please always use braces if multiple lines follow.

    Ack

  • Patch Set #2, Line 411: if (pci_filter_match(&filter, dev)) {

    Please turn into […]

    Ack

  • Patch Set #2, Line 412:

    no space before semicolon

    Ack

  • Patch Set #2, Line 424: if (name)

    Please use a conditional expression or just: […]

    Ack

  • Patch Set #2, Line 452: card

    s/card/device/ ? it doesn't have to be a "card"

    Direct copy-paste from above.

  • Patch Set #2, Line 465: - 1,

    `- 1` is not necessary

    Ack

  • Patch Set #2, Line 467: 0

    found_dev->domain ?

    Ack

• File programmer.h:

  • Patch Set #2, 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.

To view, visit change 29078. To unsubscribe, or for help writing mail filters, visit settings.