[RFCv1 PATCH 0/7] tuner-core: fix g_freq/s_std and g/s_tuner

[RFCv1 PATCH 0/7] tuner-core: fix g_freq/s_std and g/s_tuner

[Hans Verkuil]

While testing the new multi-standard tuner of the HVR-1600 (the cx18 driver)
I came across a number of bugs in tuner-core that were recently introduced.

This patch series fixes those bugs and cleans up some code that confused
me.

The bugs fixed in this series are:

1) 'type' is set by the driver, not the application for g_tuner and g_frequency,
   so don't check that field.
2) s_std and s_tuner both setup the tuner first, and then apply the new std
   or audmode. This should be the other way around.
3) s_tuner should not check the tuner 'type' field since applications don't
   need to set this. 'audmode' should just be applied to the current tuner
   mode.

All these bugs were introduced in 2.6.39. So if acked, then this patch
series should at least to into v3.0 and possibly into 2.6.39-stable.

Regards,

	Hans

[RFCv1 PATCH 1/7] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The check_mode function checks whether a mode is supported. So calling it
supported_mode is more appropriate. In addition it returned either 0 or
-EINVAL which suggests that the -EINVAL error should be passed on. However,
that's not the case so change the return type to bool.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 5748d04..083b9f1 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -723,22 +723,19 @@ static int tuner_remove(struct i2c_client *client)
  */
 
 /**
- * check_mode - Verify if tuner supports the requested mode
+ * supported_mode - Verify if tuner supports the requested mode
  * @t: a pointer to the module's internal struct_tuner
  *
  * This function checks if the tuner is capable of tuning analog TV,
  * digital TV or radio, depending on what the caller wants. If the
- * tuner can't support that mode, it returns -EINVAL. Otherwise, it
- * returns 0.
+ * tuner can't support that mode, it returns false. Otherwise, it
+ * returns true.
  * This function is needed for boards that have a separate tuner for
  * radio (like devices with tea5767).
  */
-static inline int check_mode(struct tuner *t, enum v4l2_tuner_type mode)
+static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
 {
-	if ((1 << mode & t->mode_mask) == 0)
-		return -EINVAL;
-
-	return 0;
+	return 1 << mode & t->mode_mask;
 }
 
 /**
@@ -759,7 +756,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 
 	if (mode != t->mode) {
-		if (check_mode(t, mode) == -EINVAL) {
+		if (!supported_mode(t, mode)) {
 			tuner_dbg("Tuner doesn't support mode %d. "
 				  "Putting tuner to sleep\n", mode);
 			t->standby = true;
@@ -1138,7 +1135,7 @@ static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	struct tuner *t = to_tuner(sd);
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (check_mode(t, f->type) == -EINVAL)
+	if (!supported_mode(t, f->type))
 		return 0;
 	f->type = t->mode;
 	if (fe_tuner_ops->get_frequency && !t->standby) {
@@ -1161,7 +1158,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (check_mode(t, vt->type) == -EINVAL)
+	if (!supported_mode(t, vt->type))
 		return 0;
 	vt->type = t->mode;
 	if (analog_ops->get_afc)
-- 
1.7.1

[RFCv1 PATCH 2/7] tuner-core: change return type of set_mode_freq to bool

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

set_mode_freq currently returns 0 or -EINVAL. But -EINVAL does not
indicate a error that should be passed on, it just indicates that the
tuner does not support the requested mode. So change the return type to
bool.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 083b9f1..ee43e0a 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -746,11 +746,11 @@ static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
  * @freq:	frequency to set (0 means to use the previous one)
  *
  * If tuner doesn't support the needed mode (radio or TV), prints a
- * debug message and returns -EINVAL, changing its state to standby.
- * Otherwise, changes the state and sets frequency to the last value, if
- * the tuner can sleep or if it supports both Radio and TV.
+ * debug message and returns false, changing its state to standby.
+ * Otherwise, changes the state and sets frequency to the last value
+ * and returns true.
  */
-static int set_mode_freq(struct i2c_client *client, struct tuner *t,
+static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
 			 enum v4l2_tuner_type mode, unsigned int freq)
 {
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
@@ -762,7 +762,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 			t->standby = true;
 			if (analog_ops->standby)
 				analog_ops->standby(&t->fe);
-			return -EINVAL;
+			return false;
 		}
 		t->mode = mode;
 		tuner_dbg("Changing to mode %d\n", mode);
@@ -777,7 +777,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 		set_tv_freq(client, t->tv_freq);
 	}
 
-	return 0;
+	return true;
 }
 
 /*
@@ -1075,8 +1075,7 @@ static int tuner_s_radio(struct v4l2_subdev *sd)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, V4L2_TUNER_RADIO, 0) == -EINVAL)
-		return 0;
+	set_mode_freq(client, t, V4L2_TUNER_RADIO, 0);
 	return 0;
 }
 
@@ -1110,7 +1109,7 @@ static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0) == -EINVAL)
+	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
 		return 0;
 
 	t->std = std;
@@ -1124,9 +1123,7 @@ static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, f->type, f->frequency) == -EINVAL)
-		return 0;
-
+	set_mode_freq(client, t, f->type, f->frequency);
 	return 0;
 }
 
@@ -1197,7 +1194,7 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, vt->type, 0) == -EINVAL)
+	if (!set_mode_freq(client, t, vt->type, 0))
 		return 0;
 
 	if (t->mode == V4L2_TUNER_RADIO)
-- 
1.7.1

[RFCv1 PATCH 3/7] tuner-core: fix g_frequency support.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

VIDIOC_G_FREQUENCY should not check the tuner type, instead that is
something the driver fill in.

Since apps will often leave the type at 0, the 'supported_mode' call
will return false and the frequency never gets filled in.

Remove this check.

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

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index ee43e0a..4d8dcea 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1132,8 +1132,6 @@ static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	struct tuner *t = to_tuner(sd);
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (!supported_mode(t, f->type))
-		return 0;
 	f->type = t->mode;
 	if (fe_tuner_ops->get_frequency && !t->standby) {
 		u32 abs_freq;
-- 
1.7.1

Re: [RFCv1 PATCH 3/7] tuner-core: fix g_frequency support.

[Mauro Carvalho Chehab]

Em 11-06-2011 10:34, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> VIDIOC_G_FREQUENCY should not check the tuner type, instead that is
> something the driver fill in.
> 
> Since apps will often leave the type at 0, the 'supported_mode' call
> will return false and the frequency never gets filled in.
> 
> Remove this check.

This patch is wrong, as it breaks support for devices with multiple
tuners (e. g. a tea5767 for radio and another tuner for TV).

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/tuner-core.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index ee43e0a..4d8dcea 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -1132,8 +1132,6 @@ static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>  	struct tuner *t = to_tuner(sd);
>  	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
>  
> -	if (!supported_mode(t, f->type))
> -		return 0;
>  	f->type = t->mode;
>  	if (fe_tuner_ops->get_frequency && !t->standby) {
>  		u32 abs_freq;

Re: [RFCv1 PATCH 3/7] tuner-core: fix g_frequency support.

[Hans Verkuil]

On Saturday, June 11, 2011 15:44:43 Mauro Carvalho Chehab wrote:
> Em 11-06-2011 10:34, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > VIDIOC_G_FREQUENCY should not check the tuner type, instead that is
> > something the driver fill in.
> > 
> > Since apps will often leave the type at 0, the 'supported_mode' call
> > will return false and the frequency never gets filled in.
> > 
> > Remove this check.
> 
> This patch is wrong, as it breaks support for devices with multiple
> tuners (e. g. a tea5767 for radio and another tuner for TV).

Thanks for catching this, I had it right in an earlier version, but it
got lost somewhere along the way.

What should happen is that this should be added at the beginning:

if (t->standby)
	return 0;

Ditto for g_tuner. If the current mode is not supported, standby is set to
true.

Regards,

	Hans

> 
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/video/tuner-core.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> > index ee43e0a..4d8dcea 100644
> > --- a/drivers/media/video/tuner-core.c
> > +++ b/drivers/media/video/tuner-core.c
> > @@ -1132,8 +1132,6 @@ static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
> >  	struct tuner *t = to_tuner(sd);
> >  	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
> >  
> > -	if (!supported_mode(t, f->type))
> > -		return 0;
> >  	f->type = t->mode;
> >  	if (fe_tuner_ops->get_frequency && !t->standby) {
> >  		u32 abs_freq;
> 
> 

[RFCv1 PATCH 4/7] tuner-core: simplify the standard fixup.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Get rid of a number of unnecessary tuner_dbg messages by simplifying
the std fixup function.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   92 +++++++++++---------------------------
 1 files changed, 27 insertions(+), 65 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 4d8dcea..d0630f9 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -832,7 +832,7 @@ static void set_tv_freq(struct i2c_client *c, unsigned int freq)
 /**
  * tuner_fixup_std - force a given video standard variant
  *
- * @t:	tuner internal struct
+ * @std:	TV standard
  *
  * A few devices or drivers have problem to detect some standard variations.
  * On other operational systems, the drivers generally have a per-country
@@ -842,57 +842,39 @@ static void set_tv_freq(struct i2c_client *c, unsigned int freq)
  * to distinguish all video standard variations, a modprobe parameter can
  * be used to force a video standard match.
  */
-static int tuner_fixup_std(struct tuner *t)
+static v4l2_std_id tuner_fixup_std(struct tuner *t, v4l2_std_id std)
 {
-	if ((t->std & V4L2_STD_PAL) == V4L2_STD_PAL) {
+	if (pal[0] != '-' && (std & V4L2_STD_PAL) == V4L2_STD_PAL) {
 		switch (pal[0]) {
 		case '6':
-			tuner_dbg("insmod fixup: PAL => PAL-60\n");
-			t->std = V4L2_STD_PAL_60;
-			break;
+			return V4L2_STD_PAL_60;
 		case 'b':
 		case 'B':
 		case 'g':
 		case 'G':
-			tuner_dbg("insmod fixup: PAL => PAL-BG\n");
-			t->std = V4L2_STD_PAL_BG;
-			break;
+			return V4L2_STD_PAL_BG;
 		case 'i':
 		case 'I':
-			tuner_dbg("insmod fixup: PAL => PAL-I\n");
-			t->std = V4L2_STD_PAL_I;
-			break;
+			return V4L2_STD_PAL_I;
 		case 'd':
 		case 'D':
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: PAL => PAL-DK\n");
-			t->std = V4L2_STD_PAL_DK;
-			break;
+			return V4L2_STD_PAL_DK;
 		case 'M':
 		case 'm':
-			tuner_dbg("insmod fixup: PAL => PAL-M\n");
-			t->std = V4L2_STD_PAL_M;
-			break;
+			return V4L2_STD_PAL_M;
 		case 'N':
 		case 'n':
-			if (pal[1] == 'c' || pal[1] == 'C') {
-				tuner_dbg("insmod fixup: PAL => PAL-Nc\n");
-				t->std = V4L2_STD_PAL_Nc;
-			} else {
-				tuner_dbg("insmod fixup: PAL => PAL-N\n");
-				t->std = V4L2_STD_PAL_N;
-			}
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			if (pal[1] == 'c' || pal[1] == 'C')
+				return V4L2_STD_PAL_Nc;
+			return V4L2_STD_PAL_N;
 		default:
 			tuner_warn("pal= argument not recognised\n");
 			break;
 		}
 	}
-	if ((t->std & V4L2_STD_SECAM) == V4L2_STD_SECAM) {
+	if (secam[0] != '-' && (std & V4L2_STD_SECAM) == V4L2_STD_SECAM) {
 		switch (secam[0]) {
 		case 'b':
 		case 'B':
@@ -900,63 +882,42 @@ static int tuner_fixup_std(struct tuner *t)
 		case 'G':
 		case 'h':
 		case 'H':
-			tuner_dbg("insmod fixup: SECAM => SECAM-BGH\n");
-			t->std = V4L2_STD_SECAM_B |
-				 V4L2_STD_SECAM_G |
-				 V4L2_STD_SECAM_H;
-			break;
+			return V4L2_STD_SECAM_B |
+			       V4L2_STD_SECAM_G |
+			       V4L2_STD_SECAM_H;
 		case 'd':
 		case 'D':
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: SECAM => SECAM-DK\n");
-			t->std = V4L2_STD_SECAM_DK;
-			break;
+			return V4L2_STD_SECAM_DK;
 		case 'l':
 		case 'L':
-			if ((secam[1] == 'C') || (secam[1] == 'c')) {
-				tuner_dbg("insmod fixup: SECAM => SECAM-L'\n");
-				t->std = V4L2_STD_SECAM_LC;
-			} else {
-				tuner_dbg("insmod fixup: SECAM => SECAM-L\n");
-				t->std = V4L2_STD_SECAM_L;
-			}
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			if ((secam[1] == 'C') || (secam[1] == 'c'))
+				return V4L2_STD_SECAM_LC;
+			return V4L2_STD_SECAM_L;
 		default:
 			tuner_warn("secam= argument not recognised\n");
 			break;
 		}
 	}
 
-	if ((t->std & V4L2_STD_NTSC) == V4L2_STD_NTSC) {
+	if (ntsc[0] != '-' && (std & V4L2_STD_NTSC) == V4L2_STD_NTSC) {
 		switch (ntsc[0]) {
 		case 'm':
 		case 'M':
-			tuner_dbg("insmod fixup: NTSC => NTSC-M\n");
-			t->std = V4L2_STD_NTSC_M;
-			break;
+			return V4L2_STD_NTSC_M;
 		case 'j':
 		case 'J':
-			tuner_dbg("insmod fixup: NTSC => NTSC_M_JP\n");
-			t->std = V4L2_STD_NTSC_M_JP;
-			break;
+			return V4L2_STD_NTSC_M_JP;
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: NTSC => NTSC_M_KR\n");
-			t->std = V4L2_STD_NTSC_M_KR;
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			return V4L2_STD_NTSC_M_KR;
 		default:
 			tuner_info("ntsc= argument not recognised\n");
 			break;
 		}
 	}
-	return 0;
+	return std;
 }
 
 /*
@@ -1112,8 +1073,9 @@ static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
 		return 0;
 
-	t->std = std;
-	tuner_fixup_std(t);
+	t->std = tuner_fixup_std(t, std);
+	if (t->std != std)
+		tuner_dbg("Fixup standard %llx to %llx\n", std, t->std);
 
 	return 0;
 }
-- 
1.7.1

[RFCv1 PATCH 5/7] tuner-core: fix s_std and s_tuner.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Both s_std and s_tuner are broken because set_mode_freq is called before the
new std (for s_std) and audmode (for s_tuner) are set.

This patch splits set_mode_freq in a set_mode and a set_freq and in s_std
first calls set_mode, and if that returns true (i.e. the mode is supported)
then they set t->std/t->audmode and call set_freq.

This fixes a bug where changing std or audmode would actually change it to
the previous value.

Discovered while testing analog TV standards for cx18 with a tda18271 tuner.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   57 ++++++++++++++++++++-----------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index d0630f9..8ef7790 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -739,19 +739,15 @@ static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
 }
 
 /**
- * set_mode_freq - Switch tuner to other mode.
- * @client:	struct i2c_client pointer
+ * set_mode - Switch tuner to other mode.
  * @t:		a pointer to the module's internal struct_tuner
  * @mode:	enum v4l2_type (radio or TV)
- * @freq:	frequency to set (0 means to use the previous one)
  *
  * If tuner doesn't support the needed mode (radio or TV), prints a
  * debug message and returns false, changing its state to standby.
- * Otherwise, changes the state and sets frequency to the last value
- * and returns true.
+ * Otherwise, changes the state and returns true.
  */
-static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
-			 enum v4l2_tuner_type mode, unsigned int freq)
+static bool set_mode(struct tuner *t, enum v4l2_tuner_type mode)
 {
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 
@@ -767,17 +763,27 @@ static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
 		t->mode = mode;
 		tuner_dbg("Changing to mode %d\n", mode);
 	}
+	return true;
+}
+
+/**
+ * set_freq - Set the tuner to the desired frequency.
+ * @t:		a pointer to the module's internal struct_tuner
+ * @freq:	frequency to set (0 means to use the current frequency)
+ */
+static void set_freq(struct tuner *t, unsigned int freq)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&t->sd);
+
 	if (t->mode == V4L2_TUNER_RADIO) {
-		if (freq)
-			t->radio_freq = freq;
-		set_radio_freq(client, t->radio_freq);
+		if (!freq)
+			freq = t->radio_freq;
+		set_radio_freq(client, freq);
 	} else {
-		if (freq)
-			t->tv_freq = freq;
-		set_tv_freq(client, t->tv_freq);
+		if (!freq)
+			freq = t->tv_freq;
+		set_tv_freq(client, freq);
 	}
-
-	return true;
 }
 
 /*
@@ -1034,9 +1040,9 @@ static void tuner_status(struct dvb_frontend *fe)
 static int tuner_s_radio(struct v4l2_subdev *sd)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	set_mode_freq(client, t, V4L2_TUNER_RADIO, 0);
+	set_mode(t, V4L2_TUNER_RADIO);
+	set_freq(t, 0);
 	return 0;
 }
 
@@ -1068,24 +1074,23 @@ static int tuner_s_power(struct v4l2_subdev *sd, int on)
 static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
+	if (!set_mode(t, V4L2_TUNER_ANALOG_TV))
 		return 0;
 
 	t->std = tuner_fixup_std(t, std);
 	if (t->std != std)
 		tuner_dbg("Fixup standard %llx to %llx\n", std, t->std);
-
+	set_freq(t, 0);
 	return 0;
 }
 
 static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	set_mode_freq(client, t, f->type, f->frequency);
+	if (set_mode(t, f->type))
+		set_freq(t, f->frequency);
 	return 0;
 }
 
@@ -1152,13 +1157,13 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (!set_mode_freq(client, t, vt->type, 0))
+	if (!set_mode(t, vt->type))
 		return 0;
 
 	if (t->mode == V4L2_TUNER_RADIO)
 		t->audmode = vt->audmode;
+	set_freq(t, 0);
 
 	return 0;
 }
@@ -1193,8 +1198,8 @@ static int tuner_resume(struct i2c_client *c)
 	tuner_dbg("resume\n");
 
 	if (!t->standby)
-		set_mode_freq(c, t, t->type, 0);
-
+		if (set_mode(t, t->type))
+			set_freq(t, 0);
 	return 0;
 }
 
-- 
1.7.1

[RFCv1 PATCH 6/7] tuner-core: fix g_tuner

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

g_tuner just returns the tuner data for the current tuner mode and the
application does not have to set the tuner type. So don't test for a
valid tuner type.

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

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 8ef7790..7280998 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1120,8 +1120,6 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (!supported_mode(t, vt->type))
-		return 0;
 	vt->type = t->mode;
 	if (analog_ops->get_afc)
 		vt->afc = analog_ops->get_afc(&t->fe);
-- 
1.7.1

Re: [RFCv1 PATCH 6/7] tuner-core: fix g_tuner

[Mauro Carvalho Chehab]

Em 11-06-2011 10:34, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> g_tuner just returns the tuner data for the current tuner mode and the
> application does not have to set the tuner type. So don't test for a
> valid tuner type.

This also breaks support for a separate radio tuner, as both TV and radio
tuners will touch at the g_tuner struct.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/tuner-core.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index 8ef7790..7280998 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -1120,8 +1120,6 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
>  	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
>  
> -	if (!supported_mode(t, vt->type))
> -		return 0;
>  	vt->type = t->mode;
>  	if (analog_ops->get_afc)
>  		vt->afc = analog_ops->get_afc(&t->fe);

[RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

According to the spec the tuner type field is not used when calling
S_TUNER: index, audmode and reserved are the only writable fields.

So remove the type check. Instead, just set the audmode if the current
tuner mode is set to radio.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 7280998..0ffcf54 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1156,12 +1156,10 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 {
 	struct tuner *t = to_tuner(sd);
 
-	if (!set_mode(t, vt->type))
-		return 0;
-
-	if (t->mode == V4L2_TUNER_RADIO)
+	if (t->mode == V4L2_TUNER_RADIO) {
 		t->audmode = vt->audmode;
-	set_freq(t, 0);
+		set_freq(t, 0);
+	}
 
 	return 0;
 }
-- 
1.7.1

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 11-06-2011 10:34, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> According to the spec the tuner type field is not used when calling
> S_TUNER: index, audmode and reserved are the only writable fields.
> 
> So remove the type check. Instead, just set the audmode if the current
> tuner mode is set to radio.

I suspect that this patch also breaks support for a separate radio tuner.
if tuner-type is not properly filled, then the easiest fix would be to
revert some changes done at the tuner cleanup/fixup patches applied on .39.
Yet, the previous logic were trying to hint the device mode, with is bad
(that's why it was changed).

The proper change seems to add a parameter to this callback, set by the
bridge driver, informing if the device is using radio or video mode.
We need also to patch the V4L API specs, as it allows using a video node
for radio, and vice versa. This is not well supported, and it seems silly
to keep it at the specs and needing to add hints at the drivers due to
that.

Cheers,
Mauro

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/tuner-core.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index 7280998..0ffcf54 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -1156,12 +1156,10 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  {
>  	struct tuner *t = to_tuner(sd);
>  
> -	if (!set_mode(t, vt->type))
> -		return 0;
> -
> -	if (t->mode == V4L2_TUNER_RADIO)
> +	if (t->mode == V4L2_TUNER_RADIO) {
>  		t->audmode = vt->audmode;
> -	set_freq(t, 0);
> +		set_freq(t, 0);
> +	}
>  
>  	return 0;
>  }

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
> Em 11-06-2011 10:34, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > According to the spec the tuner type field is not used when calling
> > S_TUNER: index, audmode and reserved are the only writable fields.
> > 
> > So remove the type check. Instead, just set the audmode if the current
> > tuner mode is set to radio.
> 
> I suspect that this patch also breaks support for a separate radio tuner.
> if tuner-type is not properly filled, then the easiest fix would be to
> revert some changes done at the tuner cleanup/fixup patches applied on .39.
> Yet, the previous logic were trying to hint the device mode, with is bad
> (that's why it was changed).
> 
> The proper change seems to add a parameter to this callback, set by the
> bridge driver, informing if the device is using radio or video mode.
> We need also to patch the V4L API specs, as it allows using a video node
> for radio, and vice versa. This is not well supported, and it seems silly
> to keep it at the specs and needing to add hints at the drivers due to
> that.

So, just to make sure I understand correctly what you want. The bridge or
platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
(for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
TV for anything else.

What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
in. Will we just overwrite vf->type or will we check and return -EINVAL if
the app tries to set e.g. a TV frequency on /dev/radio?

Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
should that change the tuner to tv mode?

I think the type passed to S_FREQUENCY should 1) match the device node's type
(if not, then return -EINVAL) and 2) should match the current mode (if not,
then return -EBUSY). So attempts to change the TV frequency when in radio
mode should fail. This second rule should also be valid for S_TUNER.

What should G_TUNER return on a video node when in radio mode or vice versa?
For G_FREQUENCY you can still return the last used frequency, but that's
much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
afc all to 0 if you query G_TUNER when 'in the wrong mode'.

The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
so that's OK.

And how do we switch between radio and TV? Right now opening the radio node
will set the tuner in radio mode, and calling S_STD will change the mode to
TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
at the moment.

What about this:

Opening /dev/radio effectively starts the radio mode. So if there is TV
capture in progress, then the open should return -EBUSY. Otherwise it
switches the tuner to radio mode. And it stays in radio mode until the
last filehandle of /dev/radio is closed. At that point it will automatically
switch back to TV mode (if there is one, of course).

While it is in radio mode calls to S_STD and S_FREQUENCY from /dev/video
will return -EBUSY. Any attempt to start streaming from /dev/video will
also return -EBUSY (radio 'streaming' is in progress after all).

Effectively, S_STD no longer switches back to TV mode. That only happens when
the last user of /dev/radio left. It certainly sounds a lot saner to me.

Of course, I'm ignoring DVB in this story. You may have to negotiate between
radio, Tv and DVB.

Anyway, this all sounds very nice, but it's a heck of a lot of work. I'd much
rather just fix this bug without changing the spec and behavior of drivers.
That's a nice project perhaps for a rainy day (or week...), but not for a fix
that is needed asap and that works for kernel v3.0.

The whole radio/tv/dvb tuner selection is a big mess and needs to be solved,
but let's do that in a separate project.

Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Andy Walls]

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>> > From: Hans Verkuil <hans.verkuil@cisco.com>
>> > 
>> > According to the spec the tuner type field is not used when calling
>> > S_TUNER: index, audmode and reserved are the only writable fields.
>> > 
>> > So remove the type check. Instead, just set the audmode if the
>current
>> > tuner mode is set to radio.
>> 
>> I suspect that this patch also breaks support for a separate radio
>tuner.
>> if tuner-type is not properly filled, then the easiest fix would be
>to
>> revert some changes done at the tuner cleanup/fixup patches applied
>on .39.
>> Yet, the previous logic were trying to hint the device mode, with is
>bad
>> (that's why it was changed).
>> 
>> The proper change seems to add a parameter to this callback, set by
>the
>> bridge driver, informing if the device is using radio or video mode.
>> We need also to patch the V4L API specs, as it allows using a video
>node
>> for radio, and vice versa. This is not well supported, and it seems
>silly
>> to keep it at the specs and needing to add hints at the drivers due
>to
>> that.
>
>So, just to make sure I understand correctly what you want. The bridge
>or
>platform drivers will fill in the vf->type (for g/s_frequency) or
>vt->type
>(for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
>TV for anything else.
>
>What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill
>this
>in. Will we just overwrite vf->type or will we check and return -EINVAL
>if
>the app tries to set e.g. a TV frequency on /dev/radio?
>
>Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if
>/dev/radio was
>opened, and now I open /dev/video and call S_FREQUENCY with the TV
>tuner type,
>should that change the tuner to tv mode?
>
>I think the type passed to S_FREQUENCY should 1) match the device
>node's type
>(if not, then return -EINVAL) and 2) should match the current mode (if
>not,
>then return -EBUSY). So attempts to change the TV frequency when in
>radio
>mode should fail. This second rule should also be valid for S_TUNER.
>
>What should G_TUNER return on a video node when in radio mode or vice
>versa?
>For G_FREQUENCY you can still return the last used frequency, but
>that's
>much more ambiguous for G_TUNER. One option is to set rxsubchans,
>signal and
>afc all to 0 if you query G_TUNER when 'in the wrong mode'.
>
>The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO
>only,
>so that's OK.
>
>And how do we switch between radio and TV? Right now opening the radio
>node
>will set the tuner in radio mode, and calling S_STD will change the
>mode to
>TV again. As mentioned above, what S_FREQUENCY is supposed to do is
>undefined
>at the moment.
>
>What about this:
>
>Opening /dev/radio effectively starts the radio mode. So if there is TV
>capture in progress, then the open should return -EBUSY. Otherwise it
>switches the tuner to radio mode. And it stays in radio mode until the
>last filehandle of /dev/radio is closed. At that point it will
>automatically
>switch back to TV mode (if there is one, of course).
>
>While it is in radio mode calls to S_STD and S_FREQUENCY from
>/dev/video
>will return -EBUSY. Any attempt to start streaming from /dev/video will
>also return -EBUSY (radio 'streaming' is in progress after all).
>
>Effectively, S_STD no longer switches back to TV mode. That only
>happens when
>the last user of /dev/radio left. It certainly sounds a lot saner to
>me.
>
>Of course, I'm ignoring DVB in this story. You may have to negotiate
>between
>radio, Tv and DVB.
>
>Anyway, this all sounds very nice, but it's a heck of a lot of work.
>I'd much
>rather just fix this bug without changing the spec and behavior of
>drivers.
>That's a nice project perhaps for a rainy day (or week...), but not for
>a fix
>that is needed asap and that works for kernel v3.0.
>
>The whole radio/tv/dvb tuner selection is a big mess and needs to be
>solved,
>but let's do that in a separate project.
>
>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

You have to be able to stream from a video node /dev/video24 when /dev/radio is open.  /dev/radio is a control only node.  (Under current spec behavior.)

Also there is at least one non-test app out there, brought up on the ivtv lists a few months ago in the context of v4l2 priorty handling, that, IIRC, makes calls on /dev/video24 with /dev/radio open.

Regards,
Andy

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 11-06-2011 14:27, Hans Verkuil escreveu:
> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> According to the spec the tuner type field is not used when calling
>>> S_TUNER: index, audmode and reserved are the only writable fields.
>>>
>>> So remove the type check. Instead, just set the audmode if the current
>>> tuner mode is set to radio.
>>
>> I suspect that this patch also breaks support for a separate radio tuner.
>> if tuner-type is not properly filled, then the easiest fix would be to
>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
>> Yet, the previous logic were trying to hint the device mode, with is bad
>> (that's why it was changed).
>>
>> The proper change seems to add a parameter to this callback, set by the
>> bridge driver, informing if the device is using radio or video mode.
>> We need also to patch the V4L API specs, as it allows using a video node
>> for radio, and vice versa. This is not well supported, and it seems silly
>> to keep it at the specs and needing to add hints at the drivers due to
>> that.
> 
> So, just to make sure I understand correctly what you want. The bridge or
> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
> TV for anything else.

Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
Of course, I might have left something else. Btw, the older code were also
requiring it.

The cx18 implementation were merged after the changes, so maybe it is not doing 
the right thing.

> 
> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
> in. Will we just overwrite vf->type or will we check and return -EINVAL if
> the app tries to set e.g. a TV frequency on /dev/radio?

That's a very good question. What happens is that the V4L2 API used to allow
opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
at the API specs. I think that this were changed on newer versions of the spec.

> Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
> opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
> should that change the tuner to tv mode?

Yes. I think that some applications like kradio just keeps the device node opened.
If we return -EBUSY, those applications will break. The reverse is more tricky:
e. g. if /dev/video is streaming, I think that the bridge driver should return
-EBUSY if the device can't do both TV and radio at the same time, but this is
something that it is device-specific, so such logic, if needed, should be implemented
at the bridge driver.

> I think the type passed to S_FREQUENCY should 1) match the device node's type
> (if not, then return -EINVAL) and 2) should match the current mode (if not,
> then return -EBUSY). So attempts to change the TV frequency when in radio
> mode should fail. This second rule should also be valid for S_TUNER.

See above.

> What should G_TUNER return on a video node when in radio mode or vice versa?
> For G_FREQUENCY you can still return the last used frequency, but that's
> much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
> afc all to 0 if you query G_TUNER when 'in the wrong mode'.

The current logic should handle this case well. I tested it carefully. Basically,
if the device is on Radio mode, and has a separate tuner for TV, the TV tuner
should not touch the structure. The Radio tuner should properly fill the values.
Calls to G_TUNER/G_FREQUENCY shouldn't switch the device mode, or they may break
applications like kradio, that may be always there during the entire KDE section.

> The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
> so that's OK.
> 
> And how do we switch between radio and TV? Right now opening the radio node
> will set the tuner in radio mode, and calling S_STD will change the mode to
> TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
> at the moment.

If S_FREQUENCY is called from /dev/video (or /dev/vbi), it should set it to TV. If
it is called from /dev/radio, it should put the device on radio mode.

The current logic already does that. I tested it on several devices, with both
tea5767 and without it.

> What about this:
> 
> Opening /dev/radio effectively starts the radio mode. So if there is TV
> capture in progress, then the open should return -EBUSY. Otherwise it
> switches the tuner to radio mode. And it stays in radio mode until the
> last filehandle of /dev/radio is closed. At that point it will automatically
> switch back to TV mode (if there is one, of course).

No. This would break existing applications. The mode switch should be done
at S_FREQUENCY (e. g. when the radio application is tuning into a channel).

> While it is in radio mode calls to S_STD and S_FREQUENCY from /dev/video
> will return -EBUSY. Any attempt to start streaming from /dev/video will
> also return -EBUSY (radio 'streaming' is in progress after all).

For the same reason as said above, this will cause troubles for existing
appications.


> Effectively, S_STD no longer switches back to TV mode. That only happens when
> the last user of /dev/radio left. It certainly sounds a lot saner to me.

Opening a /dev/radio device doesn't mean that radio is "streaming". A radio
is streaming if:
	- tuner was set with S_FREQUENCY
	- device is not muted.

Btw, the console application "radio" allows you to, open a device, set a frequency,
unmute the device and close the radio device:
	radio -qf 91.4

So, even having the device node closed doesn't mean that the radio is not being
used.

> 
> Of course, I'm ignoring DVB in this story. You may have to negotiate between
> radio, Tv and DVB.
> 
> Anyway, this all sounds very nice, but it's a heck of a lot of work. I'd much
> rather just fix this bug without changing the spec and behavior of drivers.
> That's a nice project perhaps for a rainy day (or week...), but not for a fix
> that is needed asap and that works for kernel v3.0.

I failed to see what's broken. I suspect that the only thing that needs to
be corrected are the bridge drivers that aren't properly setting the tuner
type before calling the tuner code.

> 
> The whole radio/tv/dvb tuner selection is a big mess and needs to be solved,
> but let's do that in a separate project.

Yes.
> 
> Regards,
> 
> 	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
> Em 11-06-2011 14:27, Hans Verkuil escreveu:
> > On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
> >> Em 11-06-2011 10:34, Hans Verkuil escreveu:
> >>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>
> >>> According to the spec the tuner type field is not used when calling
> >>> S_TUNER: index, audmode and reserved are the only writable fields.
> >>>
> >>> So remove the type check. Instead, just set the audmode if the current
> >>> tuner mode is set to radio.
> >>
> >> I suspect that this patch also breaks support for a separate radio tuner.
> >> if tuner-type is not properly filled, then the easiest fix would be to
> >> revert some changes done at the tuner cleanup/fixup patches applied on .39.
> >> Yet, the previous logic were trying to hint the device mode, with is bad
> >> (that's why it was changed).
> >>
> >> The proper change seems to add a parameter to this callback, set by the
> >> bridge driver, informing if the device is using radio or video mode.
> >> We need also to patch the V4L API specs, as it allows using a video node
> >> for radio, and vice versa. This is not well supported, and it seems silly
> >> to keep it at the specs and needing to add hints at the drivers due to
> >> that.
> > 
> > So, just to make sure I understand correctly what you want. The bridge or
> > platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
> > (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
> > TV for anything else.
> 
> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
> Of course, I might have left something else. Btw, the older code were also
> requiring it.
> 
> The cx18 implementation were merged after the changes, so maybe it is not doing 
> the right thing.

Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
just some random driver that failed to set vf/vt->type.

> 
> > 
> > What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
> > in. Will we just overwrite vf->type or will we check and return -EINVAL if
> > the app tries to set e.g. a TV frequency on /dev/radio?
> 
> That's a very good question. What happens is that the V4L2 API used to allow
> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
> at the API specs. I think that this were changed on newer versions of the spec.

If you want, then I can add an extra patch to my v4 patch series that returns
-EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
(inconsistent with the device node's type, that is).
 
> > Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
> > opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
> > should that change the tuner to tv mode?
> 
> Yes. I think that some applications like kradio just keeps the device node opened.
> If we return -EBUSY, those applications will break. The reverse is more tricky:
> e. g. if /dev/video is streaming, I think that the bridge driver should return
> -EBUSY if the device can't do both TV and radio at the same time, but this is
> something that it is device-specific, so such logic, if needed, should be implemented
> at the bridge driver.

Agreed.
 
> > I think the type passed to S_FREQUENCY should 1) match the device node's type
> > (if not, then return -EINVAL) and 2) should match the current mode (if not,
> > then return -EBUSY). So attempts to change the TV frequency when in radio
> > mode should fail. This second rule should also be valid for S_TUNER.
> 
> See above.
> 
> > What should G_TUNER return on a video node when in radio mode or vice versa?
> > For G_FREQUENCY you can still return the last used frequency, but that's
> > much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
> > afc all to 0 if you query G_TUNER when 'in the wrong mode'.
> 
> The current logic should handle this case well. I tested it carefully. Basically,
> if the device is on Radio mode, and has a separate tuner for TV, the TV tuner
> should not touch the structure. The Radio tuner should properly fill the values.

I analyzed it again and you are correct.

> Calls to G_TUNER/G_FREQUENCY shouldn't switch the device mode, or they may break
> applications like kradio, that may be always there during the entire KDE section.

Obviously.

> > The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
> > so that's OK.
> > 
> > And how do we switch between radio and TV? Right now opening the radio node
> > will set the tuner in radio mode, and calling S_STD will change the mode to
> > TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
> > at the moment.
> 
> If S_FREQUENCY is called from /dev/video (or /dev/vbi), it should set it to TV. If
> it is called from /dev/radio, it should put the device on radio mode.
> 
> The current logic already does that. I tested it on several devices, with both
> tea5767 and without it.
> 
> > What about this:
> > 
> > Opening /dev/radio effectively starts the radio mode. So if there is TV
> > capture in progress, then the open should return -EBUSY. Otherwise it
> > switches the tuner to radio mode. And it stays in radio mode until the
> > last filehandle of /dev/radio is closed. At that point it will automatically
> > switch back to TV mode (if there is one, of course).
> 
> No. This would break existing applications. The mode switch should be done
> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).

This is not what happens today as the switch to radio occurs as soon as you open
the radio node. It's the reason for the s_radio op.

The V4L2 specification is silent on this topic, unfortunately. And I'm not sure
how applications handle this.

I think only changing the mode when calling S_FREQUENCY (and S_STD) is a good
thing since this automatic mode switch when just opening a node goes against
the V4L2 philosophy. Just opening a node shouldn't change the state of the
device.

> > While it is in radio mode calls to S_STD and S_FREQUENCY from /dev/video
> > will return -EBUSY. Any attempt to start streaming from /dev/video will
> > also return -EBUSY (radio 'streaming' is in progress after all).
> 
> For the same reason as said above, this will cause troubles for existing
> appications.
> 
> 
> > Effectively, S_STD no longer switches back to TV mode. That only happens when
> > the last user of /dev/radio left. It certainly sounds a lot saner to me.
> 
> Opening a /dev/radio device doesn't mean that radio is "streaming". A radio
> is streaming if:
> 	- tuner was set with S_FREQUENCY
> 	- device is not muted.
> 
> Btw, the console application "radio" allows you to, open a device, set a frequency,
> unmute the device and close the radio device:
> 	radio -qf 91.4
> 
> So, even having the device node closed doesn't mean that the radio is not being
> used.

Yeah, that's right. It slipped my mind when I wrote that.

Regards,

	Hans

> 
> > 
> > Of course, I'm ignoring DVB in this story. You may have to negotiate between
> > radio, Tv and DVB.
> > 
> > Anyway, this all sounds very nice, but it's a heck of a lot of work. I'd much
> > rather just fix this bug without changing the spec and behavior of drivers.
> > That's a nice project perhaps for a rainy day (or week...), but not for a fix
> > that is needed asap and that works for kernel v3.0.
> 
> I failed to see what's broken. I suspect that the only thing that needs to
> be corrected are the bridge drivers that aren't properly setting the tuner
> type before calling the tuner code.
> 
> > 
> > The whole radio/tv/dvb tuner selection is a big mess and needs to be solved,
> > but let's do that in a separate project.
> 
> Yes.
> > 
> > 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
> 
> 

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 12-06-2011 08:36, Hans Verkuil escreveu:
> On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
>> Em 11-06-2011 14:27, Hans Verkuil escreveu:
>>> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>>>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>
>>>>> According to the spec the tuner type field is not used when calling
>>>>> S_TUNER: index, audmode and reserved are the only writable fields.
>>>>>
>>>>> So remove the type check. Instead, just set the audmode if the current
>>>>> tuner mode is set to radio.
>>>>
>>>> I suspect that this patch also breaks support for a separate radio tuner.
>>>> if tuner-type is not properly filled, then the easiest fix would be to
>>>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
>>>> Yet, the previous logic were trying to hint the device mode, with is bad
>>>> (that's why it was changed).
>>>>
>>>> The proper change seems to add a parameter to this callback, set by the
>>>> bridge driver, informing if the device is using radio or video mode.
>>>> We need also to patch the V4L API specs, as it allows using a video node
>>>> for radio, and vice versa. This is not well supported, and it seems silly
>>>> to keep it at the specs and needing to add hints at the drivers due to
>>>> that.
>>>
>>> So, just to make sure I understand correctly what you want. The bridge or
>>> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
>>> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
>>> TV for anything else.
>>
>> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
>> Of course, I might have left something else. Btw, the older code were also
>> requiring it.
>>
>> The cx18 implementation were merged after the changes, so maybe it is not doing 
>> the right thing.
> 
> Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
> just some random driver that failed to set vf/vt->type.

G_FREQUENCY were broken just on cx18. As I said before, filling the type were
always required. Anyway, I've added a proper documentation for it. See the
patch bellow.

The same patch should be done also for G_TUNER.

>>> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
>>> in. Will we just overwrite vf->type or will we check and return -EINVAL if
>>> the app tries to set e.g. a TV frequency on /dev/radio?
>>
>> That's a very good question. What happens is that the V4L2 API used to allow
>> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
>> at the API specs. I think that this were changed on newer versions of the spec.
> 
> If you want, then I can add an extra patch to my v4 patch series that returns
> -EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
> (inconsistent with the device node's type, that is).

The reason why check_mode returns -EINVAL is that this error code may need to be
returned to the caller. You should note, however, that the bridge code may need
to be fixed if you return the check_mode error code, as otherwise the error will
simply be discarded or it it will break devices with two tuners.

For example, currently, bttv driver uses v4l2_device_call_all() for it.
So, any errors returned by it will be simply discarded.

If some bridge driver is using v4l2_device_call_until_err(), it will stop on the first
tuner that gets an error.

The solution is to implement a v4l2_device_call_until_not_err() and use it for the
tuner commands.

>>> Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
>>> opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
>>> should that change the tuner to tv mode?
>>
>> Yes. I think that some applications like kradio just keeps the device node opened.
>> If we return -EBUSY, those applications will break. The reverse is more tricky:
>> e. g. if /dev/video is streaming, I think that the bridge driver should return
>> -EBUSY if the device can't do both TV and radio at the same time, but this is
>> something that it is device-specific, so such logic, if needed, should be implemented
>> at the bridge driver.
> 
> Agreed.
>  
>>> I think the type passed to S_FREQUENCY should 1) match the device node's type
>>> (if not, then return -EINVAL) and 2) should match the current mode (if not,
>>> then return -EBUSY). So attempts to change the TV frequency when in radio
>>> mode should fail. This second rule should also be valid for S_TUNER.
>>
>> See above.
>>
>>> What should G_TUNER return on a video node when in radio mode or vice versa?
>>> For G_FREQUENCY you can still return the last used frequency, but that's
>>> much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
>>> afc all to 0 if you query G_TUNER when 'in the wrong mode'.
>>
>> The current logic should handle this case well. I tested it carefully. Basically,
>> if the device is on Radio mode, and has a separate tuner for TV, the TV tuner
>> should not touch the structure. The Radio tuner should properly fill the values.
> 
> I analyzed it again and you are correct.
> 
>> Calls to G_TUNER/G_FREQUENCY shouldn't switch the device mode, or they may break
>> applications like kradio, that may be always there during the entire KDE section.
> 
> Obviously.
> 
>>> The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
>>> so that's OK.
>>>
>>> And how do we switch between radio and TV? Right now opening the radio node
>>> will set the tuner in radio mode, and calling S_STD will change the mode to
>>> TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
>>> at the moment.
>>
>> If S_FREQUENCY is called from /dev/video (or /dev/vbi), it should set it to TV. If
>> it is called from /dev/radio, it should put the device on radio mode.
>>
>> The current logic already does that. I tested it on several devices, with both
>> tea5767 and without it.
>>
>>> What about this:
>>>
>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
>>> capture in progress, then the open should return -EBUSY. Otherwise it
>>> switches the tuner to radio mode. And it stays in radio mode until the
>>> last filehandle of /dev/radio is closed. At that point it will automatically
>>> switch back to TV mode (if there is one, of course).
>>
>> No. This would break existing applications. The mode switch should be done
>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> 
> This is not what happens today as the switch to radio occurs as soon as you open
> the radio node. It's the reason for the s_radio op.

The s_radio op is something that I wanted to remove. It was there in the past to feed
the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
final queue (I can't remember why).

I think that the hint logic were completely removed, but we may need to take a look
on the callers for s_radio. I'll check it right now.

> The V4L2 specification is silent on this topic, unfortunately. And I'm not sure
> how applications handle this.
> 
> I think only changing the mode when calling S_FREQUENCY (and S_STD) is a good
> thing since this automatic mode switch when just opening a node goes against
> the V4L2 philosophy. Just opening a node shouldn't change the state of the
> device.

Agreed.

>>> While it is in radio mode calls to S_STD and S_FREQUENCY from /dev/video
>>> will return -EBUSY. Any attempt to start streaming from /dev/video will
>>> also return -EBUSY (radio 'streaming' is in progress after all).
>>
>> For the same reason as said above, this will cause troubles for existing
>> appications.
>>
>>
>>> Effectively, S_STD no longer switches back to TV mode. That only happens when
>>> the last user of /dev/radio left. It certainly sounds a lot saner to me.
>>
>> Opening a /dev/radio device doesn't mean that radio is "streaming". A radio
>> is streaming if:
>> 	- tuner was set with S_FREQUENCY
>> 	- device is not muted.
>>
>> Btw, the console application "radio" allows you to, open a device, set a frequency,
>> unmute the device and close the radio device:
>> 	radio -qf 91.4
>>
>> So, even having the device node closed doesn't mean that the radio is not being
>> used.
> 
> Yeah, that's right. It slipped my mind when I wrote that.
> 
> Regards,
> 
> 	Hans

commit a307ec69602894c917485f331f8d1fef31c411b8
Author: Mauro Carvalho Chehab <mchehab@redhat.com>
Date:   Sun Jun 12 08:36:34 2011 -0300

    [media] tuner: fix g_frequency subdev call and properly document it
    
    The tuner type needs to be initialized before calling g_frequency.
    cx18 doesn't do it. Fix the logic and properly document the function.
    
    While here, remove a duplicated line at cx88.
    
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
index 1933d4d..5463548 100644
--- a/drivers/media/video/cx18/cx18-ioctl.c
+++ b/drivers/media/video/cx18/cx18-ioctl.c
@@ -611,6 +611,11 @@ static int cx18_g_frequency(struct file *file, void *fh,
 	if (vf->tuner != 0)
 		return -EINVAL;
 
+	if (test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
+		vf->type = V4L2_TUNER_RADIO;
+	else
+		vf->type = V4L2_TUNER_ANALOG_TV;
+
 	cx18_call_all(cx, tuner, g_frequency, vf);
 	return 0;
 }
diff --git a/drivers/media/video/cx88/cx88-video.c b/drivers/media/video/cx88/cx88-video.c
index cef4f28..13636f7 100644
--- a/drivers/media/video/cx88/cx88-video.c
+++ b/drivers/media/video/cx88/cx88-video.c
@@ -1394,7 +1394,6 @@ static int vidioc_g_frequency (struct file *file, void *priv,
 	if (unlikely(UNSET == core->board.tuner_type))
 		return -EINVAL;
 
-	/* f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV; */
 	f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 	f->frequency = core->freq;
 
diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 5748d04..34cd9b0 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1133,6 +1133,17 @@ static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	return 0;
 }
 
+/**
+ * tuner_g_frequency - Gets the tuned frequency for the tuner
+ * @sd: pointer to struct v4l2_subdev
+ * @f: opinter to struct v4l2_frequency
+ *
+ * At return, the structure f will be filled with tuner frequency,
+ * if the tuner matches the f->type.
+ * Note: f->type should be initialized before calling it.
+ * as userspace won't initialize f->type, it is up to the bridge driver
+ * to set it to either V4L2_TUNER_RADIO or V4L2_TUNER_ANALOG_TV.
+ */
 static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 {
 	struct tuner *t = to_tuner(sd);

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> Em 12-06-2011 08:36, Hans Verkuil escreveu:
>> On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
>>> Em 11-06-2011 14:27, Hans Verkuil escreveu:
>>>> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>>>>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>
>>>>>> According to the spec the tuner type field is not used when calling
>>>>>> S_TUNER: index, audmode and reserved are the only writable fields.
>>>>>>
>>>>>> So remove the type check. Instead, just set the audmode if the current
>>>>>> tuner mode is set to radio.
>>>>>
>>>>> I suspect that this patch also breaks support for a separate radio tuner.
>>>>> if tuner-type is not properly filled, then the easiest fix would be to
>>>>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
>>>>> Yet, the previous logic were trying to hint the device mode, with is bad
>>>>> (that's why it was changed).
>>>>>
>>>>> The proper change seems to add a parameter to this callback, set by the
>>>>> bridge driver, informing if the device is using radio or video mode.
>>>>> We need also to patch the V4L API specs, as it allows using a video node
>>>>> for radio, and vice versa. This is not well supported, and it seems silly
>>>>> to keep it at the specs and needing to add hints at the drivers due to
>>>>> that.
>>>>
>>>> So, just to make sure I understand correctly what you want. The bridge or
>>>> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
>>>> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
>>>> TV for anything else.
>>>
>>> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
>>> Of course, I might have left something else. Btw, the older code were also
>>> requiring it.
>>>
>>> The cx18 implementation were merged after the changes, so maybe it is not doing 
>>> the right thing.
>>
>> Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
>> just some random driver that failed to set vf/vt->type.
> 
> G_FREQUENCY were broken just on cx18. As I said before, filling the type were
> always required. Anyway, I've added a proper documentation for it. See the
> patch bellow.
> 
> The same patch should be done also for G_TUNER.
> 
>>>> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
>>>> in. Will we just overwrite vf->type or will we check and return -EINVAL if
>>>> the app tries to set e.g. a TV frequency on /dev/radio?
>>>
>>> That's a very good question. What happens is that the V4L2 API used to allow
>>> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
>>> at the API specs. I think that this were changed on newer versions of the spec.
>>
>> If you want, then I can add an extra patch to my v4 patch series that returns
>> -EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
>> (inconsistent with the device node's type, that is).
> 
> The reason why check_mode returns -EINVAL is that this error code may need to be
> returned to the caller. You should note, however, that the bridge code may need
> to be fixed if you return the check_mode error code, as otherwise the error will
> simply be discarded or it it will break devices with two tuners.
> 
> For example, currently, bttv driver uses v4l2_device_call_all() for it.
> So, any errors returned by it will be simply discarded.
> 
> If some bridge driver is using v4l2_device_call_until_err(), it will stop on the first
> tuner that gets an error.
> 
> The solution is to implement a v4l2_device_call_until_not_err() and use it for the
> tuner commands.
> 
>>>> Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
>>>> opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
>>>> should that change the tuner to tv mode?
>>>
>>> Yes. I think that some applications like kradio just keeps the device node opened.
>>> If we return -EBUSY, those applications will break. The reverse is more tricky:
>>> e. g. if /dev/video is streaming, I think that the bridge driver should return
>>> -EBUSY if the device can't do both TV and radio at the same time, but this is
>>> something that it is device-specific, so such logic, if needed, should be implemented
>>> at the bridge driver.
>>
>> Agreed.
>>  
>>>> I think the type passed to S_FREQUENCY should 1) match the device node's type
>>>> (if not, then return -EINVAL) and 2) should match the current mode (if not,
>>>> then return -EBUSY). So attempts to change the TV frequency when in radio
>>>> mode should fail. This second rule should also be valid for S_TUNER.
>>>
>>> See above.
>>>
>>>> What should G_TUNER return on a video node when in radio mode or vice versa?
>>>> For G_FREQUENCY you can still return the last used frequency, but that's
>>>> much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
>>>> afc all to 0 if you query G_TUNER when 'in the wrong mode'.
>>>
>>> The current logic should handle this case well. I tested it carefully. Basically,
>>> if the device is on Radio mode, and has a separate tuner for TV, the TV tuner
>>> should not touch the structure. The Radio tuner should properly fill the values.
>>
>> I analyzed it again and you are correct.
>>
>>> Calls to G_TUNER/G_FREQUENCY shouldn't switch the device mode, or they may break
>>> applications like kradio, that may be always there during the entire KDE section.
>>
>> Obviously.
>>
>>>> The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
>>>> so that's OK.
>>>>
>>>> And how do we switch between radio and TV? Right now opening the radio node
>>>> will set the tuner in radio mode, and calling S_STD will change the mode to
>>>> TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
>>>> at the moment.
>>>
>>> If S_FREQUENCY is called from /dev/video (or /dev/vbi), it should set it to TV. If
>>> it is called from /dev/radio, it should put the device on radio mode.
>>>
>>> The current logic already does that. I tested it on several devices, with both
>>> tea5767 and without it.
>>>
>>>> What about this:
>>>>
>>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
>>>> capture in progress, then the open should return -EBUSY. Otherwise it
>>>> switches the tuner to radio mode. And it stays in radio mode until the
>>>> last filehandle of /dev/radio is closed. At that point it will automatically
>>>> switch back to TV mode (if there is one, of course).
>>>
>>> No. This would break existing applications. The mode switch should be done
>>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
>>
>> This is not what happens today as the switch to radio occurs as soon as you open
>> the radio node. It's the reason for the s_radio op.
> 
> The s_radio op is something that I wanted to remove. It was there in the past to feed
> the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> final queue (I can't remember why).
> 
> I think that the hint logic were completely removed, but we may need to take a look
> on the callers for s_radio. I'll check it right now.
> 

The s_radio callback requires some care, as it is used on several places. It is probably
safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
troubles seem to happen at the bridge drivers that call it during open(). It should be
called only at s_frequency. I opted to keep the callback just to avoid having a bridge
driver switching its registers to radio mode, and not having the tuner following it.

If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
remove this callback from tuner, letting it to be implemented only at the audio decoders.

Cheers,
Mauro.

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Sunday, June 12, 2011 14:13:30 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> > Em 12-06-2011 08:36, Hans Verkuil escreveu:
> >>>> What about this:
> >>>>
> >>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> >>>> capture in progress, then the open should return -EBUSY. Otherwise it
> >>>> switches the tuner to radio mode. And it stays in radio mode until the
> >>>> last filehandle of /dev/radio is closed. At that point it will automatically
> >>>> switch back to TV mode (if there is one, of course).
> >>>
> >>> No. This would break existing applications. The mode switch should be done
> >>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> >>
> >> This is not what happens today as the switch to radio occurs as soon as you open
> >> the radio node. It's the reason for the s_radio op.
> > 
> > The s_radio op is something that I wanted to remove. It was there in the past to feed
> > the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> > final queue (I can't remember why).
> > 
> > I think that the hint logic were completely removed, but we may need to take a look
> > on the callers for s_radio. I'll check it right now.
> > 
> 
> The s_radio callback requires some care, as it is used on several places. It is probably
> safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
> troubles seem to happen at the bridge drivers that call it during open(). It should be
> called only at s_frequency. I opted to keep the callback just to avoid having a bridge
> driver switching its registers to radio mode, and not having the tuner following it.
> 
> If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
> remove this callback from tuner, letting it to be implemented only at the audio decoders.

Why would the audio decoders need it? If we do the mode switch when s_freq is
called, then the audio decoders can do the same and s_radio can disappear completely.

I would like that, but I'm a bit afraid of application breakage since we're changing
the behavior of /dev/radio. It seems that pretty much every video driver with radio
capability is calling s_radio during open(): bttv, ivtv, saa7134, usbvision, em28xx,
cx18, cx88, cx231xx and tm6000.

Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Andy Walls]

On Sun, 2011-06-12 at 14:30 +0200, Hans Verkuil wrote:
> On Sunday, June 12, 2011 14:13:30 Mauro Carvalho Chehab wrote:
> > Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> > > Em 12-06-2011 08:36, Hans Verkuil escreveu:
> > >>>> What about this:
> > >>>>
> > >>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> > >>>> capture in progress, then the open should return -EBUSY. Otherwise it
> > >>>> switches the tuner to radio mode. And it stays in radio mode until the
> > >>>> last filehandle of /dev/radio is closed. At that point it will automatically
> > >>>> switch back to TV mode (if there is one, of course).
> > >>>
> > >>> No. This would break existing applications. The mode switch should be done
> > >>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> > >>
> > >> This is not what happens today as the switch to radio occurs as soon as you open
> > >> the radio node. It's the reason for the s_radio op.
> > > 
> > > The s_radio op is something that I wanted to remove. It was there in the past to feed
> > > the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> > > final queue (I can't remember why).
> > > 
> > > I think that the hint logic were completely removed, but we may need to take a look
> > > on the callers for s_radio. I'll check it right now.
> > > 
> > 
> > The s_radio callback requires some care, as it is used on several places. It is probably
> > safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
> > troubles seem to happen at the bridge drivers that call it during open(). It should be
> > called only at s_frequency. I opted to keep the callback just to avoid having a bridge
> > driver switching its registers to radio mode, and not having the tuner following it.
> > 
> > If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
> > remove this callback from tuner, letting it to be implemented only at the audio decoders.
> 
> Why would the audio decoders need it? If we do the mode switch when s_freq is
> called, then the audio decoders can do the same and s_radio can disappear completely.
> 
> I would like that, but I'm a bit afraid of application breakage since we're changing
> the behavior of /dev/radio. It seems that pretty much every video driver with radio
> capability is calling s_radio during open(): bttv, ivtv, saa7134, usbvision, em28xx,
> cx18, cx88, cx231xx and tm6000.

I think ivtvhopper relies on it:

http://www.gateways-home.org/wb/pages/mycoding/--ivtvhopper-java.php

Also, per my recommendation, ivtvhopper changes radio freq by
using /dev/video24, since V4L2 priorities got in the way:

http://ivtvdriver.org/pipermail/ivtv-users/2010-December/010097.html

Regards,
Andy

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Sunday, June 12, 2011 14:53:06 Andy Walls wrote:
> On Sun, 2011-06-12 at 14:30 +0200, Hans Verkuil wrote:
> > On Sunday, June 12, 2011 14:13:30 Mauro Carvalho Chehab wrote:
> > > Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> > > > Em 12-06-2011 08:36, Hans Verkuil escreveu:
> > > >>>> What about this:
> > > >>>>
> > > >>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> > > >>>> capture in progress, then the open should return -EBUSY. Otherwise it
> > > >>>> switches the tuner to radio mode. And it stays in radio mode until the
> > > >>>> last filehandle of /dev/radio is closed. At that point it will automatically
> > > >>>> switch back to TV mode (if there is one, of course).
> > > >>>
> > > >>> No. This would break existing applications. The mode switch should be done
> > > >>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> > > >>
> > > >> This is not what happens today as the switch to radio occurs as soon as you open
> > > >> the radio node. It's the reason for the s_radio op.
> > > > 
> > > > The s_radio op is something that I wanted to remove. It was there in the past to feed
> > > > the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> > > > final queue (I can't remember why).
> > > > 
> > > > I think that the hint logic were completely removed, but we may need to take a look
> > > > on the callers for s_radio. I'll check it right now.
> > > > 
> > > 
> > > The s_radio callback requires some care, as it is used on several places. It is probably
> > > safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
> > > troubles seem to happen at the bridge drivers that call it during open(). It should be
> > > called only at s_frequency. I opted to keep the callback just to avoid having a bridge
> > > driver switching its registers to radio mode, and not having the tuner following it.
> > > 
> > > If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
> > > remove this callback from tuner, letting it to be implemented only at the audio decoders.
> > 
> > Why would the audio decoders need it? If we do the mode switch when s_freq is
> > called, then the audio decoders can do the same and s_radio can disappear completely.
> > 
> > I would like that, but I'm a bit afraid of application breakage since we're changing
> > the behavior of /dev/radio. It seems that pretty much every video driver with radio
> > capability is calling s_radio during open(): bttv, ivtv, saa7134, usbvision, em28xx,
> > cx18, cx88, cx231xx and tm6000.
> 
> I think ivtvhopper relies on it:
> 
> http://www.gateways-home.org/wb/pages/mycoding/--ivtvhopper-java.php
> 
> Also, per my recommendation, ivtvhopper changes radio freq by
> using /dev/video24, since V4L2 priorities got in the way:
> 
> http://ivtvdriver.org/pipermail/ivtv-users/2010-December/010097.html

Well, radio support for ivtv is weird and we really need a ivtv-alsa (easier
said than done). Because it is so non-standard, I am not terribly concerned
about it.

BTW, one problem with /dev/radio and ivtv (and I think cx18 might have the same
problem) is that /dev/radio can be opened only once. A second attempt to open
it will result in -EBUSY. That's a driver bug. I wonder if that's really the
problem described in the link above instead of priority handling.

Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Andy Walls]

On Sun, 2011-06-12 at 15:23 +0200, Hans Verkuil wrote:
> On Sunday, June 12, 2011 14:53:06 Andy Walls wrote:
> > On Sun, 2011-06-12 at 14:30 +0200, Hans Verkuil wrote:
> > > On Sunday, June 12, 2011 14:13:30 Mauro Carvalho Chehab wrote:
> > > > Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> > > > > Em 12-06-2011 08:36, Hans Verkuil escreveu:
> > > > >>>> What about this:
> > > > >>>>
> > > > >>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> > > > >>>> capture in progress, then the open should return -EBUSY. Otherwise it
> > > > >>>> switches the tuner to radio mode. And it stays in radio mode until the
> > > > >>>> last filehandle of /dev/radio is closed. At that point it will automatically
> > > > >>>> switch back to TV mode (if there is one, of course).
> > > > >>>
> > > > >>> No. This would break existing applications. The mode switch should be done
> > > > >>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> > > > >>
> > > > >> This is not what happens today as the switch to radio occurs as soon as you open
> > > > >> the radio node. It's the reason for the s_radio op.
> > > > > 
> > > > > The s_radio op is something that I wanted to remove. It was there in the past to feed
> > > > > the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> > > > > final queue (I can't remember why).
> > > > > 
> > > > > I think that the hint logic were completely removed, but we may need to take a look
> > > > > on the callers for s_radio. I'll check it right now.
> > > > > 
> > > > 
> > > > The s_radio callback requires some care, as it is used on several places. It is probably
> > > > safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
> > > > troubles seem to happen at the bridge drivers that call it during open(). It should be
> > > > called only at s_frequency. I opted to keep the callback just to avoid having a bridge
> > > > driver switching its registers to radio mode, and not having the tuner following it.
> > > > 
> > > > If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
> > > > remove this callback from tuner, letting it to be implemented only at the audio decoders.
> > > 
> > > Why would the audio decoders need it? If we do the mode switch when s_freq is
> > > called, then the audio decoders can do the same and s_radio can disappear completely.
> > > 
> > > I would like that, but I'm a bit afraid of application breakage since we're changing
> > > the behavior of /dev/radio. It seems that pretty much every video driver with radio
> > > capability is calling s_radio during open(): bttv, ivtv, saa7134, usbvision, em28xx,
> > > cx18, cx88, cx231xx and tm6000.
> > 
> > I think ivtvhopper relies on it:
> > 
> > http://www.gateways-home.org/wb/pages/mycoding/--ivtvhopper-java.php
> > 
> > Also, per my recommendation, ivtvhopper changes radio freq by
> > using /dev/video24, since V4L2 priorities got in the way:
> > 
> > http://ivtvdriver.org/pipermail/ivtv-users/2010-December/010097.html
> 
> Well, radio support for ivtv is weird and we really need a ivtv-alsa (easier
> said than done). Because it is so non-standard, I am not terribly concerned
> about it.

I use /dev/radio & /dev/video24 for FM radio using ivtv-radio, myself.

BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
the device nodes open, pinning the cx18-alsa and cx18 modules in kernel.
When killed, PulseAudio respawns rapidly and reopens the nodes.
Unloading cx18 for development purposes is a real pain when the
cx18-alsa module exists.


> BTW, one problem with /dev/radio and ivtv (and I think cx18 might have the same
> problem) is that /dev/radio can be opened only once. A second attempt to open
> it will result in -EBUSY. That's a driver bug. I wonder if that's really the
> problem described in the link above instead of priority handling.

Gah, I think you are right.  It probably was a multiple open() problem
on /dev/radio for the app author.

I do remember researching that cx18 and ivtv are single open()
on /dev/radio.

I also remember finding that the V4L2 spec doesn't require multiple
opens, and implies drivers need not support it in at least two places:

        "Multiple Opens
        
        In general, V4L2 devices can be opened more than once. When this
        is supported by the driver, ..."


        "Name
        v4l2-open — Open a V4L2 device
        
        ...
        
        EBUSY
                The driver does not support multiple opens and the
                device is already in use.
        ..."
        

Regards,
Andy

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Devin Heitmueller]

On Sun, Jun 12, 2011 at 9:44 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
> the device nodes open, pinning the cx18-alsa and cx18 modules in kernel.
> When killed, PulseAudio respawns rapidly and reopens the nodes.
> Unloading cx18 for development purposes is a real pain when the
> cx18-alsa module exists.

We've talked about this before, but something just feels wrong about
this.  I don't have this problem with other drivers that provide an
"-alsa" module.  For example, my ngene tree has four ALSA PCM devices
and 16 mixer controls, yet PulseAudio doesn't keep the module in use.

The more I think about this, the more I suspect this is just some sort
of subtle bug in the cx18 ALSA driver where some resource is not being
freed.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 12-06-2011 10:57, Devin Heitmueller escreveu:
> On Sun, Jun 12, 2011 at 9:44 AM, Andy Walls <awalls@md.metrocast.net> wrote:
>> BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
>> the device nodes open, pinning the cx18-alsa and cx18 modules in kernel.
>> When killed, PulseAudio respawns rapidly and reopens the nodes.
>> Unloading cx18 for development purposes is a real pain when the
>> cx18-alsa module exists.
> 
> We've talked about this before, but something just feels wrong about
> this.  I don't have this problem with other drivers that provide an
> "-alsa" module.  For example, my ngene tree has four ALSA PCM devices
> and 16 mixer controls, yet PulseAudio doesn't keep the module in use.
> 
> The more I think about this, the more I suspect this is just some sort
> of subtle bug in the cx18 ALSA driver where some resource is not being
> freed.

It is not just cx18 that have this trouble. All drivers under media/video
with *-alsa have this issue. Also, all sound drivers suffer from the same 
issue:

# lsmod|grep snd_hda
snd_hda_codec_analog    84955  1 
snd_hda_intel          25261  2 

See: pulseaudio keep the device opened, so dev refcount were incremented.

# rmmod snd_hda_codec_analog snd_hda_intel 
ERROR: Module snd_hda_codec_analog is in use
ERROR: Module snd_hda_intel is in use

The same happens, for example, with em28xx with snd-usb-audio:

# lsmod |grep snd
snd_usb_audio          91303  1 

# rmmod snd-usb-audio
ERROR: Module snd_usb_audio is in use

What happens is that open() increments the device refcount.

Maybe the ngene has some trick for allowing it, or PulseAudio has some logic to 
detect ngene (or otherwise it fails to open ngene audio nodes).

It may have some dirty ways to trick PulseAudio, for example returning -ENODEV if
the process name is pulseaudio, but I can't think on a proper kernel solution
for it.

The proper solution is to fix PulseAudio.

Cheers,
Mauro

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Andy Walls]

Devin Heitmueller <dheitmueller@kernellabs.com> wrote:

>On Sun, Jun 12, 2011 at 9:44 AM, Andy Walls <awalls@md.metrocast.net>
>wrote:
>> BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
>> the device nodes open, pinning the cx18-alsa and cx18 modules in
>kernel.
>> When killed, PulseAudio respawns rapidly and reopens the nodes.
>> Unloading cx18 for development purposes is a real pain when the
>> cx18-alsa module exists.
>
>We've talked about this before, but something just feels wrong about
>this.  I don't have this problem with other drivers that provide an
>"-alsa" module.  For example, my ngene tree has four ALSA PCM devices
>and 16 mixer controls, yet PulseAudio doesn't keep the module in use.
>
>The more I think about this, the more I suspect this is just some sort
>of subtle bug in the cx18 ALSA driver where some resource is not being
>freed.
>
>Devin
>
>-- 
>Devin J. Heitmueller - Kernel Labs
>http://www.kernellabs.com

I'll recheck with my shiny new Fedora 15 install maybe later tonight.

The only thing that springs to mind that PA may not like is no mixer controls.  Some basic code is there in cx18-alsa-mixer.c, but never registered.

Pactl does have some magic cmd to let go of the nodes, but I can never remember it.

Regards,
Andy 

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 12-06-2011 12:34, Andy Walls escreveu:
> Devin Heitmueller <dheitmueller@kernellabs.com> wrote:
> 
>> On Sun, Jun 12, 2011 at 9:44 AM, Andy Walls <awalls@md.metrocast.net>
>> wrote:
>>> BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
>>> the device nodes open, pinning the cx18-alsa and cx18 modules in
>> kernel.
>>> When killed, PulseAudio respawns rapidly and reopens the nodes.
>>> Unloading cx18 for development purposes is a real pain when the
>>> cx18-alsa module exists.
>>
>> We've talked about this before, but something just feels wrong about
>> this.  I don't have this problem with other drivers that provide an
>> "-alsa" module.  For example, my ngene tree has four ALSA PCM devices
>> and 16 mixer controls, yet PulseAudio doesn't keep the module in use.
>>
>> The more I think about this, the more I suspect this is just some sort
>> of subtle bug in the cx18 ALSA driver where some resource is not being
>> freed.
>>
>> Devin
>>
>> -- 
>> Devin J. Heitmueller - Kernel Labs
>> http://www.kernellabs.com
> 
> I'll recheck with my shiny new Fedora 15 install maybe later tonight.
> 
> The only thing that springs to mind that PA may not like is no mixer controls.  Some basic code is there in cx18-alsa-mixer.c, but never registered.
> 
> Pactl does have some magic cmd to let go of the nodes, but I can never remember it.

This should do the job:
	# yum remove -y pulseaudio

:)

Cheers,
Mauro

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Sunday, June 12, 2011 15:44:45 Andy Walls wrote:
> On Sun, 2011-06-12 at 15:23 +0200, Hans Verkuil wrote:
> > On Sunday, June 12, 2011 14:53:06 Andy Walls wrote:
> > > On Sun, 2011-06-12 at 14:30 +0200, Hans Verkuil wrote:
> > > > On Sunday, June 12, 2011 14:13:30 Mauro Carvalho Chehab wrote:
> > > > > Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> > > > > > Em 12-06-2011 08:36, Hans Verkuil escreveu:
> > > > > >>>> What about this:
> > > > > >>>>
> > > > > >>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> > > > > >>>> capture in progress, then the open should return -EBUSY. Otherwise it
> > > > > >>>> switches the tuner to radio mode. And it stays in radio mode until the
> > > > > >>>> last filehandle of /dev/radio is closed. At that point it will automatically
> > > > > >>>> switch back to TV mode (if there is one, of course).
> > > > > >>>
> > > > > >>> No. This would break existing applications. The mode switch should be done
> > > > > >>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> > > > > >>
> > > > > >> This is not what happens today as the switch to radio occurs as soon as you open
> > > > > >> the radio node. It's the reason for the s_radio op.
> > > > > > 
> > > > > > The s_radio op is something that I wanted to remove. It was there in the past to feed
> > > > > > the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> > > > > > final queue (I can't remember why).
> > > > > > 
> > > > > > I think that the hint logic were completely removed, but we may need to take a look
> > > > > > on the callers for s_radio. I'll check it right now.
> > > > > > 
> > > > > 
> > > > > The s_radio callback requires some care, as it is used on several places. It is probably
> > > > > safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
> > > > > troubles seem to happen at the bridge drivers that call it during open(). It should be
> > > > > called only at s_frequency. I opted to keep the callback just to avoid having a bridge
> > > > > driver switching its registers to radio mode, and not having the tuner following it.
> > > > > 
> > > > > If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
> > > > > remove this callback from tuner, letting it to be implemented only at the audio decoders.
> > > > 
> > > > Why would the audio decoders need it? If we do the mode switch when s_freq is
> > > > called, then the audio decoders can do the same and s_radio can disappear completely.
> > > > 
> > > > I would like that, but I'm a bit afraid of application breakage since we're changing
> > > > the behavior of /dev/radio. It seems that pretty much every video driver with radio
> > > > capability is calling s_radio during open(): bttv, ivtv, saa7134, usbvision, em28xx,
> > > > cx18, cx88, cx231xx and tm6000.
> > > 
> > > I think ivtvhopper relies on it:
> > > 
> > > http://www.gateways-home.org/wb/pages/mycoding/--ivtvhopper-java.php
> > > 
> > > Also, per my recommendation, ivtvhopper changes radio freq by
> > > using /dev/video24, since V4L2 priorities got in the way:
> > > 
> > > http://ivtvdriver.org/pipermail/ivtv-users/2010-December/010097.html
> > 
> > Well, radio support for ivtv is weird and we really need a ivtv-alsa (easier
> > said than done). Because it is so non-standard, I am not terribly concerned
> > about it.
> 
> I use /dev/radio & /dev/video24 for FM radio using ivtv-radio, myself.
> 
> BTW, the cx18-alsa module annoys me as a developer.  PulseAudio holds
> the device nodes open, pinning the cx18-alsa and cx18 modules in kernel.
> When killed, PulseAudio respawns rapidly and reopens the nodes.
> Unloading cx18 for development purposes is a real pain when the
> cx18-alsa module exists.
> 
> 
> > BTW, one problem with /dev/radio and ivtv (and I think cx18 might have the same
> > problem) is that /dev/radio can be opened only once. A second attempt to open
> > it will result in -EBUSY. That's a driver bug. I wonder if that's really the
> > problem described in the link above instead of priority handling.
> 
> Gah, I think you are right.  It probably was a multiple open() problem
> on /dev/radio for the app author.
> 
> I do remember researching that cx18 and ivtv are single open()
> on /dev/radio.
> 
> I also remember finding that the V4L2 spec doesn't require multiple
> opens, and implies drivers need not support it in at least two places:
> 
>         "Multiple Opens
>         
>         In general, V4L2 devices can be opened more than once. When this
>         is supported by the driver, ..."
> 
> 
>         "Name
>         v4l2-open — Open a V4L2 device
>         
>         ...
>         
>         EBUSY
>                 The driver does not support multiple opens and the
>                 device is already in use.
>         ..."

One of the (loooong) list of TODOs is go through the spec remove such obsolete
stuff. You really must be able to open devices multiple times. Many old drivers
didn't support multiple opens but I think almost all of them have been fixed
now. In ivtv it was probably just laziness as well.

v4l2-compliance actually tests for this (try running v4l2-compliance -d /dev/radio0).

Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Sunday, June 12, 2011 13:59:59 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 08:36, Hans Verkuil escreveu:
> > On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
> >> Em 11-06-2011 14:27, Hans Verkuil escreveu:
> >>> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
> >>>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
> >>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>
> >>>>> According to the spec the tuner type field is not used when calling
> >>>>> S_TUNER: index, audmode and reserved are the only writable fields.
> >>>>>
> >>>>> So remove the type check. Instead, just set the audmode if the current
> >>>>> tuner mode is set to radio.
> >>>>
> >>>> I suspect that this patch also breaks support for a separate radio tuner.
> >>>> if tuner-type is not properly filled, then the easiest fix would be to
> >>>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
> >>>> Yet, the previous logic were trying to hint the device mode, with is bad
> >>>> (that's why it was changed).
> >>>>
> >>>> The proper change seems to add a parameter to this callback, set by the
> >>>> bridge driver, informing if the device is using radio or video mode.
> >>>> We need also to patch the V4L API specs, as it allows using a video node
> >>>> for radio, and vice versa. This is not well supported, and it seems silly
> >>>> to keep it at the specs and needing to add hints at the drivers due to
> >>>> that.
> >>>
> >>> So, just to make sure I understand correctly what you want. The bridge or
> >>> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
> >>> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
> >>> TV for anything else.
> >>
> >> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
> >> Of course, I might have left something else. Btw, the older code were also
> >> requiring it.
> >>
> >> The cx18 implementation were merged after the changes, so maybe it is not doing 
> >> the right thing.
> > 
> > Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
> > just some random driver that failed to set vf/vt->type.
> 
> G_FREQUENCY were broken just on cx18. As I said before, filling the type were
> always required.

No, it wasn't required in the past. I'm happy with that change, but that
should be documented in v4l2-subdev.h since that's where the subdev ops are
documented. I will add such documentation in a RFCv5 patch series.

> Anyway, I've added a proper documentation for it. See the
> patch bellow.
> 
> The same patch should be done also for G_TUNER.

And s_tuner.

> 
> >>> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
> >>> in. Will we just overwrite vf->type or will we check and return -EINVAL if
> >>> the app tries to set e.g. a TV frequency on /dev/radio?
> >>
> >> That's a very good question. What happens is that the V4L2 API used to allow
> >> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
> >> at the API specs. I think that this were changed on newer versions of the spec.
> > 
> > If you want, then I can add an extra patch to my v4 patch series that returns
> > -EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
> > (inconsistent with the device node's type, that is).
> 
> The reason why check_mode returns -EINVAL is that this error code may need to be
> returned to the caller. You should note, however, that the bridge code may need
> to be fixed if you return the check_mode error code, as otherwise the error will
> simply be discarded or it it will break devices with two tuners.
> 
> For example, currently, bttv driver uses v4l2_device_call_all() for it.
> So, any errors returned by it will be simply discarded.
> 
> If some bridge driver is using v4l2_device_call_until_err(), it will stop on the first
> tuner that gets an error.
> 
> The solution is to implement a v4l2_device_call_until_not_err() and use it for the
> tuner commands.

That's not unreasonably to do at some point in time, but it doesn't actually
answer my question, which is: should the core refuse VIDIOC_S_FREQUENCY calls
where the type doesn't match the device node (i.e. radio vs tv)? I think it
makes no sense to call VIDIOC_S_FREQUENCY on a radio node with type ANALOG_TV.

...

> >>> What about this:
> >>>
> >>> Opening /dev/radio effectively starts the radio mode. So if there is TV
> >>> capture in progress, then the open should return -EBUSY. Otherwise it
> >>> switches the tuner to radio mode. And it stays in radio mode until the
> >>> last filehandle of /dev/radio is closed. At that point it will automatically
> >>> switch back to TV mode (if there is one, of course).
> >>
> >> No. This would break existing applications. The mode switch should be done
> >> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
> > 
> > This is not what happens today as the switch to radio occurs as soon as you open
> > the radio node. It's the reason for the s_radio op.
> 
> The s_radio op is something that I wanted to remove. It was there in the past to feed
> the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> final queue (I can't remember why).
> 
> I think that the hint logic were completely removed, but we may need to take a look
> on the callers for s_radio. I'll check it right now.

I'm fairly certain s_radio was added to explicitly set the tuner into radio mode.
It replaced something even uglier, but I can't remember what that was.

> > The V4L2 specification is silent on this topic, unfortunately. And I'm not sure
> > how applications handle this.
> > 
> > I think only changing the mode when calling S_FREQUENCY (and S_STD) is a good
> > thing since this automatic mode switch when just opening a node goes against
> > the V4L2 philosophy. Just opening a node shouldn't change the state of the
> > device.
> 
> Agreed.
> 
> commit a307ec69602894c917485f331f8d1fef31c411b8
> Author: Mauro Carvalho Chehab <mchehab@redhat.com>
> Date:   Sun Jun 12 08:36:34 2011 -0300
> 
>     [media] tuner: fix g_frequency subdev call and properly document it
>     
>     The tuner type needs to be initialized before calling g_frequency.
>     cx18 doesn't do it. Fix the logic and properly document the function.
>     
>     While here, remove a duplicated line at cx88.
>     
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
> index 1933d4d..5463548 100644
> --- a/drivers/media/video/cx18/cx18-ioctl.c
> +++ b/drivers/media/video/cx18/cx18-ioctl.c
> @@ -611,6 +611,11 @@ static int cx18_g_frequency(struct file *file, void *fh,
>  	if (vf->tuner != 0)
>  		return -EINVAL;
>  
> +	if (test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
> +		vf->type = V4L2_TUNER_RADIO;
> +	else
> +		vf->type = V4L2_TUNER_ANALOG_TV;
> +

NACK.

This sets the type to the current mode. But what we want is to set the type to
the current device node. That's what my RFCv4 does (and that patch requires no
driver change).

Although, to be honest, I did something similar to this patch in pvrusb2, but
that's because I have no idea to get hold of the calling device node there.
That hardware abstraction layer in pvrusb2 abstracts way too much :-(

>  	cx18_call_all(cx, tuner, g_frequency, vf);
>  	return 0;
>  }
> diff --git a/drivers/media/video/cx88/cx88-video.c b/drivers/media/video/cx88/cx88-video.c
> index cef4f28..13636f7 100644
> --- a/drivers/media/video/cx88/cx88-video.c
> +++ b/drivers/media/video/cx88/cx88-video.c
> @@ -1394,7 +1394,6 @@ static int vidioc_g_frequency (struct file *file, void *priv,
>  	if (unlikely(UNSET == core->board.tuner_type))
>  		return -EINVAL;
>  
> -	/* f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV; */
>  	f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  	f->frequency = core->freq;

Same here. No longer needed with the RFCv4 series.

>  
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index 5748d04..34cd9b0 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -1133,6 +1133,17 @@ static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>  	return 0;
>  }
>  
> +/**
> + * tuner_g_frequency - Gets the tuned frequency for the tuner
> + * @sd: pointer to struct v4l2_subdev
> + * @f: opinter to struct v4l2_frequency
> + *
> + * At return, the structure f will be filled with tuner frequency,
> + * if the tuner matches the f->type.
> + * Note: f->type should be initialized before calling it.
> + * as userspace won't initialize f->type, it is up to the bridge driver
> + * to set it to either V4L2_TUNER_RADIO or V4L2_TUNER_ANALOG_TV.
> + */
>  static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>  {
>  	struct tuner *t = to_tuner(sd);
> 
> 

I'll add documentation to v4l2-subdev.h.

Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab]

Em 12-06-2011 09:23, Hans Verkuil escreveu:
> On Sunday, June 12, 2011 13:59:59 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 08:36, Hans Verkuil escreveu:
>>> On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
>>>> Em 11-06-2011 14:27, Hans Verkuil escreveu:
>>>>> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>>>>>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>>
>>>>>>> According to the spec the tuner type field is not used when calling
>>>>>>> S_TUNER: index, audmode and reserved are the only writable fields.
>>>>>>>
>>>>>>> So remove the type check. Instead, just set the audmode if the current
>>>>>>> tuner mode is set to radio.
>>>>>>
>>>>>> I suspect that this patch also breaks support for a separate radio tuner.
>>>>>> if tuner-type is not properly filled, then the easiest fix would be to
>>>>>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
>>>>>> Yet, the previous logic were trying to hint the device mode, with is bad
>>>>>> (that's why it was changed).
>>>>>>
>>>>>> The proper change seems to add a parameter to this callback, set by the
>>>>>> bridge driver, informing if the device is using radio or video mode.
>>>>>> We need also to patch the V4L API specs, as it allows using a video node
>>>>>> for radio, and vice versa. This is not well supported, and it seems silly
>>>>>> to keep it at the specs and needing to add hints at the drivers due to
>>>>>> that.
>>>>>
>>>>> So, just to make sure I understand correctly what you want. The bridge or
>>>>> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
>>>>> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
>>>>> TV for anything else.
>>>>
>>>> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
>>>> Of course, I might have left something else. Btw, the older code were also
>>>> requiring it.
>>>>
>>>> The cx18 implementation were merged after the changes, so maybe it is not doing 
>>>> the right thing.
>>>
>>> Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
>>> just some random driver that failed to set vf/vt->type.
>>
>> G_FREQUENCY were broken just on cx18. As I said before, filling the type were
>> always required.
> 
> No, it wasn't required in the past. I'm happy with that change, but that
> should be documented in v4l2-subdev.h since that's where the subdev ops are
> documented. I will add such documentation in a RFCv5 patch series.
> 
>> Anyway, I've added a proper documentation for it. See the
>> patch bellow.
>>
>> The same patch should be done also for G_TUNER.
> 
> And s_tuner.
> 
>>
>>>>> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
>>>>> in. Will we just overwrite vf->type or will we check and return -EINVAL if
>>>>> the app tries to set e.g. a TV frequency on /dev/radio?
>>>>
>>>> That's a very good question. What happens is that the V4L2 API used to allow
>>>> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
>>>> at the API specs. I think that this were changed on newer versions of the spec.
>>>
>>> If you want, then I can add an extra patch to my v4 patch series that returns
>>> -EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
>>> (inconsistent with the device node's type, that is).
>>
>> The reason why check_mode returns -EINVAL is that this error code may need to be
>> returned to the caller. You should note, however, that the bridge code may need
>> to be fixed if you return the check_mode error code, as otherwise the error will
>> simply be discarded or it it will break devices with two tuners.
>>
>> For example, currently, bttv driver uses v4l2_device_call_all() for it.
>> So, any errors returned by it will be simply discarded.
>>
>> If some bridge driver is using v4l2_device_call_until_err(), it will stop on the first
>> tuner that gets an error.
>>
>> The solution is to implement a v4l2_device_call_until_not_err() and use it for the
>> tuner commands.
> 
> That's not unreasonably to do at some point in time, but it doesn't actually
> answer my question, which is: should the core refuse VIDIOC_S_FREQUENCY calls
> where the type doesn't match the device node (i.e. radio vs tv)? I think it
> makes no sense to call VIDIOC_S_FREQUENCY on a radio node with type ANALOG_TV.

No. The core shouldn't do it, otherwise tuner will break. The code doesn't know if
the opened device is radio or video.

> 
> ...
> 
>>>>> What about this:
>>>>>
>>>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
>>>>> capture in progress, then the open should return -EBUSY. Otherwise it
>>>>> switches the tuner to radio mode. And it stays in radio mode until the
>>>>> last filehandle of /dev/radio is closed. At that point it will automatically
>>>>> switch back to TV mode (if there is one, of course).
>>>>
>>>> No. This would break existing applications. The mode switch should be done
>>>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
>>>
>>> This is not what happens today as the switch to radio occurs as soon as you open
>>> the radio node. It's the reason for the s_radio op.
>>
>> The s_radio op is something that I wanted to remove. It was there in the past to feed
>> the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
>> final queue (I can't remember why).
>>
>> I think that the hint logic were completely removed, but we may need to take a look
>> on the callers for s_radio. I'll check it right now.
> 
> I'm fairly certain s_radio was added to explicitly set the tuner into radio mode.
> It replaced something even uglier, but I can't remember what that was.
> 
>>> The V4L2 specification is silent on this topic, unfortunately. And I'm not sure
>>> how applications handle this.
>>>
>>> I think only changing the mode when calling S_FREQUENCY (and S_STD) is a good
>>> thing since this automatic mode switch when just opening a node goes against
>>> the V4L2 philosophy. Just opening a node shouldn't change the state of the
>>> device.
>>
>> Agreed.
>>
>> commit a307ec69602894c917485f331f8d1fef31c411b8
>> Author: Mauro Carvalho Chehab <mchehab@redhat.com>
>> Date:   Sun Jun 12 08:36:34 2011 -0300
>>
>>     [media] tuner: fix g_frequency subdev call and properly document it
>>     
>>     The tuner type needs to be initialized before calling g_frequency.
>>     cx18 doesn't do it. Fix the logic and properly document the function.
>>     
>>     While here, remove a duplicated line at cx88.
>>     
>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>
>> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
>> index 1933d4d..5463548 100644
>> --- a/drivers/media/video/cx18/cx18-ioctl.c
>> +++ b/drivers/media/video/cx18/cx18-ioctl.c
>> @@ -611,6 +611,11 @@ static int cx18_g_frequency(struct file *file, void *fh,
>>  	if (vf->tuner != 0)
>>  		return -EINVAL;
>>  
>> +	if (test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
>> +		vf->type = V4L2_TUNER_RADIO;
>> +	else
>> +		vf->type = V4L2_TUNER_ANALOG_TV;
>> +
> 
> NACK.
> 
> This sets the type to the current mode. But what we want is to set the type to
> the current device node. That's what my RFCv4 does (and that patch requires no
> driver change).

I didn't get your RFCv4 patches here yet, but the fix should be at the driver: it
needs to set the type before calling g_frequency. G_FREQUENCY shouldn't change the
device mode, but, instead, to return the frequency and mode currently in usage..

> Although, to be honest, I did something similar to this patch in pvrusb2, but
> that's because I have no idea to get hold of the calling device node there.
> That hardware abstraction layer in pvrusb2 abstracts way too much :-(
> 
>>  	cx18_call_all(cx, tuner, g_frequency, vf);
>>  	return 0;
>>  }
>> diff --git a/drivers/media/video/cx88/cx88-video.c b/drivers/media/video/cx88/cx88-video.c
>> index cef4f28..13636f7 100644
>> --- a/drivers/media/video/cx88/cx88-video.c
>> +++ b/drivers/media/video/cx88/cx88-video.c
>> @@ -1394,7 +1394,6 @@ static int vidioc_g_frequency (struct file *file, void *priv,
>>  	if (unlikely(UNSET == core->board.tuner_type))
>>  		return -EINVAL;
>>  
>> -	/* f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV; */
>>  	f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>  	f->frequency = core->freq;
> 
> Same here. No longer needed with the RFCv4 series.
> 
>>  
>> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
>> index 5748d04..34cd9b0 100644
>> --- a/drivers/media/video/tuner-core.c
>> +++ b/drivers/media/video/tuner-core.c
>> @@ -1133,6 +1133,17 @@ static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * tuner_g_frequency - Gets the tuned frequency for the tuner
>> + * @sd: pointer to struct v4l2_subdev
>> + * @f: opinter to struct v4l2_frequency
>> + *
>> + * At return, the structure f will be filled with tuner frequency,
>> + * if the tuner matches the f->type.
>> + * Note: f->type should be initialized before calling it.
>> + * as userspace won't initialize f->type, it is up to the bridge driver
>> + * to set it to either V4L2_TUNER_RADIO or V4L2_TUNER_ANALOG_TV.
>> + */
>>  static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>>  {
>>  	struct tuner *t = to_tuner(sd);
>>
>>
> 
> I'll add documentation to v4l2-subdev.h.

The doc should also be at tuner-core.

> 
> Regards,
> 
> 	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Hans Verkuil]

On Sunday, June 12, 2011 16:11:30 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 09:23, Hans Verkuil escreveu:
> > That's not unreasonably to do at some point in time, but it doesn't actually
> > answer my question, which is: should the core refuse VIDIOC_S_FREQUENCY calls
> > where the type doesn't match the device node (i.e. radio vs tv)? I think it
> > makes no sense to call VIDIOC_S_FREQUENCY on a radio node with type ANALOG_TV.
> 
> No. The core shouldn't do it, otherwise tuner will break. The code doesn't know if
> the opened device is radio or video.

I don't follow this. In v4l2-ioctl.c it is easy to tell if the opened device
is radio or not by looking at vfd->vfl_type.

> >> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
> >> index 1933d4d..5463548 100644
> >> --- a/drivers/media/video/cx18/cx18-ioctl.c
> >> +++ b/drivers/media/video/cx18/cx18-ioctl.c
> >> @@ -611,6 +611,11 @@ static int cx18_g_frequency(struct file *file, void *fh,
> >>  	if (vf->tuner != 0)
> >>  		return -EINVAL;
> >>  
> >> +	if (test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
> >> +		vf->type = V4L2_TUNER_RADIO;
> >> +	else
> >> +		vf->type = V4L2_TUNER_ANALOG_TV;
> >> +
> > 
> > NACK.
> > 
> > This sets the type to the current mode. But what we want is to set the type to
> > the current device node. That's what my RFCv4 does (and that patch requires no
> > driver change).
> 
> I didn't get your RFCv4 patches here yet, but the fix should be at the driver: it
> needs to set the type before calling g_frequency. G_FREQUENCY shouldn't change the
> device mode, but, instead, to return the frequency and mode currently in usage..

Why bother changing drivers (and probably missing a few) if you can do it
in v4l2-ioctl.c and let drivers just pass it on?

This is the patch in question, BTW:

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",

Neither of these three ioctls will change the tuner mode, BTW. With this
code in place drivers that use video_ioctl2 can now rely on the type field
being a sensible value.
 
Regards,

	Hans

Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Andy Walls]

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>On Sunday, June 12, 2011 16:11:30 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 09:23, Hans Verkuil escreveu:
>> > That's not unreasonably to do at some point in time, but it doesn't
>actually
>> > answer my question, which is: should the core refuse
>VIDIOC_S_FREQUENCY calls
>> > where the type doesn't match the device node (i.e. radio vs tv)? I
>think it
>> > makes no sense to call VIDIOC_S_FREQUENCY on a radio node with type
>ANALOG_TV.
>> 
>> No. The core shouldn't do it, otherwise tuner will break. The code
>doesn't know if
>> the opened device is radio or video.
>
>I don't follow this. In v4l2-ioctl.c it is easy to tell if the opened
>device
>is radio or not by looking at vfd->vfl_type.
>
>> >> diff --git a/drivers/media/video/cx18/cx18-ioctl.c
>b/drivers/media/video/cx18/cx18-ioctl.c
>> >> index 1933d4d..5463548 100644
>> >> --- a/drivers/media/video/cx18/cx18-ioctl.c
>> >> +++ b/drivers/media/video/cx18/cx18-ioctl.c
>> >> @@ -611,6 +611,11 @@ static int cx18_g_frequency(struct file
>*file, void *fh,
>> >>  	if (vf->tuner != 0)
>> >>  		return -EINVAL;
>> >>  
>> >> +	if (test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
>> >> +		vf->type = V4L2_TUNER_RADIO;
>> >> +	else
>> >> +		vf->type = V4L2_TUNER_ANALOG_TV;
>> >> +
>> > 
>> > NACK.
>> > 
>> > This sets the type to the current mode. But what we want is to set
>the type to
>> > the current device node. That's what my RFCv4 does (and that patch
>requires no
>> > driver change).
>> 
>> I didn't get your RFCv4 patches here yet, but the fix should be at
>the driver: it
>> needs to set the type before calling g_frequency. G_FREQUENCY
>shouldn't change the
>> device mode, but, instead, to return the frequency and mode currently
>in usage..
>
>Why bother changing drivers (and probably missing a few) if you can do
>it
>in v4l2-ioctl.c and let drivers just pass it on?
>
>This is the patch in question, BTW:
>
>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",
>
>Neither of these three ioctls will change the tuner mode, BTW. With
>this
>code in place drivers that use video_ioctl2 can now rely on the type
>field
>being a sensible value.
> 
>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

Right.  In fact for s_tuner (and g_tuner) the spec implies that app writers should not fill in the field. 

BTW, S_tuner only really changes analog audio decoding right?

Regards,
Andy