Oliver Endriss wrote:
> 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.
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.
> 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 ?
> 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) ? Suppose something exists that way, still: such a device
will need to have a hardware/fw implementation, rather than a Software implementation.
When there is a hw implementation, swzigzag for the same is totally pointless. If it were
a software implementation then, FE_CAN_RECOVER is 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 ?
>
> 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)
Cool :)
>
> CU
> Oliver
>
Thanks,
Manu
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
