On Mon, May 11, 2015 at 7:39 PM, John Stultz john.stultz@linaro.org wrote:
On Sun, May 10, 2015 at 9:38 AM, Ksenija Stanojevic ksenija.stanojevic@gmail.com wrote:
'struct timeval last_tv' is used to get the time of last signal change and 'struct timeval last_intr_tv' is used to get the time of last UART interrupt. 32-bit systems using 'struct timeval' will break in the year 2038, so we have to replace that code with more appropriate types. Here struct timeval is replaced with ktime_t.
Signed-off-by: Ksenija Stanojevic ksenija.stanojevic@gmail.com
drivers/staging/media/lirc/lirc_sir.c | 41 ++++++++++++++--------------------- 1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_sir.c b/drivers/staging/media/lirc/lirc_sir.c index 29087f6..a4ea2b1 100644 --- a/drivers/staging/media/lirc/lirc_sir.c +++ b/drivers/staging/media/lirc/lirc_sir.c @@ -44,7 +44,7 @@ #include <linux/ioport.h> #include <linux/kernel.h> #include <linux/serial_reg.h> -#include <linux/time.h> +#include <linux/ktime.h> #include <linux/string.h> #include <linux/types.h> #include <linux/wait.h> @@ -127,9 +127,9 @@ static int threshold = 3; static DEFINE_SPINLOCK(timer_lock); static struct timer_list timerlist; /* time of last signal change detected */ -static struct timeval last_tv = {0, 0}; +static ktime_t last_tv; /* time of last UART data ready interrupt */ -static struct timeval last_intr_tv = {0, 0}; +static ktime_t last_intr_tv;
So here and for the other values, the _tv postfix means timeval. So since you're changing the type, it probably would be cleaner to change the name to something that makes more sense. ie: last_signal_time, last_interrupt_time, curr_time.
static int last_value;
static DECLARE_WAIT_QUEUE_HEAD(lirc_read_queue); @@ -400,17 +400,16 @@ static void drop_chrdev(void) }
/* SECTION: Hardware */ -static long delta(struct timeval *tv1, struct timeval *tv2) +static long delta(ktime_t tv1, ktime_t tv2) { unsigned long deltv;
ktime_t delkt;
deltv = tv2->tv_sec - tv1->tv_sec;if (deltv > 15)
delkt = ktime_sub(tv2, tv1);if (ktime_compare(delkt, ktime_set(15, 0)) > 0) deltv = 0xFFFFFF; else
deltv = deltv*1000000 +tv2->tv_usec -tv1->tv_usec;
deltv = (int)ktime_to_us(delkt); return deltv;}
Can this whole function be killed and replaced w/ a ktime_to_us(ktime_sub(t1, t2))?
The if (>15 sec) return -1 bit is curious, since it doesn't seem clearly documented I don't see much error checking in the file for this case. I'm guessing its just trying to force the 32bit ceiling instead of overflowing, but that would be easy enough to preserve w/ something like
static inline long delta(ktime_t t1, ktime_t t2) { /* return the delta in 32bit usecs, but cap to UINTMAX in case the delta is greater then 32bits */ return (long) min(ktime_to_us(ktime_sub(t1, t2)), UINT_MAX); }
Hi John,
Thank you for the review.
I already did v2 of this patch because I messed up the subject line : timeva -> timeval. Arnd replied to that version and also cc linux-media group, so they might know better whether the change you proposed is correct.
Thanks, Ksenija