On Fri, Jun 05, 2015 at 03:11:10PM -0500, Bjorn Helgaas wrote: >On Thu, Jun 04, 2015 at 04:42:11PM +1000, Gavin Shan wrote: >> The patch intends to add standalone driver to support PCI hotplug >> for PowerPC PowerNV platform, which runs on top of skiboot firmware. >> The firmware identified hotpluggable slots and marked their device >> tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware". >> The driver simply scans device-tree to create/register PCI hotplug slot >> accordingly. >> >> If the skiboot firmware doesn't support slot status retrieval, the PCI >> slot device node shouldn't have property "ibm,reset-by-firmware". In >> that case, none of valid PCI slots will be detected from device tree. >> The skiboot firmware doesn't export the capability to access attention >> LEDs yet and it's something for TBD. >> >> Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com> > >Acked-by: Bjorn Helgaas <bhelg...@google.com> > >But I do have a few comments (my ack is valid whether you do anything with >them or not): >
Thanks for your review, Bjorn. >> +static void slot_power_off_handler(struct powernv_php_slot *slot) >> +{ >> + int ret; >> + >> + /* Release the firmware data for the child device nodes */ >> + remove_child_pdn(slot->dn); >> + >> + /* >> + * Release the child device nodes. If the sub-tree was >> + * built with the help of overlay, we just need revert >> + * the changes introduced by the overlay >> + */ >> + if (slot->overlay_id >= 0) { >> + ret = of_overlay_destroy(slot->overlay_id); >> + if (ret) >> + pr_warn("%s: Error %d destroying overlay %d\n", >> + __func__, ret, slot->overlay_id); > >For this and similar messages: isn't there a device you can use with >dev_warn() here? I think a device name would be much better than a >function name. > There is PCI bus referred (struct powernv_php_slot::bus), but it's not always valid. So I'll add one more field "struct pci_dev *pdev" which is initialized to the parent PCI device of the slot, then print those messages with dev_warn(). >> +scan: >> + switch (presence) { >> + case POWERNV_PHP_SLOT_PRESENT: >> + if (rescan) { >> + pci_lock_rescan_remove(); >> + pcibios_add_pci_devices(slot->bus); > >You didn't add this, but "pcibios_add_pci_devices" doesn't seem like the >right name. "pcibios" generally refers to an arch-specific hook that's >called by the generic PCI core. In this case, pcibios_add_pci_devices() >contains powerpc-specific code, and it's only called from powerpc code, so >I think using "pcibios_" in the name is a bit misleading. > Ben already suggested some better names in another reply. I'll pick it if you agree: pci_add_pci_devices(). >> + /* Remove all devices behind the slot */ >> + pci_lock_rescan_remove(); >> + pcibios_remove_pci_devices(slot->bus); > >Same comment for pcibios_remove_pci_devices(). It would be better if the >name didn't suggest that this was part of the pcibios_ interface between >the PCI core and the arch code, because it's not. > According to Ben's suggestion in another reply, it would be pci_remove_pci_devices() if you agree :-) >> + /* Slot indentifier */ > >s/indentifier/identifier/ > Thanks for pointing it out. I'll fix it up in next revision. >> + if (!php_slot_get_id(dn, &id)) >> + return NULL; >> + > >> + /* PCI bus */ >> + bus = pcibios_find_pci_bus(dn); > >And pcibios_find_pci_bus() (it's also powerpc-specific). > I'll pick Ben's suggested name if you agree: of_pci_node_to_bus(). Thanks, Gavin >Bjorn > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev