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

Anton Blanchard anton at samba.org
Thu Oct 23 23:43:15 EST 2003

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 @@

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

+ 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 :(


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

More information about the Linuxppc64-dev mailing list