Skip to contentSkip to conversation

further sim710 updates

PATCH In review Christoph Hellwig 2 participants 3 messages Feb 10, 2003 SCSI

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 further sim710 updates

James, your sim710 changes look nice, but I think there's some more stuff to do in this driver. I've started with the following (all untested due to lack of the hardware):

- move to scsi_add_host/scsi_remove_host interface
- remove a bunch of unneeded ifdefs
- add missing statics
- unregister driver templates on unload

and I think there's more that should be done, but this would change user-visiable attributes:

- remove the ugly single module option code and the command line parsing in favour of Rusty's new module_param stuff
- split the driver into two drivers: 53c700_eisa and 53c700_mca. There's no logic shared between those two busses, just a little bit of helper in the setup/remove code

And after looking a this driver I have some rants about the new mac/eisa code:

- mca_register_driver/mca_unregister_driver should check for the precense of an MCA bus by themselves instead of leaving it to the caller
- eisa_driver_register should really return 0 for sucess
- mca_register_driver/mca_unregister_driver should be named mca_driver_register/mca_driver_unregister to be more similar to the other *driver_(un)registers.

drivers/scsi/sim710.c +79 -88
@@ -32,51 +32,18 @@
 #include <linux/blk.h>
 #include <linux/device.h>
 #include <linux/init.h>
-#ifdef CONFIG_MCA
 #include <linux/mca.h>
-#endif
-#ifdef CONFIG_EISA
 #include <linux/eisa.h>
-#endif
 
 #include "scsi.h"
 #include "hosts.h"
 #include "53c700.h"
 
+
 /* Must be enough for both EISA and MCA */
 #define MAX_SLOTS 8
 static __u8 __initdata id_array[MAX_SLOTS] = { [0 ... MAX_SLOTS-1] = 7 };
 
-/* info is used to communicate global data across the driver register
- * because the struct device_driver doesn't have any info fields.  Sigh */
-struct sim710_info {
-	Scsi_Host_Template *tpnt;
-	int found;
-};
-
-static __initdata struct sim710_info sim710_global_info;
-
-#if defined(CONFIG_MCA)
-
-/* CARD ID 01BB and 01BA use the same pos values */
-
-#define MCA_01BB_IO_PORTS { 0x0000, 0x0000, 0x0800, 0x0C00, 0x1000, 0x1400, \
-			    0x1800, 0x1C00, 0x2000, 0x2400, 0x2800, \
-			    0x2C00, 0x3000, 0x3400, 0x3800, 0x3C00, \
-			    0x4000, 0x4400, 0x4800, 0x4C00, 0x5000  }
-
-#define MCA_01BB_IRQS { 3, 5, 11, 14 }
-
-/* CARD ID 004f */
-
-#define MCA_004F_IO_PORTS { 0x0000, 0x0200, 0x0300, 0x0400, 0x0500,  0x0600 }
-
-#define MCA_004F_IRQS { 5, 9, 14 }
-
-#endif
-
-#ifdef MODULE
-
 char *sim710;		/* command line passed by insmod */
 
 MODULE_AUTHOR("Richard Hirst");

@@ -85,8 +52,6 @@
 
 MODULE_PARM(sim710, "s");
 
-#endif
-
 #ifdef MODULE
 #define ARG_SEP ' '
 #else

@@ -118,12 +83,15 @@
 	}
 	return 1;
 }
-
-#ifndef MODULE
 __setup("sim710=", param_setup);
-#endif
 
-__init int
+static Scsi_Host_Template sim710_driver_template = {
+	.name			= "LSI (Symbios) 710 MCA/EISA",
+	.proc_name		= "sim710",
+	.this_id		= 7,
+};
+
+static __devinit int
 sim710_probe_common(struct device *dev, unsigned long base_addr,
 		    int irq, int clock, int differential, int scsi_id)
 {

@@ -154,7 +122,7 @@
 	hostdata->chip710 = 1;
 
 	/* and register the chip */
-	if((host = NCR_700_detect(sim710_global_info.tpnt, hostdata)) == NULL) {
+	if((host = NCR_700_detect(&sim710_driver_template, hostdata)) == NULL) {
 		printk(KERN_ERR "sim710: No host detected; card configuration problem?\n");
 		goto out_release;
 	}

@@ -168,11 +136,9 @@
 		goto out_unregister;
 	}
 
-	scsi_set_device(host, dev);
+	scsi_add_host(host, dev);
 	hostdata->dev = dev;
 
-	sim710_global_info.found++;
-
 	return 0;
 
  out_unregister:

@@ -185,10 +151,37 @@
 	return -ENODEV;
 }
 
+static __devexit int
+sim710_device_remove(struct device *dev)
+{
+	struct Scsi_Host *host = to_scsi_host(dev);
+	struct NCR_700_Host_Parameters *hostdata =
+		(struct NCR_700_Host_Parameters *)host->hostdata[0];
+
+	scsi_remove_host(host);
+	NCR_700_release(host);
+	kfree(hostdata);
+	free_irq(host->irq, host);
+	return 0;
+}
+
 #ifdef CONFIG_MCA
+
+/* CARD ID 01BB and 01BA use the same pos values */
+#define MCA_01BB_IO_PORTS { 0x0000, 0x0000, 0x0800, 0x0C00, 0x1000, 0x1400, \
+			    0x1800, 0x1C00, 0x2000, 0x2400, 0x2800, \
+			    0x2C00, 0x3000, 0x3400, 0x3800, 0x3C00, \
+			    0x4000, 0x4400, 0x4800, 0x4C00, 0x5000  }
+
+#define MCA_01BB_IRQS { 3, 5, 11, 14 }
+
+/* CARD ID 004f */
+#define MCA_004F_IO_PORTS { 0x0000, 0x0200, 0x0300, 0x0400, 0x0500,  0x0600 }
+#define MCA_004F_IRQS { 5, 9, 14 }
+
 static short sim710_mca_id_table[] = { 0x01bb, 0x01ba, 0x004f, 0};
 
-__init int
+static __init int
 sim710_mca_probe(struct device *dev)
 {
 	struct mca_device *mca_dev = to_mca_device(dev);

@@ -268,26 +261,27 @@
 				   0, id_array[slot]);
 }
 
-struct mca_driver sim710_mca_driver = {
-	.id_table = sim710_mca_id_table,
+static struct mca_driver sim710_mca_driver = {
+	.id_table		= sim710_mca_id_table,
 	.driver = {
-		.name = "sim710",
-		.bus = &mca_bus_type,
-		.probe = sim710_mca_probe,
+		.name		= "sim710",
+		.bus		= &mca_bus_type,
+		.probe		= sim710_mca_probe,
+		.remove		= __devexit_p(sim710_device_remove),
 	},
 };
 
 #endif /* CONFIG_MCA */
 
 #ifdef CONFIG_EISA
-struct eisa_device_id sim710_eisa_ids[] = {
+static struct eisa_device_id sim710_eisa_ids[] = {
 	{ "CPQ4410" },
 	{ "CPQ4411" },
 	{ "HWP0C80" },
 	{ "" }
 };
 
-__init int
+static __init int
 sim710_eisa_probe(struct device *dev)
 {
 	struct eisa_device *edev = to_eisa_device(dev);

@@ -322,36 +316,18 @@
 }
 
 struct eisa_driver sim710_eisa_driver = {
-	.id_table = sim710_eisa_ids,
+	.id_table		= sim710_eisa_ids,
 	.driver = {
-		.name =		"sim710",
-		.probe =	sim710_eisa_probe,
-		.remove =	__devexit_p(sim710_device_remove),
+		.name		= "sim710",
+		.probe		= sim710_eisa_probe,
+		.remove		= __devexit_p(sim710_device_remove),
 	},
 };
-
 #endif /* CONFIG_EISA */
 
-
-int
-sim710_release(struct Scsi_Host *host)
+static int __init sim710_init(void)
 {
-	struct D700_Host_Parameters *hostdata = 
-		(struct D700_Host_Parameters *)host->hostdata[0];
-
-	NCR_700_release(host);
-	kfree(hostdata);
-	free_irq(host->irq, host);
-	/* should do a refcount here and unregister the drivers when
-	 * it reaches zero */
-	return 1;
-}
-
-int __init
-sim710_detect(Scsi_Host_Template *tpnt)
-{
-	sim710_global_info.tpnt = tpnt;
-	sim710_global_info.found = 0;
+	int err = -ENODEV, err2;
 
 #ifdef MODULE
 	if (sim710)

@@ -359,22 +335,37 @@
 #endif
 
 #ifdef CONFIG_MCA
-	if(MCA_bus)
-		mca_register_driver(&sim710_mca_driver);
+	if (MCA_bus)
+		err = mca_register_driver(&sim710_mca_driver);
 #endif
 
 #ifdef CONFIG_EISA
-	eisa_driver_register(&sim710_eisa_driver);
+	err2 = eisa_driver_register(&sim710_eisa_driver);
+
+	/*
+	 * The eise_driver_register return values are strange.  I have
+	 * no idea why we don't just use river_register directly anyway..
+	 */
+	if (err2 == 1)
+		err2 = 0;	
 #endif
-	return sim710_global_info.found;
+
+	if (err < 0 || err2 < 0)
+		return (err < 0) ? err : err2;
+	return 0;
 }
 
-static Scsi_Host_Template driver_template = {
-	.name =			"LSI (Symbios) 710 MCA/EISA",
-	.proc_name =		"sim710",
-	.detect =		sim710_detect,
-	.release =		sim710_release,
-	.this_id =		7,
-};
+static void __exit sim710_exit(void)
+{
+#ifdef CONFIG_MCA
+	if (MCA_bus)
+		mca_unregister_driver(&sim710_mca_driver);
+#endif
+
+#ifdef CONFIG_EISA
+	eisa_driver_unregister(&sim710_eisa_driver);
+#endif
+}
 
-#include "scsi_module.c"
+module_init(sim710_init);
+module_exit(sim710_exit);
#1 lore raw
Quoted 8 lines · Christoph Hellwig

and I think there's more that should be done, but this would change user-visiable attributes:

- remove the ugly single module option code and the command line parsing in favour of Rusty's new module_param stuff
- split the driver into two drivers: 53c700_eisa and 53c700_mca. There's no logic shared between those two busses, just a little bit of helper in the setup/remove code

Sounds good if you want to do a patch.

Quoted 6 lines

And after looking a this driver I have some rants about the new mac/eisa code:

- mca_register_driver/mca_unregister_driver should check for the precense of an MCA bus by themselves instead of leaving it to the caller

The if(MCA_bus) is unnecessary. If there's no MCA bus, no IDs will be stored, so nothing will get attached.

Quoted 1 line

- eisa_driver_register should really return 0 for sucess

Yes.

Quoted 3 lines

- mca_register_driver/mca_unregister_driver should be named mca_driver_register/mca_driver_unregister to be more similar to the other *driver_(un)registers.

Which others? I modelled the interface on PCI, which has pci_register_driver() etc. I agree on standardisation, but the way I did it was standard when the MCA bus code was written...

drivers/scsi/sim710.c line 37

Quoted 9 lines
drivers/scsi/sim710.c +0 -2
@@ -32,51 +32,18 @@
 #include <linux/blk.h>
 #include <linux/device.h>
 #include <linux/init.h>
-#ifdef CONFIG_MCA
 #include <linux/mca.h>
-#endif

This can't be done otherwise the driver won't compile on non x86 archs (yes, I know, I'll fix the MCA header file...)

drivers/scsi/sim710.c line 348

Quoted 4 lines
drivers/scsi/sim710.c +4 -0
+	/*
+	 * The eise_driver_register return values are strange.  I have
+	 * no idea why we don't just use river_register directly anyway..
+	 */

I can answer that: Some of the driver registration routines have to do bus and device fixups. There's no mechanism in the current device_driver template. When taxed with the problem, Patrick Mochel decided that retaining bus specific registration routines was the better way forward.

James

author
#2 lore raw
Quoted 11 lines · James Bottomley
Quoted 8 lines · Christoph Hellwig

and I think there's more that should be done, but this would change user-visiable attributes:

- remove the ugly single module option code and the command line parsing in favour of Rusty's new module_param stuff
- split the driver into two drivers: 53c700_eisa and 53c700_mca. There's no logic shared between those two busses, just a little bit of helper in the setup/remove code

Sounds good if you want to do a patch.

This will be in the next patch revision.

Quoted 6 lines
Quoted 3 lines

- mca_register_driver/mca_unregister_driver should check for the precense of an MCA bus by themselves instead of leaving it to the caller

The if(MCA_bus) is unnecessary. If there's no MCA bus, no IDs will be stored, so nothing will get attached.

Ok.

Quoted 7 lines
Quoted 3 lines

- mca_register_driver/mca_unregister_driver should be named mca_driver_register/mca_driver_unregister to be more similar to the other *driver_(un)registers.

Which others? I modelled the interface on PCI, which has pci_register_driver() etc. I agree on standardisation, but the way I did it was standard when the MCA bus code was written...

eisa and the plain, non-prefix versions. Okay, maybe MCA is okay, but at least it's inconsistant..

Quoted 10 lines

drivers/scsi/sim710.c line 37

Quoted 7 lines
drivers/scsi/sim710.c +0 -2
@@ -32,51 +32,18 @@
 #include <linux/blk.h>
 #include <linux/device.h>
 #include <linux/init.h>
-#ifdef CONFIG_MCA
 #include <linux/mca.h>
-#endif

This can't be done otherwise the driver won't compile on non x86 archs (yes, I know, I'll fix the MCA header file...)

If I split the EISA/MCA parts into separate driver we won't have that problem anymore.