diff mbox

[v2,3/3,media] rc: remove change_protocol in rc-ir-raw.c

Message ID 1408613086-12538-4-git-send-email-zhangfei.gao@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Zhangfei Gao Aug. 21, 2014, 9:24 a.m. UTC
With commit 4924a311a62f ("[media] rc-core: rename ir-raw.c"),
empty change_protocol was introduced.
As a result, rc_register_device will set dev->enabled_protocols
addording to rc_map->rc_type, which prevent using all protocols.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/media/rc/rc-ir-raw.c |    7 -------
 1 file changed, 7 deletions(-)

Comments

Mauro Carvalho Chehab Aug. 21, 2014, 11:50 a.m. UTC | #1
Em Thu, 21 Aug 2014 17:24:45 +0800
Zhangfei Gao <zhangfei.gao@linaro.org> escreveu:

> With commit 4924a311a62f ("[media] rc-core: rename ir-raw.c"),
> empty change_protocol was introduced.

No. This was introduced on this changeset:

commit da6e162d6a4607362f8478c715c797d84d449f8b
Author: David Härdeman <david@hardeman.nu>
Date:   Thu Apr 3 20:32:16 2014 -0300

    [media] rc-core: simplify sysfs code

> As a result, rc_register_device will set dev->enabled_protocols
> addording to rc_map->rc_type, which prevent using all protocols.

I strongly suspect that this patch will break some things, as
the new code seems to expect that this is always be set. 

See the code at store_protocols(): if this callback is not set,
then it won't allow to disable a protocol.

Also, this doesn't prevent using all protocols. You can still use
"ir-keytable -p all" to enable all protocols (the "all" protocol
type were introduced recently at the userspace tool).

From the way I see, setting the protocol when a table is loaded
is not a bad thing, as:
- if RC tables are loaded, the needed protocol to decode it is
  already known;
- by running just one IR decoder, the IR handling routine will
  be faster and will consume less power;
- on a real case scenario, it is a way more likely that just one
  decoder will ever be needed by the end user.

So, I think that this is just annoying for developers when are checking
if all decoders are working, by sending keycodes from different IR types
at the same time.

> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/media/rc/rc-ir-raw.c |    7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index e8fff2a..a118539 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -240,12 +240,6 @@ ir_raw_get_allowed_protocols(void)
>  	return protocols;
>  }
>  
> -static int change_protocol(struct rc_dev *dev, u64 *rc_type)
> -{
> -	/* the caller will update dev->enabled_protocols */
> -	return 0;
> -}
> -
>  /*
>   * Used to (un)register raw event clients
>   */
> @@ -263,7 +257,6 @@ int ir_raw_event_register(struct rc_dev *dev)
>  
>  	dev->raw->dev = dev;
>  	dev->enabled_protocols = ~0;
> -	dev->change_protocol = change_protocol;
>  	rc = kfifo_alloc(&dev->raw->kfifo,
>  			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
>  			 GFP_KERNEL);
Zhangfei Gao Aug. 27, 2014, 8:42 a.m. UTC | #2
On 08/21/2014 07:50 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Aug 2014 17:24:45 +0800
> Zhangfei Gao <zhangfei.gao@linaro.org> escreveu:
>
>> With commit 4924a311a62f ("[media] rc-core: rename ir-raw.c"),
>> empty change_protocol was introduced.
>
> No. This was introduced on this changeset:
>
> commit da6e162d6a4607362f8478c715c797d84d449f8b
> Author: David Härdeman <david@hardeman.nu>
> Date:   Thu Apr 3 20:32:16 2014 -0300
>
>      [media] rc-core: simplify sysfs code
>
>> As a result, rc_register_device will set dev->enabled_protocols
>> addording to rc_map->rc_type, which prevent using all protocols.
>
> I strongly suspect that this patch will break some things, as
> the new code seems to expect that this is always be set.
>
> See the code at store_protocols(): if this callback is not set,
> then it won't allow to disable a protocol.
>
> Also, this doesn't prevent using all protocols. You can still use
> "ir-keytable -p all" to enable all protocols (the "all" protocol
> type were introduced recently at the userspace tool).
>
>  From the way I see, setting the protocol when a table is loaded
> is not a bad thing, as:
> - if RC tables are loaded, the needed protocol to decode it is
>    already known;
> - by running just one IR decoder, the IR handling routine will
>    be faster and will consume less power;
> - on a real case scenario, it is a way more likely that just one
>    decoder will ever be needed by the end user.
>
> So, I think that this is just annoying for developers when are checking
> if all decoders are working, by sending keycodes from different IR types
> at the same time.
>

Thanks Mauro for the kind explanation.

ir-keytable seems also enalbe specific protocol
-p, --protocol=PROTOCOL

Currently we use lirc user space decoder/keymap and only need 
pulse-length information from kernel.

Thanks for the info.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Aug. 27, 2014, 11:34 a.m. UTC | #3
Em Wed, 27 Aug 2014 16:42:34 +0800
zhangfei <zhangfei.gao@linaro.org> escreveu:

> 
> 
> On 08/21/2014 07:50 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 21 Aug 2014 17:24:45 +0800
> > Zhangfei Gao <zhangfei.gao@linaro.org> escreveu:
> >
> >> With commit 4924a311a62f ("[media] rc-core: rename ir-raw.c"),
> >> empty change_protocol was introduced.
> >
> > No. This was introduced on this changeset:
> >
> > commit da6e162d6a4607362f8478c715c797d84d449f8b
> > Author: David Härdeman <david@hardeman.nu>
> > Date:   Thu Apr 3 20:32:16 2014 -0300
> >
> >      [media] rc-core: simplify sysfs code
> >
> >> As a result, rc_register_device will set dev->enabled_protocols
> >> addording to rc_map->rc_type, which prevent using all protocols.
> >
> > I strongly suspect that this patch will break some things, as
> > the new code seems to expect that this is always be set.
> >
> > See the code at store_protocols(): if this callback is not set,
> > then it won't allow to disable a protocol.
> >
> > Also, this doesn't prevent using all protocols. You can still use
> > "ir-keytable -p all" to enable all protocols (the "all" protocol
> > type were introduced recently at the userspace tool).
> >
> >  From the way I see, setting the protocol when a table is loaded
> > is not a bad thing, as:
> > - if RC tables are loaded, the needed protocol to decode it is
> >    already known;
> > - by running just one IR decoder, the IR handling routine will
> >    be faster and will consume less power;
> > - on a real case scenario, it is a way more likely that just one
> >    decoder will ever be needed by the end user.
> >
> > So, I think that this is just annoying for developers when are checking
> > if all decoders are working, by sending keycodes from different IR types
> > at the same time.
> >
> 
> Thanks Mauro for the kind explanation.
> 
> ir-keytable seems also enalbe specific protocol
> -p, --protocol=PROTOCOL
> 
> Currently we use lirc user space decoder/keymap and only need 
> pulse-length information from kernel.

Well, you can use ir-keytable to disable everything but lirc, not
compile the other hardware decoders or directly write "lirc" to 
/sys/class/rc/rc0/protocols (see Documentation/ABI/testing/sysfs-class-rc).

Anyway, I suggest you to use the hardware decoder instead of lirc,
as the in-kernel decoders should be lighter than lirc and works pretty
well, but this is, of course, your decision. 

Btw, it would make sense, IMHO, to have a way to setup LIRC daemon to
enable LIRC output on a given remote controller, and, optionally,
disabling the hardware decoders that are needlessly enabled.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao Aug. 28, 2014, 9:18 a.m. UTC | #4
On 08/27/2014 07:34 PM, Mauro Carvalho Chehab wrote:

>>>> With commit 4924a311a62f ("[media] rc-core: rename ir-raw.c"),
>>>> empty change_protocol was introduced.
>>>
>>> No. This was introduced on this changeset:
>>>
>>> commit da6e162d6a4607362f8478c715c797d84d449f8b
>>> Author: David Härdeman <david@hardeman.nu>
>>> Date:   Thu Apr 3 20:32:16 2014 -0300
>>>
>>>       [media] rc-core: simplify sysfs code
>>>
>>>> As a result, rc_register_device will set dev->enabled_protocols
>>>> addording to rc_map->rc_type, which prevent using all protocols.
>>>
>>> I strongly suspect that this patch will break some things, as
>>> the new code seems to expect that this is always be set.
>>>
>>> See the code at store_protocols(): if this callback is not set,
>>> then it won't allow to disable a protocol.
>>>
>>> Also, this doesn't prevent using all protocols. You can still use
>>> "ir-keytable -p all" to enable all protocols (the "all" protocol
>>> type were introduced recently at the userspace tool).
>>>
>>>   From the way I see, setting the protocol when a table is loaded
>>> is not a bad thing, as:
>>> - if RC tables are loaded, the needed protocol to decode it is
>>>     already known;
>>> - by running just one IR decoder, the IR handling routine will
>>>     be faster and will consume less power;
>>> - on a real case scenario, it is a way more likely that just one
>>>     decoder will ever be needed by the end user.
>>>
>>> So, I think that this is just annoying for developers when are checking
>>> if all decoders are working, by sending keycodes from different IR types
>>> at the same time.
>>>
>>
>> Thanks Mauro for the kind explanation.
>>
>> ir-keytable seems also enalbe specific protocol
>> -p, --protocol=PROTOCOL
>>
>> Currently we use lirc user space decoder/keymap and only need
>> pulse-length information from kernel.
>
> Well, you can use ir-keytable to disable everything but lirc, not
> compile the other hardware decoders or directly write "lirc" to
> /sys/class/rc/rc0/protocols (see Documentation/ABI/testing/sysfs-class-rc).
>
> Anyway, I suggest you to use the hardware decoder instead of lirc,
> as the in-kernel decoders should be lighter than lirc and works pretty
> well, but this is, of course, your decision.
>
> Btw, it would make sense, IMHO, to have a way to setup LIRC daemon to
> enable LIRC output on a given remote controller, and, optionally,
> disabling the hardware decoders that are needlessly enabled.
>

Thanks Mauro

Double checked, both ir-keytable and /sys/class/rc/rc0/protocols can 
enable/disable protocols, which is much easier than dts passing 
information, and by default all protocols are disabled.

Thanks for this kind information.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index e8fff2a..a118539 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -240,12 +240,6 @@  ir_raw_get_allowed_protocols(void)
 	return protocols;
 }
 
-static int change_protocol(struct rc_dev *dev, u64 *rc_type)
-{
-	/* the caller will update dev->enabled_protocols */
-	return 0;
-}
-
 /*
  * Used to (un)register raw event clients
  */
@@ -263,7 +257,6 @@  int ir_raw_event_register(struct rc_dev *dev)
 
 	dev->raw->dev = dev;
 	dev->enabled_protocols = ~0;
-	dev->change_protocol = change_protocol;
 	rc = kfifo_alloc(&dev->raw->kfifo,
 			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
 			 GFP_KERNEL);