On Friday 14 September 2007 14:50:11 Mauro Carvalho Chehab wrote: > > > > This patch is really ugly. > > > > Well, yes. I should have known as I converted so many occurences of > > to_video_device to container_of in my second patch. > > > > > > Why can't the "to_video_device()" macro be used? Just move it to a > > > > place where it's usable! IOW, what's wrong with the *much* simpler > > > > patch below? > > > > > > There's nothing wtong in my opinion. I do not know the exact reason why > > > Mauro moved "to_video_device()" into CONFIG_VIDEO_V4L1_COMPAT. Pheraps > > > he can give more details about this change. > > > > BTW, just a few V4L2 drivers and videodev seem to use this construct: > > video/usbvision/usbvision-video.c: container_of(cd, struct > > video_device, class_dev); > > > > video/sn9c102/sn9c102_core.c: cam = > > video_get_drvdata(container_of(cd, struct video_device, > > video/sn9c102/sn9c102_core.c- > > class_dev)); > > > > video/videodev.c: struct video_device *vfd = container_of(cd, > > struct video_device, video/videodev.c- > > class_dev); > > > > And then their are drivers with other variants of container_of, e.g.: > > video/pvrusb2/pvrusb2-v4l2.c: vp = container_of(chp,struct > > pvr2_v4l2,channel); video/bt8xx/bttv-vbi.c: struct bttv_buffer *buf = > > container_of(vb,struct bttv_buffer,vb); ... > > > > I think, there should be a to_video_device macro for usage in v4l2. > > An most probable for the other container_of stuff (when more there is > > more than one occurence of a particular construct), drivers should > > provide their own macro, e.g. to_video_buffer() or so. > > > > That's what other subsystems do. It is more self-explanatory than a > > direct usage of container_of. > > > > So why not apply (my first patch ... oh no, of course that's rubbish ;-) > > Linus' below patch for 2.6.23 to fix the compilation for that particular > > driver. And to work on the conversion of container_of() stuff to > > meaningful macros for the next kernel release? > > The to_video_device and the container_of (cd, struct video_device, > class_dev) (as you noticed at the above drivers) are used to provide > procfs interface. > > On videodev, there's the v4l2 core stuff, used on all V4L drivers. It > allows some control to the V4L devices (basically, see/change the > modprobe loading parameters). > > The other drivers that uses to_video_device (or the container_of > alternative) to create other userspace interfaces, specific to each > driver and not documented at V4L2 API: > > bttv-driver.c:static CLASS_DEVICE_ATTR(card, S_IRUGO, show_card, NULL); > et61x251_core.c:static CLASS_DEVICE_ATTR(reg, S_IRUGO | S_IWUSR, > et61x251_core.c:static CLASS_DEVICE_ATTR(val, S_IRUGO | S_IWUSR, > et61x251_core.c:static CLASS_DEVICE_ATTR(i2c_reg, S_IRUGO | S_IWUSR, > et61x251_core.c:static CLASS_DEVICE_ATTR(i2c_val, S_IRUGO | S_IWUSR, > ov511.c:static CLASS_DEVICE_ATTR(custom_id, S_IRUGO, show_custom_id, NULL); > ov511.c:static CLASS_DEVICE_ATTR(model, S_IRUGO, show_model, NULL); > ov511.c:static CLASS_DEVICE_ATTR(bridge, S_IRUGO, show_bridge, NULL); > ov511.c:static CLASS_DEVICE_ATTR(sensor, S_IRUGO, show_sensor, NULL); > ov511.c:static CLASS_DEVICE_ATTR(brightness, S_IRUGO, show_brightness, > NULL); ov511.c:static CLASS_DEVICE_ATTR(saturation, S_IRUGO, > show_saturation, NULL); ov511.c:static CLASS_DEVICE_ATTR(contrast, S_IRUGO, > show_contrast, NULL); ov511.c:static CLASS_DEVICE_ATTR(hue, S_IRUGO, > show_hue, NULL); > ov511.c:static CLASS_DEVICE_ATTR(exposure, S_IRUGO, show_exposure, NULL); > pwc-if.c:static CLASS_DEVICE_ATTR(pan_tilt, S_IRUGO | S_IWUSR, > show_pan_tilt, pwc-if.c:static CLASS_DEVICE_ATTR(button, S_IRUGO | S_IWUSR, > show_snapshot_button_status, sn9c102_core.c:static CLASS_DEVICE_ATTR(reg, > S_IRUGO | S_IWUSR, > sn9c102_core.c:static CLASS_DEVICE_ATTR(val, S_IRUGO | S_IWUSR, > sn9c102_core.c:static CLASS_DEVICE_ATTR(i2c_reg, S_IRUGO | S_IWUSR, > sn9c102_core.c:static CLASS_DEVICE_ATTR(i2c_val, S_IRUGO | S_IWUSR, > sn9c102_core.c:static CLASS_DEVICE_ATTR(green, S_IWUGO, NULL, > sn9c102_store_green); sn9c102_core.c:static CLASS_DEVICE_ATTR(blue, > S_IWUGO, NULL, sn9c102_store_blue); sn9c102_core.c:static > CLASS_DEVICE_ATTR(red, S_IWUGO, NULL, sn9c102_store_red); > sn9c102_core.c:static CLASS_DEVICE_ATTR(frame_header, S_IRUGO, > stv680.c:static CLASS_DEVICE_ATTR(name, S_IRUGO, show_##name, NULL); > usbvision-video.c:static CLASS_DEVICE_ATTR(version, S_IRUGO, show_version, > NULL); usbvision-video.c:static CLASS_DEVICE_ATTR(model, S_IRUGO, > show_model, NULL); usbvision-video.c:static CLASS_DEVICE_ATTR(hue, S_IRUGO, > show_hue, NULL); usbvision-video.c:static CLASS_DEVICE_ATTR(contrast, > S_IRUGO, show_contrast, NULL); usbvision-video.c:static > CLASS_DEVICE_ATTR(brightness, S_IRUGO, show_brightness, NULL); > usbvision-video.c:static CLASS_DEVICE_ATTR(saturation, S_IRUGO, > show_saturation, NULL); usbvision-video.c:static > CLASS_DEVICE_ATTR(streaming, S_IRUGO, show_streaming, NULL); > usbvision-video.c:static CLASS_DEVICE_ATTR(compression, S_IRUGO, > show_compression, NULL); usbvision-video.c:static CLASS_DEVICE_ATTR(bridge, > S_IRUGO, show_device_bridge, NULL); > > From the above, you can clearly see that et62x251 and s9c102 provides an > interface to directly change a register at the device. The other drivers > allows reading/changing a few controls directly via /proc (this is also > possible, using V4L2 interface). This is not recommended, since V4L2 API > should be the proper way to control the devices.
through ioctl()? It's not as immediate and safe as controlling the device registers through /sysfs (not /proc). However, the sysfs interface in those drivers appeared before V4L2 had its own ioctls and we agreed to keep and export the interface to the only users selecting CONFIG_VIDEO_ADV_DEBUG (ok, this is actually valid for the sn9c102, I'll submit a patch for the et61x251 in the future). > From my POV, a driver that is creating its own userspace API is not > fully compliant with V4L2 API. Isn't Video4Linux2 ioctl() supersedeing sysfs in this case? > Cheers, > Mauro Best regards Luca Risolia - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/