On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote:
>On 21 May 2015 at 15:30, David Härdeman <david@xxxxxxxxxxx> wrote:
>> On 2015-05-21 13:51, Antti Seppälä wrote:
>>>
>>> On 21 May 2015 at 12:14, David Härdeman <david@xxxxxxxxxxx> wrote:
>>>>
>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in
>>>> its
>>>> current state. It will, given more than one enabled protocol, encode a
>>>> scancode to pulse/space events according to the rules of a randomly
>>>> chosen
>>>> protocol. That random selection will be influenced by things like *module
>>>> load order* (independent of the separate fact that passing multiple
>>>> protocols to it is completely bogus in the first place).
>>>>
>>>> To be clear: the same scancode may be encoded differently depending on if
>>>> you've load the nec decoder before or after the rc5 decoder! That kind of
>>>> behavior can't go into a release kernel (Mauro...).
>>>>
>>>
>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness
>>> caused by module load ordering you will be happy (or happier)?
>>
>>
>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list()
>> still has no idea of knowing (given more than one protocol) which protocol a
>> given scancode will be encoded according to.
>>
>
>Okay, so how about "demuxing" the first protocol before handing them
>to encoder callback? Simply something like below:
>
>- if (handler->protocols & protocols && handler->encode) {
>+ if (handler->protocols & ffs(protocols) && handler->encode) {
>
>Now the behavior is well-defined even when multiple protocols are selected.
Your patchset introduced ir_raw_encode_scancode() as well as the only
two callers of that function:
drivers/media/rc/nuvoton-cir.c
drivers/media/rc/rc-loopback.c
I realize that the sysfs wakeup_protocols file (which bakes several
protocols into one label) makes defining *the* protocol difficult, but
if you're going to add hacks like this, keep them to the sole driver
using the API rather than the core API itself.
That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a
single protocol, no matter how many are passed to it (the img-ir driver
already sets a precedent here so it wouldn't be an API change to change
to a set of protocols which might be different than what the user
suggested). (Also...yes, that'll make supporting several versions of
e.g. RC6 impossible with the current sysfs code).
Then change both ir_raw_encode_scancode() to take a single protocol enum
and change the *encode function pointer in struct ir_raw_handler to also
take a single protocol enum.
That way encoders won't have to guess (using scanmasks...!?) what
protocol they should encode to.
And Mauro...I strongly suggest you revert all of this encoding stuff
until this has been fixed...it's broken.
>>
>> Useful how?
>
>Keeping dynamically filled lists sorted is a good practice if one
>wishes to achieve determinism in behavior (like running decoders
>always in certain order too).
That's a new "good practice" to me...
--
David Härdeman
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
