Driver model ISA bus

Driver model ISA bus

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 6064 bytes --]

Hi Takashi.

During the recent discussion about ISA and platform devices I suggested
a seperate ISA bus instead of the platform bus for ISA devices due to
platform devices not providing the driver a chance to fail registration.
Russel King suggested using the ".match" bus method with this bus and
this indeed works fine -- .match is supposed to verify whether or not a
given driver supports a given device, meaning that it's a perfect
opportunity for an ISA driver to say "no, we don't support this
non-existant device" after failing a probe if it wants to.

Unlike the platform bus therefore this isa bus also distributes the
.match method up to the driver same as the other methods (probe, remove,
shutdown, suspend, resume). probe() works as before with platform
devices, but a non-match makes the device fail the registration.

Since with these ISA devices the devices also only exist by virtue of
the driver itself creating them because it might want to drive them,
actual device creation is also internal. The usage model is now very 
nicely minimal:

static struct isa_driver snd_foo_isa_driver = {
	.nr_devices	= SNDRV_CARDS,

	.match		= snd_foo_match,
	.probe		= snd_foo_probe,
	.remove		= __devexit_p(snd_foo_remove),
	.shutdown	= snd_foo_shutdown,
	.suspend	= snd_foo_suspend,
	.resume		= snd_foo_resume,
	
	.driver		= {
		.name	= DEV_NAME
	}
};

static int __init alsa_card_foo_init(void)
{
	return isa_register_driver(&snd_foo_driver);
}

static void __exit alsa_card_foo_exit(void)
{
	isa_unregister_driver(&snd_foo_isa_driver);
}

This gives all options -- if we want to keep existing behaviour of a
non-loading driver if no devices are found, we can just move the
existing platform probe() methods to the ISA match() methods, with no
aditional probe() needed. If on the other hand we want to load if a
later bind could still succeed as PCI does then we might use the match 
method only for the prerequisits, with probe() further as before:

static int __devinit snd_foo_match(struct isa_dev *isa_dev)
{
	unsigned int i = isa_dev->id;

	if (!enable[i])
		return 0;

	if (port[i] == SNDRV_AUTO_PORT)
		return 0;

	return 1;
}

static int __devinit snd_foo_probe(struct isa_dev *isa_dev)
{
	struct snd_card *card;

	...

	isa_set_drvdata(dev, card);
	return 0;
}

static int __devexit snd_foo_remove(struct isa_dev *isa_dev)
{
         snd_card_free(isa_get_drvdata(isa_dev));
         isa_set_drvdata(isa_dev, NULL);
         return 0;
}

Would you like this as a setup / model?

I also briefly looked into integrating this with PnP but this does not
seem to be a good match. The pnp_protocol {get,set}_resources() methods
would need to callback into drivers same as with this ISA bus match()
callback but this is at a time there might not _be_ a driver yet. This
is still that same fundamental difference of the ISA device only
existing due to a driver telling us it might. Even if there's a clean
way around that, you only end up inventing fake but valid-form PnP IDs
and generally catering to the PnP layer without any practical advantage
over this very simple isa_bus. The thing I suggested earlier (if you
read it) about the user setting up the device from userspace by echoing
values into /sys is... well, cute, but a horrible idea from a user
standpoint. Trying to force ISA into the PnP model just turns into
massive interface wankage.

About the code -- most of it will speak for itself. There are only two
exported functions, isa_register_driver() and isa_unregister_driver()
sinec as said the devices are completely internal and tied to the
driver. The match() method is the important difference from the platform
bus:

int isa_bus_match(struct device *dev, struct device_driver *driver)
{
     struct isa_driver *isa_driver = to_isa_driver(driver);

     if (dev->platform_data == isa_driver) {
         if (!isa_driver->match || isa_driver->match(to_isa_dev(dev)))
             return 1;
         dev->platform_data = NULL;
     }
     return 0;
}

As a first step in the match(), this assures that this device does
belong to this driver by checking if dev->platform_data is set to this
driver; isa_register_driver() abuses platform_data for this. I believe
it's available, but if not, moving the isa_driver pointer to struct
isa_dev is fine as well. Platform devices compare strings but since
these devices are internal, we can do it this way.

Then if the driver did not supply a match() method, it considers the
driver to always match and returns success. If the driver did supply a 
match method (which is assumed to return 0 or 1 as a boolean result same 
as the bus match method; if you'd prefer then < / >= 0 is ofcourse also 
an option) it's left up to the driver. If we did not match we lastly 
reset dev->platform_data to indicate this to isa_register_driver().

isa_register_driver() is basically what upto now the init methods are
doing manually (even more closely after they would've been converted 
from use of the platform_device_register_simple() API). It just loops 
over the "nr_devices" which is given as part of in the isa_driver struct 
creating devices and registering them. It then checks for successfull 
matches through dev->platfornm_data still being set, sticks sucessfully 
matched devices into a simple list hanging off the driver and 
unregisters the others again.

(this is done without any locking -- do I need any?)

If any error occurs, or no devices matched at all, it backs everything
out again and returns the error or -ENODEV.

Comments appreciated. I added the (yet again) converted adlib driver, 
using the more PCI-like model for keeping things registered and using
the match() method only for the preconditions. As said though, the
division between match() and probe() is now entirely up to the driver
and if you'd prefer (short or long term) to simply keep the behaviour of 
not loading, that is accomplished simply by moving the rest of probe() 
to match() as well.

Note, this is not being submitted at this point -- I just want to see 
first if you'd like this from the ALSA side.

Rene.



[-- Attachment #2: isa_bus.diff --]
[-- Type: text/plain, Size: 10629 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-30 23:10:10.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-30 23:11:33.000000000 +0200
@@ -5,13 +5,14 @@
 #include <sound/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/isa.h>
 #include <sound/core.h>
 #include <sound/initval.h>
 #include <sound/opl3.h>
 
 #define CRD_NAME "AdLib FM"
-#define DRV_NAME "snd_adlib"
+#define DEV_NAME "adlib"
+#define DRV_NAME "snd-" DEV_NAME
 
 MODULE_DESCRIPTION(CRD_NAME);
 MODULE_AUTHOR("Rene Herman");
@@ -31,36 +32,47 @@ MODULE_PARM_DESC(enable, "Enable " CRD_N
 module_param_array(port, long, NULL, 0444);
 MODULE_PARM_DESC(port, "Port # for " CRD_NAME " driver.");
 
-static struct platform_device *devices[SNDRV_CARDS];
+static int __devinit snd_adlib_match(struct isa_dev *isa_dev)
+{
+	unsigned int i	= isa_dev->id;
+	char *bus_id	= isa_dev->dev.bus_id;
+
+	if (!enable[i])
+		return 0;
+
+	if (port[i] == SNDRV_AUTO_PORT) {
+		snd_printk(KERN_ERR "%s: please specify port\n", bus_id);
+		return 0;
+	}
+
+	return 1;
+}
 
 static void snd_adlib_free(struct snd_card *card)
 {
 	release_and_free_resource(card->private_data);
 }
 
-static int __devinit snd_adlib_probe(struct platform_device *device)
+static int __devinit snd_adlib_probe(struct isa_dev *isa_dev)
 {
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error, i = device->id;
+	int error;
 
-	if (port[i] == SNDRV_AUTO_PORT) {
-		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
-		error = -EINVAL;
-		goto out0;
-	}
+	unsigned int i	= isa_dev->id;
+	char *bus_id	= isa_dev->dev.bus_id;
 
 	card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
 	if (!card) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create card\n");
+		snd_printk(KERN_ERR "%s: could not create card\n", bus_id);
 		error = -EINVAL;
 		goto out0;
 	}
 
 	card->private_data = request_region(port[i], 4, CRD_NAME);
 	if (!card->private_data) {
-		snd_printk(KERN_ERR DRV_NAME ": could not grab ports\n");
+		snd_printk(KERN_ERR "%s: could not grab ports\n", bus_id);
 		error = -EBUSY;
 		goto out1;
 	}
@@ -68,101 +80,62 @@ static int __devinit snd_adlib_probe(str
 
 	error = snd_opl3_create(card, port[i], port[i] + 2, OPL3_HW_AUTO, 1, &opl3);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create OPL\n");
+		snd_printk(KERN_ERR "%s: could not create OPL\n", bus_id);
 		goto out1;
 	}
 
 	error = snd_opl3_hwdep_new(opl3, 0, 0, NULL);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create FM\n");
+		snd_printk(KERN_ERR "%s: could not create FM\n", bus_id);
 		goto out1;
 	}
 
 	strcpy(card->driver, DRV_NAME);
-	strcpy(card->shortname, CRD_NAME);
+	strcpy(card->shortname, DEV_NAME);
 	sprintf(card->longname, CRD_NAME " at %#lx", port[i]);
 
-	snd_card_set_dev(card, &device->dev);
+	snd_card_set_dev(card, &isa_dev->dev);
 
 	error = snd_card_register(card);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not register card\n");
+		snd_printk(KERN_ERR "%s: could not register card\n", bus_id);
 		goto out1;
 	}
 
-	platform_set_drvdata(device, card);
+	isa_set_drvdata(isa_dev, card);
 	return 0;
 
 out1:	snd_card_free(card);
 out0:	return error;
 }
 
-static int __devexit snd_adlib_remove(struct platform_device *device)
+static int __devexit snd_adlib_remove(struct isa_dev *isa_dev)
 {
-	snd_card_free(platform_get_drvdata(device));
-	platform_set_drvdata(device, NULL);
+	snd_card_free(isa_get_drvdata(isa_dev));
+	isa_set_drvdata(isa_dev, NULL);
 	return 0;
 }
 
-static struct platform_driver snd_adlib_driver = {
+static struct isa_driver snd_adlib_driver = {
+	.nr_devices	= SNDRV_CARDS,
+
+	.match		= snd_adlib_match,
 	.probe		= snd_adlib_probe,
 	.remove		= __devexit_p(snd_adlib_remove),
 
 	.driver		= {
-		.name	= DRV_NAME
+		.name	= DEV_NAME
 	}
 };
 
 static int __init alsa_card_adlib_init(void)
 {
-	int i, cards;
-
-	if (platform_driver_register(&snd_adlib_driver) < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not register driver\n");
-		return -ENODEV;
-	}
-
-	for (cards = 0, i = 0; i < SNDRV_CARDS; i++) {
-		struct platform_device *device;
-
-		if (!enable[i])
-			continue;
-
-		device = platform_device_alloc(DRV_NAME, i);
-		if (!device)
-			continue;
-
-		if (platform_device_add(device) < 0) {
-			platform_device_put(device);
-			continue;
-		}
-
-		if (!platform_get_drvdata(device)) {
-			platform_device_unregister(device);
-			continue;
-		}
-
-		devices[i] = device;
-		cards++;
-	}
-
-	if (!cards) {
-#ifdef MODULE
-		printk(KERN_ERR CRD_NAME " soundcard not found or device busy\n");
-#endif
-		platform_driver_unregister(&snd_adlib_driver);
-		return -ENODEV;
-	}
-	return 0;
+	return isa_register_driver(&snd_adlib_driver);
 }
 
 static void __exit alsa_card_adlib_exit(void)
 {
-	int i;
-
-	for (i = 0; i < SNDRV_CARDS; i++)
-		platform_device_unregister(devices[i]);
-	platform_driver_unregister(&snd_adlib_driver);
+	isa_unregister_driver(&snd_adlib_driver);
 }
 
 module_init(alsa_card_adlib_init);
Index: local/drivers/base/isa.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/drivers/base/isa.c	2006-05-01 00:23:42.000000000 +0200
@@ -0,0 +1,169 @@
+/*
+ * ISA bus.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/isa.h>
+
+static struct device isa_bus = {
+	.bus_id		= "isa"
+};
+
+static int isa_bus_match(struct device *dev, struct device_driver *driver)
+{
+	struct isa_driver *isa_driver = to_isa_driver(driver);
+
+	if (dev->platform_data == isa_driver) {
+		if (!isa_driver->match || isa_driver->match(to_isa_dev(dev)))
+			return 1;
+		dev->platform_data = NULL;
+	}
+	return 0;
+}
+
+static int isa_bus_probe(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->probe)
+		return isa_driver->probe(to_isa_dev(dev));
+
+	return 0;
+}
+
+static int isa_bus_remove(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->remove)
+	       return isa_driver->remove(to_isa_dev(dev));
+
+	return 0;
+}
+
+static void isa_bus_shutdown(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->shutdown)
+		isa_driver->shutdown(to_isa_dev(dev));
+}
+
+static int isa_bus_suspend(struct device *dev, pm_message_t state)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->suspend)
+		return isa_driver->suspend(to_isa_dev(dev), state);
+
+	return 0;
+}
+
+static int isa_bus_resume(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->resume)
+	       return isa_driver->resume(to_isa_dev(dev));
+
+	return 0;
+}
+
+static struct bus_type isa_bus_type = {
+	.name		= "isa",
+	.match		= isa_bus_match,
+	.probe		= isa_bus_probe,
+	.remove		= isa_bus_remove,
+	.shutdown	= isa_bus_shutdown,
+	.suspend	= isa_bus_suspend,
+	.resume		= isa_bus_resume
+};
+
+static void isa_dev_release(struct device *dev)
+{
+	kfree(to_isa_dev(dev));
+}
+
+void isa_unregister_driver(struct isa_driver *isa_driver)
+{
+	struct isa_dev *isa_dev = isa_driver->devices;
+
+	while (isa_dev) {
+		struct device *dev = &isa_dev->dev;
+		isa_dev = isa_dev->next;
+		device_unregister(dev);
+	}
+	driver_unregister(&isa_driver->driver);
+}
+EXPORT_SYMBOL_GPL(isa_unregister_driver);
+
+int isa_register_driver(struct isa_driver *isa_driver)
+{
+	int error;
+	unsigned int id;
+
+	isa_driver->driver.bus	= &isa_bus_type;
+	isa_driver->devices	= NULL;
+
+	error = driver_register(&isa_driver->driver);
+	if (error)
+		return error;
+
+	for (id = 0; id < isa_driver->nr_devices; id++) {
+		struct isa_dev *isa_dev;
+
+		isa_dev = kzalloc(sizeof *isa_dev, GFP_KERNEL);
+		if (!isa_dev) {
+			error = -ENOMEM;
+			break;
+		}
+
+		isa_dev->dev.parent	= &isa_bus;
+		isa_dev->dev.bus	= &isa_bus_type;
+
+		snprintf(isa_dev->dev.bus_id, BUS_ID_SIZE, "%s.%u",
+				isa_driver->driver.name, id);
+
+		isa_dev->dev.platform_data	= isa_driver;
+		isa_dev->dev.release		= isa_dev_release;
+		isa_dev->id			= id;
+
+		error = device_register(&isa_dev->dev);
+		if (error) {
+			put_device(&isa_dev->dev);
+			break;
+		}
+
+		if (isa_dev->dev.platform_data) {
+			isa_dev->next = isa_driver->devices;
+			isa_driver->devices = isa_dev;
+		} else
+			device_unregister(&isa_dev->dev);
+	}
+
+	if (!error && !isa_driver->devices)
+		error = -ENODEV;
+
+	if (error)
+		isa_unregister_driver(isa_driver);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(isa_register_driver);
+
+static int __init isa_bus_init(void)
+{
+	int error;
+
+	error = bus_register(&isa_bus_type);
+	if (!error) {
+		error = device_register(&isa_bus);
+		if (error)
+			bus_unregister(&isa_bus_type);
+	}
+	return error;
+}
+
+device_initcall(isa_bus_init);
Index: local/include/linux/isa.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/include/linux/isa.h	2006-04-30 23:11:33.000000000 +0200
@@ -0,0 +1,40 @@
+/*
+ * ISA bus.
+ */
+
+#ifndef __LINUX_ISA_H
+#define __LINUX_ISA_H
+
+#include <linux/device.h>
+
+struct isa_dev {
+	struct device dev;
+	unsigned int id;
+	struct isa_dev *next;
+};
+
+#define to_isa_dev(x) container_of((x), struct isa_dev, dev)
+
+struct isa_driver {
+	unsigned int nr_devices;
+	struct isa_dev *devices;
+
+	int (*match)(struct isa_dev *);
+	int (*probe)(struct isa_dev *);
+	int (*remove)(struct isa_dev *);
+	void (*shutdown)(struct isa_dev *);
+	int (*suspend)(struct isa_dev *, pm_message_t);
+	int (*resume)(struct isa_dev *);
+
+	struct device_driver driver;
+};
+
+#define to_isa_driver(x) container_of((x), struct isa_driver, driver)
+
+#define isa_get_drvdata(x)	 dev_get_drvdata(&(x)->dev)
+#define isa_set_drvdata(x, data) dev_set_drvdata(&(x)->dev, (data))
+
+int isa_register_driver(struct isa_driver *);
+void isa_unregister_driver(struct isa_driver *);
+
+#endif /* __LINUX_ISA_H */
Index: local/drivers/base/Makefile
===================================================================
--- local.orig/drivers/base/Makefile	2006-04-30 23:10:10.000000000 +0200
+++ local/drivers/base/Makefile	2006-04-30 23:11:33.000000000 +0200
@@ -4,6 +4,7 @@ obj-y			:= core.o sys.o bus.o dd.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o dmapool.o \
 			   attribute_container.o transport_class.o
+obj-$(CONFIG_ISA)	+= isa.o
 obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o

Re: Driver model ISA bus

[Takashi Iwai]

At Mon, 01 May 2006 01:02:54 +0200,
Rene Herman wrote:
> 
> Hi Takashi.
> 
> During the recent discussion about ISA and platform devices I suggested
> a seperate ISA bus instead of the platform bus for ISA devices due to
> platform devices not providing the driver a chance to fail registration.
> Russel King suggested using the ".match" bus method with this bus and
> this indeed works fine -- .match is supposed to verify whether or not a
> given driver supports a given device, meaning that it's a perfect
> opportunity for an ISA driver to say "no, we don't support this
> non-existant device" after failing a probe if it wants to.
> 
> Unlike the platform bus therefore this isa bus also distributes the
> .match method up to the driver same as the other methods (probe, remove,
> shutdown, suspend, resume). probe() works as before with platform
> devices, but a non-match makes the device fail the registration.
> 
> Since with these ISA devices the devices also only exist by virtue of
> the driver itself creating them because it might want to drive them,
> actual device creation is also internal. The usage model is now very 
> nicely minimal:
> 
> static struct isa_driver snd_foo_isa_driver = {
> 	.nr_devices	= SNDRV_CARDS,
> 
> 	.match		= snd_foo_match,
> 	.probe		= snd_foo_probe,
> 	.remove		= __devexit_p(snd_foo_remove),
> 	.shutdown	= snd_foo_shutdown,
> 	.suspend	= snd_foo_suspend,
> 	.resume		= snd_foo_resume,
> 	
> 	.driver		= {
> 		.name	= DEV_NAME
> 	}
> };
> 
> static int __init alsa_card_foo_init(void)
> {
> 	return isa_register_driver(&snd_foo_driver);
> }
> 
> static void __exit alsa_card_foo_exit(void)
> {
> 	isa_unregister_driver(&snd_foo_isa_driver);
> }
> 
> This gives all options -- if we want to keep existing behaviour of a
> non-loading driver if no devices are found, we can just move the
> existing platform probe() methods to the ISA match() methods, with no
> aditional probe() needed. If on the other hand we want to load if a
> later bind could still succeed as PCI does then we might use the match 
> method only for the prerequisits, with probe() further as before:
> 
> static int __devinit snd_foo_match(struct isa_dev *isa_dev)
> {
> 	unsigned int i = isa_dev->id;
> 
> 	if (!enable[i])
> 		return 0;
> 
> 	if (port[i] == SNDRV_AUTO_PORT)
> 		return 0;
> 
> 	return 1;
> }
> 
> static int __devinit snd_foo_probe(struct isa_dev *isa_dev)
> {
> 	struct snd_card *card;
> 
> 	...
> 
> 	isa_set_drvdata(dev, card);
> 	return 0;
> }
> 
> static int __devexit snd_foo_remove(struct isa_dev *isa_dev)
> {
>          snd_card_free(isa_get_drvdata(isa_dev));
>          isa_set_drvdata(isa_dev, NULL);
>          return 0;
> }
> 
> Would you like this as a setup / model?

Yep, I like this implementation.  It's pretty straightforward.


> I also briefly looked into integrating this with PnP but this does not
> seem to be a good match. The pnp_protocol {get,set}_resources() methods
> would need to callback into drivers same as with this ISA bus match()
> callback but this is at a time there might not _be_ a driver yet. This
> is still that same fundamental difference of the ISA device only
> existing due to a driver telling us it might. Even if there's a clean
> way around that, you only end up inventing fake but valid-form PnP IDs
> and generally catering to the PnP layer without any practical advantage
> over this very simple isa_bus. The thing I suggested earlier (if you
> read it) about the user setting up the device from userspace by echoing
> values into /sys is... well, cute, but a horrible idea from a user
> standpoint. Trying to force ISA into the PnP model just turns into
> massive interface wankage.

Agreed that it's not so easy to merge it together with pnp_driver.
That's why I originally used platform_driver for ISA.


Takashi


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: Re: Driver model ISA bus

[Jaroslav Kysela]

On Tue, 2 May 2006, Takashi Iwai wrote:

> > Would you like this as a setup / model?
> 
> Yep, I like this implementation.  It's pretty straightforward.

Acked-by: Jaroslav Kysela <perex@suse.cz>

I like it too. It removes really most of duplicated code.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: Re: Driver model ISA bus

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

Jaroslav Kysela wrote:

> On Tue, 2 May 2006, Takashi Iwai wrote:
> 
>> Yep, I like this implementation.  It's pretty straightforward.
> 
> Acked-by: Jaroslav Kysela <perex@suse.cz>
> 
> I like it too. It removes really most of duplicated code.

Okay, great, thanks. I've tweaked it a _bit_ more. The device creation 
is all internal to the bus and isa_dev's only leaked out due to being 
passed to the driver callbacks. Keeping them really internal is much 
cleaner. Other than the "struct device *" we only want the ->id, so 
let's just pass that directly. Ie:

static int snd_foo_probe(struct device *dev, unsigned int i)

Makes for nicer code as well as with snd_card_set_dev(card, dev) versus 
snd_card_set_dev(card, &isa_dev.dev) and the like...

Second tweak -- move nr_devices from the isa_driver struct to a 
parameter to isa_register_driver(). Ie,

int __init alsa_card_foo_init(void)
{
	return isa_register_driver(&snd_foo_isa_driver, SNDRV_CARDS);
}

That count is only being used by isa_register_driver() so belongs there 
really, not as part of the driver struct. I doubt anyone will ever want 
to do so, but in theory you could call isa_register_driver() with the 
same driver struct but a different count depending on options, or do 
more calls then one, or ...

Attached applies against both 2.6.16.11 and 2.6.17-rc3. If no specific 
comments, further wishes or NAKs after all, I'll submit this. I think 
it's nice -- in the next message I'll attach a converted snd-adlib again 
and I like how that gets reallyreally minimal this way...

Rene.


[-- Attachment #2: isa_bus-3.diff --]
[-- Type: text/plain, Size: 5569 bytes --]

Index: local/drivers/base/isa.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/drivers/base/isa.c	2006-05-02 23:18:34.000000000 +0200
@@ -0,0 +1,180 @@
+/*
+ * ISA bus.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/isa.h>
+
+static struct device isa_bus = {
+	.bus_id		= "isa"
+};
+
+struct isa_dev {
+	struct device dev;
+	struct device *next;
+	unsigned int id;
+};
+
+#define to_isa_dev(x) container_of((x), struct isa_dev, dev)
+
+static int isa_bus_match(struct device *dev, struct device_driver *driver)
+{
+	struct isa_driver *isa_driver = to_isa_driver(driver);
+
+	if (dev->platform_data == isa_driver) {
+		if (!isa_driver->match ||
+			isa_driver->match(dev, to_isa_dev(dev)->id))
+			return 1;
+		dev->platform_data = NULL;
+	}
+	return 0;
+}
+
+static int isa_bus_probe(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->probe)
+		return isa_driver->probe(dev, to_isa_dev(dev)->id);
+
+	return 0;
+}
+
+static int isa_bus_remove(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->remove)
+		return isa_driver->remove(dev, to_isa_dev(dev)->id);
+
+	return 0;
+}
+
+static void isa_bus_shutdown(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->shutdown)
+		isa_driver->shutdown(dev, to_isa_dev(dev)->id);
+}
+
+static int isa_bus_suspend(struct device *dev, pm_message_t state)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->suspend)
+		return isa_driver->suspend(dev, to_isa_dev(dev)->id, state);
+
+	return 0;
+}
+
+static int isa_bus_resume(struct device *dev)
+{
+	struct isa_driver *isa_driver = dev->platform_data;
+
+	if (isa_driver->resume)
+		return isa_driver->resume(dev, to_isa_dev(dev)->id);
+
+	return 0;
+}
+
+static struct bus_type isa_bus_type = {
+	.name		= "isa",
+	.match		= isa_bus_match,
+	.probe		= isa_bus_probe,
+	.remove		= isa_bus_remove,
+	.shutdown	= isa_bus_shutdown,
+	.suspend	= isa_bus_suspend,
+	.resume		= isa_bus_resume
+};
+
+static void isa_dev_release(struct device *dev)
+{
+	kfree(to_isa_dev(dev));
+}
+
+void isa_unregister_driver(struct isa_driver *isa_driver)
+{
+	struct device *dev = isa_driver->devices;
+
+	while (dev) {
+		struct device *tmp = to_isa_dev(dev)->next;
+		device_unregister(dev);
+		dev = tmp;
+	}
+	driver_unregister(&isa_driver->driver);
+}
+EXPORT_SYMBOL_GPL(isa_unregister_driver);
+
+int isa_register_driver(struct isa_driver *isa_driver, unsigned int ndev)
+{
+	int error;
+	unsigned int id;
+
+	isa_driver->driver.bus	= &isa_bus_type;
+	isa_driver->devices	= NULL;
+
+	error = driver_register(&isa_driver->driver);
+	if (error)
+		return error;
+
+	for (id = 0; id < ndev; id++) {
+		struct isa_dev *isa_dev;
+
+		isa_dev = kzalloc(sizeof *isa_dev, GFP_KERNEL);
+		if (!isa_dev) {
+			error = -ENOMEM;
+			break;
+		}
+
+		isa_dev->dev.parent	= &isa_bus;
+		isa_dev->dev.bus	= &isa_bus_type;
+
+		snprintf(isa_dev->dev.bus_id, BUS_ID_SIZE, "%s.%u",
+				isa_driver->driver.name, id);
+
+		isa_dev->dev.platform_data	= isa_driver;
+		isa_dev->dev.release		= isa_dev_release;
+		isa_dev->id			= id;
+
+		error = device_register(&isa_dev->dev);
+		if (error) {
+			put_device(&isa_dev->dev);
+			break;
+		}
+
+		if (isa_dev->dev.platform_data) {
+			isa_dev->next = isa_driver->devices;
+			isa_driver->devices = &isa_dev->dev;
+		} else
+			device_unregister(&isa_dev->dev);
+	}
+
+	if (!error && !isa_driver->devices)
+		error = -ENODEV;
+
+	if (error)
+		isa_unregister_driver(isa_driver);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(isa_register_driver);
+
+static int __init isa_bus_init(void)
+{
+	int error;
+
+	error = bus_register(&isa_bus_type);
+	if (!error) {
+		error = device_register(&isa_bus);
+		if (error)
+			bus_unregister(&isa_bus_type);
+	}
+	return error;
+}
+
+device_initcall(isa_bus_init);
Index: local/include/linux/isa.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/include/linux/isa.h	2006-05-02 23:17:18.000000000 +0200
@@ -0,0 +1,28 @@
+/*
+ * ISA bus.
+ */
+
+#ifndef __LINUX_ISA_H
+#define __LINUX_ISA_H
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+
+struct isa_driver {
+	int (*match)(struct device *, unsigned int);
+	int (*probe)(struct device *, unsigned int);
+	int (*remove)(struct device *, unsigned int);
+	void (*shutdown)(struct device *, unsigned int);
+	int (*suspend)(struct device *, unsigned int, pm_message_t);
+	int (*resume)(struct device *, unsigned int);
+
+	struct device_driver driver;
+	struct device *devices;
+};
+
+#define to_isa_driver(x) container_of((x), struct isa_driver, driver)
+
+int isa_register_driver(struct isa_driver *, unsigned int);
+void isa_unregister_driver(struct isa_driver *);
+
+#endif /* __LINUX_ISA_H */
Index: local/drivers/base/Makefile
===================================================================
--- local.orig/drivers/base/Makefile	2006-05-02 22:44:00.000000000 +0200
+++ local/drivers/base/Makefile	2006-05-02 22:45:32.000000000 +0200
@@ -4,6 +4,7 @@ obj-y			:= core.o sys.o bus.o dd.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o dmapool.o \
 			   attribute_container.o transport_class.o
+obj-$(CONFIG_ISA)	+= isa.o
 obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o

Re: Re: Driver model ISA bus

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

Rene Herman wrote:

> in the next message I'll attach a converted snd-adlib again

Generated against 2.6.17-rc3. Hope that's new enough. Haven't gotten 
around to fiddling with mercurial yet.

Just as an example anyway; I'll do the conversion of all the ISA drivers 
if the bus is accepted upstream.

Rene.

[-- Attachment #2: adlib_isa_bus.diff --]
[-- Type: text/plain, Size: 4954 bytes --]

Index: linux-2.6.17-rc3/sound/isa/adlib.c
===================================================================
--- linux-2.6.17-rc3.orig/sound/isa/adlib.c	2006-05-02 23:47:30.000000000 +0200
+++ linux-2.6.17-rc3/sound/isa/adlib.c	2006-05-02 23:48:28.000000000 +0200
@@ -5,13 +5,14 @@
 #include <sound/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/isa.h>
 #include <sound/core.h>
 #include <sound/initval.h>
 #include <sound/opl3.h>
 
 #define CRD_NAME "AdLib FM"
-#define DRV_NAME "snd_adlib"
+#define DEV_NAME "adlib"
+#define DRV_NAME "snd-" DEV_NAME
 
 MODULE_DESCRIPTION(CRD_NAME);
 MODULE_AUTHOR("Rene Herman");
@@ -31,36 +32,41 @@ MODULE_PARM_DESC(enable, "Enable " CRD_N
 module_param_array(port, long, NULL, 0444);
 MODULE_PARM_DESC(port, "Port # for " CRD_NAME " driver.");
 
-static struct platform_device *devices[SNDRV_CARDS];
+static int __devinit snd_adlib_match(struct device *dev, unsigned int i)
+{
+	if (!enable[i])
+		return 0;
+
+	if (port[i] == SNDRV_AUTO_PORT) {
+		snd_printk(KERN_ERR "%s: please specify port\n", dev->bus_id);
+		return 0;
+	}
+
+	return 1;
+}
 
 static void snd_adlib_free(struct snd_card *card)
 {
 	release_and_free_resource(card->private_data);
 }
 
-static int __devinit snd_adlib_probe(struct platform_device *device)
+static int __devinit snd_adlib_probe(struct device *dev, unsigned int i)
 {
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error, i = device->id;
-
-	if (port[i] == SNDRV_AUTO_PORT) {
-		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
-		error = -EINVAL;
-		goto out0;
-	}
+	int error;
 
 	card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
 	if (!card) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create card\n");
+		snd_printk(KERN_ERR "%s: could not create card\n", dev->bus_id);
 		error = -EINVAL;
 		goto out0;
 	}
 
 	card->private_data = request_region(port[i], 4, CRD_NAME);
 	if (!card->private_data) {
-		snd_printk(KERN_ERR DRV_NAME ": could not grab ports\n");
+		snd_printk(KERN_ERR "%s: could not grab ports\n", dev->bus_id);
 		error = -EBUSY;
 		goto out1;
 	}
@@ -68,96 +74,60 @@ static int __devinit snd_adlib_probe(str
 
 	error = snd_opl3_create(card, port[i], port[i] + 2, OPL3_HW_AUTO, 1, &opl3);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create OPL\n");
+		snd_printk(KERN_ERR "%s: could not create OPL\n", dev->bus_id);
 		goto out1;
 	}
 
 	error = snd_opl3_hwdep_new(opl3, 0, 0, NULL);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not create FM\n");
+		snd_printk(KERN_ERR "%s: could not create FM\n", dev->bus_id);
 		goto out1;
 	}
 
 	strcpy(card->driver, DRV_NAME);
-	strcpy(card->shortname, CRD_NAME);
+	strcpy(card->shortname, DEV_NAME);
 	sprintf(card->longname, CRD_NAME " at %#lx", port[i]);
 
-	snd_card_set_dev(card, &device->dev);
+	snd_card_set_dev(card, dev);
 
 	error = snd_card_register(card);
 	if (error < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not register card\n");
+		snd_printk(KERN_ERR "%s: could not register card\n", dev->bus_id);
 		goto out1;
 	}
 
-	platform_set_drvdata(device, card);
+	dev_set_drvdata(dev, card);
 	return 0;
 
 out1:	snd_card_free(card);
 out0:	return error;
 }
 
-static int __devexit snd_adlib_remove(struct platform_device *device)
+static int __devexit snd_adlib_remove(struct device *dev, unsigned int i)
 {
-	snd_card_free(platform_get_drvdata(device));
-	platform_set_drvdata(device, NULL);
+	snd_card_free(dev_get_drvdata(dev));
+	dev_set_drvdata(dev, NULL);
 	return 0;
 }
 
-static struct platform_driver snd_adlib_driver = {
+static struct isa_driver snd_adlib_driver = {
+	.match		= snd_adlib_match,
 	.probe		= snd_adlib_probe,
 	.remove		= __devexit_p(snd_adlib_remove),
 
 	.driver		= {
-		.name	= DRV_NAME
+		.name	= DEV_NAME
 	}
 };
 
 static int __init alsa_card_adlib_init(void)
 {
-	int i, cards;
-
-	if (platform_driver_register(&snd_adlib_driver) < 0) {
-		snd_printk(KERN_ERR DRV_NAME ": could not register driver\n");
-		return -ENODEV;
-	}
-
-	for (cards = 0, i = 0; i < SNDRV_CARDS; i++) {
-		struct platform_device *device;
-
-		if (!enable[i])
-			continue;
-
-		device = platform_device_register_simple(DRV_NAME, i, NULL, 0);
-		if (IS_ERR(device))
-			continue;
-
-		if (!platform_get_drvdata(device)) {
-			platform_device_unregister(device);
-			continue;
-		}
-
-		devices[i] = device;
-		cards++;
-	}
-
-	if (!cards) {
-#ifdef MODULE
-		printk(KERN_ERR CRD_NAME " soundcard not found or device busy\n");
-#endif
-		platform_driver_unregister(&snd_adlib_driver);
-		return -ENODEV;
-	}
-	return 0;
+	return isa_register_driver(&snd_adlib_driver, SNDRV_CARDS);
 }
 
 static void __exit alsa_card_adlib_exit(void)
 {
-	int i;
-
-	for (i = 0; i < SNDRV_CARDS; i++)
-		platform_device_unregister(devices[i]);
-	platform_driver_unregister(&snd_adlib_driver);
+	isa_unregister_driver(&snd_adlib_driver);
 }
 
 module_init(alsa_card_adlib_init);