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.
Thank you,
--
best regards, Klimov Alexey
--
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
