[PATCH 1/3] [media] tuner-core: call has_signal for both TV and radio

[PATCH 1/3] [media] tuner-core: call has_signal for both TV and radio

[Mauro Carvalho Chehab]

If g_tuner is called and the tuner is able to return the signal strength
via has_signal(), call the tunner callback to retrieve such data for all
tuner types, not only for radio ones.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/video/tuner-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 1ad5ab6..98adeee 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1178,6 +1178,8 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 		return 0;
 	if (vt->type == t->mode && analog_ops->get_afc)
 		vt->afc = analog_ops->get_afc(&t->fe);
+	if (analog_ops->has_signal)
+		vt->signal = analog_ops->has_signal(&t->fe);
 	if (vt->type != V4L2_TUNER_RADIO) {
 		vt->capability |= V4L2_TUNER_CAP_NORM;
 		vt->rangelow = tv_range[0] * 16;
@@ -1197,8 +1199,6 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 				V4L2_TUNER_SUB_STEREO :
 				V4L2_TUNER_SUB_MONO;
 		}
-		if (analog_ops->has_signal)
-			vt->signal = analog_ops->has_signal(&t->fe);
 		vt->audmode = t->audmode;
 	}
 	vt->capability |= V4L2_TUNER_CAP_LOW | V4L2_TUNER_CAP_STEREO;
-- 
1.7.10.4

[PATCH 2/3] [media] tuner-xc2028: Fix signal strength report

[Mauro Carvalho Chehab]

There are several bugs at the signal strength algorithm:

	- It is using logical OR, instead of bit OR;
	- It doesn't wait up to 18 ms as it should;
	- the strength range is not ok.

Rework on it, in order to make it work.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/common/tuners/tuner-xc2028.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c
index 9638a69..42fdf5c 100644
--- a/drivers/media/common/tuners/tuner-xc2028.c
+++ b/drivers/media/common/tuners/tuner-xc2028.c
@@ -903,7 +903,7 @@ static int xc2028_signal(struct dvb_frontend *fe, u16 *strength)
 {
 	struct xc2028_data *priv = fe->tuner_priv;
 	u16                 frq_lock, signal = 0;
-	int                 rc;
+	int                 rc, i;
 
 	tuner_dbg("%s called\n", __func__);
 
@@ -914,21 +914,28 @@ static int xc2028_signal(struct dvb_frontend *fe, u16 *strength)
 	mutex_lock(&priv->lock);
 
 	/* Sync Lock Indicator */
-	rc = xc2028_get_reg(priv, XREG_LOCK, &frq_lock);
-	if (rc < 0)
-		goto ret;
+	for (i = 0; i < 3; i++) {
+		rc = xc2028_get_reg(priv, XREG_LOCK, &frq_lock);
+		if (rc < 0)
+			goto ret;
 
-	/* Frequency is locked */
-	if (frq_lock == 1)
-		signal = 1 << 11;
+		if (frq_lock)
+			break;
+		msleep(6);
+	}
+
+	/* Frequency was not locked */
+	if (frq_lock == 2)
+		goto ret;
 
 	/* Get SNR of the video signal */
 	rc = xc2028_get_reg(priv, XREG_SNR, &signal);
 	if (rc < 0)
 		goto ret;
 
-	/* Use both frq_lock and signal to generate the result */
-	signal = signal || ((signal & 0x07) << 12);
+	/* Signal level is 3 bits only */
+
+	signal = ((1 << 12) - 1) | ((signal & 0x07) << 12);
 
 ret:
 	mutex_unlock(&priv->lock);
-- 
1.7.10.4

[PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Mauro Carvalho Chehab]

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

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
 };
-- 
1.7.10.4

Re: [PATCH 2/3] [media] tuner-xc2028: Fix signal strength report

[Devin Heitmueller]

On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab
> -       /* Use both frq_lock and signal to generate the result */
> -       signal = signal || ((signal & 0x07) << 12);
> +       /* Signal level is 3 bits only */
> +
> +       signal = ((1 << 12) - 1) | ((signal & 0x07) << 12);

Are you sure this is correct?   It's entirely possible the original
code used a logical or because the signal level isn't valid unless
there is a lock.  The author may have been intending to say:

if (signal != 0) /* There is a lock, so set the signal level */
  signal = (signal & 0x07) << 12;
else
  signal = 0 /* Leave signal level at zero since there is no lock */

I agree that the way the original code was written is confusing, but
it may actually be correct.

Devin


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

Re: [PATCH 2/3] [media] tuner-xc2028: Fix signal strength report

[Devin Heitmueller]

On Thu, Jul 5, 2012 at 10:25 AM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab
>> -       /* Use both frq_lock and signal to generate the result */
>> -       signal = signal || ((signal & 0x07) << 12);
>> +       /* Signal level is 3 bits only */
>> +
>> +       signal = ((1 << 12) - 1) | ((signal & 0x07) << 12);
>
> Are you sure this is correct?   It's entirely possible the original
> code used a logical or because the signal level isn't valid unless
> there is a lock.  The author may have been intending to say:
>
> if (signal != 0) /* There is a lock, so set the signal level */
>   signal = (signal & 0x07) << 12;
> else
>   signal = 0 /* Leave signal level at zero since there is no lock */
>
> I agree that the way the original code was written is confusing, but
> it may actually be correct.

On second reading, it would have needed to be a logical AND, not an OR
in order for my suggestion to have been correct.

That said, empirical results are definitely a stronger argument in
this case.  You did test this change in cases with no signal, signal
lock with weak signal, and signal lock with strong signal, right?

Devin

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

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: [PATCH 2/3] [media] tuner-xc2028: Fix signal strength report

[Mauro Carvalho Chehab]

Em 05-07-2012 11:31, Devin Heitmueller escreveu:
> On Thu, Jul 5, 2012 at 10:25 AM, Devin Heitmueller
> <dheitmueller@kernellabs.com> wrote:
>> On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab
>>> -       /* Use both frq_lock and signal to generate the result */
>>> -       signal = signal || ((signal & 0x07) << 12);
>>> +       /* Signal level is 3 bits only */
>>> +
>>> +       signal = ((1 << 12) - 1) | ((signal & 0x07) << 12);
>>
>> Are you sure this is correct?   It's entirely possible the original
>> code used a logical or because the signal level isn't valid unless
>> there is a lock.  The author may have been intending to say:
>>
>> if (signal != 0) /* There is a lock, so set the signal level */
>>    signal = (signal & 0x07) << 12;
>> else
>>    signal = 0 /* Leave signal level at zero since there is no lock */
>>
>> I agree that the way the original code was written is confusing, but
>> it may actually be correct.

No, the intention there were to do a bit OR. The idea there was that, 
if a lock was given, some signal would be received. The real signal
level would be identified by the remaining bits.

What it was happening due to the code mistake was:

if (lock)
	return 1;
else
	return 0;

> On second reading, it would have needed to be a logical AND, not an OR
> in order for my suggestion to have been correct.
> 
> That said, empirical results are definitely a stronger argument in
> this case.  You did test this change in cases with no signal, signal
> lock with weak signal, and signal lock with strong signal, right?

Yes, it was tested and the new code works fine: it returns 0 without signal
and it returns a value between 0 and 65535 depending on the signal strength.

Just like the DVB API, the V4L2 API spec is not clear about what type of
range should be applied for the signal (linea range? dB?). It just says
that it should be between 0 and 65555.

In the case of xc2028/3028, there are only 3 bits for signal strengh. The 
levels are in dB, with an 8dB step, where 0 means a signal weaker than 8dB 
and 7 means 56 dB or upper.

The code should now be coherent with that.

Regards,
Mauro

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Bert Massop]

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

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Mauro Carvalho Chehab]

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

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Manu Abraham]

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

Re: [PATCH 3/3] [media] tuner, xc2028: add support for get_afc()

[Mauro Carvalho Chehab]

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