Re: [PATCH] v4l: fix build error for et61x251 driver

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/