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/
----------------------------------------------------------------
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
