David Härdeman wrote:
> 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().
OK!
>>> 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?
I want to take just the opposite direction ;) By reading the decoders on the
first patch
you submitted me in priv, I remembered the time I used to program in assembler,
back on
eigties. On that time, there programming magazines used to put a challenge
requesting for
the smallest/fastest (and trickiest) code to solve a problem ;) Of course,
nobody, author
included, were capable of understanding those codes after a few weeks, without
spending
a few hours to get the tricks.
What I'm trying to say is that those protocol demods are tricky enough
to require a lot of time to understand what the code is really doing.
As understanding the code is a requirement for reviewers and for bug fixes,
we should make life easier, by find the better way to implement the logic that
will help the decoder understanding.
>> 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.
Ok.
>
> New patch coming up...
Ah, I just forgot to comment those lines (sorry, too tired yesterday):
>- size = sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE * 2;
>- size = roundup_pow_of_two(size);
>+ ir->raw->input_dev = input_dev;
>+ INIT_WORK(&ir->raw->rx_work, ir_raw_event_work);
>
>- rc = kfifo_alloc(&ir->raw->kfifo, size, GFP_KERNEL);
>+ rc = kfifo_alloc(&ir->raw->kfifo, sizeof(int) * MAX_IR_EVENT_SIZE,
>+ GFP_KERNEL);
kfifo logic requires a power of two buffer to work, so, please keep the
original roundup_pow_of_two() logic, or add a comment before MAX_IR_EVENT_SIZE.
--
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
