On Thu, Sep 22, 2005 NooneImportant wrote:
> I had been requested to add support for the legacy dish-network switches for
> other frontends besides the stv0299. This patch does this using set_voltage
> commands in the case that fe->ops->dishnetwork_send_legacy_command isn't
> defined (as I've said before, at least the stv0299 based budget cards
> respond too slowly to a set_voltage for thegeneric method to work). this
> will likely not work with all frontends, but hopefully will work with
> enough.
>
> I also moved the timing functions into dvb_frontend.c so that we don't need
> duplicate copies for each frontend.
If you add dishnetwork_send_legacy_command code to dvb_frontend,
shouldn't stv0299 just use it, too? Otherwise it *is* a duplication
of code, isn't it?
Also, please add your Signed-off-by: if you want the patch to be
applied.
There are a few nits:
> Index: linux/drivers/media/dvb/dvb-core/dvb_frontend.c
> ===================================================================
> RCS file: /cvs/linuxtv/dvb-kernel/linux/drivers/media/dvb/dvb-core/dvb_frontend.c,v
> retrieving revision 1.112
> diff -u -r1.112 dvb_frontend.c
> --- linux/drivers/media/dvb/dvb-core/dvb_frontend.c 1 Jul 2005 23:20:19 -0000 1.112
> +++ linux/drivers/media/dvb/dvb-core/dvb_frontend.c 22 Sep 2005 17:15:13 -0000
> @@ -577,6 +577,36 @@
> fepriv->thread_pid);
> }
>
> +inline s32 dvb_frontend_calc_usec_delay (struct timeval lasttime, struct timeval curtime)
> +{
> + return ((curtime.tv_usec < lasttime.tv_usec) ?
> + 1000000 - lasttime.tv_usec + curtime.tv_usec :
> + curtime.tv_usec - lasttime.tv_usec);
> +}
shouldn't this be called something like timeval_usec_diff()? It isn't
dvb_frontend specific. BTW, it doesn't look like you need usec
precision, so it would be better to use msec units.
Also: No space between function name and opening parenthesis (repeats
all over).
> +void dvb_frontend_sleep_until (struct timeval *waketime, u32 add_usec)
> +{
> + struct timeval lasttime;
> + s32 delta, newdelta;
> +
> + waketime->tv_usec += add_usec;
> + if (waketime->tv_usec >= 1000000) {
> + waketime->tv_usec -= 1000000;
> + waketime->tv_sec++;
> + }
You might as well add timeval_usec_add().
> +
> + do_gettimeofday (&lasttime);
> + delta = dvb_frontend_calc_usec_delay (lasttime, *waketime);
> + if (delta > 2500) {
> + msleep ((delta - 1500) / 1000);
> + do_gettimeofday (&lasttime);
> + newdelta = dvb_frontend_calc_usec_delay (lasttime, *waketime);
> + delta = (newdelta > delta) ? 0 : newdelta;
> + }
> + if (delta > 0)
> + udelay (delta);
> +}
dvb_frontend_sleep_until is so idosyncratic, it needs at least a
comment. And of course it will fail to meet its deadline when system
load is high enough, or with HZ == 100. But I guess there's nothing
one ca do about it.
> static int dvb_frontend_start(struct dvb_frontend *fe)
> {
> int ret;
> @@ -728,6 +758,41 @@
> err = fe->ops->dishnetwork_send_legacy_command(fe, (unsigned int) parg);
> fepriv->state = FESTATE_DISEQC;
> fepriv->status = 0;
> + } else if (fe->ops->set_voltage) {
This also needs a comment, and if possible a reference to the
dishnetwork spec.
> + for (i=0; i<9; i++) {
Please add space between operators.
Otherwise patch looks OK.
Johannes
