Attention is currently required from: Furquan Shaikh.

Patch set 2:Code-Review +1

View Change

3 comments:

• File src/soc/intel/common/block/uart/uart.c:

  • Patch Set #2, Line 61: devfn

    Hmmm... I feel this `devfn` can confuse people (at least it confuses me). I don't like the `pci_devfn_t` type name at all. We use it for PCI_DEV(b, d, f) values, whereas devfn's are plain ints. Ugh, no good solution.

  • Patch Set #2, Line 84: pci_devfn_t devfn

    These should be renamed to `dev` to avoid confusion with actual devfn-only values elsewhere.

  • Patch Set #2, Line 215:

    if (uart_controller_needs_init(dev)) {
    		uintptr_t base;
    		pci_devfn_t devfn = PCI_BDF(dev);
    
    		base = pci_s_read_config32(devfn, PCI_BASE_ADDRESS_0) & ~0xFFF;
    		if (base)
    			uart_lpss_init(devfn, base);
    	}
    

    This function is ramstage-only. Wouldn't it be enough to simply add PCI_BDF in the `uart_lpss_init` function call?

     	if (uart_controller_needs_init(dev)) {
     		uintptr_t base;
     
     		base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF;
     
     		if (base)
     			uart_lpss_init(PCI_BDF(dev), base);
     	}

    (Also, ramstage functions should be moved to a ramstage-only file, but that's for another patch)

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