tuner-core: fix g_freq/s_std and g/s_tuner

tuner-core: fix g_freq/s_std and g/s_tuner

[Hans Verkuil]

OK, this is the fourth version of this patch series.

The first five patches are the same as before. But for fixing the g_frequency
and g/s_tuner subdev ops I've decided to follow Mauro's lead and let the core
fill in the tuner type for those ioctls. Trying to do this in the drivers is
a tricky business, but doing this in video_ioctl2 is dead easy.

The only driver not using video_ioctl2 and that has tuner support as well is
pvrusb2, so I did that manually. Mike, can you review that single patch? All
it does is fill in v4l2_tuner's type based on whether the radio or TV is
active.

The last patch fixes a typo in the bttv radio s_tuner implementation making
VIDIOC_S_TUNER work again.

This patch series has been tested with bttv, cx18, ivtv and pvrusb2.

[RFCv4 PATCH 1/8] 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

[RFCv4 PATCH 2/8] 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

Re: [RFCv4 PATCH 2/8] tuner-core: change return type of set_mode_freq to bool

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> 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 supportthe requested mode. So change the return type to
> bool.

NACK. Tuner core doesn't return the error code just because the subdev
functions don't allow, currently, at the multiple tuners case.

> 
> 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)

[RFCv4 PATCH 3/8] 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 ee43e0a..462a8f4 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

Re: [RFCv4 PATCH 3/8] tuner-core: simplify the standard fixup.

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Get rid of a number of unnecessary tuner_dbg messages by simplifying
> the std fixup function.

Seems ok to me.
> 
> 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 ee43e0a..462a8f4 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;
>  }

[RFCv4 PATCH 4/8] 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 |   60 +++++++++++++++++++++-----------------
 1 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 462a8f4..bf7fc33 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);
+	if (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;
 }
 
@@ -1154,13 +1159,14 @@ 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)
+	if (t->mode == V4L2_TUNER_RADIO) {
 		t->audmode = vt->audmode;
+		set_freq(t, 0);
+	}
 
 	return 0;
 }
@@ -1195,8 +1201,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

Re: [RFCv4 PATCH 4/8] tuner-core: fix s_std and s_tuner.

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> 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.

I need to better analyse and test this patch.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/tuner-core.c |   60 +++++++++++++++++++++-----------------
>  1 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index 462a8f4..bf7fc33 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);
> +	if (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;
>  }
>  
> @@ -1154,13 +1159,14 @@ 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)
> +	if (t->mode == V4L2_TUNER_RADIO) {
>  		t->audmode = vt->audmode;
> +		set_freq(t, 0);
> +	}
>  
>  	return 0;
>  }
> @@ -1195,8 +1201,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;
>  }
>  

[RFCv4 PATCH 5/8] tuner-core: fix tuner_resume: use t->mode instead of t->type.

[Hans Verkuil]

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

set_mode is called with t->type, which is the tuner type. Instead, use
t->mode which is the actual tuner mode (i.e. radio vs tv).

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

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index bf7fc33..ffe5de3 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1201,7 +1201,7 @@ static int tuner_resume(struct i2c_client *c)
 	tuner_dbg("resume\n");
 
 	if (!t->standby)
-		if (set_mode(t, t->type))
+		if (set_mode(t, t->mode))
 			set_freq(t, 0);
 	return 0;
 }
-- 
1.7.1

Re: [RFCv4 PATCH 5/8] tuner-core: fix tuner_resume: use t->mode instead of t->type.

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> set_mode is called with t->type, which is the tuner type. Instead, use
> t->mode which is the actual tuner mode (i.e. radio vs tv).
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Not tested, but it seems ok.

> ---
>  drivers/media/video/tuner-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index bf7fc33..ffe5de3 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -1201,7 +1201,7 @@ static int tuner_resume(struct i2c_client *c)
>  	tuner_dbg("resume\n");
>  
>  	if (!t->standby)
> -		if (set_mode(t, t->type))
> +		if (set_mode(t, t->mode))
>  			set_freq(t, 0);
>  	return 0;
>  }

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

[Hans Verkuil]

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

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

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

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

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

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

[Andy Walls]

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


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

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

-Andy

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

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The subdevs are supposed to receive a valid tuner type for the g_frequency
> and g/s_tuner subdev ops. Some drivers do this, others don't. So prefill
> this in v4l2-ioctl.c based on whether the device node from which this is
> called is a radio node or not.
> 
> The spec does not require applications to fill in the type, and if they
> leave it at 0 then the 'supported_mode' call in tuner-core.c will return
> false and the ioctl does nothing.

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

Unfortunately, this is not the case, currently.

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


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

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

[Hans Verkuil]

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

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

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

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

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

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

Regards,

	Hans

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

[Mauro Carvalho Chehab]

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

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

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

I'm OK with that.

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

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

[Hans Verkuil]

On Sunday, June 12, 2011 19:08:03 Mauro Carvalho Chehab wrote:
> > I think in the longer term we need to change the spec so that:
> > 
> > 1) Opening a radio node no longer switches to radio mode. Instead, you need to
> >    call VIDIOC_S_FREQUENCY for that.
> > 2) When VIDIOC_S_FREQUENCY the type field should match the video/radio node it
> >    is called on. So for /dev/radio type should be RADIO, for others it should be
> >    ANALOG_TV. Otherwise -EINVAL is called.
> > 
> > So this might be a good feature removal for 3.2 or 3.3.
> 
> I'm OK with that.

How about this:

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 1a9446b..9df0e09 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -600,3 +600,25 @@ Why:	Superseded by the UVCIOC_CTRL_QUERY ioctl.
 Who:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
 
 ----------------------------
+
+What:	For VIDIOC_S_FREQUENCY the type field must match the device node's type.
+	If not, return -EINVAL.
+When:	3.2
+Why:	It makes no sense to switch the tuner to radio mode by calling
+	VIDIOC_S_FREQUENCY on a video node, or to switch the tuner to tv mode by
+	calling VIDIOC_S_FREQUENCY on a radio node. This is the first step of a
+	move to more consistent handling of tv and radio tuners.
+Who:	Hans Verkuil <hans.verkuil@cisco.com>
+
+----------------------------
+
+What:	Opening a radio device node will no longer automatically switch the
+	tuner mode from tv to radio.
+When:	3.3
+Why:	Just opening a V4L device should not change the state of the hardware
+	like that. It's very unexpected and against the V4L spec. Instead, you
+	switch to radio mode by calling VIDIOC_S_FREQUENCY. This is the second
+	and last step of the move to consistent handling of tv and radio tuners.
+Who:	Hans Verkuil <hans.verkuil@cisco.com>
+
+----------------------------

Regards,

	Hans

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

[Mauro Carvalho Chehab]

Em 12-06-2011 16:41, Hans Verkuil escreveu:
> On Sunday, June 12, 2011 19:08:03 Mauro Carvalho Chehab wrote:
>>> I think in the longer term we need to change the spec so that:
>>>
>>> 1) Opening a radio node no longer switches to radio mode. Instead, you need to
>>>    call VIDIOC_S_FREQUENCY for that.
>>> 2) When VIDIOC_S_FREQUENCY the type field should match the video/radio node it
>>>    is called on. So for /dev/radio type should be RADIO, for others it should be
>>>    ANALOG_TV. Otherwise -EINVAL is called.
>>>
>>> So this might be a good feature removal for 3.2 or 3.3.
>>
>> I'm OK with that.
> 
> How about this:
> 
> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> index 1a9446b..9df0e09 100644
> --- a/Documentation/feature-removal-schedule.txt
> +++ b/Documentation/feature-removal-schedule.txt
> @@ -600,3 +600,25 @@ Why:	Superseded by the UVCIOC_CTRL_QUERY ioctl.
>  Who:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>  
>  ----------------------------
> +
> +What:	For VIDIOC_S_FREQUENCY the type field must match the device node's type.
> +	If not, return -EINVAL.
> +When:	3.2
> +Why:	It makes no sense to switch the tuner to radio mode by calling
> +	VIDIOC_S_FREQUENCY on a video node, or to switch the tuner to tv mode by
> +	calling VIDIOC_S_FREQUENCY on a radio node. This is the first step of a
> +	move to more consistent handling of tv and radio tuners.
> +Who:	Hans Verkuil <hans.verkuil@cisco.com>
> +
> +----------------------------
> +
> +What:	Opening a radio device node will no longer automatically switch the
> +	tuner mode from tv to radio.
> +When:	3.3
> +Why:	Just opening a V4L device should not change the state of the hardware
> +	like that. It's very unexpected and against the V4L spec. Instead, you
> +	switch to radio mode by calling VIDIOC_S_FREQUENCY. This is the second
> +	and last step of the move to consistent handling of tv and radio tuners.
> +Who:	Hans Verkuil <hans.verkuil@cisco.com>
> +
> +----------------------------
> 
> Regards,
> 
> 	Hans
Seems fine to me.

Thanks!
Mauro

[RFCv4 PATCH 7/8] pvrusb2: fix g/s_tuner support.

[Hans Verkuil]

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

The tuner-core subdev requires that the type field of v4l2_tuner is
filled in correctly. This is done in v4l2-ioctl.c, but pvrusb2 doesn't
use that yet, so we have to do it manually based on whether the current
input is radio or not.

Tested with my pvrusb2.

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

diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
index 9d0dd08..e98d382 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -3046,6 +3046,8 @@ static void pvr2_subdev_update(struct pvr2_hdw *hdw)
 	if (hdw->input_dirty || hdw->audiomode_dirty || hdw->force_dirty) {
 		struct v4l2_tuner vt;
 		memset(&vt, 0, sizeof(vt));
+		vt.type = (hdw->input_val == PVR2_CVAL_INPUT_RADIO) ?
+			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 		vt.audmode = hdw->audiomode_val;
 		v4l2_device_call_all(&hdw->v4l2_dev, 0, tuner, s_tuner, &vt);
 	}
@@ -5171,6 +5173,8 @@ void pvr2_hdw_status_poll(struct pvr2_hdw *hdw)
 {
 	struct v4l2_tuner *vtp = &hdw->tuner_signal_info;
 	memset(vtp, 0, sizeof(*vtp));
+	vtp->type = (hdw->input_val == PVR2_CVAL_INPUT_RADIO) ?
+		V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 	hdw->tuner_signal_stale = 0;
 	/* Note: There apparently is no replacement for VIDIOC_CROPCAP
 	   using v4l2-subdev - therefore we can't support that AT ALL right
-- 
1.7.1

Re: [RFCv4 PATCH 7/8] pvrusb2: fix g/s_tuner support.

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The tuner-core subdev requires that the type field of v4l2_tuner is
> filled in correctly. This is done in v4l2-ioctl.c, but pvrusb2 doesn't
> use that yet, so we have to do it manually based on whether the current
> input is radio or not.
> 
> Tested with my pvrusb2.

OK.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/pvrusb2/pvrusb2-hdw.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> index 9d0dd08..e98d382 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> @@ -3046,6 +3046,8 @@ static void pvr2_subdev_update(struct pvr2_hdw *hdw)
>  	if (hdw->input_dirty || hdw->audiomode_dirty || hdw->force_dirty) {
>  		struct v4l2_tuner vt;
>  		memset(&vt, 0, sizeof(vt));
> +		vt.type = (hdw->input_val == PVR2_CVAL_INPUT_RADIO) ?
> +			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  		vt.audmode = hdw->audiomode_val;
>  		v4l2_device_call_all(&hdw->v4l2_dev, 0, tuner, s_tuner, &vt);
>  	}
> @@ -5171,6 +5173,8 @@ void pvr2_hdw_status_poll(struct pvr2_hdw *hdw)
>  {
>  	struct v4l2_tuner *vtp = &hdw->tuner_signal_info;
>  	memset(vtp, 0, sizeof(*vtp));
> +	vtp->type = (hdw->input_val == PVR2_CVAL_INPUT_RADIO) ?
> +		V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  	hdw->tuner_signal_stale = 0;
>  	/* Note: There apparently is no replacement for VIDIOC_CROPCAP
>  	   using v4l2-subdev - therefore we can't support that AT ALL right

[RFCv4 PATCH 8/8] bttv: fix s_tuner for radio.

[Hans Verkuil]

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

Fix typo: g_tuner should have been s_tuner.

Tested with a bttv card.

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

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index a97cf27..834a483 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -3474,7 +3474,7 @@ static int radio_s_tuner(struct file *file, void *priv,
 	if (0 != t->index)
 		return -EINVAL;
 
-	bttv_call_all(btv, tuner, g_tuner, t);
+	bttv_call_all(btv, tuner, s_tuner, t);
 	return 0;
 }
 
-- 
1.7.1

Re: [RFCv4 PATCH 8/8] bttv: fix s_tuner for radio.

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Fix typo: g_tuner should have been s_tuner.
> 
> Tested with a bttv card.

OK.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/bt8xx/bttv-driver.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
> index a97cf27..834a483 100644
> --- a/drivers/media/video/bt8xx/bttv-driver.c
> +++ b/drivers/media/video/bt8xx/bttv-driver.c
> @@ -3474,7 +3474,7 @@ static int radio_s_tuner(struct file *file, void *priv,
>  	if (0 != t->index)
>  		return -EINVAL;
>  
> -	bttv_call_all(btv, tuner, g_tuner, t);
> +	bttv_call_all(btv, tuner, s_tuner, t);
>  	return 0;
>  }
>  

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 12-06-2011 07:59, Hans Verkuil escreveu:
> 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.

I prefer to keep returning -EINVAL. This is the proper thing to do, and
to return the result to the caller. A fixme should be added though, so,
after someone add a subdev call that would properly handle the -EINVAL
code for multiple tuners, the functions should return the error code
instead of 0.

> 
> 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)

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 07:59, Hans Verkuil escreveu:
> > 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.
> 
> I prefer to keep returning -EINVAL. This is the proper thing to do, and
> to return the result to the caller. A fixme should be added though, so,
> after someone add a subdev call that would properly handle the -EINVAL
> code for multiple tuners, the functions should return the error code
> instead of 0.

No, you can't return -EINVAL here. It is the responsibility of the bridge
driver to determine whether there is e.g. a radio tuner. So if one of these
tuner subdevs is called with mode radio while it is a tv tuner, then that
is not an error, but instead it is a request that can safely be ignored
as not relevant for that tuner. It is up to the bridge driver to ensure
that a tuner is loaded that is actually valid for the radio mode.

Subdev ops should only return errors when there is a real problem (e.g. i2c
errors) and should just return 0 if a request is not for them.

That's why I posted these first two patches: these functions suggest that you
can return an error if the mode doesn't match when you really cannot.

If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
that is returned should match a real error (e.g. an i2c error), not that one
of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
otherwise there wouldn't be a /dev/radio node.

I firmly believe that the RFCv4 series is correct and just needs an additional
patch adding some documentation.

That said, it would make sense to move the first three patches to the end
instead if you prefer. Since these are cleanups, not bug fixes like the others.

Regards,

	Hans

> 
> > 
> > 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)
> 
> 

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 12-06-2011 13:07, Hans Verkuil escreveu:
> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>> 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.
>>
>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>> to return the result to the caller. A fixme should be added though, so,
>> after someone add a subdev call that would properly handle the -EINVAL
>> code for multiple tuners, the functions should return the error code
>> instead of 0.
> 
> No, you can't return -EINVAL here. It is the responsibility of the bridge
> driver to determine whether there is e.g. a radio tuner. So if one of these
> tuner subdevs is called with mode radio while it is a tv tuner, then that
> is not an error, but instead it is a request that can safely be ignored
> as not relevant for that tuner. It is up to the bridge driver to ensure
> that a tuner is loaded that is actually valid for the radio mode.
> 
> Subdev ops should only return errors when there is a real problem (e.g. i2c
> errors) and should just return 0 if a request is not for them.
> 
> That's why I posted these first two patches: these functions suggest that you
> can return an error if the mode doesn't match when you really cannot.
> 
> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
> that is returned should match a real error (e.g. an i2c error), not that one
> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
> otherwise there wouldn't be a /dev/radio node.
> 
> I firmly believe that the RFCv4 series is correct and just needs an additional
> patch adding some documentation.
> 
> That said, it would make sense to move the first three patches to the end
> instead if you prefer. Since these are cleanups, not bug fixes like the others.


The errors at tuner should be propagated. If there's just one tuner, the error
code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
implemented at v4l2_device_call_until_err().

In order to implement the correct behaviour, the tuner driver should return -EINVAL if
check_mode/set_mode fails. However, this breaks any bridge that may be using 
v4l2_device_call_until_err(). That's why the current code returns 0.

The proper fix for it is:

	1) create a call_all function that returns 0 if one of the subdevs returned 0,
or returns an error code otherwise;
	2) change all bridge calls to tuner stuff to the new call_all function;
	3) return the check_mode/set_mode error to the bridge.

One alternative for (1) would be to simply change the v4l2_device_call_all() to return 0 if
one of the subdrivers returned 0. Something like (not tested):


#define __v4l2_device_call_subdevs_p(v4l2_dev, sd, cond, o, f, args..$
({                                                                      \
        long __rc = 0, __err = 0;                                       \
                                                                        \
        list_for_each_entry((sd), &(v4l2_dev)->subdevs, list) {		\
                if ((cond) && (sd)->ops->o && (sd)->ops->o->f) {	\
                        __err = (sd)->ops->o->f((sd) , ##args);		\
			if (_err)					\
	                        __rc = __err;                           \
		}							\
        }                                                               \
        __rc;								\
})


#define v4l2_device_call_all(v4l2_dev, grpid, o, f, args...)            \
        do {                                                            \
                struct v4l2_subdev *__sd;				\
                                                                       	\
                __v4l2_device_call_subdevs_p(v4l2_dev, __sd,		\
                        !(grpid) || __sd->grp_id == (grpid), o, f ,     \
                        ##args);                                        \
        } while (0)


As it currently doesn't return any error, this shouldn't break any driver 
(as nobody expects an error code there). We'll need to review the bridge 
drivers anyway, so that they'll return the error code from v4l_device_call().

Cheers,
Mauro.

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 12-06-2011 14:27, Mauro Carvalho Chehab escreveu:
> Em 12-06-2011 13:07, Hans Verkuil escreveu:
>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>>> 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.
>>>
>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>>> to return the result to the caller. A fixme should be added though, so,
>>> after someone add a subdev call that would properly handle the -EINVAL
>>> code for multiple tuners, the functions should return the error code
>>> instead of 0.
>>
>> No, you can't return -EINVAL here. It is the responsibility of the bridge
>> driver to determine whether there is e.g. a radio tuner. So if one of these
>> tuner subdevs is called with mode radio while it is a tv tuner, then that
>> is not an error, but instead it is a request that can safely be ignored
>> as not relevant for that tuner. It is up to the bridge driver to ensure
>> that a tuner is loaded that is actually valid for the radio mode.
>>
>> Subdev ops should only return errors when there is a real problem (e.g. i2c
>> errors) and should just return 0 if a request is not for them.
>>
>> That's why I posted these first two patches: these functions suggest that you
>> can return an error if the mode doesn't match when you really cannot.
>>
>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
>> that is returned should match a real error (e.g. an i2c error), not that one
>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
>> otherwise there wouldn't be a /dev/radio node.
>>
>> I firmly believe that the RFCv4 series is correct and just needs an additional
>> patch adding some documentation.
>>
>> That said, it would make sense to move the first three patches to the end
>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
> 
> 
> The errors at tuner should be propagated. If there's just one tuner, the error
> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
> implemented at v4l2_device_call_until_err().
> 
> In order to implement the correct behaviour, the tuner driver should return -EINVAL if
> check_mode/set_mode fails. However, this breaks any bridge that may be using 
> v4l2_device_call_until_err(). That's why the current code returns 0.
> 
> The proper fix for it is:
> 
> 	1) create a call_all function that returns 0 if one of the subdevs returned 0,
> or returns an error code otherwise;
> 	2) change all bridge calls to tuner stuff to the new call_all function;
> 	3) return the check_mode/set_mode error to the bridge.
> 
> One alternative for (1) would be to simply change the v4l2_device_call_all() to return 0 if
> one of the subdrivers returned 0. Something like (not tested):
> 

Sorry, wrong logic. It should be, instead:

#define __v4l2_device_call_subdevs_p(v4l2_dev, sd, cond, o, f, args..$
({                                                                      \
        long __rc = -ENOIOCTLCMD, __err = _rc;                          \
                                                                        \
        list_for_each_entry((sd), &(v4l2_dev)->subdevs, list) {	  	\
                if ((cond) && (sd)->ops->o && (sd)->ops->o->f) {	\
                        __err = (sd)->ops->o->f((sd) , ##args);	        \
			if (!_err)					\
 	                        __rc = 0;                               \
		}							\
        }                                                               \
	(__rc == 0) ? 0 : __err;					\
})

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 13:07, Hans Verkuil escreveu:
> > On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
> >> Em 12-06-2011 07:59, Hans Verkuil escreveu:
> >>> 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.
> >>
> >> I prefer to keep returning -EINVAL. This is the proper thing to do, and
> >> to return the result to the caller. A fixme should be added though, so,
> >> after someone add a subdev call that would properly handle the -EINVAL
> >> code for multiple tuners, the functions should return the error code
> >> instead of 0.
> > 
> > No, you can't return -EINVAL here. It is the responsibility of the bridge
> > driver to determine whether there is e.g. a radio tuner. So if one of these
> > tuner subdevs is called with mode radio while it is a tv tuner, then that
> > is not an error, but instead it is a request that can safely be ignored
> > as not relevant for that tuner. It is up to the bridge driver to ensure
> > that a tuner is loaded that is actually valid for the radio mode.
> > 
> > Subdev ops should only return errors when there is a real problem (e.g. i2c
> > errors) and should just return 0 if a request is not for them.
> > 
> > That's why I posted these first two patches: these functions suggest that you
> > can return an error if the mode doesn't match when you really cannot.
> > 
> > If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
> > that is returned should match a real error (e.g. an i2c error), not that one
> > of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
> > otherwise there wouldn't be a /dev/radio node.
> > 
> > I firmly believe that the RFCv4 series is correct and just needs an additional
> > patch adding some documentation.
> > 
> > That said, it would make sense to move the first three patches to the end
> > instead if you prefer. Since these are cleanups, not bug fixes like the others.
> 
> 
> The errors at tuner should be propagated. If there's just one tuner, the error
> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
> implemented at v4l2_device_call_until_err().

Sorry, but no, that's not true. You are trying to use the error codes from tuner
subdevs to determine whether none of the tuner subdevs support a certain tuner mode.

E.g., you want to change something for a radio tuner and there are no radio tuner
subdevs. But that's the job of the bridge driver to check. That has the overview,
the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
if they should handle it and ignore it otherwise.

Only if they have to handle it will they return a possible error. The big problem
with trying to use subdev errors codes for this is that you don't see the difference
between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
that returns -EINVAL just because it didn't understand the tuner mode.

So the bridge may return -EINVAL to the application instead of the real error, which
is -EIO.

That's the whole principle behind the sub-device model: sub-devices do not know
about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
radio tuner, then the bridge driver is the one that should detect that and return
-EINVAL.

Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
the tuner mode against the device node it's called on. But that requires tightening
of the V4L2 spec first.

Regards,

	Hans

> In order to implement the correct behaviour, the tuner driver should return -EINVAL if
> check_mode/set_mode fails. However, this breaks any bridge that may be using 
> v4l2_device_call_until_err(). That's why the current code returns 0.
> 
> The proper fix for it is:
> 
> 	1) create a call_all function that returns 0 if one of the subdevs returned 0,
> or returns an error code otherwise;
> 	2) change all bridge calls to tuner stuff to the new call_all function;
> 	3) return the check_mode/set_mode error to the bridge.
> 
> One alternative for (1) would be to simply change the v4l2_device_call_all() to return 0 if
> one of the subdrivers returned 0. Something like (not tested):
> 
> 
> #define __v4l2_device_call_subdevs_p(v4l2_dev, sd, cond, o, f, args..$
> ({                                                                      \
>         long __rc = 0, __err = 0;                                       \
>                                                                         \
>         list_for_each_entry((sd), &(v4l2_dev)->subdevs, list) {		\
>                 if ((cond) && (sd)->ops->o && (sd)->ops->o->f) {	\
>                         __err = (sd)->ops->o->f((sd) , ##args);		\
> 			if (_err)					\
> 	                        __rc = __err;                           \
> 		}							\
>         }                                                               \
>         __rc;								\
> })
> 
> 
> #define v4l2_device_call_all(v4l2_dev, grpid, o, f, args...)            \
>         do {                                                            \
>                 struct v4l2_subdev *__sd;				\
>                                                                        	\
>                 __v4l2_device_call_subdevs_p(v4l2_dev, __sd,		\
>                         !(grpid) || __sd->grp_id == (grpid), o, f ,     \
>                         ##args);                                        \
>         } while (0)
> 
> 
> As it currently doesn't return any error, this shouldn't break any driver 
> (as nobody expects an error code there). We'll need to review the bridge 
> drivers anyway, so that they'll return the error code from v4l_device_call().
> 
> Cheers,
> Mauro.
> 

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 12-06-2011 15:09, Hans Verkuil escreveu:
> On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 13:07, Hans Verkuil escreveu:
>>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>>>> 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.
>>>>
>>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>>>> to return the result to the caller. A fixme should be added though, so,
>>>> after someone add a subdev call that would properly handle the -EINVAL
>>>> code for multiple tuners, the functions should return the error code
>>>> instead of 0.
>>>
>>> No, you can't return -EINVAL here. It is the responsibility of the bridge
>>> driver to determine whether there is e.g. a radio tuner. So if one of these
>>> tuner subdevs is called with mode radio while it is a tv tuner, then that
>>> is not an error, but instead it is a request that can safely be ignored
>>> as not relevant for that tuner. It is up to the bridge driver to ensure
>>> that a tuner is loaded that is actually valid for the radio mode.
>>>
>>> Subdev ops should only return errors when there is a real problem (e.g. i2c
>>> errors) and should just return 0 if a request is not for them.
>>>
>>> That's why I posted these first two patches: these functions suggest that you
>>> can return an error if the mode doesn't match when you really cannot.
>>>
>>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
>>> that is returned should match a real error (e.g. an i2c error), not that one
>>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
>>> otherwise there wouldn't be a /dev/radio node.
>>>
>>> I firmly believe that the RFCv4 series is correct and just needs an additional
>>> patch adding some documentation.
>>>
>>> That said, it would make sense to move the first three patches to the end
>>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
>>
>>
>> The errors at tuner should be propagated. If there's just one tuner, the error
>> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
>> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
>> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
>> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
>> implemented at v4l2_device_call_until_err().
> 
> Sorry, but no, that's not true. You are trying to use the error codes from tuner
> subdevs to determine whether none of the tuner subdevs support a certain tuner mode.

Not only that. There are some cases where the tuner driver may not bind for some reason.
So, even if the bridge driver does support a certain mode, a call to G_TUNER may fail
(for example, if tea5767 probe failed). Only with a proper return code, the bridge driver
can detect if the driver found some issue.

> E.g., you want to change something for a radio tuner and there are no radio tuner
> subdevs. But that's the job of the bridge driver to check. That has the overview,
> the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
> if they should handle it and ignore it otherwise.
> 
> Only if they have to handle it will they return a possible error. The big problem
> with trying to use subdev errors codes for this is that you don't see the difference
> between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
> that returns -EINVAL just because it didn't understand the tuner mode.
> 
> So the bridge may return -EINVAL to the application instead of the real error, which
> is -EIO.

An -EIO would also be discarded, as errors at v4l2_device_call_all() calls don't return
anything. So, currently, the bridge has to assume that no errors happened and return 0.

> That's the whole principle behind the sub-device model: sub-devices do not know
> about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
> radio tuner, then the bridge driver is the one that should detect that and return
> -EINVAL.
> 
> Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
> the tuner mode against the device node it's called on. But that requires tightening
> of the V4L2 spec first.

Yes, video_ioctl2 (or, currently, the bridge driver) shouldn't allow an invalid operation.
But if the call returns an error, this error should be propagated. 

Also, as I've explained before, even adding the invalid mode check inside video_ioctl,
you may still have errors if the registered tuners don't support the mode, because one 
of the tuners didn't registered properly.

Cheers,
Mauro

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

On Monday, June 13, 2011 00:06:19 Mauro Carvalho Chehab wrote:
> Em 12-06-2011 15:09, Hans Verkuil escreveu:
> > On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
> >> Em 12-06-2011 13:07, Hans Verkuil escreveu:
> >>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
> >>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
> >>>>> 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.
> >>>>
> >>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
> >>>> to return the result to the caller. A fixme should be added though, so,
> >>>> after someone add a subdev call that would properly handle the -EINVAL
> >>>> code for multiple tuners, the functions should return the error code
> >>>> instead of 0.
> >>>
> >>> No, you can't return -EINVAL here. It is the responsibility of the bridge
> >>> driver to determine whether there is e.g. a radio tuner. So if one of these
> >>> tuner subdevs is called with mode radio while it is a tv tuner, then that
> >>> is not an error, but instead it is a request that can safely be ignored
> >>> as not relevant for that tuner. It is up to the bridge driver to ensure
> >>> that a tuner is loaded that is actually valid for the radio mode.
> >>>
> >>> Subdev ops should only return errors when there is a real problem (e.g. i2c
> >>> errors) and should just return 0 if a request is not for them.
> >>>
> >>> That's why I posted these first two patches: these functions suggest that you
> >>> can return an error if the mode doesn't match when you really cannot.
> >>>
> >>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
> >>> that is returned should match a real error (e.g. an i2c error), not that one
> >>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
> >>> otherwise there wouldn't be a /dev/radio node.
> >>>
> >>> I firmly believe that the RFCv4 series is correct and just needs an additional
> >>> patch adding some documentation.
> >>>
> >>> That said, it would make sense to move the first three patches to the end
> >>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
> >>
> >>
> >> The errors at tuner should be propagated. If there's just one tuner, the error
> >> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
> >> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
> >> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
> >> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
> >> implemented at v4l2_device_call_until_err().
> > 
> > Sorry, but no, that's not true. You are trying to use the error codes from tuner
> > subdevs to determine whether none of the tuner subdevs support a certain tuner mode.
> 
> Not only that. There are some cases where the tuner driver may not bind for some reason.
> So, even if the bridge driver does support a certain mode, a call to G_TUNER may fail
> (for example, if tea5767 probe failed). Only with a proper return code, the bridge driver
> can detect if the driver found some issue.

Surely, that's an error reported by tuner_probe, isn't it? That's supposed to ensure
that the tuner driver was loaded and initialized correctly. I'm not sure if it does,
but that's definitely where any errors of that kind should be reported.

Looking at it some more, it seems to me that s_type_addr should also return an
error if there are problems. Ditto for tuner_s_config.

An alternative solution is to keep a 'initialized' boolean that is set to true
once the tuner is fully configured. If g_tuner et al are called when the tuner
is not fully configured, then you can return -ENODEV or -EIO or something like that.

But that's a separate status check and has nothing to do with mode checking.

> > E.g., you want to change something for a radio tuner and there are no radio tuner
> > subdevs. But that's the job of the bridge driver to check. That has the overview,
> > the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
> > if they should handle it and ignore it otherwise.
> > 
> > Only if they have to handle it will they return a possible error. The big problem
> > with trying to use subdev errors codes for this is that you don't see the difference
> > between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
> > that returns -EINVAL just because it didn't understand the tuner mode.
> > 
> > So the bridge may return -EINVAL to the application instead of the real error, which
> > is -EIO.
> 
> An -EIO would also be discarded, as errors at v4l2_device_call_all() calls don't return
> anything. So, currently, the bridge has to assume that no errors happened and return 0.

Obviously, v4l2_device_call_all calls should be replaced with v4l2_device_call_until_err.
I've no problem with that.

> 
> > That's the whole principle behind the sub-device model: sub-devices do not know
> > about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
> > radio tuner, then the bridge driver is the one that should detect that and return
> > -EINVAL.
> > 
> > Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
> > the tuner mode against the device node it's called on. But that requires tightening
> > of the V4L2 spec first.
> 
> Yes, video_ioctl2 (or, currently, the bridge driver) shouldn't allow an invalid operation.
> But if the call returns an error, this error should be propagated. 
> 
> Also, as I've explained before, even adding the invalid mode check inside video_ioctl,
> you may still have errors if the registered tuners don't support the mode, because one 
> of the tuners didn't registered properly.

And that's something that tuner_probe/s_type_addr/s_config should have detected.

Or, should that be impossible (I would have to spend more time to analyze that)
we might have to add a 'validate_tuner' op that can be called to verify all tuners
are configured correctly.

Regards,

	Hans

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 13-06-2011 07:23, Hans Verkuil escreveu:
> On Monday, June 13, 2011 00:06:19 Mauro Carvalho Chehab wrote:
>> Em 12-06-2011 15:09, Hans Verkuil escreveu:
>>> On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
>>>> Em 12-06-2011 13:07, Hans Verkuil escreveu:
>>>>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>>>>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>>>>>> 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.
>>>>>>
>>>>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>>>>>> to return the result to the caller. A fixme should be added though, so,
>>>>>> after someone add a subdev call that would properly handle the -EINVAL
>>>>>> code for multiple tuners, the functions should return the error code
>>>>>> instead of 0.
>>>>>
>>>>> No, you can't return -EINVAL here. It is the responsibility of the bridge
>>>>> driver to determine whether there is e.g. a radio tuner. So if one of these
>>>>> tuner subdevs is called with mode radio while it is a tv tuner, then that
>>>>> is not an error, but instead it is a request that can safely be ignored
>>>>> as not relevant for that tuner. It is up to the bridge driver to ensure
>>>>> that a tuner is loaded that is actually valid for the radio mode.
>>>>>
>>>>> Subdev ops should only return errors when there is a real problem (e.g. i2c
>>>>> errors) and should just return 0 if a request is not for them.
>>>>>
>>>>> That's why I posted these first two patches: these functions suggest that you
>>>>> can return an error if the mode doesn't match when you really cannot.
>>>>>
>>>>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
>>>>> that is returned should match a real error (e.g. an i2c error), not that one
>>>>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
>>>>> otherwise there wouldn't be a /dev/radio node.
>>>>>
>>>>> I firmly believe that the RFCv4 series is correct and just needs an additional
>>>>> patch adding some documentation.
>>>>>
>>>>> That said, it would make sense to move the first three patches to the end
>>>>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
>>>>
>>>>
>>>> The errors at tuner should be propagated. If there's just one tuner, the error
>>>> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
>>>> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
>>>> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
>>>> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
>>>> implemented at v4l2_device_call_until_err().
>>>
>>> Sorry, but no, that's not true. You are trying to use the error codes from tuner
>>> subdevs to determine whether none of the tuner subdevs support a certain tuner mode.
>>
>> Not only that. There are some cases where the tuner driver may not bind for some reason.
>> So, even if the bridge driver does support a certain mode, a call to G_TUNER may fail
>> (for example, if tea5767 probe failed). Only with a proper return code, the bridge driver
>> can detect if the driver found some issue.
> 
> Surely, that's an error reported by tuner_probe, isn't it? That's supposed to ensure
> that the tuner driver was loaded and initialized correctly. I'm not sure if it does,
> but that's definitely where any errors of that kind should be reported.
> 
> Looking at it some more, it seems to me that s_type_addr should also return an
> error if there are problems. Ditto for tuner_s_config.
> 
> An alternative solution is to keep a 'initialized' boolean that is set to true
> once the tuner is fully configured. If g_tuner et al are called when the tuner
> is not fully configured, then you can return -ENODEV or -EIO or something like that.

NACK. This would be just an ugly workaround. 

> But that's a separate status check and has nothing to do with mode checking.
> 
>>> E.g., you want to change something for a radio tuner and there are no radio tuner
>>> subdevs. But that's the job of the bridge driver to check. That has the overview,
>>> the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
>>> if they should handle it and ignore it otherwise.
>>>
>>> Only if they have to handle it will they return a possible error. The big problem
>>> with trying to use subdev errors codes for this is that you don't see the difference
>>> between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
>>> that returns -EINVAL just because it didn't understand the tuner mode.
>>>
>>> So the bridge may return -EINVAL to the application instead of the real error, which
>>> is -EIO.
>>
>> An -EIO would also be discarded, as errors at v4l2_device_call_all() calls don't return
>> anything. So, currently, the bridge has to assume that no errors happened and return 0.
> 
> Obviously, v4l2_device_call_all calls should be replaced with v4l2_device_call_until_err.
> I've no problem with that.

See bellow.

>>
>>> That's the whole principle behind the sub-device model: sub-devices do not know
>>> about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
>>> radio tuner, then the bridge driver is the one that should detect that and return
>>> -EINVAL.
>>>
>>> Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
>>> the tuner mode against the device node it's called on. But that requires tightening
>>> of the V4L2 spec first.
>>
>> Yes, video_ioctl2 (or, currently, the bridge driver) shouldn't allow an invalid operation.
>> But if the call returns an error, this error should be propagated. 
>>
>> Also, as I've explained before, even adding the invalid mode check inside video_ioctl,
>> you may still have errors if the registered tuners don't support the mode, because one 
>> of the tuners didn't registered properly.
> 
> And that's something that tuner_probe/s_type_addr/s_config should have detected.

I'm almost sure that they don't do it, currently: s_type_addr/s_config also calls
v4l2_device_call_all(). No errors are returned back. The tuner_probe call also can't
do much, as it doesn't know in advance if the device has one or two tuners.

> Or, should that be impossible (I would have to spend more time to analyze that)
> we might have to add a 'validate_tuner' op that can be called to verify all tuners
> are configured correctly.

You're wanting to create a very complex patchset just to justify that replacing
from -EINVAL to a bool is the right thing to do. It isn't. The point is that:
if, for any reason, an ioctl fails, it should return an _error_, and _not_ a boolean.

After fixing v4l2_device_call_all() to allow it to return errors, the next step
is to review all calls to it, and add a proper handler for the errors. s_type_addr,
s_config, g_tuner, s_tuner, etc should be handling errors.

In other words, the original v4l2_device_call_all() that were just replicating the
previous I2C behaviour is a mistake, as it doesn't provide any feedback about errors.
This needs to be replaced by something that it is aware of the errors. If you take
a look at v4l2-subdev, there's just one operation that doesn't return an error
(v4l2_subdev_internal_ops.unregistered, never called from drivers). All the others 
returns an error. However, the default usage is to simply discard errors. This is wrong.
Errors should be propagated.

AFAIK, there are only a two types error propagation that are currently needed:

1) Call all subdevices. If one returns 0, assumes that the operation succeeded. This is
   used when there are multiple subdevs, but they're mutually exclusive: only one of them 
   will handle such call. It is needed by tuners and by controls, on devices that have 
   several subdevs providing different sets of controls.

2) Call all subdevices until an error. Used when the same operation needs to be set
   on multiple subdevices. The subdevices that don't implement such operation should 
   return -ENOIOCTLCMD.

Btw, v4l2_device_call_subdevs_until_err() has currently a bug: if all sub-devices return
-ENOIOCTLCMD, it returns 0. It should, instead, return -ENOIOCTLCMD, in order to allow
the bridge drivers to return an error code to the userspace, to indicate that the
IOCTL was not handled.

Eventually, we may just use (2) for everything, if we patch all subdev drivers to return
-ENOIOCTLCMD if they are discarding a subdev call, but, in this case, the bridge drivers 
will need to replace the -ENOIOCTLCMD to an error code defined at the V4L2 spec (or we
can have a macro for that).

A side note: the only error codes defined at the media API DocBook are: -EACCES, -EAGAIN,
-EBADF, -EBUSY, -EFAULT, -EIO, -EINTR, -EINVAL, -ENFILE, -ENOMEM, -ENOSPC, -ENOTTY, -ENXIO,
-EMFILE, -EPERM, -ERANGE and EPIPE. On most places, the error codes are defined per ioctl.
We need to review the DocBook and the drivers to be sure that they match the API specs, 
in terms of returned codes. It probably makes sense to create a section with the valid error
codes, remove most of error codes comments from each ioctl, and add a link to the global
error code section.

Ah, -ENODEV is not currently defined, but -ENXIO is defined on a few places. -ENXIO means 
"No such device or address". So, it may make sense to replace all -ENODEV to -ENXIO at 
the drivers.

Cheers,
Mauro

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

On Monday, June 13, 2011 13:45:14 Mauro Carvalho Chehab wrote:
> Em 13-06-2011 07:23, Hans Verkuil escreveu:
> > On Monday, June 13, 2011 00:06:19 Mauro Carvalho Chehab wrote:
> >> Em 12-06-2011 15:09, Hans Verkuil escreveu:
> >>> On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
> >>>> Em 12-06-2011 13:07, Hans Verkuil escreveu:
> >>>>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
> >>>>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
> >>>>>>> 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.
> >>>>>>
> >>>>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
> >>>>>> to return the result to the caller. A fixme should be added though, so,
> >>>>>> after someone add a subdev call that would properly handle the -EINVAL
> >>>>>> code for multiple tuners, the functions should return the error code
> >>>>>> instead of 0.
> >>>>>
> >>>>> No, you can't return -EINVAL here. It is the responsibility of the bridge
> >>>>> driver to determine whether there is e.g. a radio tuner. So if one of these
> >>>>> tuner subdevs is called with mode radio while it is a tv tuner, then that
> >>>>> is not an error, but instead it is a request that can safely be ignored
> >>>>> as not relevant for that tuner. It is up to the bridge driver to ensure
> >>>>> that a tuner is loaded that is actually valid for the radio mode.
> >>>>>
> >>>>> Subdev ops should only return errors when there is a real problem (e.g. i2c
> >>>>> errors) and should just return 0 if a request is not for them.
> >>>>>
> >>>>> That's why I posted these first two patches: these functions suggest that you
> >>>>> can return an error if the mode doesn't match when you really cannot.
> >>>>>
> >>>>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
> >>>>> that is returned should match a real error (e.g. an i2c error), not that one
> >>>>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
> >>>>> otherwise there wouldn't be a /dev/radio node.
> >>>>>
> >>>>> I firmly believe that the RFCv4 series is correct and just needs an additional
> >>>>> patch adding some documentation.
> >>>>>
> >>>>> That said, it would make sense to move the first three patches to the end
> >>>>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
> >>>>
> >>>>
> >>>> The errors at tuner should be propagated. If there's just one tuner, the error
> >>>> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
> >>>> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
> >>>> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
> >>>> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
> >>>> implemented at v4l2_device_call_until_err().
> >>>
> >>> Sorry, but no, that's not true. You are trying to use the error codes from tuner
> >>> subdevs to determine whether none of the tuner subdevs support a certain tuner mode.
> >>
> >> Not only that. There are some cases where the tuner driver may not bind for some reason.
> >> So, even if the bridge driver does support a certain mode, a call to G_TUNER may fail
> >> (for example, if tea5767 probe failed). Only with a proper return code, the bridge driver
> >> can detect if the driver found some issue.
> > 
> > Surely, that's an error reported by tuner_probe, isn't it? That's supposed to ensure
> > that the tuner driver was loaded and initialized correctly. I'm not sure if it does,
> > but that's definitely where any errors of that kind should be reported.
> > 
> > Looking at it some more, it seems to me that s_type_addr should also return an
> > error if there are problems. Ditto for tuner_s_config.
> > 
> > An alternative solution is to keep a 'initialized' boolean that is set to true
> > once the tuner is fully configured. If g_tuner et al are called when the tuner
> > is not fully configured, then you can return -ENODEV or -EIO or something like that.
> 
> NACK. This would be just an ugly workaround. 

Agreed :-)

> 
> > But that's a separate status check and has nothing to do with mode checking.
> > 
> >>> E.g., you want to change something for a radio tuner and there are no radio tuner
> >>> subdevs. But that's the job of the bridge driver to check. That has the overview,
> >>> the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
> >>> if they should handle it and ignore it otherwise.
> >>>
> >>> Only if they have to handle it will they return a possible error. The big problem
> >>> with trying to use subdev errors codes for this is that you don't see the difference
> >>> between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
> >>> that returns -EINVAL just because it didn't understand the tuner mode.
> >>>
> >>> So the bridge may return -EINVAL to the application instead of the real error, which
> >>> is -EIO.
> >>
> >> An -EIO would also be discarded, as errors at v4l2_device_call_all() calls don't return
> >> anything. So, currently, the bridge has to assume that no errors happened and return 0.
> > 
> > Obviously, v4l2_device_call_all calls should be replaced with v4l2_device_call_until_err.
> > I've no problem with that.
> 
> See bellow.
> 
> >>
> >>> That's the whole principle behind the sub-device model: sub-devices do not know
> >>> about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
> >>> radio tuner, then the bridge driver is the one that should detect that and return
> >>> -EINVAL.
> >>>
> >>> Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
> >>> the tuner mode against the device node it's called on. But that requires tightening
> >>> of the V4L2 spec first.
> >>
> >> Yes, video_ioctl2 (or, currently, the bridge driver) shouldn't allow an invalid operation.
> >> But if the call returns an error, this error should be propagated. 
> >>
> >> Also, as I've explained before, even adding the invalid mode check inside video_ioctl,
> >> you may still have errors if the registered tuners don't support the mode, because one 
> >> of the tuners didn't registered properly.
> > 
> > And that's something that tuner_probe/s_type_addr/s_config should have detected.
> 
> I'm almost sure that they don't do it, currently: s_type_addr/s_config also calls
> v4l2_device_call_all(). No errors are returned back. The tuner_probe call also can't
> do much, as it doesn't know in advance if the device has one or two tuners.

I know they don't. But that's what should happen.

> > Or, should that be impossible (I would have to spend more time to analyze that)
> > we might have to add a 'validate_tuner' op that can be called to verify all tuners
> > are configured correctly.
> 
> You're wanting to create a very complex patchset just to justify that replacing
> from -EINVAL to a bool is the right thing to do. It isn't. The point is that:
> if, for any reason, an ioctl fails, it should return an _error_, and _not_ a boolean.

True for an ioctl, not necessarily true for a subdev op. I think one thing that
confuses the issue here is that we have no clear error code for subdev ops that
return a 'not handled' code. To some extent ENOIOCTLCMD is used for that, but
it's not consistently used. Only if all subdevs called from a v4l2_device_call_all
type macro return 'not handled' should it actually return some error.

> After fixing v4l2_device_call_all() to allow it to return errors, the next step
> is to review all calls to it, and add a proper handler for the errors. s_type_addr,
> s_config, g_tuner, s_tuner, etc should be handling errors.
> 
> In other words, the original v4l2_device_call_all() that were just replicating the
> previous I2C behaviour is a mistake, as it doesn't provide any feedback about errors.
> This needs to be replaced by something that it is aware of the errors. If you take
> a look at v4l2-subdev, there's just one operation that doesn't return an error
> (v4l2_subdev_internal_ops.unregistered, never called from drivers). All the others 
> returns an error. However, the default usage is to simply discard errors. This is wrong.
> Errors should be propagated.

It's not a mistake, it's just that nobody had the time to sort out the mess.
The current behavior is basically bug-compatible with the pre-subdev days.
It's only since all bridge drivers were converted to use subdevs that we can
even think about cleaning up error handling.

> 
> AFAIK, there are only a two types error propagation that are currently needed:
> 
> 1) Call all subdevices. If one returns 0, assumes that the operation succeeded. This is
>    used when there are multiple subdevs, but they're mutually exclusive: only one of them 
>    will handle such call. It is needed by tuners and by controls, on devices that have 
>    several subdevs providing different sets of controls.
> 
> 2) Call all subdevices until an error. Used when the same operation needs to be set
>    on multiple subdevices. The subdevices that don't implement such operation should 
>    return -ENOIOCTLCMD.
> 
> Btw, v4l2_device_call_subdevs_until_err() has currently a bug: if all sub-devices return
> -ENOIOCTLCMD, it returns 0. It should, instead, return -ENOIOCTLCMD, in order to allow
> the bridge drivers to return an error code to the userspace, to indicate that the
> IOCTL was not handled.
> 
> Eventually, we may just use (2) for everything, if we patch all subdev drivers to return
> -ENOIOCTLCMD if they are discarding a subdev call, but, in this case, the bridge drivers 
> will need to replace the -ENOIOCTLCMD to an error code defined at the V4L2 spec (or we
> can have a macro for that).

Option 2 is the correct approach. If all subdev drivers return -ENOIOCTLCMD,
then return an error. If one driver returns a non-0 and non-ENOIOCTLCMD error,
then return that, otherwise return 0.

> A side note: the only error codes defined at the media API DocBook are: -EACCES, -EAGAIN,
> -EBADF, -EBUSY, -EFAULT, -EIO, -EINTR, -EINVAL, -ENFILE, -ENOMEM, -ENOSPC, -ENOTTY, -ENXIO,
> -EMFILE, -EPERM, -ERANGE and EPIPE. On most places, the error codes are defined per ioctl.
> We need to review the DocBook and the drivers to be sure that they match the API specs, 
> in terms of returned codes. It probably makes sense to create a section with the valid error
> codes, remove most of error codes comments from each ioctl, and add a link to the global
> error code section.
> 
> Ah, -ENODEV is not currently defined, but -ENXIO is defined on a few places. -ENXIO means 
> "No such device or address". So, it may make sense to replace all -ENODEV to -ENXIO at 
> the drivers.

Right, all very lovely, but I just want to fix the broken tuner code. We all know
error handling is a big mess and could keep a small army of janitors busy for a year.

For now I'll remove those two offending patches and redo the patch series without
them.

Regards,

	Hans

Re: [RFCv4 PATCH 1/8] tuner-core: rename check_mode to supported_mode

[Mauro Carvalho Chehab]

Em 13-06-2011 09:07, Hans Verkuil escreveu:
> On Monday, June 13, 2011 13:45:14 Mauro Carvalho Chehab wrote:
>> Em 13-06-2011 07:23, Hans Verkuil escreveu:
>>> On Monday, June 13, 2011 00:06:19 Mauro Carvalho Chehab wrote:
>>>> Em 12-06-2011 15:09, Hans Verkuil escreveu:
>>>>> On Sunday, June 12, 2011 19:27:21 Mauro Carvalho Chehab wrote:
>>>>>> Em 12-06-2011 13:07, Hans Verkuil escreveu:
>>>>>>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>>>>>>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>>>>>>>> to return the result to the caller. A fixme should be added though, so,
>>>>>>>> after someone add a subdev call that would properly handle the -EINVAL
>>>>>>>> code for multiple tuners, the functions should return the error code
>>>>>>>> instead of 0.
>>>>>>>
>>>>>>> No, you can't return -EINVAL here. It is the responsibility of the bridge
>>>>>>> driver to determine whether there is e.g. a radio tuner. So if one of these
>>>>>>> tuner subdevs is called with mode radio while it is a tv tuner, then that
>>>>>>> is not an error, but instead it is a request that can safely be ignored
>>>>>>> as not relevant for that tuner. It is up to the bridge driver to ensure
>>>>>>> that a tuner is loaded that is actually valid for the radio mode.
>>>>>>>
>>>>>>> Subdev ops should only return errors when there is a real problem (e.g. i2c
>>>>>>> errors) and should just return 0 if a request is not for them.
>>>>>>>
>>>>>>> That's why I posted these first two patches: these functions suggest that you
>>>>>>> can return an error if the mode doesn't match when you really cannot.
>>>>>>>
>>>>>>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
>>>>>>> that is returned should match a real error (e.g. an i2c error), not that one
>>>>>>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
>>>>>>> otherwise there wouldn't be a /dev/radio node.
>>>>>>>
>>>>>>> I firmly believe that the RFCv4 series is correct and just needs an additional
>>>>>>> patch adding some documentation.
>>>>>>>
>>>>>>> That said, it would make sense to move the first three patches to the end
>>>>>>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
>>>>>>
>>>>>>
>>>>>> The errors at tuner should be propagated. If there's just one tuner, the error
>>>>>> code should just be returned by the ioctl. But, if there are two tuners, if the bridge 
>>>>>> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL, 
>>>>>> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the 
>>>>>> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour 
>>>>>> implemented at v4l2_device_call_until_err().
>>>>>
>>>>> Sorry, but no, that's not true. You are trying to use the error codes from tuner
>>>>> subdevs to determine whether none of the tuner subdevs support a certain tuner mode.
>>>>
>>>> Not only that. There are some cases where the tuner driver may not bind for some reason.
>>>> So, even if the bridge driver does support a certain mode, a call to G_TUNER may fail
>>>> (for example, if tea5767 probe failed). Only with a proper return code, the bridge driver
>>>> can detect if the driver found some issue.
>>>
>>> Surely, that's an error reported by tuner_probe, isn't it? That's supposed to ensure
>>> that the tuner driver was loaded and initialized correctly. I'm not sure if it does,
>>> but that's definitely where any errors of that kind should be reported.
>>>
>>> Looking at it some more, it seems to me that s_type_addr should also return an
>>> error if there are problems. Ditto for tuner_s_config.
>>>
>>> An alternative solution is to keep a 'initialized' boolean that is set to true
>>> once the tuner is fully configured. If g_tuner et al are called when the tuner
>>> is not fully configured, then you can return -ENODEV or -EIO or something like that.
>>
>> NACK. This would be just an ugly workaround. 
> 
> Agreed :-)
> 
>>
>>> But that's a separate status check and has nothing to do with mode checking.
>>>
>>>>> E.g., you want to change something for a radio tuner and there are no radio tuner
>>>>> subdevs. But that's the job of the bridge driver to check. That has the overview,
>>>>> the lowly subdevs do not. The subdevs just filter the ops and check the mode to see
>>>>> if they should handle it and ignore it otherwise.
>>>>>
>>>>> Only if they have to handle it will they return a possible error. The big problem
>>>>> with trying to use subdev errors codes for this is that you don't see the difference
>>>>> between a real error of a subdev (e.g. -EIO when an i2c access fails) and a subdev
>>>>> that returns -EINVAL just because it didn't understand the tuner mode.
>>>>>
>>>>> So the bridge may return -EINVAL to the application instead of the real error, which
>>>>> is -EIO.
>>>>
>>>> An -EIO would also be discarded, as errors at v4l2_device_call_all() calls don't return
>>>> anything. So, currently, the bridge has to assume that no errors happened and return 0.
>>>
>>> Obviously, v4l2_device_call_all calls should be replaced with v4l2_device_call_until_err.
>>> I've no problem with that.
>>
>> See bellow.
>>
>>>>
>>>>> That's the whole principle behind the sub-device model: sub-devices do not know
>>>>> about 'the world outside'. So if you pass RADIO mode to S_FREQUENCY and there is no
>>>>> radio tuner, then the bridge driver is the one that should detect that and return
>>>>> -EINVAL.
>>>>>
>>>>> Actually, as mentioned before, it can also be done in video_ioctl2.c by checking
>>>>> the tuner mode against the device node it's called on. But that requires tightening
>>>>> of the V4L2 spec first.
>>>>
>>>> Yes, video_ioctl2 (or, currently, the bridge driver) shouldn't allow an invalid operation.
>>>> But if the call returns an error, this error should be propagated. 
>>>>
>>>> Also, as I've explained before, even adding the invalid mode check inside video_ioctl,
>>>> you may still have errors if the registered tuners don't support the mode, because one 
>>>> of the tuners didn't registered properly.
>>>
>>> And that's something that tuner_probe/s_type_addr/s_config should have detected.
>>
>> I'm almost sure that they don't do it, currently: s_type_addr/s_config also calls
>> v4l2_device_call_all(). No errors are returned back. The tuner_probe call also can't
>> do much, as it doesn't know in advance if the device has one or two tuners.
> 
> I know they don't. But that's what should happen.
> 
>>> Or, should that be impossible (I would have to spend more time to analyze that)
>>> we might have to add a 'validate_tuner' op that can be called to verify all tuners
>>> are configured correctly.
>>
>> You're wanting to create a very complex patchset just to justify that replacing
>> from -EINVAL to a bool is the right thing to do. It isn't. The point is that:
>> if, for any reason, an ioctl fails, it should return an _error_, and _not_ a boolean.
> 
> True for an ioctl, not necessarily true for a subdev op.

True also for subdev op. If the bridge driver will do something else or not if a subdev
has an error is up to the bridge, but the errors should be reported there.

> I think one thing that
> confuses the issue here is that we have no clear error code for subdev ops that
> return a 'not handled' code. To some extent ENOIOCTLCMD is used for that, but
> it's not consistently used. Only if all subdevs called from a v4l2_device_call_all
> type macro return 'not handled' should it actually return some error.

Agreed.

>> After fixing v4l2_device_call_all() to allow it to return errors, the next step
>> is to review all calls to it, and add a proper handler for the errors. s_type_addr,
>> s_config, g_tuner, s_tuner, etc should be handling errors.
>>
>> In other words, the original v4l2_device_call_all() that were just replicating the
>> previous I2C behaviour is a mistake, as it doesn't provide any feedback about errors.
>> This needs to be replaced by something that it is aware of the errors. If you take
>> a look at v4l2-subdev, there's just one operation that doesn't return an error
>> (v4l2_subdev_internal_ops.unregistered, never called from drivers). All the others 
>> returns an error. However, the default usage is to simply discard errors. This is wrong.
>> Errors should be propagated.
> 
> It's not a mistake, it's just that nobody had the time to sort out the mess.
> The current behavior is basically bug-compatible with the pre-subdev days.
> It's only since all bridge drivers were converted to use subdevs that we can
> even think about cleaning up error handling.
> 
>>
>> AFAIK, there are only a two types error propagation that are currently needed:
>>
>> 1) Call all subdevices. If one returns 0, assumes that the operation succeeded. This is
>>    used when there are multiple subdevs, but they're mutually exclusive: only one of them 
>>    will handle such call. It is needed by tuners and by controls, on devices that have 
>>    several subdevs providing different sets of controls.
>>
>> 2) Call all subdevices until an error. Used when the same operation needs to be set
>>    on multiple subdevices. The subdevices that don't implement such operation should 
>>    return -ENOIOCTLCMD.
>>
>> Btw, v4l2_device_call_subdevs_until_err() has currently a bug: if all sub-devices return
>> -ENOIOCTLCMD, it returns 0. It should, instead, return -ENOIOCTLCMD, in order to allow
>> the bridge drivers to return an error code to the userspace, to indicate that the
>> IOCTL was not handled.
>>
>> Eventually, we may just use (2) for everything, if we patch all subdev drivers to return
>> -ENOIOCTLCMD if they are discarding a subdev call, but, in this case, the bridge drivers 
>> will need to replace the -ENOIOCTLCMD to an error code defined at the V4L2 spec (or we
>> can have a macro for that).
> 
> Option 2 is the correct approach. If all subdev drivers return -ENOIOCTLCMD,
> then return an error. If one driver returns a non-0 and non-ENOIOCTLCMD error,
> then return that, otherwise return 0.

I'm ok with that.

>> A side note: the only error codes defined at the media API DocBook are: -EACCES, -EAGAIN,
>> -EBADF, -EBUSY, -EFAULT, -EIO, -EINTR, -EINVAL, -ENFILE, -ENOMEM, -ENOSPC, -ENOTTY, -ENXIO,
>> -EMFILE, -EPERM, -ERANGE and EPIPE. On most places, the error codes are defined per ioctl.
>> We need to review the DocBook and the drivers to be sure that they match the API specs, 
>> in terms of returned codes. It probably makes sense to create a section with the valid error
>> codes, remove most of error codes comments from each ioctl, and add a link to the global
>> error code section.
>>
>> Ah, -ENODEV is not currently defined, but -ENXIO is defined on a few places. -ENXIO means 
>> "No such device or address". So, it may make sense to replace all -ENODEV to -ENXIO at 
>> the drivers.
> 
> Right, all very lovely, but I just want to fix the broken tuner code. We all know
> error handling is a big mess and could keep a small army of janitors busy for a year.
> 
> For now I'll remove those two offending patches and redo the patch series without
> them.

OK.

> 
> Regards,
> 
> 	Hans