| Message ID | 1341497792-6066-3-git-send-email-mchehab@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: > Implement API support to return AFC frequency shift, as this device > supports it. The only other driver that implements it is tda9887, > and the frequency there is reported in Hz. So, use Hz also for this > tuner. What is AFC and why it is needed? > + > + if (frq_lock) > + break; > + msleep(6); 6 ms is too small value for msleep(). you should use usleep_range() instead. regards Antti
On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: > > On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >> >> Implement API support to return AFC frequency shift, as this device >> supports it. The only other driver that implements it is tda9887, >> and the frequency there is reported in Hz. So, use Hz also for this >> tuner. > > > What is AFC and why it is needed? > AFC is short for Automatic Frequency Control, by which a tuner automatically fine-tunes the frequency for the best reception, compensating for small offsets and oscillator frequency drift. This is however done automatically on the tuner, so its configuration is read-only. Aside from being a "nice to know" statistic, getting hold of the AFC frequency shift does as far as I know not have any practical uses related to properly operating the tuner. Regards, Bert -- 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
Em 05-07-2012 14:37, Bert Massop escreveu: > On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: >> >> On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >>> >>> Implement API support to return AFC frequency shift, as this device >>> supports it. The only other driver that implements it is tda9887, >>> and the frequency there is reported in Hz. So, use Hz also for this >>> tuner. >> >> >> What is AFC and why it is needed? >> > > AFC is short for Automatic Frequency Control, by which a tuner > automatically fine-tunes the frequency for the best reception, > compensating for small offsets and oscillator frequency drift. > This is however done automatically on the tuner, so its configuration > is read-only. Aside from being a "nice to know" statistic, getting > hold of the AFC frequency shift does as far as I know not have any > practical uses related to properly operating the tuner. AFC might be useful on a few situations. For example, my CATV operator still broadcasts some channels in both analog and digital. The analog equipment there doesn't seem to be well-maintained, as some channels have frequency shifts or have some other artifacts. Still, analog broadcast is useful for me to test drivers ;) Anyway, adjusting the channel tables to consider that offset shift help to tune them a little faster and/or get a better quality by letting the PLL to work closer to the pilot carrier. Regards, Mauro -- 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
On 07/05/2012 11:10 PM, Mauro Carvalho Chehab wrote: > Em 05-07-2012 14:37, Bert Massop escreveu: >> On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: >>> >>> On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >>>> >>>> Implement API support to return AFC frequency shift, as this device >>>> supports it. The only other driver that implements it is tda9887, >>>> and the frequency there is reported in Hz. So, use Hz also for this >>>> tuner. >>> >>> >>> What is AFC and why it is needed? >>> >> >> AFC is short for Automatic Frequency Control, by which a tuner >> automatically fine-tunes the frequency for the best reception, >> compensating for small offsets and oscillator frequency drift. >> This is however done automatically on the tuner, so its configuration >> is read-only. Aside from being a "nice to know" statistic, getting >> hold of the AFC frequency shift does as far as I know not have any >> practical uses related to properly operating the tuner. > > AFC might be useful on a few situations. For example, my CATV operator > still broadcasts some channels in both analog and digital. The analog > equipment there doesn't seem to be well-maintained, as some channels have > frequency shifts or have some other artifacts. Still, analog broadcast > is useful for me to test drivers ;) > > Anyway, adjusting the channel tables to consider that offset shift help > to tune them a little faster and/or get a better quality by letting the > PLL to work closer to the pilot carrier. We has already .get_frequency() which returns same information. It is not currently used though few drivers implements it (wrongly). So I don't see why this new callback should be added. u32 actual_freq; int afc; struct dtv_frontend_properties *c = &fe->dtv_property_cache; ret = .get_frequency(fe, &actual_freq); afc = c->frequency - actual_freq; regards Antti
Mauro, I am still waiting for your explanation for that. On 07/07/2012 01:46 PM, Antti Palosaari wrote: > On 07/05/2012 11:10 PM, Mauro Carvalho Chehab wrote: >> Em 05-07-2012 14:37, Bert Massop escreveu: >>> On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: >>>> >>>> On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >>>>> >>>>> Implement API support to return AFC frequency shift, as this device >>>>> supports it. The only other driver that implements it is tda9887, >>>>> and the frequency there is reported in Hz. So, use Hz also for this >>>>> tuner. >>>> >>>> >>>> What is AFC and why it is needed? >>>> >>> >>> AFC is short for Automatic Frequency Control, by which a tuner >>> automatically fine-tunes the frequency for the best reception, >>> compensating for small offsets and oscillator frequency drift. >>> This is however done automatically on the tuner, so its configuration >>> is read-only. Aside from being a "nice to know" statistic, getting >>> hold of the AFC frequency shift does as far as I know not have any >>> practical uses related to properly operating the tuner. >> >> AFC might be useful on a few situations. For example, my CATV operator >> still broadcasts some channels in both analog and digital. The analog >> equipment there doesn't seem to be well-maintained, as some channels have >> frequency shifts or have some other artifacts. Still, analog broadcast >> is useful for me to test drivers ;) >> >> Anyway, adjusting the channel tables to consider that offset shift help >> to tune them a little faster and/or get a better quality by letting the >> PLL to work closer to the pilot carrier. > > We has already .get_frequency() which returns same information. It is > not currently used though few drivers implements it (wrongly). So I > don't see why this new callback should be added. > > u32 actual_freq; > int afc; > > struct dtv_frontend_properties *c = &fe->dtv_property_cache; > ret = .get_frequency(fe, &actual_freq); > afc = c->frequency - actual_freq; Let me revise what I think. We have now 3 methods for resolving actual frequency after tuner is set: 1) .get_frequency() ** that is old APIv3 callback returning tuner frequency 2) fe->dtv_property_cache->frequency ** that is newer APIv5 method returning tuner frequency 3) .get_afc() ** new callback to return frequency shift from target frequency For my eyes this kind of duplicate methods are bad, causing only confusion, and should be avoided always when possible. regards Antti
On Fri, Jul 6, 2012 at 1:40 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Em 05-07-2012 14:37, Bert Massop escreveu: >> On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: >>> >>> On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >>>> >>>> Implement API support to return AFC frequency shift, as this device >>>> supports it. The only other driver that implements it is tda9887, >>>> and the frequency there is reported in Hz. So, use Hz also for this >>>> tuner. >>> >>> >>> What is AFC and why it is needed? >>> >> >> AFC is short for Automatic Frequency Control, by which a tuner >> automatically fine-tunes the frequency for the best reception, >> compensating for small offsets and oscillator frequency drift. >> This is however done automatically on the tuner, so its configuration >> is read-only. Aside from being a "nice to know" statistic, getting >> hold of the AFC frequency shift does as far as I know not have any >> practical uses related to properly operating the tuner. > > AFC might be useful on a few situations. For example, my CATV operator > still broadcasts some channels in both analog and digital. If you have really have hardware that does AFC "Automatic Frequency Control", then you shouldn't be exposing this value to userspace. It should be held in the driver alone. Technically, hardware that do not have AFC alone should expose this value to userspace, so that applications can control the dumb piece of hardware, that doesn't lock to Fc aka "Center frequency". All decent tuners do lock onto the center of the step size in any given case, these days. When the driver knows the offset, it needs to compute the offset and sum it to the resultant, so that get_frequency() retrieves the recomputed value. Manu -- 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
Em 06-08-2012 17:10, Antti Palosaari escreveu: > Mauro, I am still waiting for your explanation for that. > > On 07/07/2012 01:46 PM, Antti Palosaari wrote: >> On 07/05/2012 11:10 PM, Mauro Carvalho Chehab wrote: >>> Em 05-07-2012 14:37, Bert Massop escreveu: >>>> On Thu, Jul 5, 2012 at 5:05 PM, Antti Palosaari <crope@iki.fi> wrote: >>>>> >>>>> On 07/05/2012 05:16 PM, Mauro Carvalho Chehab wrote: >>>>>> >>>>>> Implement API support to return AFC frequency shift, as this device >>>>>> supports it. The only other driver that implements it is tda9887, >>>>>> and the frequency there is reported in Hz. So, use Hz also for this >>>>>> tuner. >>>>> >>>>> >>>>> What is AFC and why it is needed? >>>>> >>>> >>>> AFC is short for Automatic Frequency Control, by which a tuner >>>> automatically fine-tunes the frequency for the best reception, >>>> compensating for small offsets and oscillator frequency drift. >>>> This is however done automatically on the tuner, so its configuration >>>> is read-only. Aside from being a "nice to know" statistic, getting >>>> hold of the AFC frequency shift does as far as I know not have any >>>> practical uses related to properly operating the tuner. >>> >>> AFC might be useful on a few situations. For example, my CATV operator >>> still broadcasts some channels in both analog and digital. The analog >>> equipment there doesn't seem to be well-maintained, as some channels have >>> frequency shifts or have some other artifacts. Still, analog broadcast >>> is useful for me to test drivers ;) >>> >>> Anyway, adjusting the channel tables to consider that offset shift help >>> to tune them a little faster and/or get a better quality by letting the >>> PLL to work closer to the pilot carrier. >> >> We has already .get_frequency() which returns same information. It is >> not currently used though few drivers implements it (wrongly). So I >> don't see why this new callback should be added. >> >> u32 actual_freq; >> int afc; >> >> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >> ret = .get_frequency(fe, &actual_freq); >> afc = c->frequency - actual_freq; > > Let me revise what I think. We have now 3 methods for resolving actual frequency after tuner is set: > > 1) .get_frequency() > ** that is old APIv3 callback returning tuner frequency > > 2) fe->dtv_property_cache->frequency > ** that is newer APIv5 method returning tuner frequency > > 3) .get_afc() > ** new callback to return frequency shift from target frequency > > For my eyes this kind of duplicate methods are bad, causing only confusion, and should be avoided always when possible. Yes, duplication is a bad thing. Yet, I don't think get_afc() is a duplication. Let me explain: fe->dtv_property_cache->frequency is only available for DTV. For analog TV, this propery doesn't exist (or it is not visible by the analog core). The get_afc() callback was created to fulfill the analog case. For digital, fe->dtv_property_cache->frequency previous value is not stored, e. g. it stores either the user requested frequency or the frontend-detected one. So, the frequency shift is actually not directly known. Ok, some logic could be added there, if we ever need to return AFC to userspace via dvb_frontend. For frontends with software zig-zag, fe->dtv_property_cache->frequency is updated during the zig-zag logic, so, there's no need to a get_frequency callback (or anything inside the frontend driver to touch at fe->dtv_property_cache->frequency. However, for devices with hardware zig-zag, the only way to get the real frequency is to call the device, after waiting for a while for the device to lock. So, a call to set_frontend() won't work. For that to work, there are currently two ways: get_frontend() or get_frequency(). get_frontend() will not only read the frequency register, but also will read all other I2C status registers, in order to get modulation type, guard interval, etc. That can be too much I2C traffic, when just the frequency offset may be needed. Yet, maybe get_frequency() could be removed. We need to take a look where this is used. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c index 42fdf5c..4857e86 100644 --- a/drivers/media/common/tuners/tuner-xc2028.c +++ b/drivers/media/common/tuners/tuner-xc2028.c @@ -924,7 +924,7 @@ static int xc2028_signal(struct dvb_frontend *fe, u16 *strength) msleep(6); } - /* Frequency was not locked */ + /* Frequency didn't lock */ if (frq_lock == 2) goto ret; @@ -947,6 +947,49 @@ ret: return rc; } +static int xc2028_get_afc(struct dvb_frontend *fe, s32 *afc) +{ + struct xc2028_data *priv = fe->tuner_priv; + int i, rc; + u16 frq_lock = 0; + s16 afc_reg = 0; + + rc = check_device_status(priv); + if (rc < 0) + return rc; + + mutex_lock(&priv->lock); + + /* Sync Lock Indicator */ + for (i = 0; i < 3; i++) { + rc = xc2028_get_reg(priv, XREG_LOCK, &frq_lock); + if (rc < 0) + goto ret; + + if (frq_lock) + break; + msleep(6); + } + + /* Frequency didn't lock */ + if (frq_lock == 2) + goto ret; + + /* Get AFC */ + rc = xc2028_get_reg(priv, XREG_FREQ_ERROR, &afc_reg); + if (rc < 0) + return rc; + + *afc = afc_reg * 15625; /* Hz */ + + tuner_dbg("AFC is %d Hz\n", *afc); + +ret: + mutex_unlock(&priv->lock); + + return rc; +} + #define DIV 15625 static int generic_set_freq(struct dvb_frontend *fe, u32 freq /* in HZ */, @@ -1392,6 +1435,7 @@ static const struct dvb_tuner_ops xc2028_dvb_tuner_ops = { .release = xc2028_dvb_release, .get_frequency = xc2028_get_frequency, .get_rf_strength = xc2028_signal, + .get_afc = xc2028_get_afc, .set_params = xc2028_set_params, .sleep = xc2028_sleep, }; diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.h b/drivers/media/dvb/dvb-core/dvb_frontend.h index e929d56..7c64c09 100644 --- a/drivers/media/dvb/dvb-core/dvb_frontend.h +++ b/drivers/media/dvb/dvb-core/dvb_frontend.h @@ -220,6 +220,7 @@ struct dvb_tuner_ops { #define TUNER_STATUS_STEREO 2 int (*get_status)(struct dvb_frontend *fe, u32 *status); int (*get_rf_strength)(struct dvb_frontend *fe, u16 *strength); + int (*get_afc)(struct dvb_frontend *fe, s32 *afc); /** These are provided separately from set_params in order to facilitate silicon * tuners which require sophisticated tuning loops, controlling each parameter separately. */ diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c index 98adeee..b5a819a 100644 --- a/drivers/media/video/tuner-core.c +++ b/drivers/media/video/tuner-core.c @@ -228,6 +228,16 @@ static int fe_has_signal(struct dvb_frontend *fe) return strength; } +static int fe_get_afc(struct dvb_frontend *fe) +{ + s32 afc = 0; + + if (fe->ops.tuner_ops.get_afc) + fe->ops.tuner_ops.get_afc(fe, &afc); + + return 0; +} + static int fe_set_config(struct dvb_frontend *fe, void *priv_cfg) { struct dvb_tuner_ops *fe_tuner_ops = &fe->ops.tuner_ops; @@ -247,6 +257,7 @@ static struct analog_demod_ops tuner_analog_ops = { .set_params = fe_set_params, .standby = fe_standby, .has_signal = fe_has_signal, + .get_afc = fe_get_afc, .set_config = fe_set_config, .tuner_status = tuner_status };
Implement API support to return AFC frequency shift, as this device supports it. The only other driver that implements it is tda9887, and the frequency there is reported in Hz. So, use Hz also for this tuner. Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> --- drivers/media/common/tuners/tuner-xc2028.c | 46 +++++++++++++++++++++++++++- drivers/media/dvb/dvb-core/dvb_frontend.h | 1 + drivers/media/video/tuner-core.c | 11 +++++++ 3 files changed, 57 insertions(+), 1 deletion(-)