I don't particularly like how we encode PCI BDF for UART_PCI_ADDR like done in soc/intel/quark. We could do:

  uart_platform_pcidev(idx)
  {
     return PCI_DEV(0, 5, 5) | (1<<31)
  }

Then have weak declaration for the 0 case. The static Kconfig will not work if one happens to use add-on PCIe serial port, since bus number will be unknown.

These sound like valid points (to the extent that I understand PCI stuff, which is not very far ;) ), but should that really be part of this patch? As far as I can tell, all UART drivers for which uart_pci_addr actually means something are hardcoding it to CONFIG_UART_PCI_ADDR at the moment. The others are sometimes setting it to 0 and sometimes using uninitialized stack memory, which as Jacob/Coverity points out is just a bad idea overall. So what this patch does is fix that use of uninitialized data and simplify existing instances of setting the field into a single line of code (without changing behavior). I'd say that's in itself a benefit worth merging.

Now you may be right that CONFIG_UART_PCI_ADDR is a bad idea in itself and should be fixed. But that sounds like it might be a more complicated endeavor (e.g. I don't know what exactly that 0, 5, 5 means, but is that really fully platform- and architecture-independent?) which shouldn't hold up this fix?

Patch set 3:Code-Review +1

View Change

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