* 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).