Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Greg KH]

On Mon, Oct 14, 2002 at 01:54:52PM +0000, Adam Belay wrote:
>  
>  PNP SUPPORT
> -P:	Tom Lees
> -M:	tom@lpsg.demon.co.uk
> -L:	pnp-users@ferret.lmh.ox.ac.uk
> -L:	pnp-devel@ferret.lmh.ox.ac.uk
> -W:	http://www-jcr.lmh.ox.ac.uk/~pnp/
> +P:	Adam Belay
> +M:	ambx1@neo.rr.com
>  S:	Maintained

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

> +#ifdef __PNP__

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


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

Using named initializers are preferred.

> +	/* 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 @@
>  
>  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

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Adam Belay]

On Mon, Oct 14, 2002 at 11:10:28AM -0700, Greg KH wrote:
> On Mon, Oct 14, 2002 at 01:54:52PM +0000, Adam Belay wrote:
> >
> >  PNP SUPPORT
> > -P:	Tom Lees
> > -M:	tom@lpsg.demon.co.uk
> > -L:	pnp-users@ferret.lmh.ox.ac.uk
> > -L:	pnp-devel@ferret.lmh.ox.ac.uk
> > -W:	http://www-jcr.lmh.ox.ac.uk/~pnp/
> > +P:	Adam Belay
> > +M:	ambx1@neo.rr.com
> >  S:	Maintained
>
> Any word from the people I pointed you at last time?

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

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

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

>
> > +	/* 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 @@
> >
> >  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

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Greg KH]

On Tue, Oct 15, 2002 at 04:09:46PM +0000, Adam Belay wrote:
> > > +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.

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

> 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

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Jaroslav Kysela]

On Mon, 14 Oct 2002, Adam Belay wrote:

> 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 <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project  http://www.alsa-project.org
SuSE Linux    http://www.suse.com

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Jeff Garzik]

Jaroslav Kysela wrote:
> On Mon, 14 Oct 2002, Adam Belay wrote:
> 
> 
>>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...


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


> 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

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Adam Belay]

On Mon, Oct 14, 2002 at 09:10:04PM +0200, Jaroslav Kysela wrote:
> On Mon, 14 Oct 2002, Adam Belay wrote:
> 
> > 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.

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

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



diff -ur --new-file --exclude *.flags a/drivers/pnp/isapnp/Makefile b/drivers/pnp/isapnp/Makefile
--- a/drivers/pnp/isapnp/Makefile	Mon Oct 14 21:18:31 2002
+++ b/drivers/pnp/isapnp/Makefile	Mon Oct 14 18:31:04 2002
@@ -4,6 +4,6 @@
 
 export-objs := core.o
 
-obj-y := core.o
+obj-y := core.o proc.o
 
 include $(TOPDIR)/Rules.make
diff -ur --new-file --exclude *.flags a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
--- a/drivers/pnp/isapnp/core.c	Mon Oct 14 21:18:32 2002
+++ b/drivers/pnp/isapnp/core.c	Mon Oct 14 19:32:46 2002
@@ -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;
 }
diff -ur --new-file --exclude *.flags a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
--- a/drivers/pnp/isapnp/proc.c	Thu Jan  1 00:00:00 1970
+++ b/drivers/pnp/isapnp/proc.c	Mon Oct 14 19:20:39 2002
@@ -0,0 +1,171 @@
+/*
+ *  ISA Plug & Play support
+ *  Copyright (c) by Jaroslav Kysela <perex@suse.cz>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+
+#include <linux/isapnp.h>
+#include <linux/pnp.h>
+#include <linux/proc_fs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/smp_lock.h>
+#include <asm/uaccess.h>
+
+
+static struct proc_dir_entry *isapnp_proc_bus_dir = NULL;
+
+static loff_t isapnp_proc_bus_lseek(struct file *file, loff_t off, int whence)
+{
+	loff_t new = -1;
+
+	lock_kernel();
+	switch (whence) {
+	case 0:
+		new = off;
+		break;
+	case 1:
+		new = file->f_pos + off;
+		break;
+	case 2:
+		new = 256 + off;
+		break;
+	}
+	if (new < 0 || new > 256) {
+		unlock_kernel();
+		return -EINVAL;
+	}
+	unlock_kernel();
+	return (file->f_pos = new);
+}
+
+static ssize_t isapnp_proc_bus_read(struct file *file, char *buf, size_t nbytes, loff_t *ppos)
+{
+	struct inode *ino = file->f_dentry->d_inode;
+	struct proc_dir_entry *dp = PDE(ino);
+	struct pnp_dev *dev = dp->data;
+	int pos = *ppos;
+	int cnt, size = 256;
+
+	if (pos >= size)
+		return 0;
+	if (nbytes >= size)
+		nbytes = size;
+	if (pos + nbytes > size)
+		nbytes = size - pos;
+	cnt = nbytes;
+
+	if (!access_ok(VERIFY_WRITE, buf, cnt))
+		return -EINVAL;
+
+	isapnp_cfg_begin(dev->card->number, dev->number);
+	for ( ; pos < 256 && cnt > 0; pos++, buf++, cnt--) {
+		unsigned char val;
+		val = isapnp_read_byte(pos);
+		__put_user(val, buf);
+	}
+	isapnp_cfg_end();
+
+	*ppos = pos;
+	return nbytes;
+}
+
+static struct file_operations isapnp_proc_bus_file_operations =
+{
+	llseek:		isapnp_proc_bus_lseek,
+	read:		isapnp_proc_bus_read,
+};
+
+static int isapnp_proc_attach_device(struct pnp_dev *dev)
+{
+	struct pnp_card *bus = dev->card;
+	struct proc_dir_entry *de, *e;
+	char name[16];
+
+	if (!(de = bus->procdir)) {
+		sprintf(name, "%02x", bus->number);
+		de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
+		if (!de)
+			return -ENOMEM;
+	}
+	sprintf(name, "%02x", dev->number);
+	e = dev->procent = create_proc_entry(name, S_IFREG | S_IRUGO, de);
+	if (!e)
+		return -ENOMEM;
+	e->proc_fops = &isapnp_proc_bus_file_operations;
+	e->owner = THIS_MODULE;
+	e->data = dev;
+	e->size = 256;
+	return 0;
+}
+
+#ifdef MODULE
+static int __exit isapnp_proc_detach_device(struct pnp_dev *dev)
+{
+	struct pnp_card *bus = dev->card;
+	struct proc_dir_entry *de;
+	char name[16];
+
+	if (!(de = bus->procdir))
+		return -EINVAL;
+	sprintf(name, "%02x", dev->number);
+	remove_proc_entry(name, de);
+	return 0;
+}
+
+static int __exit isapnp_proc_detach_bus(struct pnp_card *bus)
+{
+	struct proc_dir_entry *de;
+	char name[16];
+
+	if (!(de = bus->procdir))
+		return -EINVAL;
+	sprintf(name, "%02x", bus->number);
+	remove_proc_entry(name, isapnp_proc_bus_dir);
+	return 0;
+}
+#endif /* MODULE */
+
+int __init isapnp_proc_init(void)
+{
+	struct pnp_dev *dev;
+	isapnp_proc_bus_dir = proc_mkdir("isapnp", proc_bus);
+	isapnp_for_each_dev(dev) {
+		isapnp_proc_attach_device(dev);
+	}
+	return 0;
+}
+
+#ifdef MODULE
+int __exit isapnp_proc_done(void)
+{
+	struct pnp_dev *dev;
+	struct pnp_bus *card;
+
+	isapnp_for_each_dev(dev) {
+		isapnp_proc_detach_device(dev);
+	}
+	isapnp_for_each_card(card) {
+		isapnp_proc_detach_bus(card);
+	}
+	if (isapnp_proc_bus_dir)
+		remove_proc_entry("isapnp", proc_bus);
+	return 0;
+}
+#endif /* MODULE */
diff -ur --new-file --exclude *.flags a/include/linux/isapnp.h b/include/linux/isapnp.h
--- a/include/linux/isapnp.h	Mon Oct 14 21:18:32 2002
+++ b/include/linux/isapnp.h	Mon Oct 14 19:29:17 2002
@@ -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 */
 
diff -ur --new-file --exclude *.flags a/include/linux/pnp.h b/include/linux/pnp.h
--- a/include/linux/pnp.h	Mon Oct 14 21:18:32 2002
+++ b/include/linux/pnp.h	Mon Oct 14 19:23:34 2002
@@ -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); \

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Greg KH]

On Mon, Oct 14, 2002 at 09:43:34PM +0000, Adam Belay wrote:
>  #ifdef CONFIG_PROC_FS
> -	/*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

Re: [PATCH] PnP Layer Rewrite V0.7 - 2.4.42

[Thomas Hood]

On Mon, 2002-10-14 at 15:54, Adam Belay wrote:
> 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:

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

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

> +  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"

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

Why isn't this one in order?

> +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")


> diff -ur --new-file --exclude *.flags a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
> --- a/drivers/pnp/pnpbios/core.c	Thu Jan  1 00:00:00 1970
> +++ b/drivers/pnp/pnpbios/core.c	Tue Oct  8 17:18:29 2002
> [...]
> +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);
> +		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?

> +		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);