Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Bengt Martensson-2
[I have not found a "comment of commit in SF, as there is on GitHub. If
I have missed something, tell me gently.]


1. Lirc and Lircd is not a "help" project for the kernel; I do not
consider "foobar has been removed from the kernel" as a commandment that
foobar needs to be removed from Lirc too. Having said that, there is a
*vast* number if "mildly useful", and hardly understood, even less used
option LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE for example.

2. Also nuke the now superfluous test

  curr_driver->features & LIRC_CAN_SET_REC_FILTER

i.e.


-- a/lircd.cpp
+++ b/lircd
@@ -490,8 +490,7 @@

  if (curr_driver->fd != -1 && curr_driver->drvctl_func) {
  if ((curr_driver->features & LIRC_CAN_SET_REC_CARRIER)
-    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT)
-    || (curr_driver->features & LIRC_CAN_SET_REC_FILTER)) {
+    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT) {
  ret = setup_frequency() && setup_timeout();
  }
  }

------------------------------------------------------------------------------
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Alec Leamas


On 27/08/16 10:24, Bengt Martensson wrote:
> [I have not found a "comment of commit in SF, as there is on GitHub. If
> I have missed something, tell me gently.]

I havn't either. That is not to say I don't miss it... This kind of
remark is just fine and much appreciated.
> 1. Lirc and Lircd is not a "help" project for the kernel; I do not
> consider "foobar has been removed from the kernel" as a commandment that
> foobar needs to be removed from Lirc too.

Problem is just that the corresponding symbols is defined in lirc.h,
which is upstreamed (sort of) to the kernel. So, when the kernel removes
these symbols we are kind of lost, the code does not compile. Add to
that that we do nothing with these symbols but forwarding them to the
kernel drivers.

We could probably raise a kernel bug here since the symbols was removed
*after* the header file was made part of the kernel public interface.
However, I see no point in it.

>   Having said that, there is a
> *vast* number if "mildly useful", and hardly understood, even less used
> option LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE for example.

Agreed.
>
> 2. Also nuke the now superfluous test
>
>    curr_driver->features & LIRC_CAN_SET_REC_FILTER

Agreed. Actually, this code should IMHO be refactored for remaining
tests + setup code.


Cheers!

--alec

------------------------------------------------------------------------------
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Jan Ceuleers-2
In reply to this post by Bengt Martensson-2
On 27/08/16 10:24, Bengt Martensson wrote:

> -- a/lircd.cpp
> +++ b/lircd
> @@ -490,8 +490,7 @@
>
>   if (curr_driver->fd != -1 && curr_driver->drvctl_func) {
>   if ((curr_driver->features & LIRC_CAN_SET_REC_CARRIER)
> -    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT)
> -    || (curr_driver->features & LIRC_CAN_SET_REC_FILTER)) {
> +    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT) {
>   ret = setup_frequency() && setup_timeout();
>   }
>   }
>

Unless I'm horribly mistaken (and this does happen a lot) the
parentheses are no longer balanced after applying this patch.

HTH, Jan

------------------------------------------------------------------------------
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Alec Leamas


On 27/08/16 12:33, Jan Ceuleers wrote:

> On 27/08/16 10:24, Bengt Martensson wrote:
>> -- a/lircd.cpp
>> +++ b/lircd
>> @@ -490,8 +490,7 @@
>>
>>     if (curr_driver->fd != -1 && curr_driver->drvctl_func) {
>>     if ((curr_driver->features & LIRC_CAN_SET_REC_CARRIER)
>> -    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT)
>> -    || (curr_driver->features & LIRC_CAN_SET_REC_FILTER)) {
>> +    || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT) {
>>     ret = setup_frequency() && setup_timeout();
>>     }
>>     }
>>
> Unless I'm horribly mistaken (and this does happen a lot) the
> parentheses are no longer balanced after applying this patch.
>
Hi Jan!

Welcome to the list!

That said, I don't see the problem here: One left-brace removed, and one
added. On top of that I don't think Bengt submits code which doesn't
compile.

Cheers!

--alec


------------------------------------------------------------------------------
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Jim Adams
I also see too few parens. The last removed line ends with 2 while the added line only ends with 1.

Sent from my iPad

> On Aug 27, 2016, at 6:55 AM, Alec Leamas <[hidden email]> wrote:
>
>
>
>> On 27/08/16 12:33, Jan Ceuleers wrote:
>>> On 27/08/16 10:24, Bengt Martensson wrote:
>>> -- a/lircd.cpp
>>> +++ b/lircd
>>> @@ -490,8 +490,7 @@
>>>
>>>       if (curr_driver->fd != -1 && curr_driver->drvctl_func) {
>>>           if ((curr_driver->features & LIRC_CAN_SET_REC_CARRIER)
>>> -            || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT)
>>> -            || (curr_driver->features & LIRC_CAN_SET_REC_FILTER)) {
>>> +            || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT) {
>>>                   ret = setup_frequency() && setup_timeout();
>>>           }
>>>       }
>> Unless I'm horribly mistaken (and this does happen a lot) the
>> parentheses are no longer balanced after applying this patch.
> Hi Jan!
>
> Welcome to the list!
>
> That said, I don't see the problem here: One left-brace removed, and one
> added. On top of that I don't think Bengt submits code which doesn't
> compile.
>
> Cheers!
>
> --alec
>
>
> ------------------------------------------------------------------------------

------------------------------------------------------------------------------
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Comment on [31fcc8] lircd: Remove use of functions killed in kernel 4.8.0

Alec Leamas
On 27/08/16 15:06, Jim Adams wrote:

> I also see too few parens. The last removed line ends with 2 while the added line only ends with 1.

Hi Jim!

Of course you and Jan Ceuleers are right. However, this discussion is
based on a simple cut-n-paste error:  the real content of (the relevant
part of)  31fcc8  is

         if ((curr_driver->features & LIRC_CAN_SET_REC_CARRIER)
             || (curr_driver->features & LIRC_CAN_SET_REC_TIMEOUT)
             || (curr_driver->features & LIRC_CAN_SET_REC_FILTER)) {
-           (void)curr_driver->drvctl_func(LIRC_SETUP_START, NULL);
-           ret = setup_frequency() && setup_timeout()
-                 && setup_filter();
-           (void)curr_driver->drvctl_func(LIRC_SETUP_END, NULL);
+               ret = setup_frequency() && setup_timeout();
         }


So, that's why it actually compiles. Sorry for my first reply where I
counted braces instead of parens. Too quick, that one.


Cheers!

--alec

PS: I know it's tricky with a phone, and I'm an occasional sinner
myself. That said, please don't top-post. DS

------------------------------------------------------------------------------
Loading...