parport_pc to use pnpbios_register_driver()

Here's a patch to make parport_pc.c use pnpbios_register_driver() instead of pnpbios_find_device.

@@ -2822,7 +2822,7 @@
 
 #define UNSET(res)   ((res).flags & IORESOURCE_UNSET)
 
-int init_pnp040x(struct pci_dev *dev)
+int init_PNP040x(struct pci_dev *dev)
 {
 	int io,iohi,irq,dma;
 

@@ -2879,6 +2879,28 @@
 
 #endif 
 
+#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
+static int pnpbios_probe_cb( struct pci_dev *dev, const struct pnpbios_device_id *id )
+{
+                return init_PNP040x(dev);
+}
+
+static struct pnpbios_device_id pnpdevs[] = {
+	/*  id, driver_data */
+	{ "PNP0400", 0L },
+	{ "PNP0401", 0L },
+	{ }
+};
+
+static struct pnpbios_driver pnpdrv = {
+	/* node     */ { },
+	/* name     */ "parport_pc",
+	/* id_table */ pnpdevs,
+	/* probe    */ pnpbios_probe_cb,
+	/* remove   */ NULL
+};
+#endif
+
 /* This function is called by parport_pc_init if the user didn't
  * specify any ports to probe.  Its job is to find some ports.  Order
  * is important here -- we want ISA ports to be registered first,

@@ -2892,7 +2914,6 @@
 static int __init parport_pc_find_ports (int autoirq, int autodma)
 {
 	int count = 0, r;
-	struct pci_dev *dev;
 
 #ifdef CONFIG_PARPORT_PC_SUPERIO
 	detect_and_report_winbond ();

@@ -2900,11 +2921,7 @@
 #endif
 
 #if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
-	dev=NULL;
-	while ((dev=pnpbios_find_device("PNP0400",dev)))
-		count+=init_pnp040x(dev);
-        while ((dev=pnpbios_find_device("PNP0401",dev)))
-                count+=init_pnp040x(dev);
+	count += pnpbios_register_driver(&pnpdrv);
 #endif
 
 	/* Onboard SuperIO chipsets that show themselves on the PCI bus. */

@@ -3015,6 +3032,10 @@
 
 	if (!user_specified)
 		pci_unregister_driver (&parport_pc_pci_driver);
+
+#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
+	pnpbios_unregister_driver(&pnpdrv);
+#endif
 
 	while (p) {
 		tmp = p->next;

linux/isapnp.h has the following code:

---
#if defined(CONFIG_ISAPNP) || (defined(CONFIG_ISAPNP_MODULE) && defined(MODULE))

#define __ISAPNP__
---

I believe the pnpbios driver should do things something similar. Users of the interface should then only check for #ifdef __PNPBIOS__.

The reasoning behind this is the following: When you have a driver built-in, but pnpbios modular, the driver cannot use pnpbios functionality. The above definition reflects exactly this.

(BTW: The drivers using ISAPnP functionality seem to generally get this wrong, I'll look into cleaning this up. In drivers/scsi/aha152x.c it's done right)

--Kai

Does this combination make sense? If you are building a pnpbios driver into the kernel then the configuration should force pnpbios support to be built in as well. We don't allow this combination for things like scsi or ide, they require the common support to be built in if any drivers are built in. IMHO this problem should be fixed in .config, not in driver source.

The affected drivers usually support different interfaces to the hardware, like ISA, ISAPnP, PCI, so it makes sense to build them w/o ISAPnP support. For instance, people may want to have built-in serial support (w/o ISAPnP support is fine, because they've only got a standard SuperI/O chip), but also modular isapnp and sound driver support, so they can demand load sound support if necessary.

Making ISAPnP mandatory only because you build a driver which supports ISAPnP hardware is not an option IMO. This would force people to build ISAPnP support who don't even have an ISA bus (only because the driver for their PCI card also supports an ISAPnP variant).

--Kai

True.

Fine.

You misunderstand. Obviously we don't want to force anyone to build in isa-pnp or pnpbios support. What Keith is saying is that IF isa-pnp code is enabled in a driver then the isa-pnp driver should also be built. Furthermore if isa-pnp code is enabled in an _integral_ (i.e., non-modular) driver, then the isa-pnp driver should also be built integrally. The combination of integral-driver-with-isa-pnp-support-enabled and modular isa-pnp driver should be disallowed by the configurator.

Thomas

Well, I understand what Keith is saying, but that just doesn't apply currently, or if so, tell me what I got wrong.

"IF isa-pnp code is enabled in a driver,..." What does that mean? There is no way to specifically enable isa-pnp code in most (all?) of the drivers.

As an example, let me take drivers/net/ne.c, though this applies to basically all drivers that support kernel-level ISAPnP. (parport is a bad example, as it apparently does not). If there was a CONFIG_NE2000_ISAPNP option, then it would make sense to enforce

"CONFIG_NE2000_ISAPNP = m -> enforce CONFIG_ISAPNP != m" and "CONFIG_NE2000_ISAPNP = y -> enforce CONFIG_ISAPNP = y"

That may be possible in CML2, in CML1 the logic is the other way round, so it would need to be:

"CONFIG_ISAPNP = y and CONFIG_NE2000 != n -> ask for CONFIG_NE2000_ISAPNP" "CONFIG_ISAPNP = m and CONFIG_NE2000 = m -> ask for CONFIG_NE2000_ISAPNP" don't ask otherwise

BTW: I think the current logic is useful, as it allows the user to specify in the beginning that he doesn't have an ISA/ISAPNP bus and thus gets never asked for drivers/features he couldn't possibly use. But that may be a matter of taste, and I believe CML2 allows for both.

My point is, there is no CONFIG_NE2000_ISAPNP option, and the decision to compile in ISAPnP support is based upon CONFIG_ISAPNP{,_MODULE}. Of course one option is to go through all the drivers, add these additional CONFIG_ variables and conditionally compile ISAPnP depending on them. However, I doubt that's a good idea, the current scheme seems to have worked fine so far.

No, if we don't change the config rules, of course it does not make sense to compile ISAPNP code into a built-in object when ISAPNP is selected as modular. However, that's what happening in most of the drivers today. It doesn't cause problems (unresolved symbols) because the isapnp.h header is smart, recognizes this case and replaces the isapnp_* functions with empty dummies. But as we've got the #ifdef in the driver anyway, we could as well be smart and just drop the calling code, as we do in the CONFIG_ISAPNP=n case.

It's just a question of replacing

#if defined(CONFIG_ISAPNP) || defined(CONFIG_ISAPNP_MODULE)

with

#if defined(CONFIG_ISAPNP) || (defined(CONFIG_ISAPNP_MODULE) && defined (MODULE)) a.k.a.
#ifdef __ISAPNP__

Now, let's get back to parport/pnpbios (hope I didn't loose all readers yet)

Looking at drivers/pnp/Config.in, it's apparent, that CONFIG_PNPBIOS is actually a bool. After reading your patch, which has "defined(CONFIG_PNPBIOS_MODULE)", I assumed that it was a tristate. As it apparently is not, the problem above doesn't arise at all. (My point still stands with regard to ISAPNP, though)

So for your driver, I would just say: delete the references to CONFIG_PNPBIOS_MODULE, which is never set.

Some further nitpicking w.r.t to your patch: why not just rename init_pnp040x() to parport_pc_pnbios_probe(), get rid of the wrapper and use the standard return values?

--Kai

Then the first set of CML2 rules which get merged should not be changing the behaviour of such things - they should be looked at later. The first merge of CML2 should have rules which _exactly_ match the existing behaviour of CML1.

--
dwmw2

Yes.

Keith's idea is to disallow integral compilation of isapnp-capable drivers when isa-pnp is modular. Want an integral (e.g.) ne2000? Make isa-pnp integral too, or else disable it entirely.

The configurator does not currently enforce this, and as you say, isapnp.h works around the problem by dummying out isa-pnp functions in integral drivers when isa-pnp is modular. I agree that this is not ideal.

Makes sense. However ....

What I would rather do is write parport_pc consistently with how all other drivers are written. Then if we decide to set all this up more intelligently in the future we can make a global change.

Someday the pnpbios code may be changed to allow compilation of the pnpbios driver as a module.

That's a good idea for a second patch. Not doing what you suggest makes my patch easier to understand.

Thanks a lot for your suggestions. I'm learning stuff. Thomas

Since everyone except parport_pc simply does:
#ifdef CONFIG_PNPBIOS
...
#endif
(i.e., no "|| defined(CONFIG_PNPBIOS_MODULE)") I have changed my patch to be consistent with that practice.

Other changes:
- Merge init_pnp040x into callback
- Use a buffer to build up message

This patch has been tested with the new modutils, and the modules.pnpbiosmap file is correctly generated.

--
Thomas Hood

The patch:

@@ -63,7 +63,7 @@
 #include <linux/parport_pc.h>
 #include <asm/parport.h>
 
-#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
+#ifdef CONFIG_PNPBIOS
 #include <linux/pnp_bios.h>
 #endif
 

@@ -2818,30 +2818,36 @@
 	return count;
 }
 
-#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
+#ifdef CONFIG_PNPBIOS
 
-#define UNSET(res)   ((res).flags & IORESOURCE_UNSET)
-
-int init_pnp040x(struct pci_dev *dev)
+/* formerly init_pnp040x() */
+static int __devinit parport_pc_pnpbios_probecb( struct pci_dev *dev, const struct pnpbios_device_id *id )
 {
+#define UNSET(res)   ((res).flags & IORESOURCE_UNSET)
 	int io,iohi,irq,dma;
+	char buffer[256], *bufw;
+
+	bufw = buffer;
 
-	printk(KERN_INFO
-		"parport: PnP BIOS reports device %s %s (node number 0x%x) is ",
+	bufw+=sprintf(bufw,
+		"parport: PnP BIOS reports device %s %s (node number 0x%x) is",
 		dev->name, dev->slot_name, dev->devfn
 	);
 
 	if ( UNSET(dev->resource[0]) ) {
-		printk("not configured.\n");
+		bufw+=sprintf(bufw, " not configured.");
+		printk(KERN_INFO "%s\n", buffer);
 		return 0;
 	}
-	io  = dev->resource[0].start;
-	printk("configured to use io 0x%04x",io);
+
+	io = dev->resource[0].start;
+	bufw+=sprintf(bufw," configured to use io 0x%04x",io);
+
 	if ( UNSET(dev->resource[1]) ) {
 		iohi = 0;
 	} else {
 		iohi = dev->resource[1].start;
-		printk(", io 0x%04x",iohi);
+		bufw+=sprintf(bufw,", io 0x%04x",iohi);
 	}
 
 	if ( UNSET(dev->irq_resource[0]) ) {

@@ -2849,10 +2855,10 @@
 	} else {
 		if ( dev->irq_resource[0].start == (unsigned long)-1 ) {
 			irq = PARPORT_IRQ_NONE;
-			printk(", irq disabled");
+			bufw+=sprintf(bufw,", irq disabled");
 		} else {
 			irq = dev->irq_resource[0].start;
-			printk(", irq %d",irq);
+			bufw+=sprintf(bufw,", irq %d",irq);
 		}
 	}
 

@@ -2861,23 +2867,38 @@
 	} else {
 		if ( dev->dma_resource[0].start == (unsigned long)-1 ) {
 			dma = PARPORT_DMA_NONE;
-			printk(", dma disabled");
+			bufw+=sprintf(bufw,", dma disabled");
 		} else {
 			dma = dev->dma_resource[0].start;
-			printk(", dma %d",dma);
+			bufw+=sprintf(bufw,", dma %d",dma);
 		}
 	}
-
-	printk("\n");
+	printk(KERN_INFO "%s.\n", buffer);
 
 	if (parport_pc_probe_port(io,iohi,irq,dma,NULL))
 		return 1;
 
 	return 0;
-}
 #undef UNSET
+}
+
+static struct pnpbios_device_id parport_pc_pnpbios_tbl[] __devinitdata = {
+	/*  id, driver_data */
+	{ "PNP0400",  },
+	{ "PNP0401",  },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pnpbios, parport_pc_pnpbios_tbl);
 
-#endif 
+static struct pnpbios_driver parport_pc_pnpbios_drv = {
+	/* node: */
+	name:         "parport_pc",
+	id_table:     parport_pc_pnpbios_tbl,
+	probe:        parport_pc_pnpbios_probecb,
+	remove:       NULL
+};
+#endif
 
 /* This function is called by parport_pc_init if the user didn't
  * specify any ports to probe.  Its job is to find some ports.  Order

@@ -2892,19 +2913,14 @@
 static int __init parport_pc_find_ports (int autoirq, int autodma)
 {
 	int count = 0, r;
-	struct pci_dev *dev;
 
 #ifdef CONFIG_PARPORT_PC_SUPERIO
 	detect_and_report_winbond ();
 	detect_and_report_smsc ();
 #endif
 
-#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)
-	dev=NULL;
-	while ((dev=pnpbios_find_device("PNP0400",dev)))
-		count+=init_pnp040x(dev);
-        while ((dev=pnpbios_find_device("PNP0401",dev)))
-                count+=init_pnp040x(dev);
+#ifdef CONFIG_PNPBIOS
+	count += pnpbios_register_driver(&parport_pc_pnpbios_drv);
 #endif
 
 	/* Onboard SuperIO chipsets that show themselves on the PCI bus. */

@@ -3015,6 +3031,10 @@
 
 	if (!user_specified)
 		pci_unregister_driver (&parport_pc_pci_driver);
+
+#ifdef CONFIG_PNPBIOS
+	pnpbios_unregister_driver(&parport_pc_pnpbios_drv);
+#endif
 
 	while (p) {
 		tmp = p->next;