Oliver Endriss wrote:
> Hi,
>
> now we had bad weather, and I had some time to review the code. ;-)
>
>
Had an unstable link last week, got fixed.
> 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.
>
I did visit this, but there result looked thus, the device is not specific to a tuner
as it is, and in each of the card configurations, the tables do have some changes
It depends on the card manufacturer.
Thinking thus, i thought of moving all the tables to a header where, something
such as stb0899_config.h where all the card specific tables are thrown in. This
doesn't reduce any of the compiled object size, but it does bring in the advantage
that budget_ci.c, budget_av.c are not cluttered with large tables.
What do you think of moving all the config's to a public config file ?
>
> 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. ;-(
Fixed.
> 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)
Done
>
>
> + /* 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.
Since you already have a check, i guess i can drop the dvbfe_sanity_check()
> /* 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.
>
This one's fixed although i see no usage of FE_CAN_RECOVER which is used in the
tree currently, rather than bogus usage
> 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)
Marco pointed to another bug a wrong copy, which breaks backward compatibility
for DVBFE_GET_PARAMS
Any further issues that you see ?
Additionally i did work upon the Multiple TS stuff, haven't got it working yet.
The concept works like this, unlike what we thought would be:
Manu,
The filtering is just like an IP address and subnet mask.
ISI_ENTRY equivalent to IP address.
IS_BIT_EN equivalent to subnet mask.
The incoming Bbheaders are compared to ISI_ENTRY through mask IS_BIT_EN.
Those Bbheaders that pass the test are allowed through to the TS. Those
that do not are deleted.
> CU
> Oliver
>
Thanks,
Manu
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
