linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
@ 2011-11-21 21:06 Manu Abraham
  2011-11-21 21:21 ` Michael Krufky
  2011-11-22  0:04 ` Andreas Oberritter
  0 siblings, 2 replies; 7+ messages in thread
From: Manu Abraham @ 2011-11-21 21:06 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Andreas Oberritter, Michael Krufky

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0004-TDA18271-Allow-frontend-to-set-DELSYS-rather-than-qu.patch --]
[-- Type: text/x-patch, Size: 4008 bytes --]

From 2ece38602678ae323450d0e35379147e6e086326 Mon Sep 17 00:00:00 2001
From: Manu Abraham <abraham.manu@gmail.com>
Date: Sat, 19 Nov 2011 19:50:09 +0530
Subject: [PATCH 04/13] TDA18271: Allow frontend to set DELSYS, rather than querying fe->ops.info.type

With any tuner that can tune to multiple delivery systems/standards, it does
query fe->ops.info.type to determine frontend type and set the delivery
system type. fe->ops.info.type can handle only 4 delivery systems, viz FE_QPSK,
FE_QAM, FE_OFDM and FE_ATSC.

The change allows the tuner to be set to any delivery system specified in
fe_delivery_system_t, thereby simplifying a lot of issues.

Signed-off-by: Manu Abraham <abraham.manu@gmail.com>
---
 drivers/media/common/tuners/tda18271-fe.c   |   80 +++++++++++++++++++++++++++
 drivers/media/common/tuners/tda18271-priv.h |    2 +
 2 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
index 3347c5b..6e29faf 100644
--- a/drivers/media/common/tuners/tda18271-fe.c
+++ b/drivers/media/common/tuners/tda18271-fe.c
@@ -928,6 +928,85 @@ fail:
 
 /* ------------------------------------------------------------------ */
 
+static int tda18271_set_state(struct dvb_frontend *fe,
+			      enum tuner_param param,
+			      struct tuner_state *state)
+{
+	struct tda18271_priv *priv = fe->tuner_priv;
+	struct tuner_state *req = &priv->request;
+	struct tda18271_std_map *std_map = &priv->std;
+	struct tda18271_std_map_item *map;
+	int ret;
+
+	BUG_ON(!priv);
+	if (param & DVBFE_TUNER_DELSYS)
+		req->delsys = state->delsys;
+	if (param & DVBFE_TUNER_FREQUENCY)
+		req->frequency = state->frequency;
+	if (param & DVBFE_TUNER_BANDWIDTH)
+		req->bandwidth = state->bandwidth;
+
+	priv->mode = TDA18271_DIGITAL;
+
+	switch (req->delsys) {
+	case SYS_ATSC:
+		map = &std_map->atsc_6;
+		req->bandwidth = 6000000;
+		break;
+	case SYS_DVBC_ANNEX_B:
+		map = &std_map->qam_6;
+		req->bandwidth = 6000000;
+		break;
+	case SYS_DVBT:
+	case SYS_DVBT2:
+		switch (req->bandwidth) {
+		case 6000000:
+			map = &std_map->dvbt_6;
+			break;
+		case 7000000:
+			map = &std_map->dvbt_7;
+			break;
+		case 8000000:
+			map = &std_map->dvbt_8;
+			break;
+		default:
+			ret = -EINVAL;
+			goto fail;
+		}
+		break;
+	case SYS_DVBC_ANNEX_AC:
+		map = &std_map->qam_8;
+		req->bandwidth = 8000000;
+		break;
+	default:
+		tda_warn("Invalid delivery system!\n");
+		ret = -EINVAL;
+		goto fail;
+	}
+	tda_dbg("Trying to tune .. delsys=%d modulation=%d frequency=%d bandwidth=%d",
+		req->delsys,
+		req->modulation,
+		req->frequency,
+		req->bandwidth);
+
+	/* When tuning digital, the analog demod must be tri-stated */
+	if (fe->ops.analog_ops.standby)
+		fe->ops.analog_ops.standby(fe);
+
+	ret = tda18271_tune(fe, map, req->frequency, req->bandwidth);
+
+	if (tda_fail(ret))
+		goto fail;
+
+	priv->if_freq   = map->if_freq;
+	priv->frequency = req->frequency;
+	priv->bandwidth = (req->delsys == SYS_DVBT || req->delsys == SYS_DVBT2) ?
+			   req->bandwidth : 0;
+fail:
+	return ret;
+}
+
+
 static int tda18271_set_params(struct dvb_frontend *fe,
 			       struct dvb_frontend_parameters *params)
 {
@@ -1249,6 +1328,7 @@ static const struct dvb_tuner_ops tda18271_tuner_ops = {
 	.init              = tda18271_init,
 	.sleep             = tda18271_sleep,
 	.set_params        = tda18271_set_params,
+	.set_state         = tda18271_set_state,
 	.set_analog_params = tda18271_set_analog_params,
 	.release           = tda18271_release,
 	.set_config        = tda18271_set_config,
diff --git a/drivers/media/common/tuners/tda18271-priv.h b/drivers/media/common/tuners/tda18271-priv.h
index 454c152..bd1bf58 100644
--- a/drivers/media/common/tuners/tda18271-priv.h
+++ b/drivers/media/common/tuners/tda18271-priv.h
@@ -126,6 +126,8 @@ struct tda18271_priv {
 
 	u32 frequency;
 	u32 bandwidth;
+
+	struct tuner_state request;
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-21 21:06 PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS Manu Abraham
@ 2011-11-21 21:21 ` Michael Krufky
  2011-11-21 21:28   ` Manu Abraham
  2011-11-22  0:04 ` Andreas Oberritter
  1 sibling, 1 reply; 7+ messages in thread
From: Michael Krufky @ 2011-11-21 21:21 UTC (permalink / raw)
  To: Manu Abraham
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Andreas Oberritter

Thank you, Manu... After the Linux Kernel Summit in Prague, I had
intentions of solving this exact problem, but you did it first -- good
job!

I have reviewed the patch to the tda18271 driver, and the changes make
good sense to me.  I have one question, however:

Perhaps my eyes have overlooked something -- I fail to see any code
that defines the new "set_state" callback or any code that calls this
new callback within dvb-core (assuming dvb_frontend.c)  I also can't
find the structure declaration of the "tuner_state" struct... ... is
this patch missing from your series, or did I just overlook it?

That missing patch is what interests me most.  Once I can see that
missing code, I'd like to begin discussion on whether we actually need
the additional callback, or if it can simply be handled by the
set_params call.  Likewise, I'm not exactly sure why we need this
affional "struct tuner_state" ...  Perhaps the answer will be
self-explanatory once I see the code - maybe no discussion is
necessary :-P

But this does look good to me so far.  I'd be happy to provide my
"reviewed-by" tag once I can see the missing code mentioned above.

Best regards,

Michael Krufky

On Mon, Nov 21, 2011 at 4:06 PM, Manu Abraham <abraham.manu@gmail.com> wrote:
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-21 21:21 ` Michael Krufky
@ 2011-11-21 21:28   ` Manu Abraham
  2011-11-21 21:42     ` Michael Krufky
  0 siblings, 1 reply; 7+ messages in thread
From: Manu Abraham @ 2011-11-21 21:28 UTC (permalink / raw)
  To: Michael Krufky
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Andreas Oberritter

On 11/22/11, Michael Krufky <mkrufky@linuxtv.org> wrote:
> Thank you, Manu... After the Linux Kernel Summit in Prague, I had
> intentions of solving this exact problem, but you did it first -- good
> job!
>
> I have reviewed the patch to the tda18271 driver, and the changes make
> good sense to me.  I have one question, however:
>
> Perhaps my eyes have overlooked something -- I fail to see any code
> that defines the new "set_state" callback or any code that calls this
> new callback within dvb-core (assuming dvb_frontend.c)  I also can't
> find the structure declaration of the "tuner_state" struct... ... is
> this patch missing from your series, or did I just overlook it?

I guess more like that. The data structure existed for quite a long
while in dvb_frontend.h and hence you don't find any new changes. Only
delivery and modulation added to it.

>
> That missing patch is what interests me most.  Once I can see that
> missing code, I'd like to begin discussion on whether we actually need
> the additional callback, or if it can simply be handled by the
> set_params call.  Likewise, I'm not exactly sure why we need this
> affional "struct tuner_state" ...  Perhaps the answer will be
> self-explanatory once I see the code - maybe no discussion is
> necessary :-P
>
> But this does look good to me so far.  I'd be happy to provide my
> "reviewed-by" tag once I can see the missing code mentioned above.

The callback is used from within a demodulator context as usual and hence.
eg:

 	/* program tuner */
-	if (fe->ops.tuner_ops.set_params)
-		fe->ops.tuner_ops.set_params(fe, params);
+	tstate.delsys = SYS_DVBC_ANNEX_AC;
+	tstate.frequency = c->frequency;
+
+	if (fe->ops.tuner_ops.set_state) {
+		fe->ops.tuner_ops.set_state(fe,
+					    DVBFE_TUNER_DELSYS    |
+					    DVBFE_TUNER_FREQUENCY,
+					    &tstate);
+	} else {
+		if (fe->ops.tuner_ops.set_params)
+			fe->ops.tuner_ops.set_params(fe, params);
+	}


Best Regards,
Manu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-21 21:28   ` Manu Abraham
@ 2011-11-21 21:42     ` Michael Krufky
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Krufky @ 2011-11-21 21:42 UTC (permalink / raw)
  To: Manu Abraham
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Andreas Oberritter

On Mon, Nov 21, 2011 at 4:28 PM, Manu Abraham <abraham.manu@gmail.com> wrote:
> On 11/22/11, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> Thank you, Manu... After the Linux Kernel Summit in Prague, I had
>> intentions of solving this exact problem, but you did it first -- good
>> job!
>>
>> I have reviewed the patch to the tda18271 driver, and the changes make
>> good sense to me.  I have one question, however:
>>
>> Perhaps my eyes have overlooked something -- I fail to see any code
>> that defines the new "set_state" callback or any code that calls this
>> new callback within dvb-core (assuming dvb_frontend.c)  I also can't
>> find the structure declaration of the "tuner_state" struct... ... is
>> this patch missing from your series, or did I just overlook it?
>
> I guess more like that. The data structure existed for quite a long
> while in dvb_frontend.h and hence you don't find any new changes. Only
> delivery and modulation added to it.
>
>>
>> That missing patch is what interests me most.  Once I can see that
>> missing code, I'd like to begin discussion on whether we actually need
>> the additional callback, or if it can simply be handled by the
>> set_params call.  Likewise, I'm not exactly sure why we need this
>> affional "struct tuner_state" ...  Perhaps the answer will be
>> self-explanatory once I see the code - maybe no discussion is
>> necessary :-P
>>
>> But this does look good to me so far.  I'd be happy to provide my
>> "reviewed-by" tag once I can see the missing code mentioned above.
>
> The callback is used from within a demodulator context as usual and hence.
> eg:
>
>        /* program tuner */
> -       if (fe->ops.tuner_ops.set_params)
> -               fe->ops.tuner_ops.set_params(fe, params);
> +       tstate.delsys = SYS_DVBC_ANNEX_AC;
> +       tstate.frequency = c->frequency;
> +
> +       if (fe->ops.tuner_ops.set_state) {
> +               fe->ops.tuner_ops.set_state(fe,
> +                                           DVBFE_TUNER_DELSYS    |
> +                                           DVBFE_TUNER_FREQUENCY,
> +                                           &tstate);
> +       } else {
> +               if (fe->ops.tuner_ops.set_params)
> +                       fe->ops.tuner_ops.set_params(fe, params);
> +       }
>
>
> Best Regards,
> Manu
>

Manu,

Thank you for explaining -- I found that structure in dvb_frontend.h,
now that you've pointed that out.

I am on board with this change -- it is a positive move in the right
direction.  I believe that after this is merged, we may be able to
obsolete and remove the set_params callback.  In fact, we can even
obsolete the set_analog_params callback as well, using set_state as
the single entry point for setting the tuner.  Of course, one step at
a time -- this is great for now.  We should consider the other
optimizations after this has been merged and tested. :-)

Reviewed-by: Michael Krufky <mkrufky@linuxtv.org>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-21 21:06 PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS Manu Abraham
  2011-11-21 21:21 ` Michael Krufky
@ 2011-11-22  0:04 ` Andreas Oberritter
  2011-11-22  0:27   ` Manu Abraham
  2011-11-24 23:40   ` Mauro Carvalho Chehab
  1 sibling, 2 replies; 7+ messages in thread
From: Andreas Oberritter @ 2011-11-22  0:04 UTC (permalink / raw)
  To: Manu Abraham
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Michael Krufky

On 21.11.2011 22:06, Manu Abraham wrote:
> 
> 0004-TDA18271-Allow-frontend-to-set-DELSYS-rather-than-qu.patch
> 
> 
> From 2ece38602678ae323450d0e35379147e6e086326 Mon Sep 17 00:00:00 2001
> From: Manu Abraham <abraham.manu@gmail.com>
> Date: Sat, 19 Nov 2011 19:50:09 +0530
> Subject: [PATCH 04/13] TDA18271: Allow frontend to set DELSYS, rather than querying fe->ops.info.type
> 
> With any tuner that can tune to multiple delivery systems/standards, it does
> query fe->ops.info.type to determine frontend type and set the delivery
> system type. fe->ops.info.type can handle only 4 delivery systems, viz FE_QPSK,
> FE_QAM, FE_OFDM and FE_ATSC.
> 
> The change allows the tuner to be set to any delivery system specified in
> fe_delivery_system_t, thereby simplifying a lot of issues.
> 
> Signed-off-by: Manu Abraham <abraham.manu@gmail.com>
> ---
>  drivers/media/common/tuners/tda18271-fe.c   |   80 +++++++++++++++++++++++++++
>  drivers/media/common/tuners/tda18271-priv.h |    2 +
>  2 files changed, 82 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
> index 3347c5b..6e29faf 100644
> --- a/drivers/media/common/tuners/tda18271-fe.c
> +++ b/drivers/media/common/tuners/tda18271-fe.c
> @@ -928,6 +928,85 @@ fail:
>  
>  /* ------------------------------------------------------------------ */
>  
> +static int tda18271_set_state(struct dvb_frontend *fe,
> +			      enum tuner_param param,
> +			      struct tuner_state *state)
> +{
> +	struct tda18271_priv *priv = fe->tuner_priv;
> +	struct tuner_state *req = &priv->request;
> +	struct tda18271_std_map *std_map = &priv->std;
> +	struct tda18271_std_map_item *map;
> +	int ret;
> +
> +	BUG_ON(!priv);

At this point priv has already been dereferenced.

> +	if (param & DVBFE_TUNER_DELSYS)
> +		req->delsys = state->delsys;
> +	if (param & DVBFE_TUNER_FREQUENCY)
> +		req->frequency = state->frequency;
> +	if (param & DVBFE_TUNER_BANDWIDTH)
> +		req->bandwidth = state->bandwidth;

What happens if one of these flags is not set, when the function is
called for the first time? priv->request doesn't seem to get initialized.

Regards,
Andreas

> +
> +	priv->mode = TDA18271_DIGITAL;
> +
> +	switch (req->delsys) {
> +	case SYS_ATSC:
> +		map = &std_map->atsc_6;
> +		req->bandwidth = 6000000;
> +		break;
> +	case SYS_DVBC_ANNEX_B:
> +		map = &std_map->qam_6;
> +		req->bandwidth = 6000000;
> +		break;
> +	case SYS_DVBT:
> +	case SYS_DVBT2:
> +		switch (req->bandwidth) {
> +		case 6000000:
> +			map = &std_map->dvbt_6;
> +			break;
> +		case 7000000:
> +			map = &std_map->dvbt_7;
> +			break;
> +		case 8000000:
> +			map = &std_map->dvbt_8;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +		break;
> +	case SYS_DVBC_ANNEX_AC:
> +		map = &std_map->qam_8;
> +		req->bandwidth = 8000000;
> +		break;
> +	default:
> +		tda_warn("Invalid delivery system!\n");
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +	tda_dbg("Trying to tune .. delsys=%d modulation=%d frequency=%d bandwidth=%d",
> +		req->delsys,
> +		req->modulation,
> +		req->frequency,
> +		req->bandwidth);
> +
> +	/* When tuning digital, the analog demod must be tri-stated */
> +	if (fe->ops.analog_ops.standby)
> +		fe->ops.analog_ops.standby(fe);
> +
> +	ret = tda18271_tune(fe, map, req->frequency, req->bandwidth);
> +
> +	if (tda_fail(ret))
> +		goto fail;
> +
> +	priv->if_freq   = map->if_freq;
> +	priv->frequency = req->frequency;
> +	priv->bandwidth = (req->delsys == SYS_DVBT || req->delsys == SYS_DVBT2) ?
> +			   req->bandwidth : 0;
> +fail:
> +	return ret;
> +}
> +
> +
>  static int tda18271_set_params(struct dvb_frontend *fe,
>  			       struct dvb_frontend_parameters *params)
>  {
> @@ -1249,6 +1328,7 @@ static const struct dvb_tuner_ops tda18271_tuner_ops = {
>  	.init              = tda18271_init,
>  	.sleep             = tda18271_sleep,
>  	.set_params        = tda18271_set_params,
> +	.set_state         = tda18271_set_state,
>  	.set_analog_params = tda18271_set_analog_params,
>  	.release           = tda18271_release,
>  	.set_config        = tda18271_set_config,
> diff --git a/drivers/media/common/tuners/tda18271-priv.h b/drivers/media/common/tuners/tda18271-priv.h
> index 454c152..bd1bf58 100644
> --- a/drivers/media/common/tuners/tda18271-priv.h
> +++ b/drivers/media/common/tuners/tda18271-priv.h
> @@ -126,6 +126,8 @@ struct tda18271_priv {
>  
>  	u32 frequency;
>  	u32 bandwidth;
> +
> +	struct tuner_state request;
>  };
>  
>  /*---------------------------------------------------------------------*/
> -- 1.7.1
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-22  0:04 ` Andreas Oberritter
@ 2011-11-22  0:27   ` Manu Abraham
  2011-11-24 23:40   ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 7+ messages in thread
From: Manu Abraham @ 2011-11-22  0:27 UTC (permalink / raw)
  To: Andreas Oberritter
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Michael Krufky

On Tue, Nov 22, 2011 at 5:34 AM, Andreas Oberritter <obi@linuxtv.org> wrote:
> On 21.11.2011 22:06, Manu Abraham wrote:
>>
>> 0004-TDA18271-Allow-frontend-to-set-DELSYS-rather-than-qu.patch
>>
>>
>> From 2ece38602678ae323450d0e35379147e6e086326 Mon Sep 17 00:00:00 2001
>> From: Manu Abraham <abraham.manu@gmail.com>
>> Date: Sat, 19 Nov 2011 19:50:09 +0530
>> Subject: [PATCH 04/13] TDA18271: Allow frontend to set DELSYS, rather than querying fe->ops.info.type
>>
>> With any tuner that can tune to multiple delivery systems/standards, it does
>> query fe->ops.info.type to determine frontend type and set the delivery
>> system type. fe->ops.info.type can handle only 4 delivery systems, viz FE_QPSK,
>> FE_QAM, FE_OFDM and FE_ATSC.
>>
>> The change allows the tuner to be set to any delivery system specified in
>> fe_delivery_system_t, thereby simplifying a lot of issues.
>>
>> Signed-off-by: Manu Abraham <abraham.manu@gmail.com>
>> ---
>>  drivers/media/common/tuners/tda18271-fe.c   |   80 +++++++++++++++++++++++++++
>>  drivers/media/common/tuners/tda18271-priv.h |    2 +
>>  2 files changed, 82 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>> index 3347c5b..6e29faf 100644
>> --- a/drivers/media/common/tuners/tda18271-fe.c
>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>> @@ -928,6 +928,85 @@ fail:
>>
>>  /* ------------------------------------------------------------------ */
>>
>> +static int tda18271_set_state(struct dvb_frontend *fe,
>> +                           enum tuner_param param,
>> +                           struct tuner_state *state)
>> +{
>> +     struct tda18271_priv *priv = fe->tuner_priv;
>> +     struct tuner_state *req = &priv->request;
>> +     struct tda18271_std_map *std_map = &priv->std;
>> +     struct tda18271_std_map_item *map;
>> +     int ret;
>> +
>> +     BUG_ON(!priv);
>
> At this point priv has already been dereferenced.


True, that check is useless.

>
>> +     if (param & DVBFE_TUNER_DELSYS)
>> +             req->delsys = state->delsys;
>> +     if (param & DVBFE_TUNER_FREQUENCY)
>> +             req->frequency = state->frequency;
>> +     if (param & DVBFE_TUNER_BANDWIDTH)
>> +             req->bandwidth = state->bandwidth;
>
> What happens if one of these flags is not set, when the function is
> called for the first time? priv->request doesn't seem to get initialized.

Request needs to be evaluated, whether it is a valid request for the
tuner operation. Need to fix.


Thanks,
Manu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS
  2011-11-22  0:04 ` Andreas Oberritter
  2011-11-22  0:27   ` Manu Abraham
@ 2011-11-24 23:40   ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2011-11-24 23:40 UTC (permalink / raw)
  To: Andreas Oberritter; +Cc: Manu Abraham, Linux Media Mailing List, Michael Krufky

Em 21-11-2011 22:04, Andreas Oberritter escreveu:
> On 21.11.2011 22:06, Manu Abraham wrote:
>>
>> 0004-TDA18271-Allow-frontend-to-set-DELSYS-rather-than-qu.patch
>>
>>
>> From 2ece38602678ae323450d0e35379147e6e086326 Mon Sep 17 00:00:00 2001
>> From: Manu Abraham <abraham.manu@gmail.com>
>> Date: Sat, 19 Nov 2011 19:50:09 +0530
>> Subject: [PATCH 04/13] TDA18271: Allow frontend to set DELSYS, rather than querying fe->ops.info.type
>>
>> With any tuner that can tune to multiple delivery systems/standards, it does
>> query fe->ops.info.type to determine frontend type and set the delivery
>> system type. fe->ops.info.type can handle only 4 delivery systems, viz FE_QPSK,
>> FE_QAM, FE_OFDM and FE_ATSC.
>>
>> The change allows the tuner to be set to any delivery system specified in
>> fe_delivery_system_t, thereby simplifying a lot of issues.
>>
>> Signed-off-by: Manu Abraham <abraham.manu@gmail.com>
>> ---
>>  drivers/media/common/tuners/tda18271-fe.c   |   80 +++++++++++++++++++++++++++
>>  drivers/media/common/tuners/tda18271-priv.h |    2 +
>>  2 files changed, 82 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>> index 3347c5b..6e29faf 100644
>> --- a/drivers/media/common/tuners/tda18271-fe.c
>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>> @@ -928,6 +928,85 @@ fail:
>>  
>>  /* ------------------------------------------------------------------ */
>>  
>> +static int tda18271_set_state(struct dvb_frontend *fe,
>> +			      enum tuner_param param,
>> +			      struct tuner_state *state)
>> +{
>> +	struct tda18271_priv *priv = fe->tuner_priv;
>> +	struct tuner_state *req = &priv->request;
>> +	struct tda18271_std_map *std_map = &priv->std;
>> +	struct tda18271_std_map_item *map;
>> +	int ret;
>> +
>> +	BUG_ON(!priv);
> 
> At this point priv has already been dereferenced.
> 
>> +	if (param & DVBFE_TUNER_DELSYS)
>> +		req->delsys = state->delsys;
>> +	if (param & DVBFE_TUNER_FREQUENCY)
>> +		req->frequency = state->frequency;
>> +	if (param & DVBFE_TUNER_BANDWIDTH)
>> +		req->bandwidth = state->bandwidth;
> 
> What happens if one of these flags is not set, when the function is
> called for the first time? priv->request doesn't seem to get initialized.
> 
> Regards,
> Andreas
> 
>> +
>> +	priv->mode = TDA18271_DIGITAL;
>> +
>> +	switch (req->delsys) {
>> +	case SYS_ATSC:
>> +		map = &std_map->atsc_6;
>> +		req->bandwidth = 6000000;
>> +		break;
>> +	case SYS_DVBC_ANNEX_B:
>> +		map = &std_map->qam_6;
>> +		req->bandwidth = 6000000;
>> +		break;
>> +	case SYS_DVBT:
>> +	case SYS_DVBT2:
>> +		switch (req->bandwidth) {
>> +		case 6000000:
>> +			map = &std_map->dvbt_6;
>> +			break;
>> +		case 7000000:
>> +			map = &std_map->dvbt_7;
>> +			break;
>> +		case 8000000:
>> +			map = &std_map->dvbt_8;
>> +			break;
>> +		default:
>> +			ret = -EINVAL;
>> +			goto fail;
>> +		}
>> +		break;
>> +	case SYS_DVBC_ANNEX_AC:
>> +		map = &std_map->qam_8;
>> +		req->bandwidth = 8000000;
>> +		break;

This premise is not correct, and causes tuning failure on places where
channels are spaced with 6MHz.

The bandwidth should be calculated as a function of the rolloff and symbol
rate. I had to fix it on a few places, for devices to work with Net Digital
Cable operator in Brazil (6MHz spaced channels, 5.217 KBauds per carrier,
DVB-C Annex A).

The correct way for doing it is:

	else if (fe->ops.info.type == FE_QAM) {
		/*
		 * Using a higher bandwidth at the tuner filter may
		 * allow inter-carrier interference.
		 * So, determine the minimal channel spacing, in order
		 * to better adjust the tuner filter.
		 * According with ITU-T J.83, the bandwidth is given by:
		 * bw = Simbol Rate * (1 + roll_off), where the roll_off
		 * is equal to 0.15 for Annex A, and 0.13 for annex C
		 */
		if (fe->dtv_property_cache.rolloff == ROLLOFF_13)
			bw = (params->u.qam.symbol_rate * 13) / 10;
		else
			bw = (params->u.qam.symbol_rate * 15) / 10;
		if (bw <= 6000000)
			Standard = HF_DVBC_6MHZ;
		else if (bw <= 7000000)
			Standard = HF_DVBC_7MHZ;
		else
			Standard = HF_DVBC_8MHZ;

(from drivers/media/dvb/frontends/tda18271c2dd.c)

Where ROLLOFF_13 is used on Annex C, and ROLLOF_15 on Annex A.

The same fix should be applied to all DVB-C capable tuners. Alternatively, we 
should put some ancillary function at the core, and let the core estimate the 
needed bandwidth for DVB-C.

PS.: While I didn't test, I suspect that places using 7MHz-spaced channels will 
also increase the reception, as less inter-channel noise will be sent to
the demod.

>> +	default:
>> +		tda_warn("Invalid delivery system!\n");
>> +		ret = -EINVAL;
>> +		goto fail;
>> +	}
>> +	tda_dbg("Trying to tune .. delsys=%d modulation=%d frequency=%d bandwidth=%d",
>> +		req->delsys,
>> +		req->modulation,
>> +		req->frequency,
>> +		req->bandwidth);
>> +
>> +	/* When tuning digital, the analog demod must be tri-stated */
>> +	if (fe->ops.analog_ops.standby)
>> +		fe->ops.analog_ops.standby(fe);
>> +
>> +	ret = tda18271_tune(fe, map, req->frequency, req->bandwidth);
>> +
>> +	if (tda_fail(ret))
>> +		goto fail;
>> +
>> +	priv->if_freq   = map->if_freq;
>> +	priv->frequency = req->frequency;
>> +	priv->bandwidth = (req->delsys == SYS_DVBT || req->delsys == SYS_DVBT2) ?
>> +			   req->bandwidth : 0;
>> +fail:
>> +	return ret;
>> +}
>> +
>> +
>>  static int tda18271_set_params(struct dvb_frontend *fe,
>>  			       struct dvb_frontend_parameters *params)
>>  {
>> @@ -1249,6 +1328,7 @@ static const struct dvb_tuner_ops tda18271_tuner_ops = {
>>  	.init              = tda18271_init,
>>  	.sleep             = tda18271_sleep,
>>  	.set_params        = tda18271_set_params,
>> +	.set_state         = tda18271_set_state,
>>  	.set_analog_params = tda18271_set_analog_params,
>>  	.release           = tda18271_release,
>>  	.set_config        = tda18271_set_config,
>> diff --git a/drivers/media/common/tuners/tda18271-priv.h b/drivers/media/common/tuners/tda18271-priv.h
>> index 454c152..bd1bf58 100644
>> --- a/drivers/media/common/tuners/tda18271-priv.h
>> +++ b/drivers/media/common/tuners/tda18271-priv.h
>> @@ -126,6 +126,8 @@ struct tda18271_priv {
>>  
>>  	u32 frequency;
>>  	u32 bandwidth;
>> +
>> +	struct tuner_state request;
>>  };
>>  
>>  /*---------------------------------------------------------------------*/
>> -- 1.7.1
>>
> 
> --
> 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-11-25  0:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-21 21:06 PATCH 04/13: 0004-TDA18271-Allow-frontend-to-set-DELSYS Manu Abraham
2011-11-21 21:21 ` Michael Krufky
2011-11-21 21:28   ` Manu Abraham
2011-11-21 21:42     ` Michael Krufky
2011-11-22  0:04 ` Andreas Oberritter
2011-11-22  0:27   ` Manu Abraham
2011-11-24 23:40   ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).