Hello Alexey,
On Tue, 31 Mar 2009 16:14:46 +0400
Alexey Klimov <klimov.linux@xxxxxxxxx> wrote:
> Hello, Hans
> Thanks for review.
>
> On Tue, Mar 31, 2009 at 2:13 AM, Hans Verkuil <hverkuil@xxxxxxxxx>
> wrote:
> >>
> >> I'm still confused about messages like v4l2_err and about
> >> unplugging procedure.
> >
> > For a simple device like this unregistering the v4l2_device in the
> > disconnect is OK. Although the best method would be to call
> > v4l2_device_disconnect() in the disconnect function and postpone
> > the v4l2_device_unregister() until the
> > usb_dsbr100_video_device_release function.
>
> I switched to v4l2_device_disconnect. Looks good, thanks.
>
> > What is really missing in the v4l2 core is a release function that
> > is called when the last video device node is closed. The
> > video_release function is only called when that particular video
> > device is released, but for drivers that open multiple device nodes
> > you still have to put in administration to wait until really the
> > last user of any device node disappears.
> >
> > I might add a feature like that to the v4l2 core in the future. It
> > shouldn't be too hard.
> >
> > Anyway, that's not relevant for a simple USB radio device :-)
> >
> > Re: the v4l2_err functions: v4l2_device_register sets up a standard
> > unique driver prefix that can be used for logging. Since the 'name'
> > can be overwritten by the driver you have more flexibility than the
> > standard dev_info functions. I also have some ideas on how to
> > improve there functions.
>
> Well, my question was - should i remove dev_ messages and use v4l2_
> messages everywhere in driver ?
>
> >> I tested it with my radio device and it works(unplugging works also
> >> without oopses).
> >> Douglas, if you can find some free time to test patch it will be
> >> very good too :)
> >>
> >> --
> >> diff -r f86c84534cb4 linux/drivers/media/radio/dsbr100.c
> >> --- a/linux/drivers/media/radio/dsbr100.c Sun Mar 29 22:54:35
> >> 2009 -0300 +++ b/linux/drivers/media/radio/dsbr100.c Mon Mar
> >> 30 21:00:51 2009 +0400 @@ -32,6 +32,9 @@
> >> Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> >> 02111-1307 USA
> >>
> >> History:
> >> +
> >> + Version 0.45:
> >> + Converted to v4l2_device.
> >>
> >> Version 0.44:
> >> Add suspend/resume functions, fix unplug of device,
> >> @@ -88,7 +91,7 @@
> >> #include <linux/slab.h>
> >> #include <linux/input.h>
> >> #include <linux/videodev2.h>
> >> -#include <media/v4l2-common.h>
> >> +#include <media/v4l2-device.h>
> >> #include <media/v4l2-ioctl.h>
> >> #include <linux/usb.h>
> >> #include "compat.h"
> >> @@ -98,39 +101,8 @@
> >> */
> >> #include <linux/version.h> /* for KERNEL_VERSION MACRO */
> >>
> >> -#define DRIVER_VERSION "v0.44"
> >> -#define RADIO_VERSION KERNEL_VERSION(0, 4, 4)
> >> -
> >> -static struct v4l2_queryctrl radio_qctrl[] = {
> >> - {
> >> - .id = V4L2_CID_AUDIO_MUTE,
> >> - .name = "Mute",
> >> - .minimum = 0,
> >> - .maximum = 1,
> >> - .default_value = 1,
> >> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> >> - },
> >> -/* HINT: the disabled controls are only here to satify kradio and
> >> such apps */
> >> - { .id = V4L2_CID_AUDIO_VOLUME,
> >> - .flags = V4L2_CTRL_FLAG_DISABLED,
> >> - },
> >> - {
> >> - .id = V4L2_CID_AUDIO_BALANCE,
> >> - .flags = V4L2_CTRL_FLAG_DISABLED,
> >> - },
> >> - {
> >> - .id = V4L2_CID_AUDIO_BASS,
> >> - .flags = V4L2_CTRL_FLAG_DISABLED,
> >> - },
> >> - {
> >> - .id = V4L2_CID_AUDIO_TREBLE,
> >> - .flags = V4L2_CTRL_FLAG_DISABLED,
> >> - },
> >> - {
> >> - .id = V4L2_CID_AUDIO_LOUDNESS,
> >> - .flags = V4L2_CTRL_FLAG_DISABLED,
> >> - },
> >> -};
> >> +#define DRIVER_VERSION "v0.45"
> >> +#define RADIO_VERSION KERNEL_VERSION(0, 4, 5)
> >>
> >> #define DRIVER_AUTHOR "Markus Demleitner
> >> <msdemlei@xxxxxxxxxxxxxxxxxx>" #define DRIVER_DESC "D-Link
> >> DSB-R100 USB FM radio driver" @@ -168,6 +140,8 @@
> >> struct dsbr100_device {
> >> struct usb_device *usbdev;
> >> struct video_device videodev;
> >> + struct v4l2_device v4l2_dev;
> >> +
> >> u8 *transfer_buffer;
> >> struct mutex lock; /* buffer locking */
> >> int curfreq;
> >> @@ -387,6 +361,7 @@
> >> mutex_unlock(&radio->lock);
> >>
> >> video_unregister_device(&radio->videodev);
> >> + v4l2_device_unregister(&radio->v4l2_dev);
> >> }
> >>
> >>
> >> @@ -482,14 +457,11 @@
> >> static int vidioc_queryctrl(struct file *file, void *priv,
> >> struct v4l2_queryctrl *qc)
> >> {
> >> - int i;
> >> + switch (qc->id) {
> >> + case V4L2_CID_AUDIO_MUTE:
> >> + return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
> >> + }
> >>
> >> - for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
> >> - if (qc->id && qc->id == radio_qctrl[i].id) {
> >> - memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
> >> - return 0;
> >> - }
> >> - }
> >> return -EINVAL;
> >> }
> >>
> >> @@ -686,22 +658,15 @@
> >> .vidioc_s_input = vidioc_s_input,
> >> };
> >>
> >> -/* V4L2 interface */
> >> -static struct video_device dsbr100_videodev_data = {
> >> - .name = "D-Link DSB-R 100",
> >> - .fops = &usb_dsbr100_fops,
> >> - .ioctl_ops = &usb_dsbr100_ioctl_ops,
> >> - .release = usb_dsbr100_video_device_release,
> >> -};
> >> -
> >> /* check if the device is present and register with v4l and usb
> >> if it is */ static int usb_dsbr100_probe(struct usb_interface
> >> *intf, const struct usb_device_id *id)
> >> {
> >> struct dsbr100_device *radio;
> >> + struct v4l2_device *v4l2_dev;
> >> int retval;
> >>
> >> - radio = kmalloc(sizeof(struct dsbr100_device), GFP_KERNEL);
> >> + radio = kzalloc(sizeof(struct dsbr100_device), GFP_KERNEL);
> >>
> >> if (!radio)
> >> return -ENOMEM;
> >> @@ -713,17 +678,35 @@
> >> return -ENOMEM;
> >> }
> >>
> >> + v4l2_dev = &radio->v4l2_dev;
> >> + strlcpy(v4l2_dev->name, "dsbr100", sizeof(v4l2_dev->name));
> >
> > Try to leave this out and let v4l2_device_register fill it in based
> > on the parent device. It should generate a decent name. If you
> > create it yourself, then make sure it is a unique name.
>
> I removed this line and it works fine. It looks like v4l2_dev->name
> contains the same name that used in dev_info/err/warn messages.
>
> >> +
> >> + retval = v4l2_device_register(&intf->dev, v4l2_dev);
> >> + if (retval < 0) {
> >> + v4l2_err(v4l2_dev, "couldn't register
> >> v4l2_device\n");
> >> + kfree(radio->transfer_buffer);
> >> + kfree(radio);
> >> + return retval;
> >> + }
> >> +
> >> + strlcpy(radio->videodev.name, v4l2_dev->name,
> >> sizeof(radio->videodev.name));
> >> + radio->videodev.v4l2_dev = v4l2_dev;
> >> + radio->videodev.fops = &usb_dsbr100_fops;
> >> + radio->videodev.ioctl_ops = &usb_dsbr100_ioctl_ops;
> >> + radio->videodev.release = usb_dsbr100_video_device_release;
> >> +
> >> mutex_init(&radio->lock);
> >> - radio->videodev = dsbr100_videodev_data;
> >>
> >> radio->removed = 0;
> >> radio->users = 0;
> >> radio->usbdev = interface_to_usbdev(intf);
> >> radio->curfreq = FREQ_MIN * FREQ_MUL;
> >> video_set_drvdata(&radio->videodev, radio);
> >> +
> >> retval = video_register_device(&radio->videodev,
> >> VFL_TYPE_RADIO, radio_nr); if (retval < 0) {
> >> dev_err(&intf->dev, "couldn't register video
> >> device\n");
> >> + v4l2_device_unregister(v4l2_dev);
> >> kfree(radio->transfer_buffer);
> >> kfree(radio);
> >> return -EIO;
> >
> > Looks good otherwise.
> >
> > Regards,
> >
> > Hans
> >
> > --
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG
>
> I sent updated version of patch to Douglas to test. If test will be
> good patch will be posted to maillist.
I have made several tests, your patch looks fine.
Cheers,
Douglas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
