On Wed, Apr 19, 2017 at 09:41:19PM +0300, Marcel Apfelbaum wrote: > On 04/19/2017 03:05 PM, Cornelia Huck wrote: > > On Tue, 18 Apr 2017 22:42:07 -0300 > > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > > > On Wed, Apr 19, 2017 at 10:29:26AM +1000, David Gibson wrote: > > > > On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote: > > > > > pci_bus_new*() and pci_register_bus() work only when the 'parent' > > > > > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they > > > > > are meant to initialize a bus that's in a PCI host bridge. > > > > > > > > > > The new function names are: > > > > > * pci_host_bus_init() (replacing pci_bus_new()) > > > > > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace()) > > > > > * pci_host_bus_init_irqs() (replacing pci_register_bus()) > > > > > > > > I like the idea, but I'm not terribly convinced by these names. > > > > Aren't functions which allocate objects usually called whatever_new() > > > > rather than whatever_init()? And pci_register_bus() appears to do > > > > more than just initialize irqs. > > > > > > I agree the names aren't terribly clear. This is what they are > > > supposed to mean: > > > > > > * pci_host_bus_init(phb) initializes phb->bus. > > > * pci_host_bus_init(phb) initializes phb->bus using an > > > already-allocated object. > > > * pci_host_bus_init_irqs() does the same as pci_host_bus_init(), > > > but also calls pci_bus_irqs(). > > > > > > I plan to submit API documentation comments later. I am open to > > > alternative name suggestions. > > > > > > > pci_host_bus_init_irqs() sounds as if it would only init irqs. What > > Right! This is what I thought too. > > > about: > > > > pci_host_bus_new() > > pci_host_bus_new_inplace() > > pci_host_bus_new_with_irqs() > > I like the names, but a following patch (5/6) modifies > the above functions to return void and create the bus > as a side effect. > So now we have a pci_host_bus_new that returns nothing?
I plan to change the approach, and not get rid of pci_bus_new(). I will probably just move the PCI_HOST_BRIDGE-specific code to a separate function (probably I will name it pci_host_init_bus()), that will call pci_bus_new(), append the bridge to pci_host_bridges, and set PCIHostState::bus. After that, we can change pci_bridge_initfn() to use pci_bus_new() instead of reimplementing it. -- Eduardo
