[RFC]: add sysfs support to parport_pc, v3

[RFC]: add sysfs support to parport_pc, v3

[Jason Dravet]

Here is a new patch to parport_pc that adds sysfs and thus udev support to 
parport_pc.  I fixed my earilier problem of the kernel oops (I forgot the 
class_destory) and I can insmod and rmmod this module all day long with no 
side effects.  I do have one question why do both lp and parport nodes have 
to be created?

What do you think of this patch?  What would be the next step to get this 
into the kernel?

Thanks,
Jason Dravet

signed-off-by: Jason Dravet <dravet@hotmail.com>

--- /usr/src/linux-2.6.14/drivers/parport/parport_pc.c.orig	2005-12-30 
13:52:48.000000000 -0600
+++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2006-01-01 
11:29:05.000000000 -0600
@@ -14,6 +14,7 @@
  * More PCI support now conditional on CONFIG_PCI, 03/2001, Paul G.
  * Various hacks, Fred Barnes, 04/2001
  * Updated probing logic - Adam Belay <ambx1@neo.rr.com>
+ * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
  */

/* This driver should work with any hardware that is broadly compatible
@@ -55,6 +56,7 @@
#include <linux/pci.h>
#include <linux/pnp.h>
#include <linux/sysctl.h>
+#include <linux/sysfs.h>

#include <asm/io.h>
#include <asm/dma.h>
@@ -99,6 +101,9 @@ static struct superio_struct {	/* For Su
	int dma;
} superios[NR_SUPERIOS] __devinitdata = { {0,},};

+static struct class *parallel_class;
+int countports = 0;
+
static int user_specified;
#if defined(CONFIG_PARPORT_PC_SUPERIO) || \
        (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
@@ -2232,6 +2237,11 @@ struct parport *parport_pc_probe_port (u
                                            is mandatory (see above) */
		p->dma = PARPORT_DMA_NONE;

+	parallel_class = class_create(THIS_MODULE, "lp");
+	class_device_create(parallel_class, NULL, MKDEV(6, countports), NULL, 
"lp%i", countports);
+	class_device_create(parallel_class, NULL, MKDEV(99, countports), NULL, 
"parport%i", countports);
+	countports++;
+
#ifdef CONFIG_PARPORT_PC_FIFO
	if (parport_ECP_supported(p) &&
	    p->dma != PARPORT_DMA_NOFIFO &&
@@ -3406,6 +3416,12 @@ static void __exit parport_pc_exit(void)
	if (pnp_registered_parport)
		pnp_unregister_driver (&parport_pc_pnp_driver);

+	for (countports--; countports >=0; countports--) {
+		class_device_destroy(parallel_class, MKDEV(99, countports));
+		class_device_destroy(parallel_class, MKDEV(6, countports));
+	}
+	class_destroy(parallel_class);
+
	spin_lock(&ports_lock);
	while (!list_empty(&ports_list)) {
		struct parport_pc_private *priv;

Re: [Linux-parport] [RFC]: add sysfs support to parport_pc, v3

[Marko Kohtala]

2006/1/2, Jason Dravet <dravet@hotmail.com>:
> Here is a new patch to parport_pc that adds sysfs and thus udev support to
> parport_pc.  I fixed my earilier problem of the kernel oops (I forgot the
> class_destory) and I can insmod and rmmod this module all day long with no
> side effects.  I do have one question why do both lp and parport nodes have
> to be created?
>
> What do you think of this patch?  What would be the next step to get this
> into the kernel?

If your problem is that you do not get /dev/lp0 created automatically,
I think a better way to get that solved is to find out a way that the
module lp is loaded automatically when a printer is found on the
parport.

Maybe you should look into drivers/parport/probe.c and have it do
request_module("lp") when it finds a printer attached to the parport.

Some paride (or SCSI) devices could also be probed for using a
pre-IEEE1284 daisy chain command that gives a 16 bit device ID, and
proper drivers loaded with some module aliases including the device
ID. If something unidentifiable is found on the parport then maybe the
ppdev could be loaded so that userspace drivers will have an interface
to work with.

I fear that some people would find this too smart to be useful. I'd
expect them to request at least a module option to parport to disable
it.

Most people are happy with just "lp" in /etc/modules.

Re: [RFC]: add sysfs support to parport_pc, v3

[Greg KH]

On Mon, Jan 02, 2006 at 09:39:50AM -0600, Jason Dravet wrote:
> Here is a new patch to parport_pc that adds sysfs and thus udev support to 
> parport_pc.  I fixed my earilier problem of the kernel oops (I forgot the 
> class_destory) and I can insmod and rmmod this module all day long with no 
> side effects.  I do have one question why do both lp and parport nodes have 
> to be created?
> 
> What do you think of this patch?  What would be the next step to get this 
> into the kernel?
> 
> Thanks,
> Jason Dravet
> 
> signed-off-by: Jason Dravet <dravet@hotmail.com>

"Signed-off-by:"

> 
> --- /usr/src/linux-2.6.14/drivers/parport/parport_pc.c.orig	2005-12-30 
> 13:52:48.000000000 -0600
> +++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2006-01-01 
> 11:29:05.000000000 -0600

Line wrapped so it can't be applied :(

> @@ -14,6 +14,7 @@
>  * More PCI support now conditional on CONFIG_PCI, 03/2001, Paul G.
>  * Various hacks, Fred Barnes, 04/2001
>  * Updated probing logic - Adam Belay <ambx1@neo.rr.com>
> + * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
>  */

Doesn't belong here, this goes in the change log.

> 
> /* This driver should work with any hardware that is broadly compatible
> @@ -55,6 +56,7 @@
> #include <linux/pci.h>
> #include <linux/pnp.h>
> #include <linux/sysctl.h>
> +#include <linux/sysfs.h>

Your email client also ate the leading spaces :(

> 
> #include <asm/io.h>
> #include <asm/dma.h>
> @@ -99,6 +101,9 @@ static struct superio_struct {	/* For Su
> 	int dma;
> } superios[NR_SUPERIOS] __devinitdata = { {0,},};
> 
> +static struct class *parallel_class;
> +int countports = 0;
> +
> static int user_specified;
> #if defined(CONFIG_PARPORT_PC_SUPERIO) || \
>        (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
> @@ -2232,6 +2237,11 @@ struct parport *parport_pc_probe_port (u
>                                            is mandatory (see above) */
> 		p->dma = PARPORT_DMA_NONE;
> 
> +	parallel_class = class_create(THIS_MODULE, "lp");
> +	class_device_create(parallel_class, NULL, MKDEV(6, countports), 
> NULL, "lp%i", countports);
> +	class_device_create(parallel_class, NULL, MKDEV(99, countports), 
> NULL, "parport%i", countports);
> +	countports++;

What does the 6 and 99 come from?  Aren't these #defined in a header
file somewhere?

thanks,

greg k-h

Re: [RFC]: add sysfs support to parport_pc, v3

[Jason Dravet]

>From: Greg KH <greg@kroah.com>
> > +++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2006-01-01
> > 11:29:05.000000000 -0600
>
>Line wrapped so it can't be applied :(
>
> > + * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
> >  */
>
>Doesn't belong here, this goes in the change log.
My mistake.

> > +#include <linux/sysfs.h>
>
>Your email client also ate the leading spaces :(
My email client is hotmail.  I have come to realize how much hotmail sucks 
for email.  It eats the leading spaces, converts tabs to spaces, and line 
wraps.  I will work on getting a new mail account.

> > +	class_device_create(parallel_class, NULL, MKDEV(6, countports),
> > NULL, "lp%i", countports);
> > +	class_device_create(parallel_class, NULL, MKDEV(99, countports),
> > NULL, "parport%i", countports);
> > +	countports++;
>
>What does the 6 and 99 come from?  Aren't these #defined in a header file 
>somewhere?
Good question.  The answer is I have no idea.  I booted Fedora Core and I 
did a ls -l /dev and wrote down the specs for lp0 and parport0.  lp0 looked 
like crw-rw----  root   lp   6,  0  date  lp0 so that is what I used.  After 
reading Linux Device Drivers I found out that 6 is the major number and 0 is 
the minor number.  The goal was to make sure that the nodes generated by my 
patch were the same as the nodes generated before my patch so I used those 
numbers.  Should I have used different major numbers?

Thanks,
Jason

Re: [RFC]: add sysfs support to parport_pc, v3

[Jan Engelhardt]

>> somewhere?
> Good question.  The answer is I have no idea.  I booted Fedora Core and I did a
> ls -l /dev and wrote down the specs for lp0 and parport0.  lp0 looked like
> crw-rw----  root   lp   6,  0  date  lp0 so that is what I used.  After reading
> Linux Device Drivers I found out that 6 is the major number and 0 is the minor
> number.  The goal was to make sure that the nodes generated by my patch were
> the same as the nodes generated before my patch so I used those numbers.
> Should I have used different major numbers?

You should have at least used the constants from <linux/major.h>


Jan Engelhardt
-- 
| Alphagate Systems, http://alphagate.hopto.org/
| jengelh's site, http://jengelh.hopto.org/

Re: [RFC]: add sysfs support to parport_pc, v3

[Andrew Morton]

"Jason Dravet" <dravet@hotmail.com> wrote:
>
> >From: Greg KH <greg@kroah.com>
> > > +++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2006-01-01
> > > 11:29:05.000000000 -0600
> >
> >Line wrapped so it can't be applied :(
> >
> > > + * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
> > >  */
> >
> >Doesn't belong here, this goes in the change log.
> My mistake.
> 
> > > +#include <linux/sysfs.h>
> >
> >Your email client also ate the leading spaces :(
> My email client is hotmail.  I have come to realize how much hotmail sucks 
> for email.  It eats the leading spaces, converts tabs to spaces, and line 
> wraps.  I will work on getting a new mail account.
> 
> > > +	class_device_create(parallel_class, NULL, MKDEV(6, countports),
> > > NULL, "lp%i", countports);
> > > +	class_device_create(parallel_class, NULL, MKDEV(99, countports),
> > > NULL, "parport%i", countports);
> > > +	countports++;
> >
> >What does the 6 and 99 come from?  Aren't these #defined in a header file 
> >somewhere?
> Good question.  The answer is I have no idea.  I booted Fedora Core and I 
> did a ls -l /dev and wrote down the specs for lp0 and parport0.  lp0 looked 
> like crw-rw----  root   lp   6,  0  date  lp0 so that is what I used.  After 
> reading Linux Device Drivers I found out that 6 is the major number and 0 is 
> the minor number.  The goal was to make sure that the nodes generated by my 
> patch were the same as the nodes generated before my patch so I used those 
> numbers.  Should I have used different major numbers?
> 

6 is OK - it's LP_MAJOR.

However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c.  And yet my
/dev/parport0 is also 99:0 (RH 7.3 and RH FC1).  I've no idea how that came
about??

bix:/home/akpm> grep parport /etc/makedev.d/*
/etc/makedev.d/generic:a generic parport
/etc/makedev.d/linux-2.4.x:c $PRINTER              99   0  1   8 parport%d

Re: [RFC]: add sysfs support to parport_pc, v3

[Marko Kohtala]

2006/1/5, Andrew Morton <akpm@osdl.org>:
> "Jason Dravet" <dravet@hotmail.com> wrote:
> > >From: Greg KH <greg@kroah.com>
> > > > + * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
> > > >  */
>
> 6 is OK - it's LP_MAJOR.
>
> However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c.  And yet my
> /dev/parport0 is also 99:0 (RH 7.3 and RH FC1).  I've no idea how that came
> about??
>
> bix:/home/akpm> grep parport /etc/makedev.d/*
> /etc/makedev.d/generic:a generic parport
> /etc/makedev.d/linux-2.4.x:c $PRINTER              99   0  1   8 parport%d

JSFD is a block device so tha majors are ok. I'm not just sure if the
PP_MAJOR from linux/ppdev.h should be moved to major.h.

The patch by Jason however is not ok. He had another problem and this
is not the fix. What he tries to do is already in lp and ppdev, where
I think they belong.

There is also something weird: why does RedHat create these nodes in
/dev when udev already does that.

Re: [RFC]: add sysfs support to parport_pc, v3

[Kay Sievers]

On Thu, Jan 05, 2006 at 06:17:51PM +0200, Marko Kohtala wrote:
> 2006/1/5, Andrew Morton <akpm@osdl.org>:
> > "Jason Dravet" <dravet@hotmail.com> wrote:
> > > >From: Greg KH <greg@kroah.com>
> > > > > + * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
> > > > >  */
> >
> > 6 is OK - it's LP_MAJOR.
> >
> > However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c.  And yet my
> > /dev/parport0 is also 99:0 (RH 7.3 and RH FC1).  I've no idea how that came
> > about??
> >
> > bix:/home/akpm> grep parport /etc/makedev.d/*
> > /etc/makedev.d/generic:a generic parport
> > /etc/makedev.d/linux-2.4.x:c $PRINTER              99   0  1   8 parport%d
> 
> JSFD is a block device so tha majors are ok. I'm not just sure if the
> PP_MAJOR from linux/ppdev.h should be moved to major.h.
> 
> The patch by Jason however is not ok. He had another problem and this
> is not the fix. What he tries to do is already in lp and ppdev, where
> I think they belong.

Yeah, I was wondering about it too.

> There is also something weird: why does RedHat create these nodes in
> /dev when udev already does that.

Probably cause they want to be safe if nothing from their sysconfig loads
the module. Opening the node will autoload it then.

Kay