[PATCH] register sysfs device for lp devices

[PATCH] register sysfs device for lp devices

[Bernhard R. Link]

Give class_device_create a device pointer parport drivers
can set via a new parport_set_dev.
Also patch parport_gsci and parport_pc as example.

Signed-of-by: Bernhard R Link <brlink@debian.org>

---

Compile and run-tested with linux-2.6.16-rc3-git4.

Index: mlinux-2.6.15-git12/drivers/char/lp.c
===================================================================
--- mlinux-2.6.15-git12.orig/drivers/char/lp.c	2006-02-10 08:22:48.000000000 +0100
+++ mlinux-2.6.15-git12/drivers/char/lp.c	2006-02-13 14:46:29.000000000 +0100
@@ -805,7 +805,7 @@
 	if (reset)
 		lp_reset(nr);
 
-	class_device_create(lp_class, NULL, MKDEV(LP_MAJOR, nr), NULL,
+	class_device_create(lp_class, NULL, MKDEV(LP_MAJOR, nr), port->dev,
 				"lp%d", nr);
 	devfs_mk_cdev(MKDEV(LP_MAJOR, nr), S_IFCHR | S_IRUGO | S_IWUGO,
 			"printers/%d", nr);
Index: mlinux-2.6.15-git12/drivers/parport/parport_gsc.c
===================================================================
--- mlinux-2.6.15-git12.orig/drivers/parport/parport_gsc.c	2006-02-13 14:27:53.000000000 +0100
+++ mlinux-2.6.15-git12/drivers/parport/parport_gsc.c	2006-02-13 14:46:29.000000000 +0100
@@ -340,6 +340,9 @@
 	parport_gsc_write_data(p, 0);
 	parport_gsc_data_forward (p);
 
+	/* Tell sysfs which device is behind this parport */
+	parport_set_dev (p, &dev->dev);
+
 	/* Now that we've told the sharing engine about the port, and
 	   found out its characteristics, let the high-level drivers
 	   know about it. */
Index: mlinux-2.6.15-git12/drivers/parport/parport_pc.c
===================================================================
--- mlinux-2.6.15-git12.orig/drivers/parport/parport_pc.c	2006-02-13 14:30:26.000000000 +0100
+++ mlinux-2.6.15-git12/drivers/parport/parport_pc.c	2006-02-13 14:46:29.000000000 +0100
@@ -2340,6 +2340,7 @@
 	spin_lock(&ports_lock);
 	list_add(&priv->list, &ports_list);
 	spin_unlock(&ports_lock);
+	parport_set_dev (p, &dev->dev);
 	parport_announce_port (p);
 
 	return p;
Index: mlinux-2.6.15-git12/drivers/parport/share.c
===================================================================
--- mlinux-2.6.15-git12.orig/drivers/parport/share.c	2006-02-13 14:30:26.000000000 +0100
+++ mlinux-2.6.15-git12/drivers/parport/share.c	2006-02-13 14:46:29.000000000 +0100
@@ -341,6 +341,11 @@
 
 	tmp->waithead = tmp->waittail = NULL;
 
+	/*
+	 * no sysfs device known unless the announcer sets it before
+	 */
+	tmp->dev = NULL;
+
 	return tmp;
 }
 
Index: mlinux-2.6.15-git12/include/linux/parport.h
===================================================================
--- mlinux-2.6.15-git12.orig/include/linux/parport.h	2006-02-13 14:30:42.000000000 +0100
+++ mlinux-2.6.15-git12/include/linux/parport.h	2006-02-13 14:46:29.000000000 +0100
@@ -258,6 +258,7 @@
 	struct semaphore irq;
 };
 
+struct device;
 /* A parallel port */
 struct parport {
 	unsigned long base;	/* base address */
@@ -313,6 +314,8 @@
 
 	struct list_head full_list;
 	struct parport *slaves[3];
+
+	struct device *dev; /* for the sysfs device symlink */
 };
 
 #define DEFAULT_SPIN_TIME 500 /* us */
@@ -331,6 +334,10 @@
 struct parport *parport_register_port(unsigned long base, int irq, int dma,
 				      struct parport_operations *ops);
 
+/* parport_set_dev set the generic device behind this port, so drivers
+ * can display it in their sysfs nodes */
+#define parport_set_dev(port,devptr) ((port)->dev = (devptr))
+
 /* Once a registered port is ready for high-level drivers to use, the
    low-level driver that registered it should announce it.  This will
    call the high-level drivers' attach() functions (after things like

Re: [PATCH] register sysfs device for lp devices

[Stephen Hemminger]

O
> +	parport_set_dev (p, &dev->dev);
>  	parport_announce_port (p);

Why does the parallel port code use a different whitespace style than
the rest of the kernel. It is incorrect and potentially confusing to
put a blank between the function name and the arguments. It is like
reading, english, with commas in the wrong spot.

Re: [PATCH] register sysfs device for lp devices

[Greg KH]

On Fri, Feb 17, 2006 at 12:38:36PM +0100, Bernhard R. Link wrote:
> Give class_device_create a device pointer parport drivers
> can set via a new parport_set_dev.
> Also patch parport_gsci and parport_pc as example.
> 
> Signed-of-by: Bernhard R Link <brlink@debian.org>
> 
> ---
> 
> Compile and run-tested with linux-2.6.16-rc3-git4.
> 
> Index: mlinux-2.6.15-git12/drivers/char/lp.c
> ===================================================================
> --- mlinux-2.6.15-git12.orig/drivers/char/lp.c	2006-02-10 08:22:48.000000000 +0100
> +++ mlinux-2.6.15-git12/drivers/char/lp.c	2006-02-13 14:46:29.000000000 +0100
> @@ -805,7 +805,7 @@
>  	if (reset)
>  		lp_reset(nr);
>  
> -	class_device_create(lp_class, NULL, MKDEV(LP_MAJOR, nr), NULL,
> +	class_device_create(lp_class, NULL, MKDEV(LP_MAJOR, nr), port->dev,
>  				"lp%d", nr);
>  	devfs_mk_cdev(MKDEV(LP_MAJOR, nr), S_IFCHR | S_IRUGO | S_IWUGO,
>  			"printers/%d", nr);
> Index: mlinux-2.6.15-git12/drivers/parport/parport_gsc.c
> ===================================================================
> --- mlinux-2.6.15-git12.orig/drivers/parport/parport_gsc.c	2006-02-13 14:27:53.000000000 +0100
> +++ mlinux-2.6.15-git12/drivers/parport/parport_gsc.c	2006-02-13 14:46:29.000000000 +0100
> @@ -340,6 +340,9 @@
>  	parport_gsc_write_data(p, 0);
>  	parport_gsc_data_forward (p);
>  
> +	/* Tell sysfs which device is behind this parport */
> +	parport_set_dev (p, &dev->dev);
> +
>  	/* Now that we've told the sharing engine about the port, and
>  	   found out its characteristics, let the high-level drivers
>  	   know about it. */
> Index: mlinux-2.6.15-git12/drivers/parport/parport_pc.c
> ===================================================================
> --- mlinux-2.6.15-git12.orig/drivers/parport/parport_pc.c	2006-02-13 14:30:26.000000000 +0100
> +++ mlinux-2.6.15-git12/drivers/parport/parport_pc.c	2006-02-13 14:46:29.000000000 +0100
> @@ -2340,6 +2340,7 @@
>  	spin_lock(&ports_lock);
>  	list_add(&priv->list, &ports_list);
>  	spin_unlock(&ports_lock);
> +	parport_set_dev (p, &dev->dev);
>  	parport_announce_port (p);
>  
>  	return p;
> Index: mlinux-2.6.15-git12/drivers/parport/share.c
> ===================================================================
> --- mlinux-2.6.15-git12.orig/drivers/parport/share.c	2006-02-13 14:30:26.000000000 +0100
> +++ mlinux-2.6.15-git12/drivers/parport/share.c	2006-02-13 14:46:29.000000000 +0100
> @@ -341,6 +341,11 @@
>  
>  	tmp->waithead = tmp->waittail = NULL;
>  
> +	/*
> +	 * no sysfs device known unless the announcer sets it before
> +	 */
> +	tmp->dev = NULL;
> +

This is not needed, as tmp is zeroed out earlier in the function.


>  	return tmp;
>  }
>  
> Index: mlinux-2.6.15-git12/include/linux/parport.h
> ===================================================================
> --- mlinux-2.6.15-git12.orig/include/linux/parport.h	2006-02-13 14:30:42.000000000 +0100
> +++ mlinux-2.6.15-git12/include/linux/parport.h	2006-02-13 14:46:29.000000000 +0100
> @@ -258,6 +258,7 @@
>  	struct semaphore irq;
>  };
>  
> +struct device;
>  /* A parallel port */
>  struct parport {
>  	unsigned long base;	/* base address */
> @@ -313,6 +314,8 @@
>  
>  	struct list_head full_list;
>  	struct parport *slaves[3];
> +
> +	struct device *dev; /* for the sysfs device symlink */
>  };
>  
>  #define DEFAULT_SPIN_TIME 500 /* us */
> @@ -331,6 +334,10 @@
>  struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  				      struct parport_operations *ops);
>  
> +/* parport_set_dev set the generic device behind this port, so drivers
> + * can display it in their sysfs nodes */
> +#define parport_set_dev(port,devptr) ((port)->dev = (devptr))

If you are going to save off a pointer to a structure, you need to
increment it's reference count.  You aren't doing that here, and bad
things might happen if it gets removed from under you :(

thanks,

greg k-h

Re: [PATCH] register sysfs device for lp devices

[Bernhard R. Link]

* Stephen Hemminger <shemminger@osdl.org> [060217 19:17]:
> O
> > +	parport_set_dev (p, &dev->dev);
> >  	parport_announce_port (p);
> 
> Why does the parallel port code use a different whitespace style than
> the rest of the kernel. It is incorrect and potentially confusing to
> put a blank between the function name and the arguments. It is like
> reading, english, with commas in the wrong spot.

What is the policy for patches of such code? Should patches adhere to
the style of the specific file or that of the whole kernel?

Hochachtungsvoll,
	Bernhard R. Link

Re: [PATCH] register sysfs device for lp devices

[Bernhard R. Link]

> > + * can display it in their sysfs nodes */
> > +#define parport_set_dev(port,devptr) ((port)->dev = (devptr))
> 
> If you are going to save off a pointer to a structure, you need to
> increment it's reference count.  You aren't doing that here, and bad
> things might happen if it gets removed from under you :(

Guess I should think a bit more and copy other code a bit less.
The two examples I looked at were sound and network devices.
While they differ by calling class_device_create more directly,
this does not call a get_device either. (Or where have I missed
it?)

I thought that parport would only exist while the device exists and
is bound to the driver, when that stops it is removed anyway.
But lp.c does not implement lp_detach, so it still keeps this data
and it is really exposed even when the device vanished.

Some unrelated strage behaviour from this: When I manualy unbind
and bind an parport supplier, via 
echo -n "id" > /sys/bus/.../drivers/.../unbind
and then echo -n "id" > /sys/bus/.../drivers/.../bind
I get a new lp device every time without the old one vanishing.

Hochachtungsvoll,
	Bernhard R. Link