| Message ID | 1408613086-12538-4-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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.
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
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.
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);
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(-)