2005-11-01 08:22:10

by Michael Krufky

[permalink] [raw]
Subject: [PATCH 18/37] dvb: let other frontends support FE_DISHNETWORK_SEND_LEGACY_CMD




Attachments:
2382.patch (6.50 kB)

2005-11-03 03:52:33

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 18/37] dvb: let other frontends support FE_DISHNETWORK_SEND_LEGACY_CMD

Michael Krufky <[email protected]> wrote:
>
> +s32 timeval_usec_diff(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);
> +}
> +EXPORT_SYMBOL(timeval_usec_diff);
> +
> +static inline void timeval_usec_add(struct timeval *curtime, u32 add_usec)
> +{
> + curtime->tv_usec += add_usec;
> + if (curtime->tv_usec >= 1000000) {
> + curtime->tv_usec -= 1000000;
> + curtime->tv_sec++;
> + }
> +}

timeval arithmetic like this really shouldn't be hidden in a dvb driver -
it's generic code.

> +/*
> + * Sleep until gettimeofday() > waketime + add_usec
> + * This needs to be as precise as possible, but as the delay is
> + * usually between 2ms and 32ms, it is done using a scheduled msleep
> + * followed by usleep (normally a busy-wait loop) for the remainder
> + */
> +void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec)
> +{
> + struct timeval lasttime;
> + s32 delta, newdelta;
> +
> + timeval_usec_add(waketime, add_usec);
> +
> + do_gettimeofday(&lasttime);
> + delta = timeval_usec_diff(lasttime, *waketime);
> + if (delta > 2500) {
> + msleep((delta - 1500) / 1000);
> + do_gettimeofday(&lasttime);
> + newdelta = timeval_usec_diff(lasttime, *waketime);
> + delta = (newdelta > delta) ? 0 : newdelta;
> + }
> + if (delta > 0)
> + udelay(delta);
> +}
> +EXPORT_SYMBOL(dvb_frontend_sleep_until);

However I don't believe that the driver should be using timevals and
do_gettimeofday() at all. Why not use jiffies-based timing like so
many other parts of the kernel?

2005-11-03 04:17:15

by NooneImportant

[permalink] [raw]
Subject: Re: [PATCH 18/37] dvb: let other frontends support FE_DISHNETWORK_SEND_LEGACY_CMD

On 11/2/05, Andrew Morton wrote:
> Michael Krufky <[email protected]> wrote:
> >
> > +s32 timeval_usec_diff(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);
> > +}
> > +EXPORT_SYMBOL(timeval_usec_diff);
> > +
> > +static inline void timeval_usec_add(struct timeval *curtime, u32 add_usec)
> > +{
> > + curtime->tv_usec += add_usec;
> > + if (curtime->tv_usec >= 1000000) {
> > + curtime->tv_usec -= 1000000;
> > + curtime->tv_sec++;
> > + }
> > +}
>
> timeval arithmetic like this really shouldn't be hidden in a dvb driver -
> it's generic code.
> However I don't believe that the driver should be using timevals and
> do_gettimeofday() at all. Why not use jiffies-based timing like so
> many other parts of the kernel?
>
To be honest, I don't like this solution very much either. It only
works when HZ is ~1000, and even then isn't reliable for everyone who
uses this code. All this is attempting to be is a high precision 8ms
timer. Accuracy needs to be +/- 500us for the code to work. I am not
a (very good) kernel programmer, and didn't find anything that would
really fit the bill when trying to figure out how to ensure a routine
gets called every 8ms (9 times). The ktimers stuff I saw recently on
lwn would seem to work well here, but if there is a way to do it with
what is in the kernel today, I'm just not aware of how to do it. So
if someone will show me the right way (or point me in the irght
direction even), I'll be happy to rework the patch.

Thanks,
Noone