[PATCH] PPC64 PCI Hot Plug Controller Driver-review request

[Anton Blanchard]

Hi Linda,

> The attached patch is the latest version of PPC64 Hot Plug Controller
> Driver for IBM pSeries platforms.

Looking good. Here are a few questions and suggestions.

I couldnt find where in the scheme of things that we do configure-connector.
Did that fragment of code end up on the mailing list?

@@ -135,6 +136,9 @@
 EXPORT_SYMBOL(pci_unmap_single);
 EXPORT_SYMBOL(pci_map_sg);
 EXPORT_SYMBOL(pci_unmap_sg);
+EXPORT_SYMBOL(pci_domain_nr);
+EXPORT_SYMBOL(pcibios_fixup_device_resources);
+EXPORT_SYMBOL(pci_read_irq_line);
 #ifdef CONFIG_PPC_ISERIES
 EXPORT_SYMBOL(iSeries_GetLocationData);
 EXPORT_SYMBOL(iSeries_Device_ToggleReset);

Could you move these right below the functions themselves?

+void ppc64php_lock(void)
+{
+	dbg("Entry %s\n", __FUNCTION__);
+
+	down(&ppc64php_sem);
+
+	dbg("Exit %s\n", __FUNCTION__);
+}

I prefer not to wrap standard locking functions but just use down etc
directly. Its very easy to forget if you have a sleeping lock or not
when its hidden away.

I noticed some of the other hotplug drivers do this, but I still dont
think its a good idea :)

+static inline int atoi(const char *s, int base)
+{
+        int i=0;
+
+        while (isdigit(*s))
+                i = i*base + *(s++) - '0';
+        return i;
+}

Could you use something generic here like simple_strtoul?

+static inline int ppc64php_get_sensor_state(int index, int *state)
+{
+	int rc;
+
+        rc = rtas_get_sensor(DR_ENTITY_SENSE, index, state);
+
+        if (rc) {
+	    if (rc ==  NEED_POWER || rc == PWR_ONLY) {
+			dbg("%s: slot must be power up to get sensor-state\n",
+				__FUNCTION__);
+	    }
+	    else if (rc == ERR_SENSE_USE) {
+			dbg("%s: slot is unusable\n", __FUNCTION__);
+	    }

Very minor, but could you place the else on the same line as the closing
brace?

+ struct pci_dev *ppc64php_find_pci_dev(struct device_node *dn)
+ static struct pci_dev *ppc64php_find_bridge_pdev(struct slot *slot)
+ static struct pci_dev *ppc64php_find_adapter_pdev(struct slot *slot)

Im wondering if these fit better in the generic ppc64 code. We can worry
about that later if and when someone else wants to use them.

+static int disable_slot(struct hotplug_slot *hotplug_slot)
+{
+	dbg("%s - Entry: slot[%s]\n",
+		__FUNCTION__, slot->name);

...

+	dbg("%s - Exit: rc[%d]\n",  __FUNCTION__, retval);
+}

I think it would be a bit more readable if you didnt have a debug entry for
every function entry and exit. I notice it seems to be the way to go in
some of the hotplug drivers :(

Anton

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/