[RFC/PATCH] sysfs'ify video4linux

[RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

  Hi,

This patch moves the video4linux subsystem from procfs to sysfs.
Changes:

  * procfs support is completely gone, i.e. /proc/video doesn't
    exist any more.

  * there is a new device class instead: /sys/class/video4linux.
    All video4linux devices which used to be listed in
    /proc/video/dev are moved to that place.

Changes required/recommended in video4linux drivers:

  * some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko 
    and ov511.ko) use the video_proc_entry() to add additional
    procfs files.  These drivers must be converted to sysfs too
    because video_proc_entry() doesn't exist any more.

  * struct video_device has a new "dev" field pointing to a struct
    device.  Drivers should fill that one if possible.  It isn't
    required through.  It will give you fancy device + driver symlinks
    in /sys/class/video4linux/<name>.
    The patch below includes the changes for bttv.

Comments?

  Gerd

--- linux-2.6.0-test1/drivers/media/video/videodev.c.sysfs	2003-07-15 13:30:26.000000000 +0200
+++ linux-2.6.0-test1/drivers/media/video/videodev.c	2003-07-15 14:52:52.000000000 +0200
@@ -15,7 +15,6 @@
  *		- Added procfs support
  */
 
-#include <linux/config.h>
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -35,33 +34,31 @@
 
 #include <linux/videodev.h>
 
-#define VIDEO_NUM_DEVICES	256 
+#define VIDEO_NUM_DEVICES	256
+#define VIDEO_NAME              "video4linux"
 
 /*
- *	Active devices 
+ *	sysfs stuff
  */
- 
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DECLARE_MUTEX(videodev_lock);
 
+static size_t show_name(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = cd->class_data;
+	return sprintf(buf,"%.*s\n",sizeof(vfd->name),vfd->name);
+}
 
-#ifdef CONFIG_VIDEO_PROC_FS
-
-#include <linux/proc_fs.h>
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 
-struct videodev_proc_data {
-	struct list_head proc_list;
-	char name[16];
-	struct video_device *vdev;
-	struct proc_dir_entry *proc_entry;
+static struct class video_class = {
+        .name = VIDEO_NAME,
 };
 
-static struct proc_dir_entry *video_dev_proc_entry = NULL;
-struct proc_dir_entry *video_proc_entry = NULL;
-EXPORT_SYMBOL(video_proc_entry);
-LIST_HEAD(videodev_proc_list);
-
-#endif /* CONFIG_VIDEO_PROC_FS */
+/*
+ *	Active devices 
+ */
+ 
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DECLARE_MUTEX(videodev_lock);
 
 struct video_device* video_devdata(struct file *file)
 {
@@ -219,156 +216,6 @@
 	return 0;
 }
 
-/*
- *	/proc support
- */
-
-#ifdef CONFIG_VIDEO_PROC_FS
-
-/* Hmm... i'd like to see video_capability information here, but
- * how can I access it (without changing the other drivers? -claudio
- */
-static int videodev_proc_read(char *page, char **start, off_t off,
-			       int count, int *eof, void *data)
-{
-	char *out = page;
-	struct video_device *vfd = data;
-	struct videodev_proc_data *d;
-	struct list_head *tmp;
-	int len;
-	char c = ' ';
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev)
-			break;
-	}
-
-	/* Sanity check */
-	if (tmp == &videodev_proc_list)
-		goto skip;
-		
-#define PRINT_VID_TYPE(x) do { if (vfd->type & x) \
-	out += sprintf (out, "%c%s", c, #x); c='|';} while (0)
-
-	out += sprintf (out, "name            : %s\n", vfd->name);
-	out += sprintf (out, "type            :");
-		PRINT_VID_TYPE(VID_TYPE_CAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_TUNER);
-		PRINT_VID_TYPE(VID_TYPE_TELETEXT);
-		PRINT_VID_TYPE(VID_TYPE_OVERLAY);
-		PRINT_VID_TYPE(VID_TYPE_CHROMAKEY);
-		PRINT_VID_TYPE(VID_TYPE_CLIPPING);
-		PRINT_VID_TYPE(VID_TYPE_FRAMERAM);
-		PRINT_VID_TYPE(VID_TYPE_SCALES);
-		PRINT_VID_TYPE(VID_TYPE_MONOCHROME);
-		PRINT_VID_TYPE(VID_TYPE_SUBCAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_ENCODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_ENCODER);
-	out += sprintf (out, "\n");
-	out += sprintf (out, "hardware        : 0x%x\n", vfd->hardware);
-#if 0
-	out += sprintf (out, "channels        : %d\n", d->vcap.channels);
-	out += sprintf (out, "audios          : %d\n", d->vcap.audios);
-	out += sprintf (out, "maxwidth        : %d\n", d->vcap.maxwidth);
-	out += sprintf (out, "maxheight       : %d\n", d->vcap.maxheight);
-	out += sprintf (out, "minwidth        : %d\n", d->vcap.minwidth);
-	out += sprintf (out, "minheight       : %d\n", d->vcap.minheight);
-#endif
-
-skip:
-	len = out - page;
-	len -= off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0)
-			return 0;
-	} else
-		len = count;
-
-	*start = page + off;
-
-	return len;
-}
-
-static void videodev_proc_create(void)
-{
-	video_proc_entry = create_proc_entry("video", S_IFDIR, &proc_root);
-
-	if (video_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video\n");
-		return;
-	}
-
-	video_proc_entry->owner = THIS_MODULE;
-	video_dev_proc_entry = create_proc_entry("dev", S_IFDIR, video_proc_entry);
-
-	if (video_dev_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video/dev\n");
-		return;
-	}
-
-	video_dev_proc_entry->owner = THIS_MODULE;
-}
-
-static void __exit videodev_proc_destroy(void)
-{
-	if (video_dev_proc_entry != NULL)
-		remove_proc_entry("dev", video_proc_entry);
-
-	if (video_proc_entry != NULL)
-		remove_proc_entry("video", &proc_root);
-}
-
-static void videodev_proc_create_dev (struct video_device *vfd, char *name)
-{
-	struct videodev_proc_data *d;
-	struct proc_dir_entry *p;
-
-	if (video_dev_proc_entry == NULL)
-		return;
-
-	d = kmalloc (sizeof (struct videodev_proc_data), GFP_KERNEL);
-	if (!d)
-		return;
-
-	p = create_proc_entry(name, S_IFREG|S_IRUGO|S_IWUSR, video_dev_proc_entry);
-	if (!p) {
-		kfree(d);
-		return;
-	}
-	p->data = vfd;
-	p->read_proc = videodev_proc_read;
-
-	d->proc_entry = p;
-	d->vdev = vfd;
-	strcpy (d->name, name);
-
-	/* How can I get capability information ? */
-
-	list_add (&d->proc_list, &videodev_proc_list);
-}
-
-static void videodev_proc_destroy_dev (struct video_device *vfd)
-{
-	struct list_head *tmp;
-	struct videodev_proc_data *d;
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev) {
-			remove_proc_entry(d->name, video_dev_proc_entry);
-			list_del (&d->proc_list);
-			kfree(d);
-			break;
-		}
-	}
-}
-
-#endif /* CONFIG_VIDEO_PROC_FS */
-
 extern struct file_operations video_fops;
 
 /**
@@ -456,15 +303,17 @@
 	devfs_mk_cdev(MKDEV(VIDEO_MAJOR, vfd->minor),
 			S_IFCHR | S_IRUSR | S_IWUSR, vfd->devfs_name);
 	init_MUTEX(&vfd->lock);
-	
-#ifdef CONFIG_VIDEO_PROC_FS
-{
-	char name[16];
-	sprintf(name, "%s%d", name_base, i - base);
-	videodev_proc_create_dev(vfd, name);
-}
-#endif
 
+	/* sysfs class */
+        memset(&vfd->class_dev, 0x00, sizeof(vfd->class_dev));
+	if (vfd->dev)
+		vfd->class_dev.dev = vfd->dev;
+	vfd->class_dev.class       = &video_class;
+	vfd->class_dev.class_data  = vfd;
+	strlcpy(vfd->class_dev.class_id, vfd->devfs_name + 4, BUS_ID_SIZE);
+	class_device_register(&vfd->class_dev);
+	class_device_create_file(&vfd->class_dev,
+				 &class_device_attr_name);
 	return 0;
 }
 
@@ -482,10 +331,7 @@
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");
 
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy_dev (vfd);
-#endif
-
+	class_device_unregister(&vfd->class_dev);
 	devfs_remove(vfd->devfs_name);
 	video_device[vfd->minor]=NULL;
 	up(&videodev_lock);
@@ -506,24 +352,18 @@
 static int __init videodev_init(void)
 {
 	printk(KERN_INFO "Linux video capture interface: v1.00\n");
-	if (register_chrdev(VIDEO_MAJOR,"video_capture", &video_fops)) {
+	if (register_chrdev(VIDEO_MAJOR,VIDEO_NAME, &video_fops)) {
 		printk("video_dev: unable to get major %d\n", VIDEO_MAJOR);
 		return -EIO;
 	}
-
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_create ();
-#endif
-	
+	class_register(&video_class);
 	return 0;
 }
 
 static void __exit videodev_exit(void)
 {
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy ();
-#endif
-	unregister_chrdev(VIDEO_MAJOR, "video_capture");
+	class_unregister(&video_class);
+	unregister_chrdev(VIDEO_MAJOR, VIDEO_NAME);
 }
 
 module_init(videodev_init)
--- linux-2.6.0-test1/drivers/media/video/bttv-driver.c.sysfs	2003-07-14 12:37:01.000000000 +0200
+++ linux-2.6.0-test1/drivers/media/video/bttv-driver.c	2003-07-15 14:29:02.000000000 +0200
@@ -3413,11 +3413,14 @@
 	memcpy(&btv->radio_dev, &radio_template,      sizeof(radio_template));
 	memcpy(&btv->vbi_dev,   &bttv_vbi_template,   sizeof(bttv_vbi_template));
         btv->video_dev.minor = -1;
-	btv->video_dev.priv = btv;
+	btv->video_dev.priv  = btv;
+	btv->video_dev.dev   = &dev->dev;
         btv->radio_dev.minor = -1;
-	btv->radio_dev.priv = btv;
+	btv->radio_dev.priv  = btv;
+	btv->radio_dev.dev   = &dev->dev;
         btv->vbi_dev.minor = -1;
-	btv->vbi_dev.priv = btv;
+	btv->vbi_dev.priv  = btv;
+	btv->vbi_dev.dev   = &dev->dev;
 	btv->has_radio=radio[btv->nr];
 	
 	/* pci stuff (init, get irq/mmip, ... */
--- linux-2.6.0-test1/include/linux/videodev.h.sysfs	2003-07-14 12:05:25.000000000 +0200
+++ linux-2.6.0-test1/include/linux/videodev.h	2003-07-15 13:40:50.000000000 +0200
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/version.h>
+#include <linux/device.h>
 
 #if 1
 /*
@@ -24,6 +25,7 @@
 struct video_device
 {
 	struct module *owner;
+	struct device *dev;
      	char name[32];
  	int type;       /* v4l1 */
  	int type2;      /* v4l2 */
@@ -39,6 +41,7 @@
 	int users;
 	struct semaphore lock;
 	char devfs_name[64];	/* devfs */
+	struct class_device class_dev;
 };
 
 #define VIDEO_MAJOR	81

Re: [RFC/PATCH] sysfs'ify video4linux

[Ronald Bultje]

Hey Gerd,

On Tue, 2003-07-15 at 16:31, Gerd Knorr wrote:
> This patch moves the video4linux subsystem from procfs to sysfs.
[..cool stuff, especially the driver integration fanciness with the
device struct stuff..]
> Comments?

Is /proc going away alltogether?

(I'm not actively following 2.5.x development; I currently can't get any
computer here boot up any recent (>=2.5.72) 2.5 kernel alltogether, see
LKML for details...)

Ronald

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>
Linux Video/Multimedia developer

Re: [RFC/PATCH] sysfs'ify video4linux

[Matt Porter]

On Tue, Jul 15, 2003 at 05:21:13PM +0200, Ronald Bultje wrote:
> Hey Gerd,
> 
> On Tue, 2003-07-15 at 16:31, Gerd Knorr wrote:
> > This patch moves the video4linux subsystem from procfs to sysfs.
> [..cool stuff, especially the driver integration fanciness with the
> device struct stuff..]
> > Comments?
> 
> Is /proc going away alltogether?

Well, it does need to provide per-process info like it was originally
intended. It's all the device crap that doesn't belong there that is
moving.

Regards,
-- 
Matt Porter
mporter@kernel.crashing.org

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote:
>   Hi,
> 
> This patch moves the video4linux subsystem from procfs to sysfs.
> Changes:
> 
>   * procfs support is completely gone, i.e. /proc/video doesn't
>     exist any more.

Yeah!

>   * there is a new device class instead: /sys/class/video4linux.
>     All video4linux devices which used to be listed in
>     /proc/video/dev are moved to that place.

Nice.

> Changes required/recommended in video4linux drivers:
> 
>   * some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko 
>     and ov511.ko) use the video_proc_entry() to add additional
>     procfs files.  These drivers must be converted to sysfs too
>     because video_proc_entry() doesn't exist any more.

I'd be glad to do this work once your change makes it into the core.  Is
there any need for these drivers to export anything through sysfs now
instead of /proc?  From what I remember, it only looked like debugging
and other general info stuff.

>   * struct video_device has a new "dev" field pointing to a struct
>     device.  Drivers should fill that one if possible.  It isn't
>     required through.  It will give you fancy device + driver symlinks
>     in /sys/class/video4linux/<name>.
>     The patch below includes the changes for bttv.

So dev should point to the dev of the video class device?

> Comments?

You _have_ to set up a release function for your class device.  You
can't just kfree it like I think you are doing, otherwise any users of
the sysfs files will oops the kernel after the video class device is
gone.  I can point you to some examples of how to do this if you like.

Other than that, how about exporting the dev_t value for the video
device?  Then you automatically get udev support, and I don't have to go
add it to this code later :)

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

> >   * some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko 
> >     and ov511.ko) use the video_proc_entry() to add additional
> >     procfs files.  These drivers must be converted to sysfs too
> >     because video_proc_entry() doesn't exist any more.
> 
> I'd be glad to do this work once your change makes it into the core.  Is
> there any need for these drivers to export anything through sysfs now
> instead of /proc?  From what I remember, it only looked like debugging
> and other general info stuff.

IIRC some tuning / debugging / info stuff, the drivers should work just
fine without.  Temporarely disableling that wouldn't be a big issue
IMHO.

> So dev should point to the dev of the video class device?

Exactly.  videodev.o will put that into class_device->dev which in turn
will produce these symlinks:

eskarina kraxel /sys/class/video4linux/video0# ll device
lrwxrwxrwx    1 root     root           53 Jul 15 17:20 device -> ../../../devices/pci0000:00/0000:00:06.0/0000:02:07.0/

> > Comments?
> 
> You _have_ to set up a release function for your class device.  You
> can't just kfree it like I think you are doing, otherwise any users of
> the sysfs files will oops the kernel after the video class device is
> gone.

class_device_unregister() is called from video_unregister_device() which
looks fine to me.  Or do you talk about something else?

> Other than that, how about exporting the dev_t value for the video
> device?  Then you automatically get udev support, and I don't have to go
> add it to this code later :)

Do you have a pointer to sample code for that?

  Gerd

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Mark McClelland]

Greg KH wrote:

>On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote:
>  
>
>>Changes required/recommended in video4linux drivers:
>>
>>  * some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko 
>>    and ov511.ko) use the video_proc_entry() to add additional
>>    procfs files.  These drivers must be converted to sysfs too
>>    because video_proc_entry() doesn't exist any more.
>>    
>>
>
>I'd be glad to do this work once your change makes it into the core. 
>

I can do the ov511 work if you want. I can have it done in two days (or 
less).

>Is there any need for these drivers to export anything through sysfs now
>instead of /proc?
>

Yes, at least with ov511. Some of the info that it puts in /proc is no 
longer necessary. However, there are various bits of hardware info that 
still need to get to userspace, for scripts that need to tell otherwise 
identical (same VID/PID/revision) cameras apart when creating /dev nodes.

Is there a convention for naming driver-specific files in /sys? E.g.: 
ov511_foo, _foo, etc...? I don't want to pollute the namespace.

>From what I remember, it only looked like debugging and other general info stuff.
>

There *is* some debugging stuff I would like to keep, at least for now. 
Approximately half of the bug reports I get are resolved after I see the 
user's /proc info. Is it acceptable to put it all in one file, since it 
isn't meant to be machine-parseable anyway? Some of the HCI drivers do 
that, right?

-- 
Mark McClelland
mark@alpha.dyndns.org

Re: [RFC/PATCH] sysfs'ify video4linux

[root]

> 
> Greg KH wrote:
> 
> >On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote:
> >  
> >
> >>Changes required/recommended in video4linux drivers:
> >>
> >>  * some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko 
> >>    and ov511.ko) use the video_proc_entry() to add additional
> >>    procfs files.  These drivers must be converted to sysfs too
> >>    because video_proc_entry() doesn't exist any more.
<snip>
> >Is there any need for these drivers to export anything through sysfs now
> >instead of /proc?
> >
> 
> Yes, at least with ov511. Some of the info that it puts in /proc is no 
> longer necessary. However, there are various bits of hardware info that 
> still need to get to userspace, for scripts that need to tell otherwise 
> identical (same VID/PID/revision) cameras apart when creating /dev nodes.

Also, there is some other information that scripts need.
Exposure gives information about light levels, which can be used to 
compute the absolute brightness of a scene captured.

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Wed, Jul 16, 2003 at 10:44:48AM +0200, Gerd Knorr wrote:
> > > Comments?
> > 
> > You _have_ to set up a release function for your class device.  You
> > can't just kfree it like I think you are doing, otherwise any users of
> > the sysfs files will oops the kernel after the video class device is
> > gone.
> 
> class_device_unregister() is called from video_unregister_device() which
> looks fine to me.  Or do you talk about something else?

Heh, yes, that is correct.  But then what do you do with the object that
had the class_device in it after class_device_unregister() returns?  I
don't see you freeing the device directly, which is good, but who does?

You _have_ to let the structure that the kobject is in free itself in a
release() function, you can't guess at what the kobject reference count
is at any time and just assume you can throw it away at that moment.

LWN did an article about how to do this properly at:
	http://lwn.net/Articles/36850/

But for some code that does this in a simple manner, look at
drivers/char/tty_io.c, the release_tty_dev() function.  It's set up in
the tty_class to handle destroying the structure when the last reference
to the tty_dev structure goes away.  Look at the
tty_remove_class_device() to see that the structure isn't kfreed on it's
own.

It looks like the video drivers include a "struct video_device"
structure within their own structures, right?  That will have to be
changed to a pointer to that structure in order for the lifetime rules
to work properly.  I know Hanna is fighting this same battle with the
input layer right now...

Does this help out?

As an example of why you _have_ to do this, consider a user who opens a
sysfs file of a video device, and then removes the video device from the
system.  Then they do a read on the sysfs file.  Oops...

> > Other than that, how about exporting the dev_t value for the video
> > device?  Then you automatically get udev support, and I don't have to go
> > add it to this code later :)
> 
> Do you have a pointer to sample code for that?

Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in
/sys/class/usb/*, and so on...

I need to make a general function to support this to make it easier for
everyone to export a dev_t to userspace, but until then, if you want to
look at the show_dev() function in drivers/usb/core/file.c it shows what
you need to do.

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Wed, Jul 16, 2003 at 06:33:02AM -0700, Mark McClelland wrote:
> 
> Is there a convention for naming driver-specific files in /sys? E.g.: 
> ov511_foo, _foo, etc...? I don't want to pollute the namespace.

You get your own subdirectory to play in, so there really isn't much you
can do to hurt the namespace :)

That being said, stay away from files named "device", "driver", and
"dev".  They are used by the driver core (well, "dev" isn't, but each
class provides it...).

Just remember, one value per file and everyone will be happy.

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

> It looks like the video drivers include a "struct video_device"
> structure within their own structures, right?

Yes, it is allocated/freed by the driver, most seem to simply include
one ore more "struct video_device" somewhere in the per-device struct.

> That will have to be changed to a pointer to that structure in order
> for the lifetime rules to work properly.

Hmm.  I doubt it will be that simple.  struct video_device has a priv
field which can be used by the drivers to hook in some driver-private
data.  That may point into nowhere if struct video_device has a longer
live time due to some kobject still being referenced.  Wouldn't be a
issue for videodev.o itself, but might become a problem for drivers
which want add private properties and rely on video_device->priv
for finding the per-device data.  Problem isn't solved but justed
moved to the next corner ...

Maybe let video_unregister sleep on a semaphore which gets woken up
by the release function?  That should make sure the sysfs objects are
not referenced any more if video_unregister() returns.  I use a similar
method in some places when shutting down kernel threads, to make sure it
is really stopped before rmmod frees the memory.

> Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in
> /sys/class/usb/*, and so on...

I've found the code in drivers/block/genhd.c in the meantime :)

  Gerd

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote:
> > It looks like the video drivers include a "struct video_device"
> > structure within their own structures, right?
> 
> Yes, it is allocated/freed by the driver, most seem to simply include
> one ore more "struct video_device" somewhere in the per-device struct.

So you CAN NOT just blindly put a kobject (meaning a class_device)
structure inside of there.

> > That will have to be changed to a pointer to that structure in order
> > for the lifetime rules to work properly.
> 
> Hmm.  I doubt it will be that simple.  struct video_device has a priv
> field which can be used by the drivers to hook in some driver-private
> data.  That may point into nowhere if struct video_device has a longer
> live time due to some kobject still being referenced.  Wouldn't be a
> issue for videodev.o itself, but might become a problem for drivers
> which want add private properties and rely on video_device->priv
> for finding the per-device data.  Problem isn't solved but justed
> moved to the next corner ...

No, just have the video drivers have a release callback to do the
freeing.  It's pretty simple, look at usb_host_release() in
drivers/usb/core/hcd.c.  That's exactly what that is for.  Hm, I
shouldn't have to check for bus->release() there, as release is now
required...

> Maybe let video_unregister sleep on a semaphore which gets woken up
> by the release function?  That should make sure the sysfs objects are
> not referenced any more if video_unregister() returns.  I use a similar
> method in some places when shutting down kernel threads, to make sure it
> is really stopped before rmmod frees the memory.

Ick, no.  Try doing what I did for usb hosts, it's much simpler.

> > Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in
> > /sys/class/usb/*, and so on...
> 
> I've found the code in drivers/block/genhd.c in the meantime :)

genhd.c uses "raw" kobjects.  It might be easier to look at a
class_device example like the above mentioned usb_host one.

If you have any other questions/problems, feel free to ask.

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

On Wed, Jul 16, 2003 at 02:08:00PM -0700, Greg KH wrote:
> On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote:
> > Yes, it is allocated/freed by the driver, most seem to simply include
> > one ore more "struct video_device" somewhere in the per-device struct.
> 
> So you CAN NOT just blindly put a kobject (meaning a class_device)
> structure inside of there.

Why not ...

> > which want add private properties and rely on video_device->priv
> > for finding the per-device data.  Problem isn't solved but justed
> > moved to the next corner ...
> 
> No, just have the video drivers have a release callback to do the
> freeing.

... if a ->release() callback is required anyway to fix it?  I see two
ways to handle it:

  (1) mandatory ->release() callback, drivers must make sure the stuff
      is not freed before the callback was called.  In that case the
      class_device can be left embedded inside the drivers provate
      structs.
  (2) optional ->release() callback (for those drivers which want add
      private attributes), "struct video_device" must be moved out of
      the drivers private structs then and released in a new function
      (which also calls the drivers ->release callback if present).
      Should probably also be allocated by videodev.c for symmetry.

Both approaches require touching all v4l drivers in non-trivial ways
through, not sure whenever it is a good idea to do that now.  Any chance
to get that in before 2.6.0?  Or should I better make that change in
2.7.x and live with /proc for the time being?

  Gerd

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Thu, Jul 17, 2003 at 02:01:21PM +0200, Gerd Knorr wrote:
> On Wed, Jul 16, 2003 at 02:08:00PM -0700, Greg KH wrote:
> > On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote:
> > > Yes, it is allocated/freed by the driver, most seem to simply include
> > > one ore more "struct video_device" somewhere in the per-device struct.
> > 
> > So you CAN NOT just blindly put a kobject (meaning a class_device)
> > structure inside of there.
> 
> Why not ...
> 
> > > which want add private properties and rely on video_device->priv
> > > for finding the per-device data.  Problem isn't solved but justed
> > > moved to the next corner ...
> > 
> > No, just have the video drivers have a release callback to do the
> > freeing.
> 
> ... if a ->release() callback is required anyway to fix it?  I see two
> ways to handle it:
> 
>   (1) mandatory ->release() callback, drivers must make sure the stuff
>       is not freed before the callback was called.  In that case the
>       class_device can be left embedded inside the drivers provate
>       structs.

That sounds fine, and is the same as what I did for usb host devices.

>   (2) optional ->release() callback (for those drivers which want add
>       private attributes), "struct video_device" must be moved out of
>       the drivers private structs then and released in a new function
>       (which also calls the drivers ->release callback if present).
>       Should probably also be allocated by videodev.c for symmetry.

That's "prettier", but probably requires a lot more work in every v4l
driver, right?

> Both approaches require touching all v4l drivers in non-trivial ways
> through, not sure whenever it is a good idea to do that now.  Any chance
> to get that in before 2.6.0?  Or should I better make that change in
> 2.7.x and live with /proc for the time being?

I'd say you should try for it now.  It's required if you want support
for things like udev in 2.6, which is what a lot of people seem to
want...

I know there are still a few other subsystems that need this kind of
change in 2.6, so you are not alone (input, misc, parallel port, sound,
etc.)

I'd be glad to help you out with this if you want.

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

> > ... if a ->release() callback is required anyway to fix it?  I see two
> > ways to handle it:
> > 
> >   (1) mandatory ->release() callback, drivers must make sure the stuff
> >       is not freed before the callback was called.  In that case the
> >       class_device can be left embedded inside the drivers provate
> >       structs.
> 
> That sounds fine, and is the same as what I did for usb host devices.

Ok.

> >   (2) optional ->release() callback (for those drivers which want add
> >       private attributes), "struct video_device" must be moved out of
> >       the drivers private structs then and released in a new function
> >       (which also calls the drivers ->release callback if present).
> >       Should probably also be allocated by videodev.c for symmetry.
> 
> That's "prettier", but probably requires a lot more work in every v4l
> driver, right?

Not sure which of them is actually more work.

Version (1) can be done without breaking the build, with a hack along 
the lines "if (no release callback) printk(KERN_WARN please fix your
driver)", so the drivers can be fixed step-by-step afterwards.

Fixing the drivers might be non-trivial through.  Some drivers are
hot-pluggable (usb cams), some drivers register more than one v4l device
(bttv wants up to three).  Those drivers can't just call kfree() in the
->release callback because there might be other references, some open
file handle for a unplugged usb cam, another not-yet unregistered
v4l device, whatever.  Probably they must implement some reference
counting scheme to get that right.  The very simple ones (in
drivers/media/radio for example) likely just need the kfree() moved
from the ->remove function into the new ->release() callback.

Version (2) needs every v4l driver touched to make it compile again.
Not sure how hard it is to fixup them.  I'd guess it is pretty straigt
forward to do the conversion, it's just alot of work.  I also could do
a number of other cleanups along the way.  Maybe I trap into some hidden
pitfalls through, havn't looked at all the drivers in detail yet.

  Gerd

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Greg KH]

On Thu, Jul 17, 2003 at 06:37:15PM +0200, Gerd Knorr wrote:
> 
> Version (1) can be done without breaking the build, with a hack along 
> the lines "if (no release callback) printk(KERN_WARN please fix your
> driver)", so the drivers can be fixed step-by-step afterwards.

That sounds like a nice way to start.

> Fixing the drivers might be non-trivial through.  Some drivers are
> hot-pluggable (usb cams), some drivers register more than one v4l device
> (bttv wants up to three).  Those drivers can't just call kfree() in the
> ->release callback because there might be other references, some open
> file handle for a unplugged usb cam, another not-yet unregistered
> v4l device, whatever.  Probably they must implement some reference
> counting scheme to get that right.  The very simple ones (in
> drivers/media/radio for example) likely just need the kfree() moved
> from the ->remove function into the new ->release() callback.

I don't think it will be that bad for them.  Just have them change the
v4l device from being a structure included in their structure, into a
pointer, and then create it before registering, and free it in the
release() callback.

That being said, I haven't actually looked at the code to verify that
this is all that is needed :)

> Version (2) needs every v4l driver touched to make it compile again.
> Not sure how hard it is to fixup them.  I'd guess it is pretty straigt
> forward to do the conversion, it's just alot of work.  I also could do
> a number of other cleanups along the way.  Maybe I trap into some hidden
> pitfalls through, havn't looked at all the drivers in detail yet.

Breaking the build is a very good thing to do at times, to ensure that
stuff gets fixed properly.  Users might go for a while without realizing
that there really is a problem in their driver.

But in the end, it's up to you...

thanks,

greg k-h

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

On Thu, Jul 17, 2003 at 02:49:07PM -0700, Greg KH wrote:
> On Thu, Jul 17, 2003 at 06:37:15PM +0200, Gerd Knorr wrote:
> > 
> > Version (1) can be done without breaking the build, with a hack along 
> > the lines "if (no release callback) printk(KERN_WARN please fix your
> > driver)", so the drivers can be fixed step-by-step afterwards.
> 
> That sounds like a nice way to start.
> 
> > Fixing the drivers might be non-trivial through.  Some drivers are
> > hot-pluggable (usb cams), some drivers register more than one v4l device
> > (bttv wants up to three).  Those drivers can't just call kfree() in the
> > ->release callback because there might be other references, some open
> > file handle for a unplugged usb cam, another not-yet unregistered
> > v4l device, whatever.  Probably they must implement some reference
> > counting scheme to get that right.  The very simple ones (in
> > drivers/media/radio for example) likely just need the kfree() moved
> > from the ->remove function into the new ->release() callback.
> 
> I don't think it will be that bad for them.  Just have them change the
> v4l device from being a structure included in their structure, into a
> pointer, and then create it before registering, and free it in the
> release() callback.
> 
> That being said, I haven't actually looked at the code to verify that
> this is all that is needed :)
> 
> > Version (2) needs every v4l driver touched to make it compile again.
> > Not sure how hard it is to fixup them.  I'd guess it is pretty straigt
> > forward to do the conversion, it's just alot of work.  I also could do
> > a number of other cleanups along the way.  Maybe I trap into some hidden
> > pitfalls through, havn't looked at all the drivers in detail yet.
> 
> Breaking the build is a very good thing to do at times, to ensure that
> stuff gets fixed properly.  Users might go for a while without realizing
> that there really is a problem in their driver.
> 
> But in the end, it's up to you...
> 
> thanks,
> 
> greg k-h

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Gerd Knorr]

> Hm, it just looks like you quoted my message and didn't write anything
> new.  Did something not go through correctly?

:-/  ... I should learn to use my mailer correctly ...
Should have been that one:

==============================[ cut here ]==============================
To: Greg KH <greg@kroah.com>
Subject: Re: [RFC/PATCH] sysfs'ify video4linux
In-Reply-To: <20030717214907.GA3255@kroah.com>

> > Version (1) can be done without breaking the build, with a hack along 
> > the lines "if (no release callback) printk(KERN_WARN please fix your
> > driver)", so the drivers can be fixed step-by-step afterwards.
> 
> That sounds like a nice way to start.

> I don't think it will be that bad for them.  Just have them change the
> v4l device from being a structure included in their structure, into a
> pointer, and then create it before registering, and free it in the
> release() callback.

Good point.  So the mandatory ->release() callback version is also the
more flexible one.  I like that :)

> Breaking the build is a very good thing to do at times, to ensure that
> stuff gets fixed properly.  Users might go for a while without realizing
> that there really is a problem in their driver.
> 
> But in the end, it's up to you...

Breaking the build _right now_ with 2.6 becoming stable is IMHO not a
good idea, I think I better try to avoid that until 2.7.

New patch will come later today or early next week.

  Gerd

-- 
sigfault

Re: [RFC/PATCH] sysfs'ify video4linux

[Ronald Bultje]

Hi Gerd,

On Mon, 2003-07-21 at 09:28, Gerd Knorr wrote:
> Breaking the build _right now_ with 2.6 becoming stable is IMHO not a
> good idea, I think I better try to avoid that until 2.7.

The build is broken for quite some v4l drivers already (zr36120,
zr36067, the mpeg card, afaik), and really, the ones not in the kernel
will not officially "support" 2.6.x anyway before it's stable. It
shouldn't be that much work. I'd suggest to try to get it in before
2.6.0. This whole "we'll now turn number 5->6 and everything will be
stable" is there to help developers in releasing a new stable version,
not to prevent them from innovating and improving. I mean, this won't
actually be a totally new subsystem like the VM that needs to be
debugged in the next few months just after we've release 2.6.0, right?
It's just a small move. Fixing the drivers shouldn't be hard, and if
we/you ("we" as in the v4l driver developers) synchronize that nicely
with each other, there shouldn't be an actual problem.

(Related, I [finally] got 2.6.0-test1 to work so a new patch for my
driver is coming up in one of the next few days; please, someone kick it
forward to Linus then).

Ronald

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>

[RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core

[Gerd Knorr]

> New patch will come later today or early next week.

Here we go.  Changes:

 * dropped /proc support from videodev.c
 * added sysfs support to videodev.c
 * added a number of helper functions for v4l
   drivers.

v4l drivers will continue to work, but must be adapted to be
race-free[tm].  videodev.o will print a warning on not-yet
fixed drivers.

  Gerd

==============================[ cut here ]==============================
diff -u linux-2.6.0-test1/drivers/media/video/videodev.c linux/drivers/media/video/videodev.c
--- linux-2.6.0-test1/drivers/media/video/videodev.c	2003-07-21 11:49:26.000000000 +0200
+++ linux/drivers/media/video/videodev.c	2003-07-21 15:03:36.000000000 +0200
@@ -15,7 +15,6 @@
  *		- Added procfs support
  */
 
-#include <linux/config.h>
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -28,6 +27,7 @@
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 #include <linux/devfs_fs_kernel.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -35,33 +35,67 @@
 
 #include <linux/videodev.h>
 
-#define VIDEO_NUM_DEVICES	256 
+#define VIDEO_NUM_DEVICES	256
+#define VIDEO_NAME              "video4linux"
 
 /*
- *	Active devices 
+ *	sysfs stuff
  */
- 
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DECLARE_MUTEX(videodev_lock);
 
+static ssize_t show_name(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = to_video_device(cd);
+	return sprintf(buf,"%.*s\n",(int)sizeof(vfd->name),vfd->name);
+}
+
+static ssize_t show_dev(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = to_video_device(cd);
+	dev_t dev = MKDEV(VIDEO_MAJOR, vfd->minor);
+	return sprintf(buf,"%04x\n",(int)dev);
+}
 
-#ifdef CONFIG_VIDEO_PROC_FS
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static CLASS_DEVICE_ATTR(dev,  S_IRUGO, show_dev, NULL);
 
-#include <linux/proc_fs.h>
+struct video_device *video_device_alloc(void)
+{
+	struct video_device *vfd;
 
-struct videodev_proc_data {
-	struct list_head proc_list;
-	char name[16];
-	struct video_device *vdev;
-	struct proc_dir_entry *proc_entry;
-};
+	vfd = kmalloc(sizeof(*vfd),GFP_KERNEL);
+	if (NULL == vfd)
+		return NULL;
+	memset(vfd,0,sizeof(*vfd));
+	return vfd;
+}
+
+void video_device_release(struct video_device *vfd)
+{
+	kfree(vfd);
+}
+
+static void video_release(struct class_device *cd)
+{
+	struct video_device *vfd = container_of(cd, struct video_device, class_dev);
 
-static struct proc_dir_entry *video_dev_proc_entry = NULL;
-struct proc_dir_entry *video_proc_entry = NULL;
-EXPORT_SYMBOL(video_proc_entry);
-LIST_HEAD(videodev_proc_list);
+#if 1 /* needed until all drivers are fixed */
+	if (!vfd->release)
+		return;
+#endif
+	vfd->release(vfd);
+}
 
-#endif /* CONFIG_VIDEO_PROC_FS */
+static struct class video_class = {
+        .name    = VIDEO_NAME,
+	.release = video_release,
+};
+
+/*
+ *	Active devices 
+ */
+ 
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DECLARE_MUTEX(videodev_lock);
 
 struct video_device* video_devdata(struct file *file)
 {
@@ -219,156 +253,6 @@
 	return 0;
 }
 
-/*
- *	/proc support
- */
-
-#ifdef CONFIG_VIDEO_PROC_FS
-
-/* Hmm... i'd like to see video_capability information here, but
- * how can I access it (without changing the other drivers? -claudio
- */
-static int videodev_proc_read(char *page, char **start, off_t off,
-			       int count, int *eof, void *data)
-{
-	char *out = page;
-	struct video_device *vfd = data;
-	struct videodev_proc_data *d;
-	struct list_head *tmp;
-	int len;
-	char c = ' ';
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev)
-			break;
-	}
-
-	/* Sanity check */
-	if (tmp == &videodev_proc_list)
-		goto skip;
-		
-#define PRINT_VID_TYPE(x) do { if (vfd->type & x) \
-	out += sprintf (out, "%c%s", c, #x); c='|';} while (0)
-
-	out += sprintf (out, "name            : %s\n", vfd->name);
-	out += sprintf (out, "type            :");
-		PRINT_VID_TYPE(VID_TYPE_CAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_TUNER);
-		PRINT_VID_TYPE(VID_TYPE_TELETEXT);
-		PRINT_VID_TYPE(VID_TYPE_OVERLAY);
-		PRINT_VID_TYPE(VID_TYPE_CHROMAKEY);
-		PRINT_VID_TYPE(VID_TYPE_CLIPPING);
-		PRINT_VID_TYPE(VID_TYPE_FRAMERAM);
-		PRINT_VID_TYPE(VID_TYPE_SCALES);
-		PRINT_VID_TYPE(VID_TYPE_MONOCHROME);
-		PRINT_VID_TYPE(VID_TYPE_SUBCAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_ENCODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_ENCODER);
-	out += sprintf (out, "\n");
-	out += sprintf (out, "hardware        : 0x%x\n", vfd->hardware);
-#if 0
-	out += sprintf (out, "channels        : %d\n", d->vcap.channels);
-	out += sprintf (out, "audios          : %d\n", d->vcap.audios);
-	out += sprintf (out, "maxwidth        : %d\n", d->vcap.maxwidth);
-	out += sprintf (out, "maxheight       : %d\n", d->vcap.maxheight);
-	out += sprintf (out, "minwidth        : %d\n", d->vcap.minwidth);
-	out += sprintf (out, "minheight       : %d\n", d->vcap.minheight);
-#endif
-
-skip:
-	len = out - page;
-	len -= off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0)
-			return 0;
-	} else
-		len = count;
-
-	*start = page + off;
-
-	return len;
-}
-
-static void videodev_proc_create(void)
-{
-	video_proc_entry = create_proc_entry("video", S_IFDIR, &proc_root);
-
-	if (video_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video\n");
-		return;
-	}
-
-	video_proc_entry->owner = THIS_MODULE;
-	video_dev_proc_entry = create_proc_entry("dev", S_IFDIR, video_proc_entry);
-
-	if (video_dev_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video/dev\n");
-		return;
-	}
-
-	video_dev_proc_entry->owner = THIS_MODULE;
-}
-
-static void __exit videodev_proc_destroy(void)
-{
-	if (video_dev_proc_entry != NULL)
-		remove_proc_entry("dev", video_proc_entry);
-
-	if (video_proc_entry != NULL)
-		remove_proc_entry("video", &proc_root);
-}
-
-static void videodev_proc_create_dev (struct video_device *vfd, char *name)
-{
-	struct videodev_proc_data *d;
-	struct proc_dir_entry *p;
-
-	if (video_dev_proc_entry == NULL)
-		return;
-
-	d = kmalloc (sizeof (struct videodev_proc_data), GFP_KERNEL);
-	if (!d)
-		return;
-
-	p = create_proc_entry(name, S_IFREG|S_IRUGO|S_IWUSR, video_dev_proc_entry);
-	if (!p) {
-		kfree(d);
-		return;
-	}
-	p->data = vfd;
-	p->read_proc = videodev_proc_read;
-
-	d->proc_entry = p;
-	d->vdev = vfd;
-	strcpy (d->name, name);
-
-	/* How can I get capability information ? */
-
-	list_add (&d->proc_list, &videodev_proc_list);
-}
-
-static void videodev_proc_destroy_dev (struct video_device *vfd)
-{
-	struct list_head *tmp;
-	struct videodev_proc_data *d;
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev) {
-			remove_proc_entry(d->name, video_dev_proc_entry);
-			list_del (&d->proc_list);
-			kfree(d);
-			break;
-		}
-	}
-}
-
-#endif /* CONFIG_VIDEO_PROC_FS */
-
 extern struct file_operations video_fops;
 
 /**
@@ -456,15 +340,23 @@
 	devfs_mk_cdev(MKDEV(VIDEO_MAJOR, vfd->minor),
 			S_IFCHR | S_IRUSR | S_IWUSR, vfd->devfs_name);
 	init_MUTEX(&vfd->lock);
-	
-#ifdef CONFIG_VIDEO_PROC_FS
-{
-	char name[16];
-	sprintf(name, "%s%d", name_base, i - base);
-	videodev_proc_create_dev(vfd, name);
-}
-#endif
 
+	/* sysfs class */
+        memset(&vfd->class_dev, 0x00, sizeof(vfd->class_dev));
+	if (vfd->dev)
+		vfd->class_dev.dev = vfd->dev;
+	vfd->class_dev.class       = &video_class;
+	strlcpy(vfd->class_dev.class_id, vfd->devfs_name + 4, BUS_ID_SIZE);
+	class_device_register(&vfd->class_dev);
+	video_device_create_file(vfd, &class_device_attr_name);
+	video_device_create_file(vfd, &class_device_attr_dev);
+
+#if 1 /* needed until all drivers are fixed */
+	if (!vfd->release)
+		printk(KERN_WARNING "videodev: \"%s\" has no release callback. "
+		       "Please fix your driver for proper sysfs support, see "
+		       "http://lwn.net/Articles/36850/\n", vfd->name);
+#endif
 	return 0;
 }
 
@@ -482,10 +374,7 @@
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");
 
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy_dev (vfd);
-#endif
-
+	class_device_unregister(&vfd->class_dev);
 	devfs_remove(vfd->devfs_name);
 	video_device[vfd->minor]=NULL;
 	up(&videodev_lock);
@@ -506,24 +395,18 @@
 static int __init videodev_init(void)
 {
 	printk(KERN_INFO "Linux video capture interface: v1.00\n");
-	if (register_chrdev(VIDEO_MAJOR,"video_capture", &video_fops)) {
+	if (register_chrdev(VIDEO_MAJOR,VIDEO_NAME, &video_fops)) {
 		printk("video_dev: unable to get major %d\n", VIDEO_MAJOR);
 		return -EIO;
 	}
-
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_create ();
-#endif
-	
+	class_register(&video_class);
 	return 0;
 }
 
 static void __exit videodev_exit(void)
 {
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy ();
-#endif
-	unregister_chrdev(VIDEO_MAJOR, "video_capture");
+	class_unregister(&video_class);
+	unregister_chrdev(VIDEO_MAJOR, VIDEO_NAME);
 }
 
 module_init(videodev_init)
@@ -535,6 +418,8 @@
 EXPORT_SYMBOL(video_usercopy);
 EXPORT_SYMBOL(video_exclusive_open);
 EXPORT_SYMBOL(video_exclusive_release);
+EXPORT_SYMBOL(video_device_alloc);
+EXPORT_SYMBOL(video_device_release);
 
 MODULE_AUTHOR("Alan Cox");
 MODULE_DESCRIPTION("Device registrar for Video4Linux drivers");
diff -u linux-2.6.0-test1/include/linux/videodev.h linux/include/linux/videodev.h
--- linux-2.6.0-test1/include/linux/videodev.h	2003-07-21 11:47:48.000000000 +0200
+++ linux/include/linux/videodev.h	2003-07-21 14:53:28.000000000 +0200
@@ -3,18 +3,10 @@
 
 #include <linux/types.h>
 #include <linux/version.h>
+#include <linux/device.h>
 
-#if 1
-/*
- * v4l2 is still work-in-progress, integration planed for 2.5.x
- *   documentation:           http://bytesex.org/v4l/
- *   patches available from:  http://bytesex.org/patches/
- */ 
-# define HAVE_V4L2 1
-# include <linux/videodev2.h>
-#else
-# undef HAVE_V4L2
-#endif
+#define HAVE_V4L2 1
+#include <linux/videodev2.h>
 
 #ifdef __KERNEL__
 
@@ -23,35 +15,70 @@
 
 struct video_device
 {
-	struct module *owner;
+	/* device info */
+	struct device *dev;
      	char name[32];
  	int type;       /* v4l1 */
  	int type2;      /* v4l2 */
 	int hardware;
 	int minor;
 
- 	/* new interface -- we will use file_operations directly
- 	 * like soundcore does. */
+	/* device ops + callbacks */
  	struct file_operations *fops;
-	void *priv;		/* Used to be 'private' but that upsets C++ */
+	void (*release)(struct video_device *vfd);
+
+#if 1 /* to be removed in 2.7.x */
+	/* obsolete -- fops->owner is used instead */
+	struct module *owner;
+	/* dev->driver_data will be used instead some day.
+	 * Use the video_{get|set}_drvdata() helper functions,
+	 * so the switch over will be transparent for you.
+	 * Or use {pci|usb|dev}_{get|set}_drvdata() directly. */
+	void *priv;
+#endif
 
-	/* for videodev.c intenal usage -- don't touch */
-	int users;
-	struct semaphore lock;
-	char devfs_name[64];	/* devfs */
+	/* for videodev.c intenal usage -- please don't touch */
+	int users;                     /* video_exclusive_{open|close} ... */
+	struct semaphore lock;         /* ... helper function uses these   */
+	char devfs_name[64];	       /* devfs */
+	struct class_device class_dev; /* sysfs */
 };
 
 #define VIDEO_MAJOR	81
-extern int video_register_device(struct video_device *, int type, int nr);
 
 #define VFL_TYPE_GRABBER	0
 #define VFL_TYPE_VBI		1
 #define VFL_TYPE_RADIO		2
 #define VFL_TYPE_VTX		3
 
+extern int video_register_device(struct video_device *, int type, int nr);
 extern void video_unregister_device(struct video_device *);
 extern struct video_device* video_devdata(struct file*);
 
+#define to_video_device(cd) container_of(cd, struct video_device, class_dev)
+static inline void
+video_device_create_file(struct video_device *vfd,
+			 struct class_device_attribute *attr)
+{
+	class_device_create_file(&vfd->class_dev, attr);
+}
+
+/* helper functions to alloc / release struct video_device, the
+   later can be used for video_device->release() */
+struct video_device *video_device_alloc(void);
+void video_device_release(struct video_device *vfd);
+
+/* helper functions to access driver private data. */
+static inline void *video_get_drvdata(struct video_device *dev)
+{
+	return dev->priv;
+}
+
+static inline void video_set_drvdata(struct video_device *dev, void *data)
+{
+	dev->priv = data;
+}
+
 extern int video_exclusive_open(struct inode *inode, struct file *file);
 extern int video_exclusive_release(struct inode *inode, struct file *file);
 extern int video_usercopy(struct inode *inode, struct file *file,

[RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver

[Gerd Knorr]

  Hi,

This patch adapts the bttv driver to the recent videodev changes.
struct video_device is now dynamically allocated and freed on
->release() to fix sysfs race.  Also adds a private sysfs driver
attribute file.

  Gerd

==============================[ cut here ]==============================
diff -u linux-2.6.0-test1/drivers/media/video/bttv-cards.c linux/drivers/media/video/bttv-cards.c
--- linux-2.6.0-test1/drivers/media/video/bttv-cards.c	2003-07-21 11:49:42.000000000 +0200
+++ linux/drivers/media/video/bttv-cards.c	2003-07-21 11:49:43.000000000 +0200
@@ -1851,12 +1851,8 @@
 		btv->type=card[btv->nr];
 	
 	/* print which card config we are using */
-	sprintf(btv->video_dev.name,"BT%d%s(%.23s)",
-		btv->id,
-		(btv->id==848 && btv->revision==0x12) ? "A" : "",
-		bttv_tvcards[btv->type].name);
 	printk(KERN_INFO "bttv%d: using: %s [card=%d,%s]\n",btv->nr,
-	       btv->video_dev.name,btv->type,
+	       bttv_tvcards[btv->type].name, btv->type,
 	       card[btv->nr] < bttv_num_tvcards
 	       ? "insmod option" : "autodetected");
 
diff -u linux-2.6.0-test1/drivers/media/video/bttv-driver.c linux/drivers/media/video/bttv-driver.c
--- linux-2.6.0-test1/drivers/media/video/bttv-driver.c	2003-07-21 11:49:42.000000000 +0200
+++ linux/drivers/media/video/bttv-driver.c	2003-07-21 17:08:32.183882024 +0200
@@ -32,6 +32,7 @@
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/kdev_t.h>
+#include <linux/device.h>
 
 #include <asm/io.h>
 #include <asm/byteorder.h>
@@ -123,6 +124,17 @@
 #endif
 
 /* ----------------------------------------------------------------------- */
+/* sysfs                                                                   */
+
+static ssize_t show_card(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = to_video_device(cd);
+	struct bttv *btv = dev_get_drvdata(vfd->dev);
+	return sprintf(buf, "%d\n", btv ? btv->type : UNSET);
+}
+static CLASS_DEVICE_ATTR(card, S_IRUGO, show_card, NULL);
+
+/* ----------------------------------------------------------------------- */
 /* static data                                                             */
 
 /* special timing tables from conexant... */
@@ -2039,7 +2051,7 @@
                 struct video_capability *cap = arg;
 
 		memset(cap,0,sizeof(*cap));
-                strcpy(cap->name,btv->video_dev.name);
+                strcpy(cap->name,btv->video_dev->name);
 		if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type) {
 			/* vbi */
 			cap->type = VID_TYPE_TUNER|VID_TYPE_TELETEXT;
@@ -2390,7 +2402,7 @@
 		if (0 == v4l2)
 			return -EINVAL;
                 strcpy(cap->driver,"bttv");
-                strlcpy(cap->card,btv->video_dev.name,sizeof(cap->card));
+                strlcpy(cap->card,btv->video_dev->name,sizeof(cap->card));
 		sprintf(cap->bus_info,"PCI:%s",btv->dev->slot_name);
 		cap->version = BTTV_VERSION_CODE;
 		cap->capabilities =
@@ -2767,12 +2779,12 @@
 	dprintk(KERN_DEBUG "bttv: open minor=%d\n",minor);
 
 	for (i = 0; i < bttv_num; i++) {
-		if (bttvs[i].video_dev.minor == minor) {
+		if (bttvs[i].video_dev->minor == minor) {
 			btv = &bttvs[i];
 			type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 			break;
 		}
-		if (bttvs[i].vbi_dev.minor == minor) {
+		if (bttvs[i].vbi_dev->minor == minor) {
 			btv = &bttvs[i];
 			type = V4L2_BUF_TYPE_VBI_CAPTURE;
 			break;
@@ -2873,8 +2885,8 @@
 static struct video_device bttv_video_template =
 {
 	.name     = "UNSET",
-	type:     VID_TYPE_CAPTURE|VID_TYPE_TUNER|VID_TYPE_OVERLAY|
-	          VID_TYPE_CLIPPING|VID_TYPE_SCALES,
+	.type     = VID_TYPE_CAPTURE|VID_TYPE_TUNER|VID_TYPE_OVERLAY|
+	            VID_TYPE_CLIPPING|VID_TYPE_SCALES,
 	.hardware = VID_HARDWARE_BT848,
 	.fops     = &bttv_fops,
 	.minor    = -1,
@@ -2902,7 +2914,7 @@
 	dprintk("bttv: open minor=%d\n",minor);
 
 	for (i = 0; i < bttv_num; i++) {
-		if (bttvs[i].radio_dev.minor == minor) {
+		if (bttvs[i].radio_dev->minor == minor) {
 			btv = &bttvs[i];
 			break;
 		}
@@ -2947,7 +2959,7 @@
                 struct video_capability *cap = arg;
 
 		memset(cap,0,sizeof(*cap));
-                strcpy(cap->name,btv->radio_dev.name);
+                strcpy(cap->name,btv->radio_dev->name);
                 cap->type = VID_TYPE_TUNER;
 		cap->channels = 1;
 		cap->audios = 1;
@@ -3337,30 +3349,83 @@
 /* ----------------------------------------------------------------------- */
 /* initialitation                                                          */
 
+static struct video_device *vdev_init(struct bttv *btv,
+				      struct video_device *template,
+				      char *type)
+{
+	struct video_device *vfd;
+
+	vfd = video_device_alloc();
+	if (NULL == vfd)
+		return NULL;
+	*vfd = *template;
+	vfd->minor   = -1;
+	vfd->release = video_device_release;
+	vfd->dev     = &btv->dev->dev;
+	snprintf(vfd->name, sizeof(vfd->name), "BT%d%s %s (%s)",
+		 btv->id, (btv->id==848 && btv->revision==0x12) ? "A" : "",
+		 type, bttv_tvcards[btv->type].name);
+	return vfd;
+}
+
 /* register video4linux devices */
 static int __devinit bttv_register_video(struct bttv *btv)
 {
-        if(video_register_device(&btv->video_dev,VFL_TYPE_GRABBER,video_nr)<0)
-                return -1;
+	/* video */
+	btv->video_dev = vdev_init(btv, &bttv_video_template, "video");
+        if (NULL == btv->video_dev)
+		goto err;
+	if (video_register_device(btv->video_dev,VFL_TYPE_GRABBER,video_nr)<0)
+		goto err;
 	printk(KERN_INFO "bttv%d: registered device video%d\n",
-	       btv->nr,btv->video_dev.minor & 0x1f);
+	       btv->nr,btv->video_dev->minor & 0x1f);
+	video_device_create_file(btv->video_dev, &class_device_attr_card);
 
-        if(video_register_device(&btv->vbi_dev,VFL_TYPE_VBI,vbi_nr)<0) {
-                video_unregister_device(&btv->video_dev);
-                return -1;
-        }
+	/* vbi */
+	btv->vbi_dev = vdev_init(btv, &bttv_vbi_template, "vbi");
+        if (NULL == btv->vbi_dev)
+		goto err;
+        if (video_register_device(btv->vbi_dev,VFL_TYPE_VBI,vbi_nr)<0)
+		goto err;
 	printk(KERN_INFO "bttv%d: registered device vbi%d\n",
-	       btv->nr,btv->vbi_dev.minor & 0x1f);
+	       btv->nr,btv->vbi_dev->minor & 0x1f);
 
         if (!btv->has_radio)
 		return 0;
-	if (video_register_device(&btv->radio_dev, VFL_TYPE_RADIO,radio_nr)<0) {
-		video_unregister_device(&btv->vbi_dev);
-		video_unregister_device(&btv->video_dev);
-		return -1;
-        }
+	/* radio */
+	btv->radio_dev = vdev_init(btv, &radio_template, "radio");
+        if (NULL == btv->radio_dev)
+		goto err;
+	if (video_register_device(btv->radio_dev, VFL_TYPE_RADIO,radio_nr)<0)
+		goto err;
 	printk(KERN_INFO "bttv%d: registered device radio%d\n",
-	       btv->nr,btv->radio_dev.minor & 0x1f);
+	       btv->nr,btv->radio_dev->minor & 0x1f);
+
+	/* all done */
+	return 0;
+
+ err:
+	if (btv->video_dev) {
+		if (-1 != btv->video_dev->minor)
+			video_unregister_device(btv->video_dev);
+		else
+			video_device_release(btv->video_dev);
+		btv->video_dev = NULL;
+	}
+	if (btv->vbi_dev) {
+		if (-1 != btv->vbi_dev->minor)
+			video_unregister_device(btv->vbi_dev);
+		else
+			video_device_release(btv->vbi_dev);
+		btv->vbi_dev = NULL;
+	}
+	if (btv->radio_dev) {
+		if (-1 != btv->radio_dev->minor)
+			video_unregister_device(btv->radio_dev);
+		else
+			video_device_release(btv->radio_dev);
+		btv->radio_dev = NULL;
+	}
         return 0;
 }
 
@@ -3408,16 +3473,6 @@
         btv->i2c_rc = -1;
         btv->tuner_type  = UNSET;
         btv->pinnacle_id = UNSET;
-
-	memcpy(&btv->video_dev, &bttv_video_template, sizeof(bttv_video_template));
-	memcpy(&btv->radio_dev, &radio_template,      sizeof(radio_template));
-	memcpy(&btv->vbi_dev,   &bttv_vbi_template,   sizeof(bttv_vbi_template));
-        btv->video_dev.minor = -1;
-	btv->video_dev.priv = btv;
-        btv->radio_dev.minor = -1;
-	btv->radio_dev.priv = btv;
-        btv->vbi_dev.minor = -1;
-	btv->vbi_dev.priv = btv;
 	btv->has_radio=radio[btv->nr];
 	
 	/* pci stuff (init, get irq/mmip, ... */
@@ -3576,12 +3631,18 @@
 		i2c_bit_del_bus(&btv->i2c_adap);
 
 	/* unregister video4linux */
-        if (btv->video_dev.minor!=-1)
-                video_unregister_device(&btv->video_dev);
-        if (btv->radio_dev.minor!=-1)
-                video_unregister_device(&btv->radio_dev);
-	if (btv->vbi_dev.minor!=-1)
-                video_unregister_device(&btv->vbi_dev);
+        if (btv->video_dev) {
+                video_unregister_device(btv->video_dev);
+		btv->video_dev = NULL;
+	}
+        if (btv->radio_dev) {
+                video_unregister_device(btv->radio_dev);
+		btv->radio_dev = NULL;
+	}
+	if (btv->vbi_dev) {
+                video_unregister_device(btv->vbi_dev);
+		btv->vbi_dev = NULL;
+	}
 
 	/* free allocated memory */
 	btcx_riscmem_free(btv->dev,&btv->main);
diff -u linux-2.6.0-test1/drivers/media/video/bttvp.h linux/drivers/media/video/bttvp.h
--- linux-2.6.0-test1/drivers/media/video/bttvp.h	2003-07-21 11:49:42.000000000 +0200
+++ linux/drivers/media/video/bttvp.h	2003-07-21 11:49:43.000000000 +0200
@@ -276,9 +276,9 @@
 	int                        i2c_state, i2c_rc;
 
 	/* video4linux (1) */
-	struct video_device video_dev;
-	struct video_device radio_dev;
-	struct video_device vbi_dev;
+	struct video_device *video_dev;
+	struct video_device *radio_dev;
+	struct video_device *vbi_dev;
 
 	/* locking */
 	spinlock_t s_lock;

Re: [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core

[Greg KH]

On Mon, Jul 21, 2003 at 05:43:40PM +0200, Gerd Knorr wrote:
> > New patch will come later today or early next week.
> 
> Here we go.  Changes:
> 
>  * dropped /proc support from videodev.c
>  * added sysfs support to videodev.c
>  * added a number of helper functions for v4l
>    drivers.
> 
> v4l drivers will continue to work, but must be adapted to be
> race-free[tm].  videodev.o will print a warning on not-yet
> fixed drivers.

Very nice, looks good to me.  Thanks for putting up with the lack of
documentation for some of this :)

thanks,

greg k-h