Re: Driver model ISA bus

Re: Driver model ISA bus

[Simon White]

> > Is the intent of name to be the cards address, and ndev to be the
> > function on a specific card?
> 
> No, the name is just an identifier under which the driver (and 
> devices) show up in sysfs and ndev the number of devices we want to 
> the driver code to call our methods with -- given that ISA devices 
> do not announce themselves we have to tell the driver core this.

If I understood correctly, it is the maximum number of devices our
driver will support.  I got confused with an earlier version of this
whereby devices were to be registered with the isa bus on finding
them.

One further thing I'd like to check.  In my case there can only be
a maximum of 4 cards, limited by the possible hardware addresses
manually selectable.  Will the probe calls just happen or do they
require some userspace activity to occur (referring to that echo
bind in the example).

> By the way, please CC people on LKML. I'm still being busy and had 
> to pick this out of the trash where it caught my eye by chance...

Sorry, I caught this message on lkml.org and it had stripped the
email addresses.  Not sure what else to use (am not currently
subscribed to the list either).  I also seem to notice hitting
reply to a message appears to start a new thread rather than
add to an existing one...

Simon

-- 
___________________________________________________
Play 100s of games for FREE! http://games.mail.com/

Re: Driver model ISA bus

[Rene Herman]

Simon White wrote:

>> No, the name is just an identifier under which the driver (and 
>> devices) show up in sysfs and ndev the number of devices we want to 
>> the driver code to call our methods with -- given that ISA devices 
>> do not announce themselves we have to tell the driver core this.
> 
> If I understood correctly, it is the maximum number of devices our
> driver will support. 

Yes.

> I got confused with an earlier version of this whereby devices were
> to be registered with the isa bus on finding them.
> 
> One further thing I'd like to check.  In my case there can only be a
> maximum of 4 cards, limited by the possible hardware addresses 
> manually selectable.  Will the probe calls just happen or do they 
> require some userspace activity to occur (referring to that echo bind
> in the example).

If you provide a match() method as part of the isa_driver struct the 
calls to that method will just happen and if it returns non-zero, 
indicating a match between this driver and the device, the calls to the 
probe method will just happen. On a non-match (0), the device is 
unregistered again.

The driver model ISA bus is meant to provide a framework for ISA drivers 
close to the model from saner busses such as PCI. PCI drivers load 
always (even without their PCI ID present) since the user could do 
things such as supply a new PCI ID to the driver after it's loaded 
through sysfs (the new_id file).

As the example says, the match() method is intended only for things 
which should make the device fail to register since it could never be 
useful anymore -- in the example, if a port value hasn't been passed in 
we can't do anything so fail the device registration. Due to the fact 
that most other problems could be fixed later while the driver is loaded 
(such as by unloading a different driver which was keeping our ports 
busy) things such as that are also really the only things you should 
check in the match method, and do everything else from the probe method.

On the other hand, if you are really dead set against loading when you 
can't immediately drive something that's up to you as well -- you decide 
when to fail the match().

Rene.

Re: Driver model ISA bus

[Rene Herman]

Rene Herman wrote:

> On the other hand, if you are really dead set against loading when
> you can't immediately drive something that's up to you as well -- you
> decide when to fail the match().

Oh, forgot to mention, if vice-versa you don't mind always loading you 
don't _have_ to supply a match() method at all.

Maybe you make your port/irq/dma parameter variables in the driver 
writable through sysfs or, heaven forbid, you do autoprobing meaning 
that there's nothing left that could not be fixed while the driver is 
loaded. If you don't provide a .match, your .probe is called always 
directly.

Rene.

Re: Driver model ISA bus

[Simon White]

> On Tue, Jun 06, 2006 at 11:54:02PM +0200, Rene Herman wrote:
>> Hi Greg.
>> 
>> The below was sent a month ago and I haven't heard anything back. Saw 
>> you saying "it's getting there" about this thing on the kernelnewbies 
>> list but where's there?
>> 
> 
> Sorry for the delay.  It looks great to me so I've added it to my tree
> and will push it upstream when I can.

Would it be better to have a name variable directly in isa_device and
then copy that to driver in isa_register_device (like
pci_register_device does)?

I was trying to look for use examples of this code in 2.6.18-rc2 but
didn't see any.  Is the intent of name to be the cards address, and
ndev to be the function on a specific card?  Would it be better to
seperate name from the thing that ends up in bus_id?

I only ask as was looking to use something similiar to other code
I've seen in kernel in that it seemed the bus_type + bus_id would
uniquely identify a particular card that would pretty much persist
(on removal/addition of hardware?).  When a legacy isa device was
found by this code they switched to mangling an id from the cards
address.

I think between that and a probe test you could make pretty sure
you had exactly the same card.  Am wondering if this code allowed
for something similiar, in that bus_type + bus_id could be stored
and used later to locate the same card.

Regards,
Simon


-- 
___________________________________________________
Play 100s of games for FREE! http://games.mail.com/

Re: Driver model ISA bus

[Rene Herman]

Simon White wrote:

> Would it be better to have a name variable directly in isa_device and
> then copy that to driver in isa_register_device (like
> pci_register_device does)?

No, that wouldn't be useful. The point of this code is largely that the 
devices belong to the driver; do not have a life of their own. As such, 
naming them after the driver is the correct thibng to do.

> I was trying to look for use examples of this code in 2.6.18-rc2 but
> didn't see any.

Yes, apologies. I was converting ALSA ISA drivers to use it but had (and 
have) to deal with a few other matters first all of a sudden. I'll get 
to it shortly. There is a usage example I posted on the kernelnewbies 
list a while ago:

http://www.spinics.net/lists/newbies/msg21845.html

> Is the intent of name to be the cards address, and ndev to be the
> function on a specific card?

No, the name is just an identifier under which the driver (and devices) 
show up in sysfs and ndev the number of devices we want to the driver 
code to call our methods with -- given that ISA devices do not announce 
themselves we have to tell the driver core this.

By the way, please CC people on LKML. I'm still being busy and had to 
pick this out of the trash where it caught my eye by chance...

Rene.

Driver model ISA bus

[Rene Herman]

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

Hi Greg.

The below was sent a month ago and I haven't heard anything back. Saw 
you saying "it's getting there" about this thing on the kernelnewbies 
list but where's there?

Signed-off-by: Rene Herman <rene.herman@keyaccess.nl>

===
During the recent "isa drivers using platform devices" discussion it was
pointed out that (ALSA) ISA drivers ran into the problem of not having
the option to fail driver load (device registration rather) upon not
finding their hardware due to a probe() error not being passed up
through the driver model. In the course of that, I suggested a seperate
ISA bus might be best; Russell King agreed and suggested this bus could
use the .match() method for the actual device discovery.

The attached does this. For this old non (generically) discoverable ISA
hardware only the driver itself can do discovery so as a difference with
the platform_bus, this isa_bus also distributes match() up to the driver.

As another difference: these devices only exist in the driver model due
to the driver creating them because it might want to drive them, meaning
that all device creation has been made internal as well.

The usage model this provides is nice, and has been acked from the ALSA
side by Takashi Iwai and Jaroslav Kysela. The ALSA driver module_init's
now (for oldisa-only drivers) become:

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

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

Quite like the other bus models therefore. This removes a lot of
duplicated init code from the ALSA ISA drivers.

The passed in isa_driver struct is the regular driver struct embedding a
struct device_driver, the normal probe/remove/shutdown/suspend/resume
callbacks, and as indicated that .match callback.

The "SNDRV_CARDS" you see being passed in is a "unsigned int ndev"
parameter, indicating how many devices to create and call our methods with.

The platform_driver callbacks are called with a platform_device param;
the isa_driver callbacks are being called with a "struct device *dev,
unsigned int id" pair directly -- with the device creation completely
internal to the bus it's much cleaner to not leak isa_dev's by passing
them in at all. The id is the only thing we ever want other then the
struct device * anyways, and it makes for nicer code in the callbacks as
well.

With this additional .match() callback ISA drivers have all options. If
ALSA would want to keep the old non-load behaviour, it could stick all
of the old .probe in .match, which would only keep them registered after
everything was found to be present and accounted for. If it wanted the
behaviour of always loading as it inadvertently did for a bit after the
changeover to platform devices, it could just not provide a .match() and
do everything in .probe() as before.

If it, as Takashi Iwai already suggested earlier as a way of following
the model from saner buses more closely, wants to load when a later bind
could conceivably succeed, it could use .match() for the prerequisites
(such as checking the user wants the card enabled and that port/irq/dma
values have been passed in) and .probe() for everything else. This is
the nicest model.

To the code...

This exports only two functions; isa_{,un}register_driver().

isa_register_driver() register's the struct device_driver, and then
loops over the passed in ndev creating devices and registering them.
This causes the bus match method to be called for them, which is:

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;
}

The first thing this does is check if this device is in fact one of this
driver's devices by seeing if the device's platform_data pointer is set
to this driver. Platform devices compare strings, but we don't need to
do that with everything being internal, so isa_register_driver() abuses
dev->platform_data as a isa_driver pointer which we can then check here.
I believe platform_data is available for this, but if rather not, moving
the isa_driver pointer to the private struct isa_dev is ofcourse fine as
well.

Then, if the the driver did not provide a .match, it matches. If it did,
the driver match() method is called to determine a match.

If it did _not_ match, dev->platform_data is reset to indicate this to
isa_register_driver which can then unregister the device again.

If during all this, there's any error, or no devices matched at all
everything is backed out again and the error, or -ENODEV, is returned.

isa_unregister_driver() just unregisters the matched devices and the
driver itself.

More global points/questions...

- I'm introducing include/linux/isa.h. It was available but is ofcourse
a somewhat generic name. Moving more isa stuff over to it in time is
ofcourse fine, so can I have it please? :)

- I'm using device_initcall() and added the isa.o (dependent on
CONFIG_ISA) after the base driver model things in the Makefile. Will
this do, or I really need to stick it in drivers/base/init.c, inside
#ifdef CONFIG_ISA? It's working fine.

Lastly -- I also looked, a bit, into integrating with PnP. "Old ISA"
could be another pnp_protocol, but this does not seem to be a good
match, largely due to the same reason platform_devices weren't -- the
devices do not have a life of their own outside the driver, meaning the
pnp_protocol {get,set}_resources callbacks would need to callback into
driver -- which again means you first need to _have_ that driver. Even
if there's 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 advantages over this very simple isa_bus. The thing I also
suggested earlier about the user echoing values into /sys to set up the
hardware from userspace first is... well, cute, but a horrible idea from
a user standpoint.

Comments ofcourse appreciated. Hope it's okay. As said, the usage model
is nice at least.
===

Rene.


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

Index: local/drivers/base/isa.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/drivers/base/isa.c	2006-05-31 01:57:00.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-31 01:23:24.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-31 01:22:52.000000000 +0200
+++ local/drivers/base/Makefile	2006-05-31 01:23:24.000000000 +0200
@@ -5,6 +5,7 @@ obj-y			:= core.o sys.o bus.o dd.o \
 			   cpu.o firmware.o init.o map.o dmapool.o \
 			   attribute_container.o transport_class.o
 obj-y			+= power/
+obj-$(CONFIG_ISA)	+= isa.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o

Re: Driver model ISA bus

[Greg KH]

On Tue, Jun 06, 2006 at 11:54:02PM +0200, Rene Herman wrote:
> Hi Greg.
> 
> The below was sent a month ago and I haven't heard anything back. Saw 
> you saying "it's getting there" about this thing on the kernelnewbies 
> list but where's there?
> 

Sorry for the delay.  It looks great to me so I've added it to my tree
and will push it upstream when I can.

thanks,

greg k-h

Re: Driver model ISA bus

[Greg KH]

On Tue, Jun 06, 2006 at 11:54:02PM +0200, Rene Herman wrote:
> Hi Greg.
> 
> The below was sent a month ago and I haven't heard anything back. Saw 
> you saying "it's getting there" about this thing on the kernelnewbies 
> list but where's there?
> 

Sorry for the delay.  It looks great to me so I've added it to my tree
and will push it upstream when I can.

thanks,

greg k-h

Re: Driver model ISA bus

[Rene Herman]

Greg KH wrote:

> Sorry for the delay.  It looks great to me so I've added it to my
> tree and will push it upstream when I can.

Okay, lovely, thanks.

Takashi: I'll be converting ALSA ISA drivers over to this now. What I'd 
in fact would like to do is do that one driver at a time. I have a 
metric buttload of these old ISA soundcards and believe it would be a 
nice opportunity to actually test these old drivers (and possibly do 
some cleanups) as I go along. Would you mind that?

If you'd rather the isa_bus thing was one discrete change, that's also 
okay though. I'd then just do that as a somewhat mechanical change and 
then revisit those drivers at some later time for additional testing 
and/or cleanup.

In either case, I'll ofcourse not submit anything untill this thing has 
made -mm.

Rene.

Re: Driver model ISA bus

[Rene Herman]

Greg KH wrote:

> Sorry for the delay.  It looks great to me so I've added it to my
> tree and will push it upstream when I can.

Okay, lovely, thanks.

Takashi: I'll be converting ALSA ISA drivers over to this now. What I'd 
in fact would like to do is do that one driver at a time. I have a 
metric buttload of these old ISA soundcards and believe it would be a 
nice opportunity to actually test these old drivers (and possibly do 
some cleanups) as I go along. Would you mind that?

If you'd rather the isa_bus thing was one discrete change, that's also 
okay though. I'd then just do that as a somewhat mechanical change and 
then revisit those drivers at some later time for additional testing 
and/or cleanup.

In either case, I'll ofcourse not submit anything untill this thing has 
made -mm.

Rene.

Re: Driver model ISA bus

[Takashi Iwai]

At Sun, 18 Jun 2006 20:51:20 +0200,
Rene Herman wrote:
> 
> Greg KH wrote:
> 
> > Sorry for the delay.  It looks great to me so I've added it to my
> > tree and will push it upstream when I can.
> 
> Okay, lovely, thanks.
> 
> Takashi: I'll be converting ALSA ISA drivers over to this now. What I'd 
> in fact would like to do is do that one driver at a time. I have a 
> metric buttload of these old ISA soundcards and believe it would be a 
> nice opportunity to actually test these old drivers (and possibly do 
> some cleanups) as I go along. Would you mind that?
> 
> If you'd rather the isa_bus thing was one discrete change, that's also 
> okay though. I'd then just do that as a somewhat mechanical change and 
> then revisit those drivers at some later time for additional testing 
> and/or cleanup.

Yes, I prefer splitting the patches.  It makes easier to find possible
regressions.

Thanks,

Takashi

Driver model ISA bus

[Rene Herman]

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

Hi Greg.

The below was sent a month ago and I haven't heard anything back. Saw 
you saying "it's getting there" about this thing on the kernelnewbies 
list but where's there?

Signed-off-by: Rene Herman <rene.herman@keyaccess.nl>

===
During the recent "isa drivers using platform devices" discussion it was
pointed out that (ALSA) ISA drivers ran into the problem of not having
the option to fail driver load (device registration rather) upon not
finding their hardware due to a probe() error not being passed up
through the driver model. In the course of that, I suggested a seperate
ISA bus might be best; Russell King agreed and suggested this bus could
use the .match() method for the actual device discovery.

The attached does this. For this old non (generically) discoverable ISA
hardware only the driver itself can do discovery so as a difference with
the platform_bus, this isa_bus also distributes match() up to the driver.

As another difference: these devices only exist in the driver model due
to the driver creating them because it might want to drive them, meaning
that all device creation has been made internal as well.

The usage model this provides is nice, and has been acked from the ALSA
side by Takashi Iwai and Jaroslav Kysela. The ALSA driver module_init's
now (for oldisa-only drivers) become:

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

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

Quite like the other bus models therefore. This removes a lot of
duplicated init code from the ALSA ISA drivers.

The passed in isa_driver struct is the regular driver struct embedding a
struct device_driver, the normal probe/remove/shutdown/suspend/resume
callbacks, and as indicated that .match callback.

The "SNDRV_CARDS" you see being passed in is a "unsigned int ndev"
parameter, indicating how many devices to create and call our methods with.

The platform_driver callbacks are called with a platform_device param;
the isa_driver callbacks are being called with a "struct device *dev,
unsigned int id" pair directly -- with the device creation completely
internal to the bus it's much cleaner to not leak isa_dev's by passing
them in at all. The id is the only thing we ever want other then the
struct device * anyways, and it makes for nicer code in the callbacks as
well.

With this additional .match() callback ISA drivers have all options. If
ALSA would want to keep the old non-load behaviour, it could stick all
of the old .probe in .match, which would only keep them registered after
everything was found to be present and accounted for. If it wanted the
behaviour of always loading as it inadvertently did for a bit after the
changeover to platform devices, it could just not provide a .match() and
do everything in .probe() as before.

If it, as Takashi Iwai already suggested earlier as a way of following
the model from saner buses more closely, wants to load when a later bind
could conceivably succeed, it could use .match() for the prerequisites
(such as checking the user wants the card enabled and that port/irq/dma
values have been passed in) and .probe() for everything else. This is
the nicest model.

To the code...

This exports only two functions; isa_{,un}register_driver().

isa_register_driver() register's the struct device_driver, and then
loops over the passed in ndev creating devices and registering them.
This causes the bus match method to be called for them, which is:

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;
}

The first thing this does is check if this device is in fact one of this
driver's devices by seeing if the device's platform_data pointer is set
to this driver. Platform devices compare strings, but we don't need to
do that with everything being internal, so isa_register_driver() abuses
dev->platform_data as a isa_driver pointer which we can then check here.
I believe platform_data is available for this, but if rather not, moving
the isa_driver pointer to the private struct isa_dev is ofcourse fine as
well.

Then, if the the driver did not provide a .match, it matches. If it did,
the driver match() method is called to determine a match.

If it did _not_ match, dev->platform_data is reset to indicate this to
isa_register_driver which can then unregister the device again.

If during all this, there's any error, or no devices matched at all
everything is backed out again and the error, or -ENODEV, is returned.

isa_unregister_driver() just unregisters the matched devices and the
driver itself.

More global points/questions...

- I'm introducing include/linux/isa.h. It was available but is ofcourse
a somewhat generic name. Moving more isa stuff over to it in time is
ofcourse fine, so can I have it please? :)

- I'm using device_initcall() and added the isa.o (dependent on
CONFIG_ISA) after the base driver model things in the Makefile. Will
this do, or I really need to stick it in drivers/base/init.c, inside
#ifdef CONFIG_ISA? It's working fine.

Lastly -- I also looked, a bit, into integrating with PnP. "Old ISA"
could be another pnp_protocol, but this does not seem to be a good
match, largely due to the same reason platform_devices weren't -- the
devices do not have a life of their own outside the driver, meaning the
pnp_protocol {get,set}_resources callbacks would need to callback into
driver -- which again means you first need to _have_ that driver. Even
if there's 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 advantages over this very simple isa_bus. The thing I also
suggested earlier about the user echoing values into /sys to set up the
hardware from userspace first is... well, cute, but a horrible idea from
a user standpoint.

Comments ofcourse appreciated. Hope it's okay. As said, the usage model
is nice at least.
===

Rene.


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

Index: local/drivers/base/isa.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ local/drivers/base/isa.c	2006-05-31 01:57:00.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-31 01:23:24.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-31 01:22:52.000000000 +0200
+++ local/drivers/base/Makefile	2006-05-31 01:23:24.000000000 +0200
@@ -5,6 +5,7 @@ obj-y			:= core.o sys.o bus.o dd.o \
 			   cpu.o firmware.o init.o map.o dmapool.o \
 			   attribute_container.o transport_class.o
 obj-y			+= power/
+obj-$(CONFIG_ISA)	+= isa.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Driver model ISA bus

[Rene Herman]

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

Hi Greg.

During the recent "isa drivers using platform devices" discussion it was 
pointed out that (ALSA) ISA drivers ran into the problem of not having 
the option to fail driver load (device registration rather) upon not 
finding their hardware due to a probe() error not being passed up 
through the driver model. In the course of that, I suggested a seperate 
ISA bus might be best; Russel King agreed and suggested this bus could 
use the .match() method for the actual device discovery.

The attached does this. For this old non (generically) discoverable ISA 
hardware only the driver itself can do discovery so as a difference with 
the platform_bus, this isa_bus also distributes match() up to the driver.

As another difference: these devices only exist in the driver model due 
to the driver creating them because it might want to drive them, meaning 
that all device creation has been made internal as well.

The usage model this provides is nice, and has been acked from the ALSA 
side by Takashi Iwai and Jaroslav Kysela. The ALSA driver module_init's 
now (for oldisa-only drivers) become:

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

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

Quite like the other bus models therefore. This removes a lot of 
duplicated init code from the ALSA ISA drivers.

The passed in isa_driver struct is the regular driver struct embedding a 
struct device_driver, the normal probe/remove/shutdown/suspend/resume 
callbacks, and as indicated that .match callback.

The "SNDRV_CARDS" you see being passed in is a "unsigned int ndev" 
parameter, indicating how many devices to create and call our methods with.

The platform_driver callbacks are called with a platform_device param; 
the isa_driver callbacks are being called with a "struct device *dev, 
unsigned int id" pair directly -- with the device creation completely 
internal to the bus it's much cleaner to not leak isa_dev's by passing 
them in at all. The id is the only thing we ever want other then the 
struct device * anyways, and it makes for nicer code in the callbacks as 
well.

With this additional .match() callback ISA drivers have all options. If 
ALSA would want to keep the old non-load behaviour, it could stick all 
of the old .probe in .match, which would only keep them registered after 
everything was found to be present and accounted for. If it wanted the 
behaviour of always loading as it inadvertently did for a bit after the 
changeover to platform devices, it could just not provide a .match() and 
do everything in .probe() as before.

If it, as Takashi Iwai already suggested earlier as a way of following 
the model from saner buses more closely, wants to load when a later bind 
could conceivably succeed, it could use .match() for the prerequisites 
(such as checking the user wants the card enabled and that port/irq/dma 
values have been passed in) and .probe() for everything else. This is 
the nicest model.

To the code...

This exports only two functions; isa_{,un}register_driver().

isa_register_driver() register's the struct device_driver, and then 
loops over the passed in ndev creating devices and registering them. 
This causes the bus match method to be called for them, which is:

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;
}

The first thing this does is check if this device is in fact one of this 
driver's devices by seeing if the device's platform_data pointer is set 
to this driver. Platform devices compare strings, but we don't need to 
do that with everything being internal, so isa_register_driver() abuses 
dev->platform_data as a isa_driver pointer which we can then check here. 
I believe platform_data is available for this, but if rather not, moving 
the isa_driver pointer to the private struct isa_dev is ofcourse fine as 
well.

Then, if the the driver did not provide a .match, it matches. If it did, 
the driver match() method is called to determine a match.

If it did _not_ match, dev->platform_data is reset to indicate this to 
isa_register_driver which can then unregister the device again.

If during all this, there's any error, or no devices matched at all 
everything is backed out again and the error, or -ENODEV, is returned.

isa_unregister_driver() just unregisters the matched devices and the 
driver itself.

More global points/questions...

- I'm introducing include/linux/isa.h. It was available but is ofcourse 
a somewhat generic name. Moving more isa stuff over to it in time is 
ofcourse fine, so can I have it please? :)

- I'm using device_initcall() and added the isa.o (dependent on 
CONFIG_ISA) after the base driver model things in the Makefile. Will 
this do, or I really need to stick it in drivers/base/init.c, inside 
#ifdef CONFIG_ISA? It's working fine.

Lastly -- I also looked, a bit, into integrating with PnP. "Old ISA" 
could be another pnp_protocol, but this does not seem to be a good 
match, largely due to the same reason platform_devices weren't -- the 
devices do not have a life of their own outside the driver, meaning the 
pnp_protocol {get,set}_resources callbacks would need to callback into 
driver -- which again means you first need to _have_ that driver. Even 
if there's 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 advantages over this very simple isa_bus. The thing I also 
suggested earlier about the user echoing values into /sys to set up the 
hardware from userspace first is... well, cute, but a horrible idea from 
a user standpoint.

Comments ofcourse appreciated. Hope it's okay. As said, the usage model 
is nice at least.

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

Driver model ISA bus

[Rene Herman]

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

Hi Greg.

During the recent "isa drivers using platform devices" discussion it was 
pointed out that (ALSA) ISA drivers ran into the problem of not having 
the option to fail driver load (device registration rather) upon not 
finding their hardware due to a probe() error not being passed up 
through the driver model. In the course of that, I suggested a seperate 
ISA bus might be best; Russel King agreed and suggested this bus could 
use the .match() method for the actual device discovery.

The attached does this. For this old non (generically) discoverable ISA 
hardware only the driver itself can do discovery so as a difference with 
the platform_bus, this isa_bus also distributes match() up to the driver.

As another difference: these devices only exist in the driver model due 
to the driver creating them because it might want to drive them, meaning 
that all device creation has been made internal as well.

The usage model this provides is nice, and has been acked from the ALSA 
side by Takashi Iwai and Jaroslav Kysela. The ALSA driver module_init's 
now (for oldisa-only drivers) become:

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

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

Quite like the other bus models therefore. This removes a lot of 
duplicated init code from the ALSA ISA drivers.

The passed in isa_driver struct is the regular driver struct embedding a 
struct device_driver, the normal probe/remove/shutdown/suspend/resume 
callbacks, and as indicated that .match callback.

The "SNDRV_CARDS" you see being passed in is a "unsigned int ndev" 
parameter, indicating how many devices to create and call our methods with.

The platform_driver callbacks are called with a platform_device param; 
the isa_driver callbacks are being called with a "struct device *dev, 
unsigned int id" pair directly -- with the device creation completely 
internal to the bus it's much cleaner to not leak isa_dev's by passing 
them in at all. The id is the only thing we ever want other then the 
struct device * anyways, and it makes for nicer code in the callbacks as 
well.

With this additional .match() callback ISA drivers have all options. If 
ALSA would want to keep the old non-load behaviour, it could stick all 
of the old .probe in .match, which would only keep them registered after 
everything was found to be present and accounted for. If it wanted the 
behaviour of always loading as it inadvertently did for a bit after the 
changeover to platform devices, it could just not provide a .match() and 
do everything in .probe() as before.

If it, as Takashi Iwai already suggested earlier as a way of following 
the model from saner buses more closely, wants to load when a later bind 
could conceivably succeed, it could use .match() for the prerequisites 
(such as checking the user wants the card enabled and that port/irq/dma 
values have been passed in) and .probe() for everything else. This is 
the nicest model.

To the code...

This exports only two functions; isa_{,un}register_driver().

isa_register_driver() register's the struct device_driver, and then 
loops over the passed in ndev creating devices and registering them. 
This causes the bus match method to be called for them, which is:

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;
}

The first thing this does is check if this device is in fact one of this 
driver's devices by seeing if the device's platform_data pointer is set 
to this driver. Platform devices compare strings, but we don't need to 
do that with everything being internal, so isa_register_driver() abuses 
dev->platform_data as a isa_driver pointer which we can then check here. 
I believe platform_data is available for this, but if rather not, moving 
the isa_driver pointer to the private struct isa_dev is ofcourse fine as 
well.

Then, if the the driver did not provide a .match, it matches. If it did, 
the driver match() method is called to determine a match.

If it did _not_ match, dev->platform_data is reset to indicate this to 
isa_register_driver which can then unregister the device again.

If during all this, there's any error, or no devices matched at all 
everything is backed out again and the error, or -ENODEV, is returned.

isa_unregister_driver() just unregisters the matched devices and the 
driver itself.

More global points/questions...

- I'm introducing include/linux/isa.h. It was available but is ofcourse 
a somewhat generic name. Moving more isa stuff over to it in time is 
ofcourse fine, so can I have it please? :)

- I'm using device_initcall() and added the isa.o (dependent on 
CONFIG_ISA) after the base driver model things in the Makefile. Will 
this do, or I really need to stick it in drivers/base/init.c, inside 
#ifdef CONFIG_ISA? It's working fine.

Lastly -- I also looked, a bit, into integrating with PnP. "Old ISA" 
could be another pnp_protocol, but this does not seem to be a good 
match, largely due to the same reason platform_devices weren't -- the 
devices do not have a life of their own outside the driver, meaning the 
pnp_protocol {get,set}_resources callbacks would need to callback into 
driver -- which again means you first need to _have_ that driver. Even 
if there's 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 advantages over this very simple isa_bus. The thing I also 
suggested earlier about the user echoing values into /sys to set up the 
hardware from userspace first is... well, cute, but a horrible idea from 
a user standpoint.

Comments ofcourse appreciated. Hope it's okay. As said, the usage model 
is nice at least.

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

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