[PATCH] tea575x: fix HW seek

[PATCH] tea575x: fix HW seek

[Ondrej Zary]

Fix HW seek in TEA575x to work properly:
 - a delay must be present after search start and before first register read
   or the seek does weird things
 - when the search stops, the new frequency is not available immediately, we
   must wait until it appears in the register (fortunately, we can clear the
   frequency bits when starting the search as it starts at the frequency
   currently set, not from the value written)
 - sometimes, seek remains on the current frequency (or moves only a little),
   so repeat it until it moves by at least 50 kHz

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

--- a/sound/i2c/other/tea575x-tuner.c
+++ b/sound/i2c/other/tea575x-tuner.c
@@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x *tea, unsigned int val)
 		tea->ops->set_pins(tea, 0);
 }
 
-static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
+static u32 snd_tea575x_read(struct snd_tea575x *tea)
 {
 	u16 l, rdata;
 	u32 data = 0;
@@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
 	return data;
 }
 
+static void snd_tea575x_get_freq(struct snd_tea575x *tea)
+{
+	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
+
+	if (freq == 0) {
+		tea->freq = 0;
+		return;
+	}
+
+	/* freq *= 12.5 */
+	freq *= 125;
+	freq /= 10;
+	/* crystal fixup */
+	if (tea->tea5759)
+		freq += TEA575X_FMIF;
+	else
+		freq -= TEA575X_FMIF;
+
+	tea->freq = clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
+}
+
 static void snd_tea575x_set_freq(struct snd_tea575x *tea)
 {
 	u32 freq = tea->freq;
@@ -203,6 +224,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	if (f->tuner != 0)
 		return -EINVAL;
 	f->type = V4L2_TUNER_RADIO;
+	if (!tea->cannot_read_data)
+		snd_tea575x_get_freq(tea);
 	f->frequency = tea->freq;
 	return 0;
 }
@@ -225,36 +248,50 @@ static int vidioc_s_hw_freq_seek(struct file *file, void *fh,
 					struct v4l2_hw_freq_seek *a)
 {
 	struct snd_tea575x *tea = video_drvdata(file);
+	int i, old_freq;
+	unsigned long timeout;
 
 	if (tea->cannot_read_data)
 		return -ENOTTY;
+
+	snd_tea575x_get_freq(tea);
+	old_freq = tea->freq;
+	/* clear the frequency, HW will fill it in */
+	tea->val &= ~TEA575X_BIT_FREQ_MASK;
 	tea->val |= TEA575X_BIT_SEARCH;
-	tea->val &= ~TEA575X_BIT_UPDOWN;
 	if (a->seek_upward)
 		tea->val |= TEA575X_BIT_UPDOWN;
+	else
+		tea->val &= ~TEA575X_BIT_UPDOWN;
 	snd_tea575x_write(tea, tea->val);
+	timeout = jiffies + msecs_to_jiffies(10000);
 	for (;;) {
-		unsigned val = snd_tea575x_read(tea);
-
-		if (!(val & TEA575X_BIT_SEARCH)) {
-			/* Found a frequency */
-			val &= TEA575X_BIT_FREQ_MASK;
-			val = (val * 10) / 125;
-			if (tea->tea5759)
-				val += TEA575X_FMIF;
-			else
-				val -= TEA575X_FMIF;
-			tea->freq = clamp(val * 16, FREQ_LO, FREQ_HI);
-			return 0;
-		}
+		if (time_after(jiffies, timeout))
+			break;
 		if (schedule_timeout_interruptible(msecs_to_jiffies(10))) {
 			/* some signal arrived, stop search */
 			tea->val &= ~TEA575X_BIT_SEARCH;
 			snd_tea575x_write(tea, tea->val);
 			return -ERESTARTSYS;
 		}
+		if (!(snd_tea575x_read(tea) & TEA575X_BIT_SEARCH)) {
+			/* Found a frequency, wait until it can be read */
+			for (i = 0; i < 100; i++) {
+				msleep(10);
+				snd_tea575x_get_freq(tea);
+				if (tea->freq != 0) /* available */
+					break;
+			}
+			/* if we moved by less than 50 kHz, continue seeking */
+			if (abs(old_freq - tea->freq) < 16 * 500) {
+				snd_tea575x_write(tea, tea->val);
+				continue;
+			}
+			tea->val &= ~TEA575X_BIT_SEARCH;
+			return 0;
+		}
 	}
-	return 0;
+	return -ETIMEDOUT;
 }
 
 static int tea575x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -318,7 +355,7 @@ int snd_tea575x_init(struct snd_tea575x *tea)
 			return -ENODEV;
 	}
 
-	tea->val = TEA575X_BIT_BAND_FM | TEA575X_BIT_SEARCH_10_40;
+	tea->val = TEA575X_BIT_BAND_FM | TEA575X_BIT_SEARCH_5_28;
 	tea->freq = 90500 * 16;		/* 90.5Mhz default */
 	snd_tea575x_set_freq(tea);
 



-- 
Ondrej Zary

Re: [PATCH] tea575x: fix HW seek

[Hans Verkuil]

On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> Fix HW seek in TEA575x to work properly:
>  - a delay must be present after search start and before first register read
>    or the seek does weird things
>  - when the search stops, the new frequency is not available immediately, we
>    must wait until it appears in the register (fortunately, we can clear the
>    frequency bits when starting the search as it starts at the frequency
>    currently set, not from the value written)
>  - sometimes, seek remains on the current frequency (or moves only a little),
>    so repeat it until it moves by at least 50 kHz
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> 
> --- a/sound/i2c/other/tea575x-tuner.c
> +++ b/sound/i2c/other/tea575x-tuner.c
> @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x *tea, unsigned int val)
>  		tea->ops->set_pins(tea, 0);
>  }
>  
> -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> +static u32 snd_tea575x_read(struct snd_tea575x *tea)
>  {
>  	u16 l, rdata;
>  	u32 data = 0;
> @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
>  	return data;
>  }
>  
> +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> +{
> +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> +
> +	if (freq == 0) {
> +		tea->freq = 0;

Wouldn't it be better to return -EBUSY in this case? VIDIOC_G_FREQUENCY
should not return frequencies outside the valid frequency range. In this case
returning -EBUSY seems to make more sense to me.

I'm proposing adding this patch:

diff --git a/sound/i2c/other/tea575x-tuner.c b/sound/i2c/other/tea575x-tuner.c
index 474bb81..02da22f 100644
--- a/sound/i2c/other/tea575x-tuner.c
+++ b/sound/i2c/other/tea575x-tuner.c
@@ -120,14 +120,12 @@ static u32 snd_tea575x_read(struct snd_tea575x *tea)
 	return data;
 }
 
-static void snd_tea575x_get_freq(struct snd_tea575x *tea)
+static int snd_tea575x_get_freq(struct snd_tea575x *tea)
 {
 	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
 
-	if (freq == 0) {
-		tea->freq = 0;
-		return;
-	}
+	if (freq == 0)
+		return -EBUSY;
 
 	/* freq *= 12.5 */
 	freq *= 125;
@@ -139,6 +137,7 @@ static void snd_tea575x_get_freq(struct snd_tea575x *tea)
 		freq -= TEA575X_FMIF;
 
 	tea->freq = clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
+	return 0;
 }
 
 static void snd_tea575x_set_freq(struct snd_tea575x *tea)
@@ -220,14 +219,15 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 					struct v4l2_frequency *f)
 {
 	struct snd_tea575x *tea = video_drvdata(file);
+	int ret = 0;
 
 	if (f->tuner != 0)
 		return -EINVAL;
 	f->type = V4L2_TUNER_RADIO;
 	if (!tea->cannot_read_data)
-		snd_tea575x_get_freq(tea);
+		ret = snd_tea575x_get_freq(tea);
 	f->frequency = tea->freq;
-	return 0;
+	return ret;
 }
 
 static int vidioc_s_frequency(struct file *file, void *priv,
@@ -250,11 +250,14 @@ static int vidioc_s_hw_freq_seek(struct file *file, void *fh,
 	struct snd_tea575x *tea = video_drvdata(file);
 	int i, old_freq;
 	unsigned long timeout;
+	int ret;
 
 	if (tea->cannot_read_data)
 		return -ENOTTY;
 
-	snd_tea575x_get_freq(tea);
+	ret = snd_tea575x_get_freq(tea);
+	if (ret)
+		return ret;
 	old_freq = tea->freq;
 	/* clear the frequency, HW will fill it in */
 	tea->val &= ~TEA575X_BIT_FREQ_MASK;
@@ -278,8 +281,8 @@ static int vidioc_s_hw_freq_seek(struct file *file, void *fh,
 			/* Found a frequency, wait until it can be read */
 			for (i = 0; i < 100; i++) {
 				msleep(10);
-				snd_tea575x_get_freq(tea);
-				if (tea->freq != 0) /* available */
+				ret = snd_tea575x_get_freq(tea);
+				if (ret == 0) /* available */
 					break;
 			}
 			/* if we moved by less than 50 kHz, continue seeking */

If you are OK with this, then I'll put everything together and make a final
pull request.

Regards,

	Hans

Re: [PATCH] tea575x: fix HW seek

[Ondrej Zary]

On Tuesday 21 February 2012, Hans Verkuil wrote:
> On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> > Fix HW seek in TEA575x to work properly:
> >  - a delay must be present after search start and before first register
> > read or the seek does weird things
> >  - when the search stops, the new frequency is not available immediately,
> > we must wait until it appears in the register (fortunately, we can clear
> > the frequency bits when starting the search as it starts at the frequency
> > currently set, not from the value written)
> >  - sometimes, seek remains on the current frequency (or moves only a
> > little), so repeat it until it moves by at least 50 kHz
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- a/sound/i2c/other/tea575x-tuner.c
> > +++ b/sound/i2c/other/tea575x-tuner.c
> > @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x *tea,
> > unsigned int val) tea->ops->set_pins(tea, 0);
> >  }
> >
> > -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> > +static u32 snd_tea575x_read(struct snd_tea575x *tea)
> >  {
> >  	u16 l, rdata;
> >  	u32 data = 0;
> > @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct
> > snd_tea575x *tea) return data;
> >  }
> >
> > +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> > +{
> > +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> > +
> > +	if (freq == 0) {
> > +		tea->freq = 0;
>
> Wouldn't it be better to return -EBUSY in this case? VIDIOC_G_FREQUENCY
> should not return frequencies outside the valid frequency range. In this
> case returning -EBUSY seems to make more sense to me.

The device returns zero frequency when the scan fails to find a frequency. 
This is not an error, just an indication that "nothing" is tuned. So maybe we 
can return some bogus frequency in vidioc_g_frequency (like FREQ_LO) in this 
case (don't know if -EBUSY will break anything). But HW seek should get the 
real one (i.e. zero when it's there).

> I'm proposing adding this patch:
>
> diff --git a/sound/i2c/other/tea575x-tuner.c
> b/sound/i2c/other/tea575x-tuner.c index 474bb81..02da22f 100644
> --- a/sound/i2c/other/tea575x-tuner.c
> +++ b/sound/i2c/other/tea575x-tuner.c
> @@ -120,14 +120,12 @@ static u32 snd_tea575x_read(struct snd_tea575x *tea)
>  	return data;
>  }
>
> -static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> +static int snd_tea575x_get_freq(struct snd_tea575x *tea)
>  {
>  	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
>
> -	if (freq == 0) {
> -		tea->freq = 0;
> -		return;
> -	}
> +	if (freq == 0)
> +		return -EBUSY;
>
>  	/* freq *= 12.5 */
>  	freq *= 125;
> @@ -139,6 +137,7 @@ static void snd_tea575x_get_freq(struct snd_tea575x
> *tea) freq -= TEA575X_FMIF;
>
>  	tea->freq = clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
> +	return 0;
>  }
>
>  static void snd_tea575x_set_freq(struct snd_tea575x *tea)
> @@ -220,14 +219,15 @@ static int vidioc_g_frequency(struct file *file, void
> *priv, struct v4l2_frequency *f)
>  {
>  	struct snd_tea575x *tea = video_drvdata(file);
> +	int ret = 0;
>
>  	if (f->tuner != 0)
>  		return -EINVAL;
>  	f->type = V4L2_TUNER_RADIO;
>  	if (!tea->cannot_read_data)
> -		snd_tea575x_get_freq(tea);
> +		ret = snd_tea575x_get_freq(tea);
>  	f->frequency = tea->freq;
> -	return 0;
> +	return ret;
>  }
>
>  static int vidioc_s_frequency(struct file *file, void *priv,
> @@ -250,11 +250,14 @@ static int vidioc_s_hw_freq_seek(struct file *file,
> void *fh, struct snd_tea575x *tea = video_drvdata(file);
>  	int i, old_freq;
>  	unsigned long timeout;
> +	int ret;
>
>  	if (tea->cannot_read_data)
>  		return -ENOTTY;
>
> -	snd_tea575x_get_freq(tea);
> +	ret = snd_tea575x_get_freq(tea);
> +	if (ret)
> +		return ret;
>  	old_freq = tea->freq;
>  	/* clear the frequency, HW will fill it in */
>  	tea->val &= ~TEA575X_BIT_FREQ_MASK;
> @@ -278,8 +281,8 @@ static int vidioc_s_hw_freq_seek(struct file *file,
> void *fh, /* Found a frequency, wait until it can be read */
>  			for (i = 0; i < 100; i++) {
>  				msleep(10);
> -				snd_tea575x_get_freq(tea);
> -				if (tea->freq != 0) /* available */
> +				ret = snd_tea575x_get_freq(tea);
> +				if (ret == 0) /* available */
>  					break;
>  			}
>  			/* if we moved by less than 50 kHz, continue seeking */
>
> If you are OK with this, then I'll put everything together and make a final
> pull request.
>
> Regards,
>
> 	Hans



-- 
Ondrej Zary

Re: [PATCH] tea575x: fix HW seek

[Hans Verkuil]

On Wednesday, February 22, 2012 09:35:28 Ondrej Zary wrote:
> On Tuesday 21 February 2012, Hans Verkuil wrote:
> > On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> > > Fix HW seek in TEA575x to work properly:
> > >  - a delay must be present after search start and before first register
> > > read or the seek does weird things
> > >  - when the search stops, the new frequency is not available immediately,
> > > we must wait until it appears in the register (fortunately, we can clear
> > > the frequency bits when starting the search as it starts at the frequency
> > > currently set, not from the value written)
> > >  - sometimes, seek remains on the current frequency (or moves only a
> > > little), so repeat it until it moves by at least 50 kHz
> > >
> > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > >
> > > --- a/sound/i2c/other/tea575x-tuner.c
> > > +++ b/sound/i2c/other/tea575x-tuner.c
> > > @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x *tea,
> > > unsigned int val) tea->ops->set_pins(tea, 0);
> > >  }
> > >
> > > -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> > > +static u32 snd_tea575x_read(struct snd_tea575x *tea)
> > >  {
> > >  	u16 l, rdata;
> > >  	u32 data = 0;
> > > @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct
> > > snd_tea575x *tea) return data;
> > >  }
> > >
> > > +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> > > +{
> > > +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> > > +
> > > +	if (freq == 0) {
> > > +		tea->freq = 0;
> >
> > Wouldn't it be better to return -EBUSY in this case? VIDIOC_G_FREQUENCY
> > should not return frequencies outside the valid frequency range. In this
> > case returning -EBUSY seems to make more sense to me.
> 
> The device returns zero frequency when the scan fails to find a frequency. 
> This is not an error, just an indication that "nothing" is tuned. So maybe we 
> can return some bogus frequency in vidioc_g_frequency (like FREQ_LO) in this 
> case (don't know if -EBUSY will break anything). But HW seek should get the 
> real one (i.e. zero when it's there).

How about the following patch? vidioc_g_frequency just returns the last set
frequency and the hw_seek restores the original frequency if it can't find
another channel.

Also note that the check for < 50 kHz in hw_seek actually checked for < 500 kHz.
I've fixed that, but I can't test it.

Do you also know what happens at the boundaries of the frequency range? Does
it wrap around, or do you get a timeout?

Regards,

	Hans

diff --git a/sound/i2c/other/tea575x-tuner.c b/sound/i2c/other/tea575x-tuner.c
index 474bb81..1bdf1f3 100644
--- a/sound/i2c/other/tea575x-tuner.c
+++ b/sound/i2c/other/tea575x-tuner.c
@@ -120,14 +120,12 @@ static u32 snd_tea575x_read(struct snd_tea575x *tea)
 	return data;
 }
 
-static void snd_tea575x_get_freq(struct snd_tea575x *tea)
+static u32 snd_tea575x_get_freq(struct snd_tea575x *tea)
 {
 	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
 
-	if (freq == 0) {
-		tea->freq = 0;
-		return;
-	}
+	if (freq == 0)
+		return freq;
 
 	/* freq *= 12.5 */
 	freq *= 125;
@@ -138,7 +136,7 @@ static void snd_tea575x_get_freq(struct snd_tea575x *tea)
 	else
 		freq -= TEA575X_FMIF;
 
-	tea->freq = clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
+	return clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
 }
 
 static void snd_tea575x_set_freq(struct snd_tea575x *tea)
@@ -224,8 +222,6 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	if (f->tuner != 0)
 		return -EINVAL;
 	f->type = V4L2_TUNER_RADIO;
-	if (!tea->cannot_read_data)
-		snd_tea575x_get_freq(tea);
 	f->frequency = tea->freq;
 	return 0;
 }
@@ -248,14 +244,12 @@ static int vidioc_s_hw_freq_seek(struct file *file, void *fh,
 					struct v4l2_hw_freq_seek *a)
 {
 	struct snd_tea575x *tea = video_drvdata(file);
-	int i, old_freq;
 	unsigned long timeout;
+	int i;
 
 	if (tea->cannot_read_data)
 		return -ENOTTY;
 
-	snd_tea575x_get_freq(tea);
-	old_freq = tea->freq;
 	/* clear the frequency, HW will fill it in */
 	tea->val &= ~TEA575X_BIT_FREQ_MASK;
 	tea->val |= TEA575X_BIT_SEARCH;
@@ -271,26 +265,33 @@ static int vidioc_s_hw_freq_seek(struct file *file, void *fh,
 		if (schedule_timeout_interruptible(msecs_to_jiffies(10))) {
 			/* some signal arrived, stop search */
 			tea->val &= ~TEA575X_BIT_SEARCH;
-			snd_tea575x_write(tea, tea->val);
+			snd_tea575x_set_freq(tea);
 			return -ERESTARTSYS;
 		}
 		if (!(snd_tea575x_read(tea) & TEA575X_BIT_SEARCH)) {
+			u32 freq;
+
 			/* Found a frequency, wait until it can be read */
 			for (i = 0; i < 100; i++) {
 				msleep(10);
-				snd_tea575x_get_freq(tea);
-				if (tea->freq != 0) /* available */
+				freq = snd_tea575x_get_freq(tea);
+				if (freq) /* available */
 					break;
 			}
+			if (freq == 0) /* shouldn't happen */
+				break;
 			/* if we moved by less than 50 kHz, continue seeking */
-			if (abs(old_freq - tea->freq) < 16 * 500) {
+			if (abs(tea->freq - freq) < 16 * 50) {
 				snd_tea575x_write(tea, tea->val);
 				continue;
 			}
+			tea->freq = freq;
 			tea->val &= ~TEA575X_BIT_SEARCH;
 			return 0;
 		}
 	}
+	tea->val &= ~TEA575X_BIT_SEARCH;
+	snd_tea575x_set_freq(tea);
 	return -ETIMEDOUT;
 }
 

Re: [PATCH] tea575x: fix HW seek

[Ondrej Zary]

On Friday 24 February 2012 10:00:01 Hans Verkuil wrote:
> On Wednesday, February 22, 2012 09:35:28 Ondrej Zary wrote:
> > On Tuesday 21 February 2012, Hans Verkuil wrote:
> > > On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> > > > Fix HW seek in TEA575x to work properly:
> > > >  - a delay must be present after search start and before first
> > > > register read or the seek does weird things
> > > >  - when the search stops, the new frequency is not available
> > > > immediately, we must wait until it appears in the register
> > > > (fortunately, we can clear the frequency bits when starting the
> > > > search as it starts at the frequency currently set, not from the
> > > > value written)
> > > >  - sometimes, seek remains on the current frequency (or moves only a
> > > > little), so repeat it until it moves by at least 50 kHz
> > > >
> > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > > >
> > > > --- a/sound/i2c/other/tea575x-tuner.c
> > > > +++ b/sound/i2c/other/tea575x-tuner.c
> > > > @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x
> > > > *tea, unsigned int val) tea->ops->set_pins(tea, 0);
> > > >  }
> > > >
> > > > -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> > > > +static u32 snd_tea575x_read(struct snd_tea575x *tea)
> > > >  {
> > > >  	u16 l, rdata;
> > > >  	u32 data = 0;
> > > > @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct
> > > > snd_tea575x *tea) return data;
> > > >  }
> > > >
> > > > +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> > > > +{
> > > > +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> > > > +
> > > > +	if (freq == 0) {
> > > > +		tea->freq = 0;
> > >
> > > Wouldn't it be better to return -EBUSY in this case? VIDIOC_G_FREQUENCY
> > > should not return frequencies outside the valid frequency range. In
> > > this case returning -EBUSY seems to make more sense to me.
> >
> > The device returns zero frequency when the scan fails to find a
> > frequency. This is not an error, just an indication that "nothing" is
> > tuned. So maybe we can return some bogus frequency in vidioc_g_frequency
> > (like FREQ_LO) in this case (don't know if -EBUSY will break anything).
> > But HW seek should get the real one (i.e. zero when it's there).
>
> How about the following patch? vidioc_g_frequency just returns the last set
> frequency and the hw_seek restores the original frequency if it can't find
> another channel.

Seems to work. That's probably the right thing to do.

> Also note that the check for < 50 kHz in hw_seek actually checked for < 500
> kHz. I've fixed that, but I can't test it.

Thanks. It finds more stations now. To improve reliability, an additional 
check should be added - the seek sometimes stop at the same station, just a 
bit more than 50kHz of the original frequency, often in wrong direction. 
Something like this:

--- a/sound/i2c/other/tea575x-tuner.c
+++ b/sound/i2c/other/tea575x-tuner.c
@@ -280,8 +280,13 @@ static int vidioc_s_hw_freq_seek(struct file *file, void 
*fh,
                        }
                        if (freq == 0) /* shouldn't happen */
                                break;
-                       /* if we moved by less than 50 kHz, continue seeking 
*/
-                       if (abs(tea->freq - freq) < 16 * 50) {
+                       /*
+                        * if we moved by less than 50 kHz, or in the wrong
+                        * direction, continue seeking
+                        */
+                       if (abs(tea->freq - freq) < 16 * 50 ||
+                           (a->seek_upward && freq < tea->freq) ||
+                           (!a->seek_upward && freq > tea->freq)) {
                                snd_tea575x_write(tea, tea->val);
                                continue;
                        }


> Do you also know what happens at the boundaries of the frequency range?
> Does it wrap around, or do you get a timeout?

No wraparound, it times out so the original frequency is restored. I wonder 
if -ETIMEDOUT is correct here.

> Regards,
>
> 	Hans
>
> diff --git a/sound/i2c/other/tea575x-tuner.c
> b/sound/i2c/other/tea575x-tuner.c index 474bb81..1bdf1f3 100644
> --- a/sound/i2c/other/tea575x-tuner.c
> +++ b/sound/i2c/other/tea575x-tuner.c
> @@ -120,14 +120,12 @@ static u32 snd_tea575x_read(struct snd_tea575x *tea)
>  	return data;
>  }
>
> -static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> +static u32 snd_tea575x_get_freq(struct snd_tea575x *tea)
>  {
>  	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
>
> -	if (freq == 0) {
> -		tea->freq = 0;
> -		return;
> -	}
> +	if (freq == 0)
> +		return freq;
>
>  	/* freq *= 12.5 */
>  	freq *= 125;
> @@ -138,7 +136,7 @@ static void snd_tea575x_get_freq(struct snd_tea575x
> *tea) else
>  		freq -= TEA575X_FMIF;
>
> -	tea->freq = clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
> +	return clamp(freq * 16, FREQ_LO, FREQ_HI); /* from kHz */
>  }
>
>  static void snd_tea575x_set_freq(struct snd_tea575x *tea)
> @@ -224,8 +222,6 @@ static int vidioc_g_frequency(struct file *file, void
> *priv, if (f->tuner != 0)
>  		return -EINVAL;
>  	f->type = V4L2_TUNER_RADIO;
> -	if (!tea->cannot_read_data)
> -		snd_tea575x_get_freq(tea);
>  	f->frequency = tea->freq;
>  	return 0;
>  }
> @@ -248,14 +244,12 @@ static int vidioc_s_hw_freq_seek(struct file *file,
> void *fh, struct v4l2_hw_freq_seek *a)
>  {
>  	struct snd_tea575x *tea = video_drvdata(file);
> -	int i, old_freq;
>  	unsigned long timeout;
> +	int i;
>
>  	if (tea->cannot_read_data)
>  		return -ENOTTY;
>
> -	snd_tea575x_get_freq(tea);
> -	old_freq = tea->freq;
>  	/* clear the frequency, HW will fill it in */
>  	tea->val &= ~TEA575X_BIT_FREQ_MASK;
>  	tea->val |= TEA575X_BIT_SEARCH;
> @@ -271,26 +265,33 @@ static int vidioc_s_hw_freq_seek(struct file *file,
> void *fh, if (schedule_timeout_interruptible(msecs_to_jiffies(10))) {
>  			/* some signal arrived, stop search */
>  			tea->val &= ~TEA575X_BIT_SEARCH;
> -			snd_tea575x_write(tea, tea->val);
> +			snd_tea575x_set_freq(tea);
>  			return -ERESTARTSYS;
>  		}
>  		if (!(snd_tea575x_read(tea) & TEA575X_BIT_SEARCH)) {
> +			u32 freq;
> +
>  			/* Found a frequency, wait until it can be read */
>  			for (i = 0; i < 100; i++) {
>  				msleep(10);
> -				snd_tea575x_get_freq(tea);
> -				if (tea->freq != 0) /* available */
> +				freq = snd_tea575x_get_freq(tea);
> +				if (freq) /* available */
>  					break;
>  			}
> +			if (freq == 0) /* shouldn't happen */
> +				break;
>  			/* if we moved by less than 50 kHz, continue seeking */
> -			if (abs(old_freq - tea->freq) < 16 * 500) {
> +			if (abs(tea->freq - freq) < 16 * 50) {
>  				snd_tea575x_write(tea, tea->val);
>  				continue;
>  			}
> +			tea->freq = freq;
>  			tea->val &= ~TEA575X_BIT_SEARCH;
>  			return 0;
>  		}
>  	}
> +	tea->val &= ~TEA575X_BIT_SEARCH;
> +	snd_tea575x_set_freq(tea);
>  	return -ETIMEDOUT;
>  }



-- 
Ondrej Zary

Re: [PATCH] tea575x: fix HW seek

[Hans Verkuil]

On Sunday, February 26, 2012 22:02:51 Ondrej Zary wrote:
> On Friday 24 February 2012 10:00:01 Hans Verkuil wrote:
> > On Wednesday, February 22, 2012 09:35:28 Ondrej Zary wrote:
> > > On Tuesday 21 February 2012, Hans Verkuil wrote:
> > > > On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> > > > > Fix HW seek in TEA575x to work properly:
> > > > >  - a delay must be present after search start and before first
> > > > > register read or the seek does weird things
> > > > >  - when the search stops, the new frequency is not available
> > > > > immediately, we must wait until it appears in the register
> > > > > (fortunately, we can clear the frequency bits when starting the
> > > > > search as it starts at the frequency currently set, not from the
> > > > > value written)
> > > > >  - sometimes, seek remains on the current frequency (or moves only a
> > > > > little), so repeat it until it moves by at least 50 kHz
> > > > >
> > > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > > > >
> > > > > --- a/sound/i2c/other/tea575x-tuner.c
> > > > > +++ b/sound/i2c/other/tea575x-tuner.c
> > > > > @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct snd_tea575x
> > > > > *tea, unsigned int val) tea->ops->set_pins(tea, 0);
> > > > >  }
> > > > >
> > > > > -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> > > > > +static u32 snd_tea575x_read(struct snd_tea575x *tea)
> > > > >  {
> > > > >  	u16 l, rdata;
> > > > >  	u32 data = 0;
> > > > > @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct
> > > > > snd_tea575x *tea) return data;
> > > > >  }
> > > > >
> > > > > +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> > > > > +{
> > > > > +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> > > > > +
> > > > > +	if (freq == 0) {
> > > > > +		tea->freq = 0;
> > > >
> > > > Wouldn't it be better to return -EBUSY in this case? VIDIOC_G_FREQUENCY
> > > > should not return frequencies outside the valid frequency range. In
> > > > this case returning -EBUSY seems to make more sense to me.
> > >
> > > The device returns zero frequency when the scan fails to find a
> > > frequency. This is not an error, just an indication that "nothing" is
> > > tuned. So maybe we can return some bogus frequency in vidioc_g_frequency
> > > (like FREQ_LO) in this case (don't know if -EBUSY will break anything).
> > > But HW seek should get the real one (i.e. zero when it's there).
> >
> > How about the following patch? vidioc_g_frequency just returns the last set
> > frequency and the hw_seek restores the original frequency if it can't find
> > another channel.
> 
> Seems to work. That's probably the right thing to do.
> 
> > Also note that the check for < 50 kHz in hw_seek actually checked for < 500
> > kHz. I've fixed that, but I can't test it.
> 
> Thanks. It finds more stations now. To improve reliability, an additional 
> check should be added - the seek sometimes stop at the same station, just a 
> bit more than 50kHz of the original frequency, often in wrong direction. 
> Something like this:
> 
> --- a/sound/i2c/other/tea575x-tuner.c
> +++ b/sound/i2c/other/tea575x-tuner.c
> @@ -280,8 +280,13 @@ static int vidioc_s_hw_freq_seek(struct file *file, void 
> *fh,
>                         }
>                         if (freq == 0) /* shouldn't happen */
>                                 break;
> -                       /* if we moved by less than 50 kHz, continue seeking 
> */
> -                       if (abs(tea->freq - freq) < 16 * 50) {
> +                       /*
> +                        * if we moved by less than 50 kHz, or in the wrong
> +                        * direction, continue seeking
> +                        */
> +                       if (abs(tea->freq - freq) < 16 * 50 ||
> +                           (a->seek_upward && freq < tea->freq) ||
> +                           (!a->seek_upward && freq > tea->freq)) {
>                                 snd_tea575x_write(tea, tea->val);
>                                 continue;
>                         }

Added to the patch series.

> 
> 
> > Do you also know what happens at the boundaries of the frequency range?
> > Does it wrap around, or do you get a timeout?
> 
> No wraparound, it times out so the original frequency is restored. I wonder 
> if -ETIMEDOUT is correct here.

That's actually wrong, it should be -EAGAIN according to the spec.

I'm now returning -EINVAL if the wrap_around value is not supported and I've
updated the spec to mention that possibility explicitly.

My latest tree is here:

The following changes since commit a3db60bcf7671cc011ab4f848cbc40ff7ab52c1e:

  [media] xc5000: declare firmware configuration structures as static const (2012-02-14 17:22:46 -0200)

are available in the git repository at:
  git://linuxtv.org/hverkuil/media_tree.git radio-pci

Hans Verkuil (4):
      tea575x-tuner: update to latest V4L2 framework requirements.
      tea575x: fix HW seek
      radio-maxiradio: use the tea575x framework.
      V4L2 Spec: return -EINVAL on unsupported wrap_around value.

 .../DocBook/media/v4l/vidioc-s-hw-freq-seek.xml    |    6 +-
 drivers/media/radio/Kconfig                        |    2 +-
 drivers/media/radio/radio-maxiradio.c              |  379 ++++----------------
 drivers/media/radio/radio-sf16fmr2.c               |   61 +++-
 include/sound/tea575x-tuner.h                      |    6 +-
 sound/i2c/other/tea575x-tuner.c                    |  169 ++++++---
 sound/pci/es1968.c                                 |   15 +
 sound/pci/fm801.c                                  |   20 +-
 8 files changed, 273 insertions(+), 385 deletions(-)

If there are no more comments, then I want to make a pull request for this
by the end of the week.

Regards,

	Hans

Re: [PATCH] tea575x: fix HW seek

[Ondrej Zary]

On Monday 27 February 2012 09:42:40 Hans Verkuil wrote:
> On Sunday, February 26, 2012 22:02:51 Ondrej Zary wrote:
> > On Friday 24 February 2012 10:00:01 Hans Verkuil wrote:
> > > On Wednesday, February 22, 2012 09:35:28 Ondrej Zary wrote:
> > > > On Tuesday 21 February 2012, Hans Verkuil wrote:
> > > > > On Saturday, February 18, 2012 17:45:45 Ondrej Zary wrote:
> > > > > > Fix HW seek in TEA575x to work properly:
> > > > > >  - a delay must be present after search start and before first
> > > > > > register read or the seek does weird things
> > > > > >  - when the search stops, the new frequency is not available
> > > > > > immediately, we must wait until it appears in the register
> > > > > > (fortunately, we can clear the frequency bits when starting the
> > > > > > search as it starts at the frequency currently set, not from the
> > > > > > value written)
> > > > > >  - sometimes, seek remains on the current frequency (or moves
> > > > > > only a little), so repeat it until it moves by at least 50 kHz
> > > > > >
> > > > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > > > > >
> > > > > > --- a/sound/i2c/other/tea575x-tuner.c
> > > > > > +++ b/sound/i2c/other/tea575x-tuner.c
> > > > > > @@ -89,7 +89,7 @@ static void snd_tea575x_write(struct
> > > > > > snd_tea575x *tea, unsigned int val) tea->ops->set_pins(tea, 0);
> > > > > >  }
> > > > > >
> > > > > > -static unsigned int snd_tea575x_read(struct snd_tea575x *tea)
> > > > > > +static u32 snd_tea575x_read(struct snd_tea575x *tea)
> > > > > >  {
> > > > > >  	u16 l, rdata;
> > > > > >  	u32 data = 0;
> > > > > > @@ -120,6 +120,27 @@ static unsigned int snd_tea575x_read(struct
> > > > > > snd_tea575x *tea) return data;
> > > > > >  }
> > > > > >
> > > > > > +static void snd_tea575x_get_freq(struct snd_tea575x *tea)
> > > > > > +{
> > > > > > +	u32 freq = snd_tea575x_read(tea) & TEA575X_BIT_FREQ_MASK;
> > > > > > +
> > > > > > +	if (freq == 0) {
> > > > > > +		tea->freq = 0;
> > > > >
> > > > > Wouldn't it be better to return -EBUSY in this case?
> > > > > VIDIOC_G_FREQUENCY should not return frequencies outside the valid
> > > > > frequency range. In this case returning -EBUSY seems to make more
> > > > > sense to me.
> > > >
> > > > The device returns zero frequency when the scan fails to find a
> > > > frequency. This is not an error, just an indication that "nothing" is
> > > > tuned. So maybe we can return some bogus frequency in
> > > > vidioc_g_frequency (like FREQ_LO) in this case (don't know if -EBUSY
> > > > will break anything). But HW seek should get the real one (i.e. zero
> > > > when it's there).
> > >
> > > How about the following patch? vidioc_g_frequency just returns the last
> > > set frequency and the hw_seek restores the original frequency if it
> > > can't find another channel.
> >
> > Seems to work. That's probably the right thing to do.
> >
> > > Also note that the check for < 50 kHz in hw_seek actually checked for <
> > > 500 kHz. I've fixed that, but I can't test it.
> >
> > Thanks. It finds more stations now. To improve reliability, an additional
> > check should be added - the seek sometimes stop at the same station, just
> > a bit more than 50kHz of the original frequency, often in wrong
> > direction. Something like this:
> >
> > --- a/sound/i2c/other/tea575x-tuner.c
> > +++ b/sound/i2c/other/tea575x-tuner.c
> > @@ -280,8 +280,13 @@ static int vidioc_s_hw_freq_seek(struct file *file,
> > void *fh,
> >                         }
> >                         if (freq == 0) /* shouldn't happen */
> >                                 break;
> > -                       /* if we moved by less than 50 kHz, continue
> > seeking */
> > -                       if (abs(tea->freq - freq) < 16 * 50) {
> > +                       /*
> > +                        * if we moved by less than 50 kHz, or in the
> > wrong +                        * direction, continue seeking
> > +                        */
> > +                       if (abs(tea->freq - freq) < 16 * 50 ||
> > +                           (a->seek_upward && freq < tea->freq) ||
> > +                           (!a->seek_upward && freq > tea->freq)) {
> >                                 snd_tea575x_write(tea, tea->val);
> >                                 continue;
> >                         }
>
> Added to the patch series.
>
> > > Do you also know what happens at the boundaries of the frequency range?
> > > Does it wrap around, or do you get a timeout?
> >
> > No wraparound, it times out so the original frequency is restored. I
> > wonder if -ETIMEDOUT is correct here.
>
> That's actually wrong, it should be -EAGAIN according to the spec.
>
> I'm now returning -EINVAL if the wrap_around value is not supported and
> I've updated the spec to mention that possibility explicitly.
>
> My latest tree is here:
>
> The following changes since commit
> a3db60bcf7671cc011ab4f848cbc40ff7ab52c1e:
>
>   [media] xc5000: declare firmware configuration structures as static const
> (2012-02-14 17:22:46 -0200)
>
> are available in the git repository at:
>   git://linuxtv.org/hverkuil/media_tree.git radio-pci
>
> Hans Verkuil (4):
>       tea575x-tuner: update to latest V4L2 framework requirements.
>       tea575x: fix HW seek
>       radio-maxiradio: use the tea575x framework.
>       V4L2 Spec: return -EINVAL on unsupported wrap_around value.
>
>  .../DocBook/media/v4l/vidioc-s-hw-freq-seek.xml    |    6 +-
>  drivers/media/radio/Kconfig                        |    2 +-
>  drivers/media/radio/radio-maxiradio.c              |  379
> ++++---------------- drivers/media/radio/radio-sf16fmr2.c               |  
> 61 +++-
>  include/sound/tea575x-tuner.h                      |    6 +-
>  sound/i2c/other/tea575x-tuner.c                    |  169 ++++++---
>  sound/pci/es1968.c                                 |   15 +
>  sound/pci/fm801.c                                  |   20 +-
>  8 files changed, 273 insertions(+), 385 deletions(-)
>
> If there are no more comments, then I want to make a pull request for this
> by the end of the week.

It works. Tested with SF16-FMR2, SF64-PCR, SF64-PCE2 and SF256-PCP.


-- 
Ondrej Zary