[PATCH] further sim710 updates

[PATCH] further sim710 updates

[Christoph Hellwig]

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.


--- 1.8/drivers/scsi/sim710.c	Sun Feb  9 11:07:34 2003
+++ edited/drivers/scsi/sim710.c	Mon Feb 10 08:03:50 2003
@@ -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);

Re: [PATCH] further sim710 updates

[James Bottomley]

On Mon, 2003-02-10 at 01:59, Christoph Hellwig wrote:
> 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.

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

>   - eisa_driver_register should really return 0 for sucess

Yes.

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

> --- 1.8/drivers/scsi/sim710.c	Sun Feb  9 11:07:34 2003
> +++ edited/drivers/scsi/sim710.c	Mon Feb 10 08:03:50 2003
> @@ -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...)

> +	/*
> +	 * 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

Re: [PATCH] further sim710 updates

[Christoph Hellwig]

On Mon, Feb 10, 2003 at 09:27:27AM -0600, James Bottomley wrote:
> On Mon, 2003-02-10 at 01:59, Christoph Hellwig wrote:
> > 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.

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

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

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