diff mbox

[RFCv4,6/8] v4l2-ioctl.c: prefill tuner type for g_frequency and g/s_tuner.

Message ID e2a61ca8e17b7354a69bcb1b5ca35301efb5581e.1307875512.git.hans.verkuil@cisco.com (mailing list archive)
State RFC
Headers show

Commit Message

Hans Verkuil June 12, 2011, 10:59 a.m. UTC 
From: Hans Verkuil <hans.verkuil@cisco.com>

The subdevs are supposed to receive a valid tuner type for the g_frequency
and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
this in v4l2-ioctl.c based on whether the device node from which this is
called is a radio node or not.

The spec does not require applications to fill in the type, and if they
leave it at 0 then the 'supported_mode' call in tuner-core.c will return
false and the ioctl does nothing.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ioctl.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Comments

Andy Walls June 12, 2011, 12:41 p.m. UTC | #1 
On Sun, 2011-06-12 at 12:59 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The subdevs are supposed to receive a valid tuner type for the g_frequency
> and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
> this in v4l2-ioctl.c based on whether the device node from which this is
> called is a radio node or not.
> 
> The spec does not require applications to fill in the type, and if they
> leave it at 0 then the 'supported_mode' call in tuner-core.c will return
> false and the ioctl does nothing.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/v4l2-ioctl.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 213ba7d..26bf3bf 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -1822,6 +1822,8 @@ static long __video_do_ioctl(struct file *file,
>  		if (!ops->vidioc_g_tuner)
>  			break;
>  
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		ret = ops->vidioc_g_tuner(file, fh, p);
>  		if (!ret)
>  			dbgarg(cmd, "index=%d, name=%s, type=%d, "
> @@ -1840,6 +1842,8 @@ static long __video_do_ioctl(struct file *file,
>  
>  		if (!ops->vidioc_s_tuner)
>  			break;
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		dbgarg(cmd, "index=%d, name=%s, type=%d, "
>  				"capability=0x%x, rangelow=%d, "
>  				"rangehigh=%d, signal=%d, afc=%d, "
> @@ -1858,6 +1862,8 @@ static long __video_do_ioctl(struct file *file,
>  		if (!ops->vidioc_g_frequency)
>  			break;
>  
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		ret = ops->vidioc_g_frequency(file, fh, p);
>  		if (!ret)
>  			dbgarg(cmd, "tuner=%d, type=%d, frequency=%d\n",


Wow, that was easy.  And from what I can tell, it is spec compliant
too. :)

Reviewed-by: Andy Walls <awalls@md.metrocast.net>

-Andy






--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab June 12, 2011, 2:36 p.m. UTC | #2 
Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The subdevs are supposed to receive a valid tuner type for the g_frequency
> and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
> this in v4l2-ioctl.c based on whether the device node from which this is
> called is a radio node or not.
> 
> The spec does not require applications to fill in the type, and if they
> leave it at 0 then the 'supported_mode' call in tuner-core.c will return
> false and the ioctl does nothing.

Interesting solution. Yes, this is the proper fix, but only after being sure
that no drivers allow switch to radio using the video device, and vice-versa.

Unfortunately, this is not the case, currently.

Most drivers allow this, following the previous V4L2 specs. Changing such
behavior will probably require to write something at 
Documentation/feature-removal-schedule.txt, as we're changing the behavior.


> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/v4l2-ioctl.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 213ba7d..26bf3bf 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -1822,6 +1822,8 @@ static long __video_do_ioctl(struct file *file,
>  		if (!ops->vidioc_g_tuner)
>  			break;
>  
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		ret = ops->vidioc_g_tuner(file, fh, p);
>  		if (!ret)
>  			dbgarg(cmd, "index=%d, name=%s, type=%d, "
> @@ -1840,6 +1842,8 @@ static long __video_do_ioctl(struct file *file,
>  
>  		if (!ops->vidioc_s_tuner)
>  			break;
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		dbgarg(cmd, "index=%d, name=%s, type=%d, "
>  				"capability=0x%x, rangelow=%d, "
>  				"rangehigh=%d, signal=%d, afc=%d, "
> @@ -1858,6 +1862,8 @@ static long __video_do_ioctl(struct file *file,
>  		if (!ops->vidioc_g_frequency)
>  			break;
>  
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		ret = ops->vidioc_g_frequency(file, fh, p);
>  		if (!ret)
>  			dbgarg(cmd, "tuner=%d, type=%d, frequency=%d\n",

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil June 12, 2011, 3:46 p.m. UTC | #3 
On Sunday, June 12, 2011 16:36:11 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 07:59, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The subdevs are supposed to receive a valid tuner type for the g_frequency
> > and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
> > this in v4l2-ioctl.c based on whether the device node from which this is
> > called is a radio node or not.
> > 
> > The spec does not require applications to fill in the type, and if they
> > leave it at 0 then the 'supported_mode' call in tuner-core.c will return
> > false and the ioctl does nothing.
> 
> Interesting solution. Yes, this is the proper fix, but only after being sure
> that no drivers allow switch to radio using the video device, and vice-versa.

Why would that be a problem? What this patch does is that it fixes those
drivers that do *not* set vf/vt->type (i.e. leave it at 0). For those that already
set it nothing changes.

> Unfortunately, this is not the case, currently.
> 
> Most drivers allow this, following the previous V4L2 specs. Changing such
> behavior will probably require to write something at 
> Documentation/feature-removal-schedule.txt, as we're changing the behavior.

I think in the longer term we need to change the spec so that:

1) Opening a radio node no longer switches to radio mode. Instead, you need to
   call VIDIOC_S_FREQUENCY for that.
2) When VIDIOC_S_FREQUENCY the type field should match the video/radio node it
   is called on. So for /dev/radio type should be RADIO, for others it should be
   ANALOG_TV. Otherwise -EINVAL is called.

So this might be a good feature removal for 3.2 or 3.3.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab June 12, 2011, 5:08 p.m. UTC | #4 
Em 12-06-2011 12:46, Hans Verkuil escreveu:
> On Sunday, June 12, 2011 16:36:11 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> The subdevs are supposed to receive a valid tuner type for the g_frequency
>>> and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
>>> this in v4l2-ioctl.c based on whether the device node from which this is
>>> called is a radio node or not.
>>>
>>> The spec does not require applications to fill in the type, and if they
>>> leave it at 0 then the 'supported_mode' call in tuner-core.c will return
>>> false and the ioctl does nothing.
>>
>> Interesting solution. Yes, this is the proper fix, but only after being sure
>> that no drivers allow switch to radio using the video device, and vice-versa.
> 
> Why would that be a problem? What this patch does is that it fixes those
> drivers that do *not* set vf/vt->type (i.e. leave it at 0). For those that already
> set it nothing changes.

Yeah, I realized it after after answering. Yes, your patch seems to be ok, as
bridge drivers can override it.

> 
>> Unfortunately, this is not the case, currently.
>>
>> Most drivers allow this, following the previous V4L2 specs. Changing such
>> behavior will probably require to write something at 
>> Documentation/feature-removal-schedule.txt, as we're changing the behavior.
> 
> I think in the longer term we need to change the spec so that:
> 
> 1) Opening a radio node no longer switches to radio mode. Instead, you need to
>    call VIDIOC_S_FREQUENCY for that.
> 2) When VIDIOC_S_FREQUENCY the type field should match the video/radio node it
>    is called on. So for /dev/radio type should be RADIO, for others it should be
>    ANALOG_TV. Otherwise -EINVAL is called.
> 
> So this might be a good feature removal for 3.2 or 3.3.

I'm OK with that.

> 
> Regards,
> 
> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 213ba7d..26bf3bf 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1822,6 +1822,8 @@  static long __video_do_ioctl(struct file *file,
 		if (!ops->vidioc_g_tuner)
 			break;
 
+		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 		ret = ops->vidioc_g_tuner(file, fh, p);
 		if (!ret)
 			dbgarg(cmd, "index=%d, name=%s, type=%d, "
@@ -1840,6 +1842,8 @@  static long __video_do_ioctl(struct file *file,
 
 		if (!ops->vidioc_s_tuner)
 			break;
+		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 		dbgarg(cmd, "index=%d, name=%s, type=%d, "
 				"capability=0x%x, rangelow=%d, "
 				"rangehigh=%d, signal=%d, afc=%d, "
@@ -1858,6 +1862,8 @@  static long __video_do_ioctl(struct file *file,
 		if (!ops->vidioc_g_frequency)
 			break;
 
+		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 		ret = ops->vidioc_g_frequency(file, fh, p);
 		if (!ret)
 			dbgarg(cmd, "tuner=%d, type=%d, frequency=%d\n",