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@xxxxxxxxxxxxxxxxxxxx> > > > > > > > > --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html