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

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

[mika.penttila]

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


> 
> Lähettäjä: Dominik Brodowski <linux@brodo.de>
> Päiväys: 2003/03/04 ti AM 09:39:15 GMT+02:00
> Vastaanottaja: torvalds@transmeta.com,  jt@hpl.hp.com
> Kopio: Linux kernel mailing list <linux-kernel@vger.kernel.org>, 
> 	Patrick Mochel <mochel@osdl.org>
> Aihe: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]
> 
> Hi,
> 
> On Mon, Mar 03, 2003 at 05:30:20PM -0800, Jean Tourrilhes wrote:
> > 	Hi,
> > 
> > 	I'm trying to get i82365 to work again, because I need to test
> <snip>
> > 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
> --- linux-original/drivers/pcmcia/hd64465_ss.c	2003-03-04 08:27:06.000000000 +0100
> +++ linux/drivers/pcmcia/hd64465_ss.c	2003-03-04 08:30:37.000000000 +0100
> @@ -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
> --- linux-original/drivers/pcmcia/i82365.c	2003-03-04 08:27:06.000000000 +0100
> +++ linux/drivers/pcmcia/i82365.c	2003-03-04 08:28:28.000000000 +0100
> @@ -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
> --- linux-original/drivers/pcmcia/tcic.c	2003-03-04 08:27:06.000000000 +0100
> +++ linux/drivers/pcmcia/tcic.c	2003-03-04 08:30:03.000000000 +0100
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[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?

Thanks,
	Dominik

On Tue, Mar 04, 2003 at 10:30:31AM +0200, mika.penttila@kolumbus.fi 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
> 
> 
> > 
> > Lähettäjä: Dominik Brodowski <linux@brodo.de>
> > Päiväys: 2003/03/04 ti AM 09:39:15 GMT+02:00
> > Vastaanottaja: torvalds@transmeta.com,  jt@hpl.hp.com
> > Kopio: Linux kernel mailing list <linux-kernel@vger.kernel.org>, 
> > 	Patrick Mochel <mochel@osdl.org>
> > Aihe: [PATCH] pcmcia: get initialization ordering right [Was: [PATCH 2.5] : i82365 & platform_bus_type]
> > 
> > Hi,
> > 
> > On Mon, Mar 03, 2003 at 05:30:20PM -0800, Jean Tourrilhes wrote:
> > > 	Hi,
> > > 
> > > 	I'm trying to get i82365 to work again, because I need to test
> > <snip>
> > > 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
> > --- linux-original/drivers/pcmcia/hd64465_ss.c	2003-03-04 08:27:06.000000000 +0100
> > +++ linux/drivers/pcmcia/hd64465_ss.c	2003-03-04 08:30:37.000000000 +0100
> > @@ -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
> > --- linux-original/drivers/pcmcia/i82365.c	2003-03-04 08:27:06.000000000 +0100
> > +++ linux/drivers/pcmcia/i82365.c	2003-03-04 08:28:28.000000000 +0100
> > @@ -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
> > --- linux-original/drivers/pcmcia/tcic.c	2003-03-04 08:27:06.000000000 +0100
> > +++ linux/drivers/pcmcia/tcic.c	2003-03-04 08:30:03.000000000 +0100
> > @@ -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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 

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

[Patrick Mochel]

On Tue, 4 Mar 2003, Dominik Brodowski wrote:

> 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 =====
--- 1.5/drivers/base/platform.c	Fri Oct 18 13:27:29 2002
+++ edited/drivers/base/platform.c	Tue Mar  4 08:34:10 2003
@@ -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;
 }
 

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

[Jean Tourrilhes]

On Tue, Mar 04, 2003 at 08:35:05AM -0600, Patrick Mochel wrote:
> 
> 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.

> 	-pat

	Thanks.

	Jean

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

[Patrick Mochel]

On Tue, 4 Mar 2003, Jean Tourrilhes wrote:

> On Tue, Mar 04, 2003 at 08:35:05AM -0600, Patrick Mochel wrote:
> > 
> > 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

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

[Jean Tourrilhes]

On Tue, Mar 04, 2003 at 11:48:22AM -0600, Patrick Mochel wrote:
> 
> 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).

> Thanks,
> 
> 	-pat

	Have fun...

	Jean

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

[Dominik Brodowski]

On Tue, Mar 04, 2003 at 10:54:28AM -0800, Jean Tourrilhes wrote:
> On Tue, Mar 04, 2003 at 11:48:22AM -0600, Patrick Mochel wrote:
> > 
> > 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.

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

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

[Dominik Brodowski]

Hi Pat,

On Tue, Mar 04, 2003 at 08:35:05AM -0600, Patrick Mochel wrote:
> 
> On Tue, 4 Mar 2003, Dominik Brodowski wrote:
> 
> > 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. 
...
> +
> +	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?

diff -ruN linux-original/drivers/base/platform.c linux/drivers/base/platform.c
--- linux-original/drivers/base/platform.c	2003-03-05 07:19:19.000000000 +0100
+++ linux/drivers/base/platform.c	2003-03-05 07:22:31.000000000 +0100
@@ -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 = {

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

[Patrick Mochel]

> 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

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

[Jean Tourrilhes]

On Wed, Mar 05, 2003 at 10:16:46AM -0600, Patrick Mochel wrote:
> 
> > 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 :
----------------------------------------------------
diff -u -p linux/drivers/base/platform.c.original linux/drivers/base/platform.c
--- linux/drivers/base/platform.c.original      Fri Mar 14 17:47:16 2003
+++ linux/drivers/base/platform.c       Fri Mar 14 17:47:40 2003
@@ -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