Re: [RFC2] Teach drivers/media/IR/ir-raw-event.c to use durations

["David Härdeman" <david@hardeman.nu>]

On Thu, Apr 08, 2010 at 02:10:40AM -0300, Mauro Carvalho Chehab wrote:
> David Härdeman wrote:
> > 
> > o The RX decoding is now handled via a workqueue (I can break that up into a
> >   separate patch later, but I think it helps the discussion to have it in for
> >   now), with inspiration from Andy's code.
> 
> I'm in doubt about that. the workqueue is called just after an event. this means
> that, just after every IRQ trigger (assuming the worse case), the workqueue will 
> be called.

No

> On the previous code, it is drivers responsibility to call the function that 
> de-queue. On saa7134, I've scheduled it to wake after 15 ms. So, instead of
> 32 wakeups, just one is done, and the additional delay introduced by it is not
> enough to disturb the user.

It's still the case with my patch, the ir_raw_event_handle() function is 
still there and it will call schedule_work().

>> o int's are still used to represent pulse/space durations in ms.
>>   Mauro and I seem to disagree on this one but I'm right :)
> 
> :)
> 
> We both have different opinions on that. I didn't hear a good argument from you
> why your're right and I am wrong ;)
> 
> Maybe we can try to make a deal on it.
> 
> What you're really doing is:
> 
> struct rc_event {
> 	u32	mark : 1;
> 	u32	duration :31;	/* in microsseconds */
> };
> 
> Please, correct me if I'm wrong, but I suspect that the technical reasons behind your
> proposal are:
> 	1) to save space at kfifo and argument exchange with the functions;
> 	2) nanoseconds is too short for a carrier at the order of 10^4 
> 	Hz;
> 
> My proposal is to do something like:
> 
> struct rc_event {
> 	enum rc_event_type type;
> 	u64 duration		/* in nanoseconds */
> 
> My rationale are:
> 	1) To make the decoder code less obfuscated;

Subjective

> 	2) To use the same time measurement as used on kernel timers, avoiding an uneeded division
> for IRQ and polling-based devices.

Are you sure you don't want to rewrite ir_raw_event_store_edge() and 
ir_raw_event_store() in assembly?
> 
> It might have some other non-technical issues, like foo/bar uses this/that, this means less changes
> on some code, etc, but we shouldn't consider those non-technical issues when discussing
> the architecture.
> 
> So, here's the deal:
>
> 
> Let's do something in-between. While I still think that using a different measure for
> duration will add an unnecessary runtime conversion from kernel ktime into
> microsseconds, for me, the most important point is to avoid obfuscating the code.
> 
> So, we can define a opaque type:
> 
> typedef u32 mark_duration_t;
> 
> To represent the rc_event struct (this isn't a number anymore - it is a struct with one
> bit for mark/space and 31 bits for unsigned duration). The use of an opaque type may
> avoid people to do common mistakes.

I've seldom seen a case where a "typedef gobbledygook" is considered 
clearer than a native data type.

> And use some macros to convert from this type, like:
> 		
> #define DURATION(mark_duration)	abs(mark_duration)
> #define MARK(duration)	(abs(duration))
> #define SPACE(duration)	(-abs(duration))
> #define IS_MARK(mark_duration)	((duration > 0) ? 1 : 0)
> #define IS_SPACE(mark_duration)	((duration < 0) ? 1 : 0)
> #define DURATION(mark_duration)	abs(mark_duration)
> #define TO_UNITS(mark_duration, unit)	\
> 	do { \
> 		a = DIV_ROUND_CLOSEST(DURATION(mark_duration), unit); \
> 		a = (mark_duration < 0) ? -a: a; \
> 	} while (0)
> 
> And use it along the decoders:

If you think a couple of defines would make it that much clearer, I can 
add some defines. If the division in ktime_us_delta() worries you that 
much, I can avoid it as well.

So how about:

s64 duration; /* signed to represent pulse/space, in ns */

This is the return value from ktime subtraction, so no conversion 
necessary. Then I'll also add defines along your lines.

New patch coming up...

-- 
David Härdeman