[Review] Multiproto tree (was: Re: Future of Multiproto)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

now we had bad weather, and I had some time to review the code. ;-)


General note
------------
Obviously, the multiproto tree has not been updated from master for a
very long time. When merging care must be taken that no regressions flow
back to the main development tree.

For now I ignored all differences, except for:
- frontend.h
- dvb_frontend.[ch]
- budget-ci.c
- budget-av.c


API extensions (frontend.h)
---------------------------
looks fine


Card drivers (budget-ci, budget-av)
-----------------------------------
I didn't check the details, but the extensions look ok.
You might consider whether parts of the stb0899/stb6100 stuff could be
factored out into a header file. See bsru6.h for an example.
It would reduce the file size, and tables etc could be reused.


dvb-core: dvb_frontend.c
------------------------
fe->legacy is turned on/off by various ioctls:
- FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
- DVBFE_SET_PARAMS -> 0/1
- DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0

fe->legacy controls how the frontend thread works.

Since the frontend device can be accessed by multiple readers,
fe->legacy might be toggled in funny ways.
This might confuse the fe thread or dvb_frontend_add_event. ;-(

Imho ioctls should not set fe->legacy. All parameter conversions should
be done within the ioctl. For example:
- new driver: FE_SET_FRONTEND converts parms to new driver API
- old driver: DVBFE_SET_PARAMS converts parms to old driver API

Then the fe thread can simply use the old driver interface
(fe->legacy==1) or the new one (fe->legacy==0).


The error msg should display the offending parameter:
Instead of
+               printk("%s: Unsupported FEC\n", __func__);
you might use
+               printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__, new_fec);
(same way for other conversion routines)


+       /* Legacy       */
+       if (fe->legacy) {
+               if (fe->ops.set_frontend)
+                       fe->ops.set_frontend(fe, &fepriv->parameters);
+       } else {
+//             if ((dvbfe_sanity_check(fe) == 0)) {
+                       /* Superseding  */
+                       if (fe->ops.set_params)
+                               fe->ops.set_params(fe, &fepriv->fe_params);
+//             } else
+//                     return -EINVAL;
+       }

Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
(See HG master where I added this some time ago.)
Otherwise the application would not see an error status.


       /* don't actually do anything if we're in the LOSTLOCK state,
-        * the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
-       if ((fepriv->state & FESTATE_LOSTLOCK) &&
-           (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift == 0)) {
-               dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
-               return;
+        * the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
+        */
+       /* Legacy       */
+       if (fe->legacy) {
+               if ((fepriv->state & FESTATE_LOSTLOCK) && (fepriv->max_drift == 0)) {
+                       if (fe->ops.get_frontend_algo)
+                               if (fe->ops.get_frontend_algo(fe) == DVBFE_ALGO_RECOVERY)
+                                       dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
+
+                       return 0;
+               }
+       } else {
+               if (fepriv->state & FESTATE_LOSTLOCK) {
+                       if (fe->ops.get_frontend_algo) {
+                               if ((fe->ops.get_frontend_algo(fe) == DVBFE_ALGO_RECOVERY) &&
+                                   (fepriv->max_drift == 0)) {
+
+                                       dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
+                                       return 0;
+                               }
+                       }
+               }
        }

The 'if (fe->legacy)' branch looks broken:
fe->ops.get_frontend_algo is most likely zero, so
dvb_frontend_swzigzag_update_delay will not be called for old drivers.


Finally, I did a quick test with this tree, and it worked. ;-)
- dvb-ttpci driver with DVB-S Rev 1.3 (ves1x93)
- budget driver with DVB-S Nova Rev 1.0 (stv0299)
- VDR (using old API)


CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[Index of Archives]     [Linux Media]     [Video 4 Linux]     [Asterisk]     [Samba]     [Xorg]     [Xfree86]     [Linux USB]

  Powered by Linux