[PATCH] parport_pc to use pnpbios_register_driver()

[PATCH] parport_pc to use pnpbios_register_driver()

[Thomas Hood]

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


--- linux-2.4.13-ac8_ORIG/drivers/parport/parport_pc.c	Fri Oct 26 18:13:48 2001
+++ linux-2.4.13-ac8/drivers/parport/parport_pc.c	Fri Nov  9 23:42:11 2001
@@ -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;

Re: [PATCH] parport_pc to use pnpbios_register_driver() #2

[Thomas Hood]

Here is a nicer version of the patch.
(Thanks to Helge Deller for the suggestion.)

--
Thomas

The patch:
--- linux-2.4.13-ac8_ORIG/drivers/parport/parport_pc.c	Fri Oct 26 18:13:48 2001
+++ linux-2.4.13-ac8/drivers/parport/parport_pc.c	Sat Nov 10 23:47:38 2001
@@ -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 pnpbiosdrv = {
+	/* 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(&pnpbiosdrv);
 #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(&pnpbiosdrv);
+#endif
 
 	while (p) {
 		tmp = p->next;

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Kai Germaschewski]

On Sat, 10 Nov 2001, Thomas Hood wrote:

> > Here's a patch to make parport_pc.c use pnpbios_register_driver()
> instead of pnpbios_find_device.
> 
> 
> [...]
>  
> +#if defined (CONFIG_PNPBIOS) || defined (CONFIG_PNPBIOS_MODULE)

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

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Keith Owens]

On Sun, 11 Nov 2001 21:42:34 +0100 (CET), 
Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
>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.

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.

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Kai Germaschewski]

On Mon, 12 Nov 2001, Keith Owens wrote:

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

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Thomas Hood]

On Mon, 2001-11-12 at 03:30, Kai Germaschewski wrote:
> On Mon, 12 Nov 2001, Keith Owens wrote:
> > 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.

True.

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

Fine.

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

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

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Kai Germaschewski]

On 13 Nov 2001, Thomas Hood wrote:

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

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

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[David Woodhouse]

kai@tp1.ruhr-uni-bochum.de said:
>  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. 

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

[PATCH] parport_pc to use pnpbios_register_driver() #4

[Thomas Hood]

I wrote:
> 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.

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:
--- linux-2.4.13-ac8_ORIG/drivers/parport/parport_pc.c	Fri Oct 26 18:13:48 2001
+++ linux-2.4.13-ac8/drivers/parport/parport_pc.c	Thu Nov 15 08:21:35 2001
@@ -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;

Re: [PATCH] parport_pc to use pnpbios_register_driver()

[Thomas Hood]

> the decision to compile in ISAPnP support is based
> upon CONFIG_ISAPNP{,_MODULE}.

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.

> 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  the CONFIG_ISAPNP=n case.

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.

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

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

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

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