[linux-dvb] [Review] Multiproto tree

Oliver Endriss o.endriss at gmx.de
Mon Nov 12 18:44:44 CET 2007


Manu Abraham wrote:
> Oliver Endriss wrote:
> > ...
> > 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.
> 
> The tables in some cases can't be reused, but the functions can be, due to 
> the initial cut and paste (tables), both set of tables have some amount of 
> common factor, But still in the end there will be a large common factor 
> altogether.
> 
> Ok, It does make sense to move it out to a common header.

If there are only minor differences, some settings could be moved to the
xxx_config struct. A different approach might use #ifdef blocks within
the header file. One could easily select a configuration by doing

#define xxx_CONFIG_A
#include "xxx.h"

> > 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).
> 
> What's your thought, if i just checked for the new callbacks and handled the
> legacy "switch", ie: the check occuring in the init, so on an fe_open() the
> legacy switch will be set, depending upon the driver. That way things would 
> be set just before the thread is started and still be independent of any ioctl 
> handling, thereby avoiding the flip-flop case with an ioctl.
> 
> What do you think ?

Doing this in dvb_register_frontend() seems to be the perfect place,
because all callbacks have been set, and fe device/thread do not exist
yet.

> > 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)
> 
> 
> Ok, fine. Better in fact.
>  
>  
> > +       /* 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.
> 
> True, didn't think about returning the error back to the app. Will fix.
>  
> >        /* 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.
> 
> 
> As a side note in here: 
> FE_CAN_RECOVER for a frontend is crap. ie no such thing really exists. iirc it was 
> added in by Holger for the cinergyT2. Strictly, if it were true:
> 
> Moreover, if such a device which can Recover (FE_CAN_RECOVER), why should it loose 
> lock (FESTATE_LOSTLOCK) ?

Reception might be interrupted due to bad weather.

> Suppose something exists that way, still: such a device  
> will need to have a hardware/fw implementation, rather than a Software implementation.

An intelligent USB device, for example.

> When there is a hw implementation, swzigzag for the same is totally pointless. If it were
> a software implementation then, cis pointless there as well, since we 
> do the regular stuff and there is nothing different about it. So in either case, whether
> it is a hardware or a software implementation, i guess this one flag is totally crap, which
> achieves nothing but gives some people a feeling that their hardware is doing something
> very important. ;-) (At least what i feel)
> 
> Getting back:
> In such a case update_delay is called only for those devices which can recover.
> 
> Or am in thinking something else ?

Basically 'fepriv->max_drift == 0' means 'swzigzag disabled', so the fe
thread does nothing except calling dvb_frontend_swzigzag_update_delay()
in this case.

CU
Oliver

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



More information about the linux-dvb mailing list