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 <gwshan@xxxxxxxxxxxxxxxxxx>
>
>Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
>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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
