2020-06-17 18:55:22

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [RFC 3/4] media: dvb_frontend: move algo-specific settings to a function

As we're planning to call this code on a separate place, let's
fist move it to a different function.

Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/dvb-core/dvb_frontend.c | 90 +++++++++++++++------------
1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 06ea30a689d7..ed85dc2a9183 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1790,6 +1790,54 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
}

+static void prepare_tuning_algo_parameters(struct dvb_frontend *fe)
+{
+ struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+ struct dvb_frontend_private *fepriv = fe->frontend_priv;
+ struct dvb_frontend_tune_settings fetunesettings;
+
+ /* get frontend-specific tuning settings */
+ memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
+ if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
+ fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
+ fepriv->max_drift = fetunesettings.max_drift;
+ fepriv->step_size = fetunesettings.step_size;
+ } else {
+ /* default values */
+ switch (c->delivery_system) {
+ case SYS_DVBS:
+ case SYS_DVBS2:
+ case SYS_ISDBS:
+ case SYS_TURBO:
+ case SYS_DVBC_ANNEX_A:
+ case SYS_DVBC_ANNEX_C:
+ fepriv->min_delay = HZ / 20;
+ fepriv->step_size = c->symbol_rate / 16000;
+ fepriv->max_drift = c->symbol_rate / 2000;
+ break;
+ case SYS_DVBT:
+ case SYS_DVBT2:
+ case SYS_ISDBT:
+ case SYS_DTMB:
+ fepriv->min_delay = HZ / 20;
+ fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
+ fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
+ break;
+ default:
+ /*
+ * FIXME: This sounds wrong! if freqency_stepsize is
+ * defined by the frontend, why not use it???
+ */
+ fepriv->min_delay = HZ / 20;
+ fepriv->step_size = 0; /* no zigzag */
+ fepriv->max_drift = 0;
+ break;
+ }
+ }
+ if (dvb_override_tune_delay > 0)
+ fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
+}
+
/**
* dtv_property_process_set - Sets a single DTV property
* @fe: Pointer to &struct dvb_frontend
@@ -2182,7 +2230,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
{
struct dvb_frontend_private *fepriv = fe->frontend_priv;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
- struct dvb_frontend_tune_settings fetunesettings;
u32 rolloff = 0;

if (dvb_frontend_check_parameters(fe) < 0)
@@ -2260,46 +2307,7 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
if (c->hierarchy == HIERARCHY_NONE && c->code_rate_LP == FEC_NONE)
c->code_rate_LP = FEC_AUTO;

- /* get frontend-specific tuning settings */
- memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
- if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
- fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
- fepriv->max_drift = fetunesettings.max_drift;
- fepriv->step_size = fetunesettings.step_size;
- } else {
- /* default values */
- switch (c->delivery_system) {
- case SYS_DVBS:
- case SYS_DVBS2:
- case SYS_ISDBS:
- case SYS_TURBO:
- case SYS_DVBC_ANNEX_A:
- case SYS_DVBC_ANNEX_C:
- fepriv->min_delay = HZ / 20;
- fepriv->step_size = c->symbol_rate / 16000;
- fepriv->max_drift = c->symbol_rate / 2000;
- break;
- case SYS_DVBT:
- case SYS_DVBT2:
- case SYS_ISDBT:
- case SYS_DTMB:
- fepriv->min_delay = HZ / 20;
- fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
- fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
- break;
- default:
- /*
- * FIXME: This sounds wrong! if freqency_stepsize is
- * defined by the frontend, why not use it???
- */
- fepriv->min_delay = HZ / 20;
- fepriv->step_size = 0; /* no zigzag */
- fepriv->max_drift = 0;
- break;
- }
- }
- if (dvb_override_tune_delay > 0)
- fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
+ prepare_tuning_algo_parameters(fe);

fepriv->state = FESTATE_RETUNE;

--
2.26.2


2020-06-18 09:25:56

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [RFC 3/4] media: dvb_frontend: move algo-specific settings to a function

On 17/06/2020 20:52, Mauro Carvalho Chehab wrote:

> As we're planning to call this code on a separate place, let's

s/on/from/
Suggest: "to call this code from somewhere else"

> fist move it to a different function.

s/fist/first
Suggest: "first move it to its own function"

> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/dvb-core/dvb_frontend.c | 90 +++++++++++++++------------
> 1 file changed, 49 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 06ea30a689d7..ed85dc2a9183 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1790,6 +1790,54 @@ static int dvbv3_set_delivery_system(struct dvb_frontend *fe)
> return emulate_delivery_system(fe, delsys);
> }
>
> +static void prepare_tuning_algo_parameters(struct dvb_frontend *fe)
> +{
> + struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> + struct dvb_frontend_private *fepriv = fe->frontend_priv;
> + struct dvb_frontend_tune_settings fetunesettings;

Suggest: fetunesettings = { 0 };
then we can remove the memset() below.

> +
> + /* get frontend-specific tuning settings */
> + memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
> + if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
> + fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
> + fepriv->max_drift = fetunesettings.max_drift;
> + fepriv->step_size = fetunesettings.step_size;
> + } else {
> + /* default values */
> + switch (c->delivery_system) {
> + case SYS_DVBS:
> + case SYS_DVBS2:
> + case SYS_ISDBS:
> + case SYS_TURBO:
> + case SYS_DVBC_ANNEX_A:
> + case SYS_DVBC_ANNEX_C:
> + fepriv->min_delay = HZ / 20;
> + fepriv->step_size = c->symbol_rate / 16000;
> + fepriv->max_drift = c->symbol_rate / 2000;
> + break;
> + case SYS_DVBT:
> + case SYS_DVBT2:
> + case SYS_ISDBT:
> + case SYS_DTMB:
> + fepriv->min_delay = HZ / 20;
> + fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
> + fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
> + break;
> + default:
> + /*
> + * FIXME: This sounds wrong! if freqency_stepsize is
> + * defined by the frontend, why not use it???
> + */
> + fepriv->min_delay = HZ / 20;
> + fepriv->step_size = 0; /* no zigzag */
> + fepriv->max_drift = 0;
> + break;
> + }
> + }
> + if (dvb_override_tune_delay > 0)
> + fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
> +}
> +
> /**
> * dtv_property_process_set - Sets a single DTV property
> * @fe: Pointer to &struct dvb_frontend
> @@ -2182,7 +2230,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
> {
> struct dvb_frontend_private *fepriv = fe->frontend_priv;
> struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> - struct dvb_frontend_tune_settings fetunesettings;
> u32 rolloff = 0;
>
> if (dvb_frontend_check_parameters(fe) < 0)
> @@ -2260,46 +2307,7 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
> if (c->hierarchy == HIERARCHY_NONE && c->code_rate_LP == FEC_NONE)
> c->code_rate_LP = FEC_AUTO;
>
> - /* get frontend-specific tuning settings */
> - memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings));
> - if (fe->ops.get_tune_settings && (fe->ops.get_tune_settings(fe, &fetunesettings) == 0)) {
> - fepriv->min_delay = (fetunesettings.min_delay_ms * HZ) / 1000;
> - fepriv->max_drift = fetunesettings.max_drift;
> - fepriv->step_size = fetunesettings.step_size;
> - } else {
> - /* default values */
> - switch (c->delivery_system) {
> - case SYS_DVBS:
> - case SYS_DVBS2:
> - case SYS_ISDBS:
> - case SYS_TURBO:
> - case SYS_DVBC_ANNEX_A:
> - case SYS_DVBC_ANNEX_C:
> - fepriv->min_delay = HZ / 20;
> - fepriv->step_size = c->symbol_rate / 16000;
> - fepriv->max_drift = c->symbol_rate / 2000;
> - break;
> - case SYS_DVBT:
> - case SYS_DVBT2:
> - case SYS_ISDBT:
> - case SYS_DTMB:
> - fepriv->min_delay = HZ / 20;
> - fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2;
> - fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1;
> - break;

As an aside, I find it confusing that there are 3 sources for the stepsize.
1) fe->ops.get_tune_settings().step_size
2) fe->ops.info.frequency_stepsize_hz
3) fe->ops.tuner_ops.info.frequency_step_hz

> - default:
> - /*
> - * FIXME: This sounds wrong! if freqency_stepsize is
> - * defined by the frontend, why not use it???
> - */
> - fepriv->min_delay = HZ / 20;
> - fepriv->step_size = 0; /* no zigzag */
> - fepriv->max_drift = 0;
> - break;
> - }
> - }
> - if (dvb_override_tune_delay > 0)
> - fepriv->min_delay = (dvb_override_tune_delay * HZ) / 1000;
> + prepare_tuning_algo_parameters(fe);

LGTM

Reviewed-by: Marc Gonzalez <[email protected]>