pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]

PATCH In review [email protected] 4 participants 10 messages Mar 4, 2003 – Mar 15, 2003 Driver core

lore

Share & tools

lore

[email protected]

author

2003-03-04 08:20 UTC #0 lore raw

I think the problem is platform_match() :

static int platform_match(struct device * dev, struct device_driver * drv)

{
	return 0;
}

which effectively makes driver binding impossible, pcmcia_socket_class->add_device isn't called.

--Mika

Quoted 86 lines

Lähettäjä: Dominik Brodowski <[email protected]>
Päiväys: 2003/03/04 ti AM 09:39:15 GMT+02:00
Vastaanottaja: [email protected], [email protected]
Kopio: Linux kernel mailing list <[email protected]>,
Patrick Mochel <[email protected]>
Aihe: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]
Hi,
Quoted 3 lines · Jean Tourrilhes
	Hi,

	I'm trying to get i82365 to work again, because I need to test
<snip>
Quoted 6 lines
Intel PCIC probe:
Vadem VG-469 ISA-to-PCMCIA at port 0x3e0 ofs 0x00, 2 sockets
host opts [0]: none
host opts [1]: none
ISA irqs (scanned) = 4,5 polling interval = 1000 ms
ds: no socket drivers loaded!
Sorry about that -- I mixed up the ordering of initializing the class data and registering the platform device. Here's a bugfix for the three pcmcia socket drivers that are platform devices.
Please apply,
Dominik
diff -ruN linux-original/drivers/pcmcia/hd64465_ss.c linux/drivers/pcmcia/hd64465_ss.c
drivers/pcmcia/hd64465_ss.c +1 -1
@@ -1070,8 +1070,8 @@
 	}
 
 /*	hd64465_io_debug = 0; */
-	platform_device_register(&hd64465_device);
 	hd64465_device.dev.class_data = &hd64465_data;
+	platform_device_register(&hd64465_device);
 
 	return 0;
 }
diff -ruN linux-original/drivers/pcmcia/i82365.c linux/drivers/pcmcia/i82365.c
drivers/pcmcia/i82365.c +2 -2
@@ -1628,11 +1628,11 @@
 	request_irq(cs_irq, pcic_interrupt, 0, "i82365", pcic_interrupt);
 #endif
 
-    platform_device_register(&i82365_device);
-
     i82365_data.nsock = sockets;
     i82365_device.dev.class_data = &i82365_data;
 
+    platform_device_register(&i82365_device);
+
     /* Finally, schedule a polling interrupt */
     if (poll_interval != 0) {
 	poll_timer.function = pcic_interrupt_wrapper;
diff -ruN linux-original/drivers/pcmcia/tcic.c linux/drivers/pcmcia/tcic.c
drivers/pcmcia/tcic.c +2 -3
@@ -452,8 +452,6 @@
 	sockets++;
     }
 
-    platform_device_register(&tcic_device);
-
     switch (socket_table[0].id) {
     case TCIC_ID_DB86082:
 	printk("DB86082"); break;
@@ -527,6 +525,8 @@
     tcic_data.nsock = sockets;
     tcic_device.dev.class_data = &tcic_data;
 
+    platform_device_register(&tcic_device);
+
     return 0;
 
 } /* init_tcic */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Dominik Brodowski

2003-03-04 09:45 UTC #1 lore raw

Hi Pat,

How is it supposed to work then? I thought adding a platform_device and platform_driver with the same name and bus_id causes the platform_driver to be bound to the platform_device?

Thanks,
Dominik

Quoted 99 lines · On Tue, Mar 04, 2003 at 10:30:31AM +0200, [email protected] wrote:

I think the problem is platform_match() :
static int platform_match(struct device * dev, struct device_driver * drv)
{
	return 0;
}
which effectively makes driver binding impossible, pcmcia_socket_class->add_device isn't called.
--Mika
Quoted 86 lines
Lähettäjä: Dominik Brodowski <[email protected]>
Päiväys: 2003/03/04 ti AM 09:39:15 GMT+02:00
Vastaanottaja: [email protected], [email protected]
Kopio: Linux kernel mailing list <[email protected]>,
Patrick Mochel <[email protected]>
Aihe: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]
Hi,
Quoted 3 lines · Jean Tourrilhes
	Hi,

	I'm trying to get i82365 to work again, because I need to test
<snip>
Quoted 6 lines
Intel PCIC probe:
Vadem VG-469 ISA-to-PCMCIA at port 0x3e0 ofs 0x00, 2 sockets
host opts [0]: none
host opts [1]: none
ISA irqs (scanned) = 4,5 polling interval = 1000 ms
ds: no socket drivers loaded!
Sorry about that -- I mixed up the ordering of initializing the class data and registering the platform device. Here's a bugfix for the three pcmcia socket drivers that are platform devices.
Please apply,
Dominik
diff -ruN linux-original/drivers/pcmcia/hd64465_ss.c linux/drivers/pcmcia/hd64465_ss.c
drivers/pcmcia/hd64465_ss.c +1 -1
@@ -1070,8 +1070,8 @@
 	}
 
 /*	hd64465_io_debug = 0; */
-	platform_device_register(&hd64465_device);
 	hd64465_device.dev.class_data = &hd64465_data;
+	platform_device_register(&hd64465_device);
 
 	return 0;
 }
diff -ruN linux-original/drivers/pcmcia/i82365.c linux/drivers/pcmcia/i82365.c
drivers/pcmcia/i82365.c +2 -2
@@ -1628,11 +1628,11 @@
 	request_irq(cs_irq, pcic_interrupt, 0, "i82365", pcic_interrupt);
 #endif
 
-    platform_device_register(&i82365_device);
-
     i82365_data.nsock = sockets;
     i82365_device.dev.class_data = &i82365_data;
 
+    platform_device_register(&i82365_device);
+
     /* Finally, schedule a polling interrupt */
     if (poll_interval != 0) {
 	poll_timer.function = pcic_interrupt_wrapper;
diff -ruN linux-original/drivers/pcmcia/tcic.c linux/drivers/pcmcia/tcic.c
drivers/pcmcia/tcic.c +2 -3
@@ -452,8 +452,6 @@
 	sockets++;
     }
 
-    platform_device_register(&tcic_device);
-
     switch (socket_table[0].id) {
     case TCIC_ID_DB86082:
 	printk("DB86082"); break;
@@ -527,6 +525,8 @@
     tcic_data.nsock = sockets;
     tcic_device.dev.class_data = &tcic_data;
 
+    platform_device_register(&tcic_device);
+
     return 0;
 
 } /* init_tcic */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Patrick Mochel

2003-03-04 14:48 UTC #2 lore raw

Quoted 5 lines · Dominik Brodowski

Hi Pat,
How is it supposed to work then? I thought adding a platform_device and platform_driver with the same name and bus_id causes the platform_driver to be bound to the platform_device?

Erm yes. Color me lazy, I just hadn't implemented that yet.. You've hit something else no one had used before.

This patch is completley untested, but it should work.

-pat

===== drivers/base/platform.c 1.5 vs edited =====

drivers/base/platform.c +21 -1

@@ -41,9 +41,29 @@
 	if (pdev)
 		device_unregister(&pdev->dev);
 }
-	
+
+
+/**
+ *	platform_match - bind platform device to platform driver.
+ *	@dev:	device.
+ *	@drv:	driver.
+ *
+ *	Platform device IDs are assumed to be encoded like this: 
+ *	"<name><instance>", where <name> is a short description of the 
+ *	type of device, like "pci" or "floppy", and <instance> is the 
+ *	enumerated instance of the device, like '0' or '42'.
+ *	Driver IDs are simply "<name>". 
+ *	So, extract the <name> from the device, and compare it against 
+ *	the name of the driver. Return whether they match or not.
+ */
+
 static int platform_match(struct device * dev, struct device_driver * drv)
 {
+	char name[BUS_ID_SIZE];
+
+	if (sscanf(dev->bus_id,"%s",name))
+		return (strcmp(name,drv->name) == 0);
+
 	return 0;
 }

Jean Tourrilhes

2003-03-04 17:06 UTC #3 lore raw

Quoted 2 lines · Patrick Mochel

This patch is completley untested, but it should work.

I think you are hitting the nail on the head, various people
are doing many "obvious" changes all over the place without bothering to check them, resulting in non functional code.
That's not the way I want to work, and that's why I won't
commit the new Wireless Extensions stuff until I can really test it.

Quoted 1 line

-pat

Thanks.

Jean

Patrick Mochel

2003-03-04 18:01 UTC #4 lore raw

Quoted 9 lines · Jean Tourrilhes

Quoted 2 lines · Patrick Mochel
This patch is completley untested, but it should work.
I think you are hitting the nail on the head, various people
are doing many "obvious" changes all over the place without bothering to check them, resulting in non functional code.
That's not the way I want to work, and that's why I won't
commit the new Wireless Extensions stuff until I can really test it.

Whoa, calm down.

The patch was meant essentially for Dominik only, so he could verify that it worked with his new PCMCIA code. The only reason the patch is untested is because I don't have the hardware in question.

The reason the patch was unwritten in the first place is because, as I mentioned in the previous email, no one has used this feature. Dominik followed the usage model exactly, and the feature was simple, and predefined. That's the only reason it was obvious, and sent in the first place.

Surely you're sore that your code has required some modifications since Dominik has started working on PCMCIA, and I'm sure that no harm was intended. It's had some bumps, but IMO, he's done a great job, and the result is a vast improvement. The least you could is give the guy some slack, instead of whining about your own inconveniences.

Thanks,

-pat

Jean Tourrilhes

2003-03-04 18:44 UTC #5 lore raw

Quoted 6 lines · Patrick Mochel

Surely you're sore that your code has required some modifications since Dominik has started working on PCMCIA, and I'm sure that no harm was intended. It's had some bumps, but IMO, he's done a great job, and the result is a vast improvement. The least you could is give the guy some slack, instead of whining about your own inconveniences.

I don't mind the changes, changes are usually good. In 2.5.X,
I had to change my code to accomodate the new PCI interface, the removal of global IRQ, the new module interface, the various USB API changes and other changes. And actually, your work currently hasn't had any impact on the source code I follow (yet).
What I mind is the lack of basic testing. From your patch, the
initialisation order mixup and the other obvious bug fix I sent you, this code had zero chances of working at all and it's obvious that nobody bothered to check if it could work or not for at least two kernel releases.
Yeah, I know that I should not complain because at least the
code did compile (modulo other minor obvious fixes that are already in Linus BK).

Quoted 3 lines

Thanks,
-pat

	Have fun...

	Jean

Dominik Brodowski

2003-03-04 20:09 UTC #6 lore raw

Quoted 8 lines · Jean Tourrilhes

Quoted 6 lines · Patrick Mochel
Surely you're sore that your code has required some modifications since Dominik has started working on PCMCIA, and I'm sure that no harm was intended. It's had some bumps, but IMO, he's done a great job, and the result is a vast improvement. The least you could is give the guy some slack, instead of whining about your own inconveniences.

Quoted 10 lines

I don't mind the changes, changes are usually good. In 2.5.X,
I had to change my code to accomodate the new PCI interface, the removal of global IRQ, the new module interface, the various USB API changes and other changes. And actually, your work currently hasn't had any impact on the source code I follow (yet).
What I mind is the lack of basic testing. From your patch, the
initialisation order mixup and the other obvious bug fix I sent you, this code had zero chances of working at all and it's obvious that nobody bothered to check if it could work or not for at least two kernel releases.

The problem is that I only have one yenta-compatible cardbus controller, and one pcmcia card -- no real "infrastructure" to test the patches.[*] And I really try to verify that my patches work, but obviously I had a bad day when I wrote the "let's add a pcmcia socket devices class" patch. Nonetheless, one point you mention is perfectly valid -- the kernel is in a feature freeze.

Dominik

[*] Needless to say, the patches I sent tend to work on the hardware I own...

Dominik Brodowski

2003-03-05 06:31 UTC #7 lore raw

PATCH 1/1 driver model: fix platform_match [Was: Re: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]]

In reply to Patrick Mochel

Hi Pat,

Quoted 13 lines · Patrick Mochel

Quoted 5 lines · Dominik Brodowski
Hi Pat,
How is it supposed to work then? I thought adding a platform_device and platform_driver with the same name and bus_id causes the platform_driver to be bound to the platform_device?
Erm yes. Color me lazy, I just hadn't implemented that yet.. You've hit something else no one had used before.
This patch is completley untested, but it should work.

...

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+	if (sscanf(dev->bus_id,"%s",name))
+		return (strcmp(name,drv->name) == 0);

Unfortunately, this won't work: digits are perfectly valid entries of strings. However, we have the name without the appending instance still saved in platform_device pdev->name... so what about this?

drivers/base/platform.c +2 -5

diff -ruN linux-original/drivers/base/platform.c linux/drivers/base/platform.c
@@ -59,12 +59,9 @@
 
 static int platform_match(struct device * dev, struct device_driver * drv)
 {
-	char name[BUS_ID_SIZE];
+	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
 
-	if (sscanf(dev->bus_id,"%s",name))
-		return (strcmp(name,drv->name) == 0);
-
-	return 0;
+	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
 }
 
 struct bus_type platform_bus_type = {

Patrick Mochel

2003-03-05 16:30 UTC #8 lore raw

Re: [PATCH] driver model: fix platform_match [Was: Re: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]]

Quoted 4 lines

Unfortunately, this won't work: digits are perfectly valid entries of strings. However, we have the name without the appending instance still saved in platform_device pdev->name... so what about this?

D'oh. You're completely right.

Thanks,

-pat

Jean Tourrilhes

2003-03-15 01:52 UTC #9 lore raw

Re: [PATCH] driver model: fix platform_match [Was: Re: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]]

Quoted 11 lines · Patrick Mochel

Quoted 4 lines
Unfortunately, this won't work: digits are perfectly valid entries of strings. However, we have the name without the appending instance still saved in platform_device pdev->name... so what about this?
D'oh. You're completely right.
Thanks,
-pat

	Hi,

	I tried 2.5.64-bk9, and it doesn't work !

	You simply forgot to apply the patch I sent you at the

very beggining of this discussion :
----------------------------------------------------

drivers/base/platform.c Fri Mar 14 17:47:40 2003 renamed from drivers/base/platform.c.original Fri Mar 14 17:47:16 2003 +1 -0

diff -u -p linux/drivers/base/platform.c.original linux/drivers/base/platform.c
@@ -75,5 +75,6 @@ int __init platform_bus_init(void)
        return bus_register(&platform_bus_type);
 }
 
+EXPORT_SYMBOL(platform_bus_type);
 EXPORT_SYMBOL(platform_device_register);
 EXPORT_SYMBOL(platform_device_unregister);

----------------------------------------------------

On the other hand, with the above patch, now everything is
working properly and my Pcmcia card work wonderfully.

Thanks a lot for the great work, and have fun...

Jean