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

[Linda Xie]

Hi Anton,

Thank you very much for viewing the code. Please see my responses below
starting with LINDA.

Linda

[ Anton Blanchard <anton at samba.org> writes: ]

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

LINDA: Things like configure-connector and isolate/unisolate are done
      by user-land tools.

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

LINDA: They will be reomved from ppc_syms.c and put in
            the definition files.

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

LINDA:  I will remove them and call down/up directly.

+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?

LINDA: I will remove it and use 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?

LINDA: I will change it.

+ 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.

LINDA: Sounds good.

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

LINDA: I will remove some of them.

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