PnP Layer Rewrite V0.7 - 2.4.42

PATCH Greg KH 5 participants 8 messages Oct 14, 2002 – Oct 15, 2002 1 message not in the archive

Subsystems (2)

Pnp support Isapnp

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Greg KH

author

2002-10-14 18:04 UTC #1 lore raw

Review

Quoted 10 lines · Adam Belay

PNP SUPPORT

(quoted diff) +2 -5

-P:	Tom Lees
-M:	[email protected]
-L:	[email protected]
-L:	[email protected]
-W:	http://www-jcr.lmh.ox.ac.uk/~pnp/
+P:	Adam Belay
+M:	[email protected]
 S:	Maintained

Any word from the people I pointed you at last time?

Quoted 1 line

+#ifdef __PNP__

No, don't redefine CONFIG variables. What's wrong with using CONFIG_PNP?

Review

Quoted 3 lines

(quoted diff) +3 -0

+static const struct pnp_id pnp_dev_table[] = {
+	/* Standard LPT Printer Port */
+	{	"PNP0400",		0	},

Using named initializers are preferred.

Quoted 17 lines

(quoted diff) +13 -0

+	/* ECP Printer Port */
+	{	"PNP0401",		0	},
+	{	"",			0	}
+};
+
+/* we only need the pnp layer to activate the device, at least for now */
+static struct pnp_driver parport_pc_pnp_driver = {
+	.name		= "parport_pc",
+	.card_id_table	= NULL,
+	.id_table	= pnp_dev_table,
+};
+#endif
+
 /* This is called by parport_pc_find_nonpci_ports (in asm/parport.h) */
 static int __init __attribute__((unused))
 parport_pc_find_isa_ports (int autoirq, int autodma)

Review line 3044

Quoted 7 lines

(quoted diff) +4 -0

@@ -3020,6 +3038,10 @@
 
 int __init parport_pc_init (int *io, int *io_hi, int *irq, int *dma)
 {
+#ifdef __PNP__
+	/* try to activate any PnP parports first */
+	pnp_register_driver(&parport_pc_pnp_driver);
+#endif

pnp_register_driver() should be implemented so that you don't need a #ifdef around it to call it. Put the #ifdef in the header file.

thanks,

greg k-h

Adam Belay

2002-10-15 20:05 UTC #2 lore raw

Quoted 13 lines · Greg KH

Review
Quoted 10 lines · Adam Belay
PNP SUPPORT
(quoted diff) +2 -5
-P:	Tom Lees
-M:	[email protected]
-L:	[email protected]
-L:	[email protected]
-W:	http://www-jcr.lmh.ox.ac.uk/~pnp/
+P:	Adam Belay
+M:	[email protected]
 S:	Maintained
Any word from the people I pointed you at last time?

This would be the first time I sent it to them.

Quoted 5 lines

Quoted 2 lines
+#ifdef __PNP__
No, don't redefine CONFIG variables. What's wrong with using CONFIG_PNP?

Ok I'll use CONFIG_PNP instead. Thanks for pointing this out.

Quoted 7 lines

Review
Quoted 5 lines
(quoted diff) +3 -0
+static const struct pnp_id pnp_dev_table[] = {
+	/* Standard LPT Printer Port */
+	{	"PNP0400",		0	},
Using named initializers are preferred.

I'm not quite sure what you mean here.

Quoted 19 lines

(quoted diff) +13 -0

+	/* ECP Printer Port */
+	{	"PNP0401",		0	},
+	{	"",			0	}
+};
+
+/* we only need the pnp layer to activate the device, at least for now */
+static struct pnp_driver parport_pc_pnp_driver = {
+	.name		= "parport_pc",
+	.card_id_table	= NULL,
+	.id_table	= pnp_dev_table,
+};
+#endif
+
 /* This is called by parport_pc_find_nonpci_ports (in asm/parport.h) */
 static int __init __attribute__((unused))
 parport_pc_find_isa_ports (int autoirq, int autodma)

@@ -3020,6 +3038,10 @@

Quoted 9 lines

Review line 3044
Quoted 6 lines
(quoted diff) +4 -0
 int __init parport_pc_init (int *io, int *io_hi, int *irq, int *dma)
 {
+#ifdef __PNP__
+	/* try to activate any PnP parports first */
+	pnp_register_driver(&parport_pc_pnp_driver);
+#endif
pnp_register_driver() should be implemented so that you don't need a #ifdef around it to call it. Put the #ifdef in the header file.

Actually pnp_register_driver is implemented in this way. The reason it has #ifdef around it is becuase of the previous #ifdef statement (where parport_pc_pnp_driver is defined).

Also I had a hotplug related question? Is it possible for pnp drivers to use this and if so what do I need to do?

MODULE_DEVICE_TABLE(pnp, pnp_dev_table);

These comments have been very helpful.

Thanks,
Adam

Greg KH

author

2002-10-15 20:26 UTC #3 lore raw

Quoted 7 lines · Adam Belay

Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+static const struct pnp_id pnp_dev_table[] = {
+	/* Standard LPT Printer Port */
+	{	"PNP0400",		0	},
Using named initializers are preferred.
I'm not quite sure what you mean here.

Something like:

	static const struct pnp_id pnp_dev_table[] = {
	/* standard printer port */
	{ .name = "PNP0400", .data = 0},

or whatever those fields are called.

Quoted 6 lines

Quoted 2 lines
pnp_register_driver() should be implemented so that you don't need a #ifdef around it to call it. Put the #ifdef in the header file.
Actually pnp_register_driver is implemented in this way. The reason it has #ifdef around it is becuase of the previous #ifdef statement (where parport_pc_pnp_driver is defined).

Removing #ifdefs is also nice :)

Quoted 4 lines

Also I had a hotplug related question? Is it possible for pnp drivers to use this and if so what do I need to do?
MODULE_DEVICE_TABLE(pnp, pnp_dev_table);

To fully support this, you need to modify modutils to generate the proper modules.pnpmap file from the .o files. Take a look at the source for it for how to do this.

Also, some kind of /sbin/hotplug notification when a pnp device is found is a good idea. Hm, looks like you already get that for free right now with the existing driver code, nevermind :)

thanks,

greg k-h

Jaroslav Kysela

maintainer

2002-10-14 19:08 UTC #4 lore raw

In reply to a message not in the archive

Quoted 5 lines · Adam Belay

Linux Plug and Play Rewrite V0.7
After much testing the Linux PnP Rewrite is ready to be included. For those who would like to try it, be sure to enable debugging or else it will operate silently. Also enable both PnP protocols.

A few notes. Please, could you leave the raw proc interface for ISA PnP? I mean isapnp_proc_bus_read() function. Also, encoding device/vendor to 7-byte string seems like wasting bytes and CPU cycles. If you use 2/2 byte format, you'll spare 3 bytes and comparing of two short values (or one int value) is always less expensive.

Anyway, I like this code. It seems that you don't use standard pci_dev / pci_bus structures as I was forced by Linus at ISA PnP code inclusion time. But it's true that we have new device model, so these things might be private. Also, don't forget to remove additional ISA PnP members from pci structures when Linus approves pnp_dev and pnp_card structures.

Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project http://www.alsa-project.org
SuSE Linux http://www.suse.com

Jeff Garzik

2002-10-14 19:17 UTC #5 lore raw

Quoted 11 lines · Jaroslav Kysela

Quoted 5 lines · Adam Belay
Linux Plug and Play Rewrite V0.7
After much testing the Linux PnP Rewrite is ready to be included. For those who would like to try it, be sure to enable debugging or else it will operate silently. Also enable both PnP protocols.
A few notes. Please, could you leave the raw proc interface for ISA PnP?

If a rewrite is being done, there are a lot better ways to do this than via ->proc_read and ->proc_write... plus overall procfs usage should be deprecated where reasonable/possible...

Quoted 1 line

I mean isapnp_proc_bus_read() function. Also, encoding device/vendor to 7-byte string seems like wasting bytes and CPU cycles. If you use 2/2 byte format, you'll spare 3 bytes and comparing of two short values (or one int value) is always less expensive.

ASCII has always been preferred. google for "Linus", "linux-kernel", and "ASCII" to see Linus's several postings on the subject...

Quoted 1 line

Anyway, I like this code. It seems that you don't use standard pci_dev / pci_bus structures as I was forced by Linus at ISA PnP code inclusion time. But it's true that we have new device model, so these things might be private. Also, don't forget to remove additional ISA PnP members from pci structures when Linus approves pnp_dev and pnp_card structures.

agreed.

Regards,

Jeff

Adam Belay

2002-10-15 01:35 UTC #6 lore raw

In reply to Jaroslav Kysela

Quoted 9 lines · Jaroslav Kysela

Quoted 5 lines · Adam Belay
Linux Plug and Play Rewrite V0.7
After much testing the Linux PnP Rewrite is ready to be included. For those who would like to try it, be sure to enable debugging or else it will operate silently. Also enable both PnP protocols.
A few notes. Please, could you leave the raw proc interface for ISA PnP?

Sure, a patch is included at the end of the email. Let me know what you think.

Quoted 4 lines

I mean isapnp_proc_bus_read() function. Also, encoding device/vendor to 7-byte string seems like wasting bytes and CPU cycles. If you use 2/2 byte format, you'll spare 3 bytes and comparing of two short values (or one int value) is always less expensive.

Actually I've been thinking about this for quite some time. I see two advantages in using ASCII text.

1.) It allows for wildcards, for example.

the following is from the serial pnp driver:

	/* Unkown PnP modems */
	{	"PNPCXXX",		UNKNOWN_DEV	},

this will match with any device that begins as PNPC.

2.) It makes dev_id tables easier to enter and maintain.

I'll look into this some more.

Quoted 6 lines

Anyway, I like this code. It seems that you don't use standard pci_dev / pci_bus structures as I was forced by Linus at ISA PnP code inclusion time. But it's true that we have new device model, so these things might be private. Also, don't forget to remove additional ISA PnP members from pci structures when Linus approves pnp_dev and pnp_card structures.

I appreciate your comments and recommendations.

Thanks,
Adam

drivers/pnp/isapnp/Makefile +1 -1

diff -ur --new-file --exclude *.flags a/drivers/pnp/isapnp/Makefile b/drivers/pnp/isapnp/Makefile
@@ -4,6 +4,6 @@
 
 export-objs := core.o
 
-obj-y := core.o
+obj-y := core.o proc.o
 
 include $(TOPDIR)/Rules.make

drivers/pnp/isapnp/core.c +2 -2

diff -ur --new-file --exclude *.flags a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
@@ -1053,7 +1053,7 @@
 	return 0;
 }
 
-static struct pnp_protocol isapnp_protocol = {
+struct pnp_protocol isapnp_protocol = {
 	name:	"ISA Plug and Play",
 	get:	isapnp_get_resources,
 	set:	isapnp_set_resources,

@@ -1164,7 +1164,7 @@
 	isapnp_init_device_tree();
 
 #ifdef CONFIG_PROC_FS
-	/*isapnp_proc_init();*/
+	isapnp_proc_init();
 #endif
 	return 0;
 }

drivers/pnp/isapnp/proc.c +171 -0

171 lines not on this page · raw message

include/linux/isapnp.h +2 -1

diff -ur --new-file --exclude *.flags a/include/linux/isapnp.h b/include/linux/isapnp.h
@@ -220,11 +220,12 @@
 
 extern struct list_head isapnp_cards;
 extern struct list_head isapnp_devices;
+extern struct pnp_protocol isapnp_protocol;
 
 #define isapnp_for_each_card(card) \
 	for(card = to_pnp_card(isapnp_cards.next); card != to_pnp_card(&isapnp_cards); card = to_pnp_card(card->node.next))
 #define isapnp_for_each_dev(dev) \
-	for(dev = pci_dev_g(isapnp_devices.next); dev != pci_dev_g(&isapnp_devices); dev = pci_dev_g(dev->global_list.next))
+	for(dev = protocol_to_pnp_dev(isapnp_protocol.devices.next); dev != protocol_to_pnp_dev(&isapnp_protocol.devices); dev = protocol_to_pnp_dev(dev->dev_list.next))
 
 #else /* !CONFIG_ISAPNP */

include/linux/pnp.h +3 -0

diff -ur --new-file --exclude *.flags a/include/linux/pnp.h b/include/linux/pnp.h
@@ -26,6 +26,7 @@
 	unsigned char	productver;	/* product version */
 	unsigned int	serial;		/* serial number */
 	unsigned char	checksum;	/* if zero - checksum passed */
+	struct proc_dir_entry *procdir;	/* directory entry in /proc/bus/isapnp */
 };
 
 #define to_pnp_card(n) list_entry(n, struct pnp_card, node)

@@ -52,10 +53,12 @@
 	struct	device	    dev;	/* Driver Model device interface */
 	void  		  * driver_data;/* data private to the driver */
 	void		  * protocol_data;
+	struct proc_dir_entry *procent;	/* device entry in /proc/bus/isapnp */
 };
 
 #define global_to_pnp_dev(n) list_entry(n, struct pnp_dev, global_list)
 #define card_to_pnp_dev(n) list_entry(n, struct pnp_dev, card_list)
+#define protocol_to_pnp_dev(n) list_entry(n, struct pnp_dev, dev_list)
 #define	to_pnp_dev(n) container_of(n, struct pnp_dev, dev)
 #define pnp_for_each_dev(dev) \
 	for(dev = global_to_pnp_dev(pnp_global.next); \

Greg KH

author

2002-10-15 03:07 UTC #7 lore raw

drivers/pnp/isapnp/core.c line 1168

Quoted 4 lines · Adam Belay

#ifdef CONFIG_PROC_FS
drivers/pnp/isapnp/core.c +1 -1
-	/*isapnp_proc_init();*/
+	isapnp_proc_init();
 #endif

Please bury the #ifdef in the .h file, it shouldn't be in the .c file.

thanks,

greg k-h

Thomas Hood

2002-10-15 15:31 UTC #8 lore raw

In reply to a message not in the archive

Quoted 1 line · Adam Belay

Linux Plug and Play Rewrite V0.7

General:

1. I would appreciate more carefully worded comments (or any comments, especially descriptions of interfaces to be used by other drivers)

2. Please convert *all* initialisers to the new style. Not

          { tag: value, ... }
   but
          { .tag = value, ... }

In the configuration help:

Review

Quoted 5 lines

(quoted diff) +5 -0

+  devices. You should then also say Y to all of the protocols below.
+  Alternatively, you can say N here and configure your PnP devices
+  using user space utilities such as the isapnptools package.
+
+  If unsure, say Y.

Can pnp support no longer be compiled as a module?

Review

Quoted 4 lines

(quoted diff) +4 -0

+  system resources.  Say Y here if you want to reserve these resources.
+  
+  In most cases you should say Y.  If this is causing a conflict then
+  say N.

What do you mean by "causing a conflict"?

Quoted 1 line

+ Some features (e.g. event notification, Docking station information,

"Docking" -> "docking"

For the device list ...

On my Thinkpad 600X, lspnp prints out: "PNP0680 mass storage device: IDE"

Quoted 1 line

+ID("PNP0802", "Microsoft Sound System or Compatible Device (obsolete)")

Why isn't this one in order?

Quoted 1 line

+ID("PNPcXXX", "Unkowwn Modem")

Typo.

Somewhere you might want to include these:
ID("IBM3780", "IBM pointing device")
ID("IBM0071", "IBM infrared communications device") ID("IBM3760", "IBM DSP")
ID("CSC0000", "Crystal Semiconductor CS423x sound -- SB/WSS/OPL3 emulation") ID("CSC0010", "Crystal Semiconductor CS423x sound -- control") ID("CSC0001", "Crystal Semiconductor CS423x sound -- joystick") ID("CSC0003", "Crystal Semiconductor CS423x sound -- MPU401")

Quoted 36 lines

diff -ur --new-file --exclude *.flags a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c

drivers/pnp/pnpbios/core.c +32 -0

[...]
+static void __init build_devlist(void)
+{
+	u8 nodenum;
+	char id[7];
+	unsigned char *pos;
+	unsigned int nodes_got = 0;
+	unsigned int devs = 0;
+	struct pnp_bios_node *node;
+	struct pnp_dev_node_info node_info;
+	struct pnp_dev *dev;
+	struct pnp_id *dev_id;
+
+	if (!pnp_bios_present())
+		return;
+
+	if (pnp_bios_dev_node_info(&node_info) != 0)
+		return;
+
+	node = pnpbios_kmalloc(node_info.max_node_size, GFP_KERNEL);
+	if (!node)
+		return;
+
+	for(nodenum=0; nodenum<0xff; ) {
+		u8 thisnodenum = nodenum;
+		/* We build the list from the "boot" config because
+		 * asking for the "current" config causes some
+		 * BIOSes to crash.
+		 */
+		if (pnp_bios_get_dev_node(&nodenum, (char )0 , node))
+			break;
+		nodes_got++;
+		dev =  pnpbios_kmalloc(sizeof (struct pnp_dev), GFP_KERNEL);

drivers/pnp/pnpbios/core.c

Quoted 10 lines

drivers/pnp/pnpbios/core.c +10 -0

+		if (!dev)
+			break;
+		memset(dev,0,sizeof(struct pnp_dev));
+		dev_id =  pnpbios_kmalloc(sizeof (struct pnp_id), GFP_KERNEL);
+		if (!dev_id)
+			break;
+		memset(dev_id,0,sizeof(struct pnp_id));
+		pnp_init_device(dev);
+		dev->number = thisnodenum;
+		memcpy(dev->name,"Unkown Device",13);

                                  ^^^^^^

Typo. And is the length OK?

Quoted 4 lines

drivers/pnp/pnpbios/core.c +4 -0

+		pnpid32_to_pnpid(node->eisa_id,id);
+		memcpy(dev_id->id,id,8);
+		pnp_add_id(dev_id, dev);
+		pos = node_current_resource_data_to_dev(node,dev);