duplicate code in media drivers

Hi Mauro,

I found some duplicate code with the help of Coccinelle and Coverity. Notice that these are not code patches, they only point out the duplicate code in some media drivers:

diff -u -p drivers/media/pci/bt8xx/dvb-bt8xx.c /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c

diff -u -p drivers/media/usb/dvb-usb/dib0700_devices.c /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c

tmp/nothing/media/dvb-frontends/mb86a16.c renamed from media/dvb-frontends/mb86a16.c +0 -0

                                                if (sync_chk(state, &junk) == 0) {
                                                        iq_vt_set(state, 1);

diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

@@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc
                        AU8522_TOREGAAGC_REG0E5H_CVBS);

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

        /* Load the Video Decoder Filter Coefficients */
        for (i = 0; i < NUM_FILTER_COEF; i++) {
 

I wonder if some of the cases above were intentionally coded that way or some code needs to be removed.

Thanks
--
Gustavo

Quoted 7 lines

diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

@@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc
                        AU8522_TOREGAAGC_REG0E5H_CVBS);
        au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
 

tmp/nothing/media/dvb-frontends/au8522_decoder.c static void setup_decoder_defaults(struc · line 290

Quoted 8 lines

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -5

-       if (is_svideo) {
                /* Despite what the table says, for the HVR-950q we still need
                   to be in CVBS mode for the S-Video input (reason unknown). */
                /* filter_coef_type = 3; */
-               filter_coef_type = 5;
-       } else {
-               filter_coef_type = 5;
-       }

Better ask Devin about this (c/c).

Quoted 7 lines

Quoted 7 lines

diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

@@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc
                         AU8522_TOREGAAGC_REG0E5H_CVBS);
         au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
 

Quoted 10 lines

tmp/nothing/media/dvb-frontends/au8522_decoder.c static void setup_decoder_defaults(struc · line 290

Quoted 8 lines

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -5

-       if (is_svideo) {
                 /* Despite what the table says, for the HVR-950q we still need
                    to be in CVBS mode for the S-Video input (reason unknown). */
                 /* filter_coef_type = 3; */
-               filter_coef_type = 5;
-       } else {
-               filter_coef_type = 5;
-       }

Better ask Devin about this (c/c).

This was a case where the implementation didn't match the datasheet, and it wasn't clear why the filter coefficients weren't working properly. Essentially I should have labeled that as a TODO or FIXME when I disabled the "right" value and forced it to always be five. It was also likely that the filter coefficients would need to differ if taking video over the IF interface as opposed to CVBS/S-video, which is why I didn't want to get rid of the logic entirely. That said, the only product I've ever seen with the tda18271 mated to the au8522 will likely never be supported for analog video under Linux for unrelated reasons.

That said, it's worked "good enough" since I wrote the code nine years ago, so if somebody wants to submit a patch to either get rid of the if() statement or mark it as a FIXME that will likely never actually get fixed, I wouldn't have an objection to either.

Em Mon, 21 May 2018 14:39:51 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 8 lines

Hi Mauro,

I found some duplicate code with the help of Coccinelle and Coverity. Notice that these are not code patches, they only point out the duplicate code in some media drivers:

diff -u -p drivers/media/pci/bt8xx/dvb-bt8xx.c /tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c

tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c static int advbt771_samsung_tdtc9251dh0_ · line 395

Quoted 9 lines

tmp/nothing/media/pci/bt8xx/dvb-bt8xx.c renamed from media/pci/bt8xx/dvb-bt8xx.c +0 -2

@@ -389,9 +389,7 @@ static int advbt771_samsung_tdtc9251dh0_
         else if (c->frequency < 600000000)
                 bs = 0x08;
         else if (c->frequency < 730000000)
-               bs = 0x08;
         else
-               bs = 0x08;
 
         pllbuf[0] = 0x61;
         pllbuf[1] = div >> 8;

Hmm... I *suspect* that "bs" here controls the frequency range for the tuner. Analog tuners have separate frequency regions that are controlled via a register, into a 4 or 5 bytes I2C sequence. They're all somewhat a clone of an old Philips design.

It should be safe to convert the "BS" sequence on something like:

	if (c->frequency < 173000000)
                 bs = 0x01;
         else if (c->frequency < 470000000)
                 bs = 0x02;
         else
                 bs = 0x08;

Quoted 4 lines

diff -u -p drivers/media/usb/dvb-usb/dib0700_devices.c /tmp/nothing/media/usb/dvb-usb/dib0700_devices.c

tmp/nothing/media/usb/dvb-usb/dib0700_devices.c static int dib809x_tuner_attach(struct d · line 1750

Quoted 10 lines

tmp/nothing/media/usb/dvb-usb/dib0700_devices.c renamed from media/usb/dvb-usb/dib0700_devices.c +0 -7

@@ -1741,13 +1741,6 @@ static int dib809x_tuner_attach(struct d
         struct dib0700_adapter_state *st = adap->priv;
         struct i2c_adapter *tun_i2c = st->dib8000_ops.get_i2c_master(adap->fe_adap[0].fe, DIBX000_I2C_INTERFACE_TUNER, 1);
 
-       if (adap->id == 0) {
-               if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL)
-                       return -ENODEV;
-       } else {
-               if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c, &dib809x_dib0090_config) == NULL)
-                       return -ENODEV;
-       }

I'm almost sure that, on the second if, it should be adap->fe_adap[1].fe. I tried in the past to check this, but didn't got an answer from the one that wrote the code.

Maybe we could add a /* FIXME: check if it is fe_adap[1] */ on the second clause.

tmp/nothing/media/dvb-frontends/mb86a16.c static int mb86a16_set_fe(struct mb86a16 · line 1471

Quoted 13 lines

tmp/nothing/media/usb/dvb-usb/dib0700_devices.c renamed from media/usb/dvb-usb/dib0700_devices.c +0 -0

         st->set_param_save = adap->fe_adap[0].fe->ops.tuner_ops.set_params;
         adap->fe_adap[0].fe->ops.tuner_ops.set_params = dib8096_set_param_override;

diff -u -p drivers/media/dvb-frontends/mb86a16.c /tmp/nothing/media/dvb-frontends/mb86a16.c

tmp/nothing/media/dvb-frontends/mb86a16.c renamed from media/dvb-frontends/mb86a16.c +0 -2

@@ -1466,9 +1466,7 @@ static int mb86a16_set_fe(struct mb86a16
                                                         wait_t = (1572864 + state->srate / 2) / state->srate;
                                                 if (state->srate < 5000)
                                                         /* FIXME ! , should be a long wait ! */
-                                                       msleep_interruptible(wait_t);
                                                 else
-                                                       msleep_interruptible(wait_t);

I suspect that the goal here is to point that sleeping for (1572864 + state->srate / 2) / state->srate when srate is low will mean that it will take a lot of time to converge (probably causing timeout at userspace).

Basically, if srate is < 5000, the sleep time will be between 314 and 1575364 ms. The worse case scenario - although not realistic, in practice - is to wait up to 26 seconds. This is a very long time!

Probably, the right fix here would be to check if wait_t is bigger than a certain amount of time. If so, return an error.

I'm not against removing the if, but, if so, better to add a /* FIXME */ block explaining that.

That's said, this is an old device. I doubt anyone would fix it.

Quoted 8 lines

tmp/nothing/media/dvb-frontends/mb86a16.c renamed from media/dvb-frontends/mb86a16.c +0 -0

                                                 if (sync_chk(state, &junk) == 0) {
                                                         iq_vt_set(state, 1);

diff -u -p drivers/media/dvb-frontends/au8522_decoder.c /tmp/nothing/media/dvb-frontends/au8522_decoder.c

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

@@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc
                         AU8522_TOREGAAGC_REG0E5H_CVBS);

tmp/nothing/media/dvb-frontends/au8522_decoder.c static void setup_decoder_defaults(struc · line 290

Quoted 10 lines

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -5

         au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
 
-       if (is_svideo) {
                 /* Despite what the table says, for the HVR-950q we still need
                    to be in CVBS mode for the S-Video input (reason unknown). */
                 /* filter_coef_type = 3; */
-               filter_coef_type = 5;
-       } else {
-               filter_coef_type = 5;
-       }

Better ask Devin about this (c/c).

Quoted 10 lines

tmp/nothing/media/dvb-frontends/au8522_decoder.c renamed from media/dvb-frontends/au8522_decoder.c +0 -0

         /* Load the Video Decoder Filter Coefficients */
         for (i = 0; i < NUM_FILTER_COEF; i++) {
 

I wonder if some of the cases above were intentionally coded that way or some code needs to be removed.

Thanks
--
Gustavo

Thanks,
Mauro