Matthew Wilcox wrote:
+static int __devinit
+advansys_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+ int ioport;
+ struct Scsi_Host *shost;
+
+ if (pci_enable_device(pdev))
+ goto fail;
+
+ ioport = pci_resource_start(pdev, 0);
+ shost = advansys_board_found(ioport, &pdev->dev, ASC_IS_PCI);
+
+ if (shost) {
+ pci_set_drvdata(pdev, shost);
+ return 0;
+ }
+
+ pci_disable_device(pdev);
+ fail:
+ return -ENODEV;
+}
1) you should propagate pci_enable_device return value to caller on error
1.1) in general, following my comment #1 will make this function look
more like other normal Linux code, i.e.
rc = foo
if (rc)
goto err_out
return 0;
err_out:
return rc;
2) you dropped the check for pci_resource_start() returning zero (look
for 'iop == 0' in original code)
3) what happened to PCI BAR #1 ? shouldn't you move that call here too?
it gets ioremapped (look at 'pci_memory_address' in old code, right
next to pci_resource_start call)
4) it is often wise to add sanity checks that ensure that PCI BAR #0 ==
IORESOURCE_IO and PCI BAR #1 == IORESOURCE_MEM
+static void __devexit advansys_pci_remove(struct pci_dev *pdev)
+{
+ int i;
+ struct Scsi_Host *shost = pci_get_drvdata(pdev);
+ scsi_remove_host(shost);
+ advansys_release(shost);
+
+ for (i = 0; i < asc_board_count; i++) {
+ if (asc_host[i] == shost) {
+ asc_host[i] = NULL;
+ break;
+ }
+ }
+
+ pci_disable_device(pdev);
5) call pci_set_drvdata(pdev, NULL) as with other PCI drivers
+static struct pci_driver advansys_pci_driver = {
+ .name = "advansys",
+ .id_table = advansys_pci_tbl,
+ .probe = advansys_pci_probe,
+ .remove = __devexit_p(advansys_pci_remove),
+};
6) suggestions: tab alignment for struct member values; makes it far
easier to read.
+static int __init advansys_init(void)
+{
+ int count, error;
+ count = advansys_detect();
+ error = pci_register_driver(&advansys_pci_driver);
+
+ /*
+ * If we found some ISA, EISA or VLB devices, we must not fail.
+ * We may not drive any PCI devices, but it's better to drive
+ * the cards that we successfully discovered than none at all.
+ */
+ if (count > 0)
+ error = 0;
+ return error;
+}
+
+static void __exit advansys_exit(void)
+{
+ int i;
+
+ pci_unregister_driver(&advansys_pci_driver);
7) bug: don't unregister, if pci_register_driver() failed during init
+ for (i = 0; i < asc_board_count; i++) {
+ struct Scsi_Host *host = asc_host[i];
+ if (!host)
+ continue;
+ scsi_remove_host(host);
+ advansys_release(host);
+ asc_host[i] = NULL;
this last line of code is rather pointless, isn't it?
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
