diff mbox series

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

Message ID 76c2ea87882c87bd35066a85cb48292a36a79fce.1639140967.git.mchehab+huawei@kernel.org
State Accepted
Commit 98c65a3dac95b54bc105e29d492ce18c49353e67
Headers show
Series media: si2157: do some minor improvements at the driver | expand

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 <mchehab+huawei@kernel.org>
---

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/cover.1639140967.git.mchehab+huawei@kernel.org/

 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" <mchehab+huawei@kernel.org>
An: unlisted-recipients:;
Cc: linuxarm@huawei.com, mauro.chehab@huawei.com, "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>, "Antti Palosaari" <crope@iki.fi>, "Mauro Carvalho Chehab" <mchehab@kernel.org>, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
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 <mchehab+huawei@kernel.org>
---

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/cover.1639140967.git.mchehab+huawei@kernel.org/

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" <Robert.Schlabbach@gmx.net>
An: "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>
Cc: mauro.chehab@huawei.com, "Antti Palosaari" <crope@iki.fi>, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
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" <mchehab+huawei@kernel.org>
An: unlisted-recipients:;
Cc: linuxarm@huawei.com, mauro.chehab@huawei.com, "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>, "Antti Palosaari" <crope@iki.fi>, "Mauro Carvalho Chehab" <mchehab@kernel.org>, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
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 <mchehab+huawei@kernel.org>
---

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/cover.1639140967.git.mchehab+huawei@kernel.org/

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 <Robert.Schlabbach@gmx.net> 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" <Robert.Schlabbach@gmx.net>
> An: "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>
> Cc: mauro.chehab@huawei.com, "Antti Palosaari" <crope@iki.fi>, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
> 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" <mchehab+huawei@kernel.org>
> An: unlisted-recipients:;
> Cc: linuxarm@huawei.com, mauro.chehab@huawei.com, "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>, "Antti Palosaari" <crope@iki.fi>, "Mauro Carvalho Chehab" <mchehab@kernel.org>, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
> 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 <mchehab+huawei@kernel.org>
> ---
> 
> 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/cover.1639140967.git.mchehab+huawei@kernel.org/
> 
> 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" <mchehab@kernel.org> 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 <robert_s@gmx.net> escreveu:

> "Mauro Carvalho Chehab" <mchehab@kernel.org> 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" <mchehab@kernel.org> 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
Robert Schlabbach Jan. 10, 2022, 12:52 p.m. UTC | #7
From: "Mauro Carvalho Chehab" <mchehab@kernel.org>
> 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 | #8
Em Mon, 10 Jan 2022 13:52:43 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> From: "Mauro Carvalho Chehab" <mchehab@kernel.org>
> > 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
diff mbox series

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"