[2/3] media: si2157: add support for 1.7MHz and 6.1 MHz

Commit Message

Mauro Carvalho Chehab Dec. 10, 2021, 12:59 p.m. UTC

Some tuners allow setting the bandwidth filter to 1.7MHz and
6.1 MHz. Add support for it upstream, as the narrower is the
bandwidth filter, the better.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/[e-mail]/

 drivers/media/tuners/si2157.c      | 4 ++++
 drivers/media/tuners/si2157_priv.h | 5 +++++
 2 files changed, 9 insertions(+)

Comments

Robert Schlabbach Jan. 6, 2022, 3:39 p.m. UTC | #1

Sorry for the late test and response, but there is a BAD BUG in this patch:
 
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
+ bandwidth = 0x09;
if (c->bandwidth_hz <= 6000000)
bandwidth = 0x06;
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+ bandwidth = 0x10;
else if (c->bandwidth_hz <= 7000000)
bandwidth = 0x07;
else if (c->bandwidth_hz <= 8000000)

The additions omitted the "else", which resulted in the bandwidth setting for
6MHz being overwritten with the one for 6.1MHz - and that completely breaks
6MHz channels, as the tuner then refuses to accept the tune command!

As a result, e.g. MCNS aka ClearQAM aka DVB-C Annex B no longer works after
this patch.

I don't know if the 1.7Mhz and 6.1MHz settings are even usable, if the tuner
(in my case, the Si2157-A30) then no longer accepts the tune command. Maybe
they're not suited for frequency-based tuning, but only for channel-based
tuning? That would not fit the DVB-API, I think.

And the 1.7MHz bandwidth setting currently can't do any harm, as it is always
overwritten by the 6MHz bandwidth setting, also due to an "else" missing.

Best Regards,
-Robert Schlabbach
 
 

Gesendet: Freitag, 10. Dezember 2021 um 13:59 Uhr
Von: "Mauro Carvalho Chehab" <[e-mail]>
An: unlisted-recipients:;
Cc: [e-mail], [e-mail], "Mauro Carvalho Chehab" <[e-mail]>, "Antti Palosaari" <[e-mail]>, "Mauro Carvalho Chehab" <[e-mail]>, [e-mail], [e-mail]
Betreff: [PATCH 2/3] media: si2157: add support for 1.7MHz and 6.1 MHz
Some tuners allow setting the bandwidth filter to 1.7MHz and
6.1 MHz. Add support for it upstream, as the narrower is the
bandwidth filter, the better.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/[e-mail]/

drivers/media/tuners/si2157.c | 4 ++++
drivers/media/tuners/si2157_priv.h | 5 +++++
2 files changed, 9 insertions(+)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index aeecb38b147f..2d3937af4f5f 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -458,8 +458,12 @@ static int si2157_set_params(struct dvb_frontend *fe)
goto err;
}

+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
+ bandwidth = 0x09;
if (c->bandwidth_hz <= 6000000)
bandwidth = 0x06;
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+ bandwidth = 0x10;
else if (c->bandwidth_hz <= 7000000)
bandwidth = 0x07;
else if (c->bandwidth_hz <= 8000000)
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index df17a5f03561..24849c8ed398 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -66,6 +66,11 @@ struct si2157_cmd {
unsigned rlen;
};

+#define SUPPORTS_1700KHz(dev) (((dev)->part_id == SI2141) || \
+ ((dev)->part_id == SI2147) || \
+ ((dev)->part_id == SI2157) || \
+ ((dev)->part_id == SI2177))
+
/* Old firmware namespace */
#define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
#define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"
--
2.33.1

Robert Schlabbach Jan. 6, 2022, 6:25 p.m. UTC | #2

It turns out there are actually two bugs in there:
 
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+ bandwidth = 0x10;

The si2157 code for bandwidth 6.1MHz is _decimal_ 10, not
hexadecimal 0x10. Removing the "0x" from the above line makes
the tuner work again, even with the other bug that makes it use
the 6.1MHz setting when 6MHz is requested.

Another issue with the bandwidth setting: The si2157 code is
also stored in dev->bandwidth and returned in the get_bandwidth()
call. Now this was not well before, because the call is supposed
to return the bandwidth in Hz, not in MHz, but now it can return
9 and 10, but those do not correspond to 9 and 10MHz.

Also, the default case uses si2157 code 0x0f, which also seems
like a bad idea.

And another, unrelated issue: The delivery_system switch is
missing case DVBC_ANNEX_C, even though it should operate just as
DVBC_ANNEX_A.

I'll see if I can come up with a patch which addresses all of the
above.

Best Regards,
-Robert Schlabbach 


Gesendet: Donnerstag, 06. Januar 2022 um 16:39 Uhr
Von: "Robert Schlabbach" <[e-mail]>
An: "Mauro Carvalho Chehab" <[e-mail]>
Cc: [e-mail], "Antti Palosaari" <[e-mail]>, [e-mail], [e-mail]
Betreff: Re: [PATCH 2/3] media: si2157: add support for 1.7MHz and 6.1 MHz

Sorry for the late test and response, but there is a BAD BUG in this patch:
 
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
+ bandwidth = 0x09;
if (c->bandwidth_hz <= 6000000)
bandwidth = 0x06;
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+ bandwidth = 0x10;
else if (c->bandwidth_hz <= 7000000)
bandwidth = 0x07;
else if (c->bandwidth_hz <= 8000000)

The additions omitted the "else", which resulted in the bandwidth setting for
6MHz being overwritten with the one for 6.1MHz - and that completely breaks
6MHz channels, as the tuner then refuses to accept the tune command!

As a result, e.g. MCNS aka ClearQAM aka DVB-C Annex B no longer works after
this patch.

I don't know if the 1.7Mhz and 6.1MHz settings are even usable, if the tuner
(in my case, the Si2157-A30) then no longer accepts the tune command. Maybe
they're not suited for frequency-based tuning, but only for channel-based
tuning? That would not fit the DVB-API, I think.

And the 1.7MHz bandwidth setting currently can't do any harm, as it is always
overwritten by the 6MHz bandwidth setting, also due to an "else" missing.

Best Regards,
-Robert Schlabbach
 
 

Gesendet: Freitag, 10. Dezember 2021 um 13:59 Uhr
Von: "Mauro Carvalho Chehab" <[e-mail]>
An: unlisted-recipients:;
Cc: [e-mail], [e-mail], "Mauro Carvalho Chehab" <[e-mail]>, "Antti Palosaari" <[e-mail]>, "Mauro Carvalho Chehab" <[e-mail]>, [e-mail], [e-mail]
Betreff: [PATCH 2/3] media: si2157: add support for 1.7MHz and 6.1 MHz
Some tuners allow setting the bandwidth filter to 1.7MHz and
6.1 MHz. Add support for it upstream, as the narrower is the
bandwidth filter, the better.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/[e-mail]/

drivers/media/tuners/si2157.c | 4 ++++
drivers/media/tuners/si2157_priv.h | 5 +++++
2 files changed, 9 insertions(+)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index aeecb38b147f..2d3937af4f5f 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -458,8 +458,12 @@ static int si2157_set_params(struct dvb_frontend *fe)
goto err;
}

+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
+ bandwidth = 0x09;
if (c->bandwidth_hz <= 6000000)
bandwidth = 0x06;
+ if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+ bandwidth = 0x10;
else if (c->bandwidth_hz <= 7000000)
bandwidth = 0x07;
else if (c->bandwidth_hz <= 8000000)
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index df17a5f03561..24849c8ed398 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -66,6 +66,11 @@ struct si2157_cmd {
unsigned rlen;
};

+#define SUPPORTS_1700KHz(dev) (((dev)->part_id == SI2141) || \
+ ((dev)->part_id == SI2147) || \
+ ((dev)->part_id == SI2157) || \
+ ((dev)->part_id == SI2177))
+
/* Old firmware namespace */
#define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
#define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"
--
2.33.1

Mauro Carvalho Chehab Jan. 6, 2022, 8:30 p.m. UTC | #3

Em Thu, 6 Jan 2022 19:25:25 +0100
Robert Schlabbach <[e-mail]> escreveu:

> It turns out there are actually two bugs in there:
>  
> + if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
> + bandwidth = 0x10;
> 
> The si2157 code for bandwidth 6.1MHz is _decimal_ 10, not
> hexadecimal 0x10. Removing the "0x" from the above line makes
> the tuner work again, even with the other bug that makes it use
> the 6.1MHz setting when 6MHz is requested.

Gah, true.

> Another issue with the bandwidth setting: The si2157 code is
> also stored in dev->bandwidth and returned in the get_bandwidth()
> call. Now this was not well before, because the call is supposed
> to return the bandwidth in Hz, not in MHz, but now it can return
> 9 and 10, but those do not correspond to 9 and 10MHz.

I suspect that the entire get_bandwidth() code on drivers are
dead code, as the core doesn't call it anymore. This used to be
needed before DVBv5 API.

Probably, the right fix here would be to simply strip this function
from all drivers.

> Also, the default case uses si2157 code 0x0f, which also seems
> like a bad idea.

Yes. it should default to the maximum bandwidth (8MHz on those
chipsets).

> And another, unrelated issue: The delivery_system switch is
> missing case DVBC_ANNEX_C, even though it should operate just as
> DVBC_ANNEX_A.

The rolloff is different for Annex/A and Annex/C. Annex/A uses
1.15, while Annex/C uses 1.13.

The DVB core already handles it when it calculates the bandwidth
for a given symbol rate, though, so, it shouldn't make any
difference at the tuner, except if the rolloff filter there
can be adjusted.

Btw, I suspect that the 6.1 MHz bandwidth could be there due to
Annex/A and Annex/C differences.

> I'll see if I can come up with a patch which addresses all of the
> above.

OK! I'll wait for your patch.

> 
> Best Regards,
> -Robert Schlabbach 
> 
> 
> Gesendet: Donnerstag, 06. Januar 2022 um 16:39 Uhr
> Von: "Robert Schlabbach" <[e-mail]>
> An: "Mauro Carvalho Chehab" <[e-mail]>
> Cc: [e-mail], "Antti Palosaari" <[e-mail]>, [e-mail], [e-mail]
> Betreff: Re: [PATCH 2/3] media: si2157: add support for 1.7MHz and 6.1 MHz
> 
> Sorry for the late test and response, but there is a BAD BUG in this patch:
>  
> + if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
> + bandwidth = 0x09;
> if (c->bandwidth_hz <= 6000000)
> bandwidth = 0x06;
> + if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
> + bandwidth = 0x10;
> else if (c->bandwidth_hz <= 7000000)
> bandwidth = 0x07;
> else if (c->bandwidth_hz <= 8000000)
> 
> The additions omitted the "else", which resulted in the bandwidth setting for
> 6MHz being overwritten with the one for 6.1MHz - and that completely breaks
> 6MHz channels, as the tuner then refuses to accept the tune command!
> 
> As a result, e.g. MCNS aka ClearQAM aka DVB-C Annex B no longer works after
> this patch.
> 
> I don't know if the 1.7Mhz and 6.1MHz settings are even usable, if the tuner
> (in my case, the Si2157-A30) then no longer accepts the tune command. Maybe
> they're not suited for frequency-based tuning, but only for channel-based
> tuning? That would not fit the DVB-API, I think.
> 
> And the 1.7MHz bandwidth setting currently can't do any harm, as it is always
> overwritten by the 6MHz bandwidth setting, also due to an "else" missing.
> 
> Best Regards,
> -Robert Schlabbach
>  
>  
> 
> Gesendet: Freitag, 10. Dezember 2021 um 13:59 Uhr
> Von: "Mauro Carvalho Chehab" <[e-mail]>
> An: unlisted-recipients:;
> Cc: [e-mail], [e-mail], "Mauro Carvalho Chehab" <[e-mail]>, "Antti Palosaari" <[e-mail]>, "Mauro Carvalho Chehab" <[e-mail]>, [e-mail], [e-mail]
> Betreff: [PATCH 2/3] media: si2157: add support for 1.7MHz and 6.1 MHz
> Some tuners allow setting the bandwidth filter to 1.7MHz and
> 6.1 MHz. Add support for it upstream, as the narrower is the
> bandwidth filter, the better.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> ---
> 
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH 0/3] at: https://lore.kernel.org/all/[e-mail]/
> 
> drivers/media/tuners/si2157.c | 4 ++++
> drivers/media/tuners/si2157_priv.h | 5 +++++
> 2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index aeecb38b147f..2d3937af4f5f 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -458,8 +458,12 @@ static int si2157_set_params(struct dvb_frontend *fe)
> goto err;
> }
> 
> + if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
> + bandwidth = 0x09;
> if (c->bandwidth_hz <= 6000000)
> bandwidth = 0x06;
> + if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
> + bandwidth = 0x10;
> else if (c->bandwidth_hz <= 7000000)
> bandwidth = 0x07;
> else if (c->bandwidth_hz <= 8000000)
> diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
> index df17a5f03561..24849c8ed398 100644
> --- a/drivers/media/tuners/si2157_priv.h
> +++ b/drivers/media/tuners/si2157_priv.h
> @@ -66,6 +66,11 @@ struct si2157_cmd {
> unsigned rlen;
> };
> 
> +#define SUPPORTS_1700KHz(dev) (((dev)->part_id == SI2141) || \
> + ((dev)->part_id == SI2147) || \
> + ((dev)->part_id == SI2157) || \
> + ((dev)->part_id == SI2177))
> +
> /* Old firmware namespace */
> #define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
> #define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"
> --
> 2.33.1
>  



Thanks,
Mauro

Robert Schlabbach Jan. 6, 2022, 11:16 p.m. UTC | #4

"Mauro Carvalho Chehab" <[e-mail]> wrote:
> I suspect that the entire get_bandwidth() code on drivers are
> dead code, as the core doesn't call it anymore. This used to be
> needed before DVBv5 API.
>
> Probably, the right fix here would be to simply strip this function
> from all drivers.

Hmm, I am actually doing this in a frontend driver I'm currently
developing, in the get_frontend() callback function:

if (fe->ops.tuner_ops.get_bandwidth) {
	ret = fe->ops.tuner_ops.get_bandwidth(fe, &bandwidth);
	if (ret)
		goto err;
	props->bandwidth_hz = bandwidth;
}

The documentation for get_frontend() states that it should return
the parameters actually in use. And these might differ from the
requested ones. So I see some value in filling in the actually
applied bandwidth filter there.

> OK! I'll wait for your patch.

Posted. Thanks for your time and patience.

Best Regards,
-Robert Schlabbach

Mauro Carvalho Chehab Jan. 7, 2022, 7:06 a.m. UTC | #5

Em Fri, 7 Jan 2022 00:16:45 +0100
Robert Schlabbach <[e-mail]> escreveu:

> "Mauro Carvalho Chehab" <[e-mail]> wrote:
> > I suspect that the entire get_bandwidth() code on drivers are
> > dead code, as the core doesn't call it anymore. This used to be
> > needed before DVBv5 API.
> >
> > Probably, the right fix here would be to simply strip this function
> > from all drivers.  
> 
> Hmm, I am actually doing this in a frontend driver I'm currently
> developing, in the get_frontend() callback function:
> 
> if (fe->ops.tuner_ops.get_bandwidth) {
> 	ret = fe->ops.tuner_ops.get_bandwidth(fe, &bandwidth);
> 	if (ret)
> 		goto err;
> 	props->bandwidth_hz = bandwidth;
> }
> 
> The documentation for get_frontend() states that it should return
> the parameters actually in use. And these might differ from the
> requested ones. So I see some value in filling in the actually
> applied bandwidth filter there.
> 

Ok, but the tuner driver could just update props->bandwidth_hz directly.

On the other hand, there's not much value on updating it, IMO. 

See, channels are spaced by bandwidth. So, let's say, a 6MHz based
channeling (like ATSC) would have a frequency table like:

	Channel 14: 471.25 MHz
	Channel 15: 477.25 MHz
	Channel 16: 483.25 MHz

Let's suppose the user wants to tune channel 15.

If the tuner bandwidth filter is lower than 6MHz, the demod won't be
able to lock, as the FEC inner coding (Viterbi) will have too many
errors.

If channels 14 and 16 are empty, there won't be co-channel interference,
so any bandwidth between 6 MHz and 12 MHz should work.

If either channel 14 or 16 are used and the bandwidth filter is
higher than 6 MHz, demod will get crosstalk noise, causing troubles to
FEC inner coding. So, it won't properly lock. The end result is that
the tune will also fail. Even if it can eventually tune, the decoded 
stream will have a very poor quality, probably making the channel 
useless.

As the DVB core already stores the bandwidth used to tune at props,
since the introduction of DVBv5 API, any get calls will return the
tuned bandwidth. So, there's no practical need to update 
props->bandwidth_hz.

There's also another reason why this shouldn't be updated. For cable
and satellite tuning, the bandwidth is indirectly estimated by the
DVB core, depending on the roll-off factor, at dtv_set_frontend():

	switch (c->delivery_system) {
	...
	case SYS_DVBC_ANNEX_A:
                rolloff = 115;
                break;
        case SYS_DVBC_ANNEX_C:
                rolloff = 113;
                break;
	...
	}
	if (rolloff)
                c->bandwidth_hz = mult_frac(c->symbol_rate, rolloff, 100);

So, bandwidth_hz actually contain the actual required bandwidth in
order to exclude all co-channel and spurious frequencies, in order
to minimize the noise. This is documented at DVB uAPI[1].

[1] See note 2 at:
    https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/dvb/fe_property_parameters.html

So, for for DVB-C/S/S2, if the tuner driver touches it, it will not 
report the estimated value anymore, violating the uAPI.

So, it sounds that the better is to not use the returned value, which
effectively makes the get_bandwidth() callback useless.

So, I may end preparing a patch series some day to remove
get_bandwidth() everywhere, if I have enough time, but for now,
I'll accept your fix patch.

> > OK! I'll wait for your patch.  
> 
> Posted. Thanks for your time and patience.

Thanks, patches look sane on my eyes.

Thanks,
Mauro

Robert Schlabbach Jan. 7, 2022, 10:38 a.m. UTC | #6

"Mauro Carvalho Chehab" <[e-mail]> wrote:

> As the DVB core already stores the bandwidth used to tune at props,
> since the introduction of DVBv5 API, any get calls will return the
> tuned bandwidth.

No, not the _tuned_ bandwidth, the "requested" bandwidth, that was
estimated. I see no value in that information, as the user app can
easily calculate that by itself. This is not information that the
kernel or driver needs to provide, as it is solely derived from
the information the application has given.

Whereas the _actually applied_ bandwidth filter is an information
that only the tuner driver can deliver. For example, there are 5MHz
DVB-T2 channels, but the si2157 only offers a 6MHz bandwidth filter.

What should get_frontend() return, the requested/nominal 5MHz, or
the actually used 6MHz?

Reading the include file, the answer seems clear to me:

https://git.linuxtv.org/media_tree.git/tree/include/media/dvb_frontend.h

> * @get_frontend:	callback function used to inform the parameters
> *			actuall in use.

So following that documentation, I would say the actually used 6MHz
should be put into the property cache by that callback.

> Thanks, patches look sane on my eyes.

Thanks for your quick review.

Best Regards,
-Robert Schlabbach

Mauro Carvalho Chehab Jan. 9, 2022, 7:09 a.m. UTC | #7

Em Fri, 7 Jan 2022 11:38:37 +0100
Robert Schlabbach <[e-mail]> escreveu:

> "Mauro Carvalho Chehab" <[e-mail]> wrote:
> 
> > As the DVB core already stores the bandwidth used to tune at props,
> > since the introduction of DVBv5 API, any get calls will return the
> > tuned bandwidth.  
> 
> No, not the _tuned_ bandwidth, the "requested" bandwidth, that was
> estimated. I see no value in that information, as the user app can
> easily calculate that by itself. This is not information that the
> kernel or driver needs to provide, as it is solely derived from
> the information the application has given.
> 
> Whereas the _actually applied_ bandwidth filter is an information
> that only the tuner driver can deliver. For example, there are 5MHz
> DVB-T2 channels, but the si2157 only offers a 6MHz bandwidth filter.
> 
> What should get_frontend() return, the requested/nominal 5MHz, or
> the actually used 6MHz?
> 
> Reading the include file, the answer seems clear to me:
> 
> https://git.linuxtv.org/media_tree.git/tree/include/media/dvb_frontend.h
> 
> > * @get_frontend:	callback function used to inform the parameters
> > *			actuall in use.  
> 
> So following that documentation, I would say the actually used 6MHz
> should be put into the property cache by that callback.

For the better or for the worse, the userspace API clearly states that,
for DVB-C/S/S2, it should return the estimated bandwidth, calculated from
Roll-off and signal rate. So, at least for such delivery systems,
props->bandwidh_hz should not be touched by drivers.

There's actually a couple of reasons why bandwidth_hz is calculated
at the core:

1. It helps to have the minimum required bandwidth filter on a single
   place at the core;
2. It helped to address issues with DVB-C Annex-C;
3. It is interesting to allow checking what bandwidth the Kernel actually
   requested the driver, when debugging them. I used this a couple of
   times when fixing Annex-C support on some drivers (I used to have
   a DVB-C Annex-C 6MHz-spaced cable).

For DVB-T/T2, it is even worse, as the properties returned by
FE_GET_PROPERTY are used to store the channel properties to userspace
by tools like dvbv5-scan. Those should reflect the channel 
properties/requirements, not the actual bandwidth filter applied by the
tuner, as the same channel configs can be used by different tuners.

So, a change like that for DVB-T/T2 can actually cause regressions and
bad channel reports when an user is sending patches to the DTV channel
settings on this git tree:

	https://git.linuxtv.org/dtv-scan-tables.git/

-

Now, get_frontend is an internal kAPI. If I'm not mistaken, it was
designed to be used between tuners and demods, for the cases where the
demod would need to know what was the bandwidth set at the tuner.
So, I'm OK if it returns the actually applied bandwidth, provided that
such value is not returned to userspace via DTV_BANDWIDTH_HZ. So,
props->bandwidth_hz should not be updated after its call.

Now, if you think it would be worth to pass the actual bandwidth set
by the hardware to userspace (frankly, I don't know why userspace
might need it), then another DTV property is needed.

Regards,
Mauro

Robert Schlabbach Jan. 10, 2022, 12:52 p.m. UTC | #8

From: "Mauro Carvalho Chehab" <[e-mail]>
> Now, get_frontend is an internal kAPI. If I'm not mistaken, it was
> designed to be used between tuners and demods, for the cases where the
> demod would need to know what was the bandwidth set at the tuner.
> So, I'm OK if it returns the actually applied bandwidth, provided that
> such value is not returned to userspace via DTV_BANDWIDTH_HZ. So,
> props->bandwidth_hz should not be updated after its call.

Good point. It might indeed be interesting for demod drivers, e.g. to
adapt the AFC range setting to the (band)width of the signal delivered
by the tuner (not that I know any driver which does, but hypothetically),
but beyond that, it has no use. So I agree it should not be passed to
userspace.

But for the hypothetical use by the demod driver, the si2157 driver
should return correct values, so the patch I submitted can remain as
it is.

Best Regards,
-Robert Schlabbach

Mauro Carvalho Chehab Jan. 10, 2022, 3:05 p.m. UTC | #9

Em Mon, 10 Jan 2022 13:52:43 +0100
Robert Schlabbach <[e-mail]> escreveu:

> From: "Mauro Carvalho Chehab" <[e-mail]>
> > Now, get_frontend is an internal kAPI. If I'm not mistaken, it was
> > designed to be used between tuners and demods, for the cases where the
> > demod would need to know what was the bandwidth set at the tuner.
> > So, I'm OK if it returns the actually applied bandwidth, provided that
> > such value is not returned to userspace via DTV_BANDWIDTH_HZ. So,
> > props->bandwidth_hz should not be updated after its call.  
> 
> Good point. It might indeed be interesting for demod drivers, e.g. to
> adapt the AFC range setting to the (band)width of the signal delivered
> by the tuner (not that I know any driver which does, but hypothetically),
> but beyond that, it has no use. So I agree it should not be passed to
> userspace.
> 
> But for the hypothetical use by the demod driver, the si2157 driver
> should return correct values, so the patch I submitted can remain as
> it is.

Ok.

FYI, just applied the patches on media-stage.

> 
> Best Regards,
> -Robert Schlabbach



Thanks,
Mauro

Patch

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index aeecb38b147f..2d3937af4f5f 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -458,8 +458,12 @@  static int si2157_set_params(struct dvb_frontend *fe)
 		goto err;
 	}
 
+	if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 1700000)
+		bandwidth = 0x09;
 	if (c->bandwidth_hz <= 6000000)
 		bandwidth = 0x06;
+	if (SUPPORTS_1700KHz(dev) && c->bandwidth_hz <= 6100000)
+		bandwidth = 0x10;
 	else if (c->bandwidth_hz <= 7000000)
 		bandwidth = 0x07;
 	else if (c->bandwidth_hz <= 8000000)
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index df17a5f03561..24849c8ed398 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -66,6 +66,11 @@  struct si2157_cmd {
 	unsigned rlen;
 };
 
+#define SUPPORTS_1700KHz(dev) (((dev)->part_id == SI2141) || \
+			       ((dev)->part_id == SI2147) || \
+			       ((dev)->part_id == SI2157) || \
+			       ((dev)->part_id == SI2177))
+
 /* Old firmware namespace */
 #define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
 #define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"