| Message ID | 1346464629-22458-1-git-send-email-changbin.du@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 01, 2012 at 09:57:09AM +0800, Du, Changbin wrote: > From: "Du, Changbin" <changbin.du@gmail.com> > > Each rc-raw device has a property "allowed_protos" stored in structure > ir_raw_event_ctrl. But it didn't work because all decoders would be > called when decoding. This path makes only allowed protocol decoders > been invoked. > > Signed-off-by: Du, Changbin <changbin.du@gmail.com> > --- > drivers/media/rc/ir-raw.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/rc/ir-raw.c b/drivers/media/rc/ir-raw.c > index a820251..198b6d8 100644 > --- a/drivers/media/rc/ir-raw.c > +++ b/drivers/media/rc/ir-raw.c > @@ -63,8 +63,12 @@ static int ir_raw_event_thread(void *data) > spin_unlock_irq(&raw->lock); > > mutex_lock(&ir_raw_handler_lock); > - list_for_each_entry(handler, &ir_raw_handler_list, list) > - handler->decode(raw->dev, ev); > + list_for_each_entry(handler, &ir_raw_handler_list, list) { > + /* use all protocol by default */ > + if (raw->dev->allowed_protos == RC_TYPE_UNKNOWN || > + raw->dev->allowed_protos & handler->protocols) > + handler->decode(raw->dev, ev); > + } Each IR protocol decoder already checks whether it is enabled or not; should it not be so that only allowed protocols can be enabled rather than checking both enabled_protocols and allowed_protocols? Just from reading store_protocols it looks like decoders which aren't in allowed_protocols can be enabled, which makes no sense. Also ir_raw_event_register all protocols are enabled rather than the allowed ones. Lastely I don't know why raw ir drivers should dictate which protocols can be enabled. Would it not be better to remove it entirely? > raw->prev_ev = ev; > mutex_unlock(&ir_raw_handler_lock); > } > -- > 1.7.9.5 > > -- > 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 -- 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
> > mutex_lock(&ir_raw_handler_lock); > > - list_for_each_entry(handler, &ir_raw_handler_list, list) > > - handler->decode(raw->dev, ev); > > + list_for_each_entry(handler, &ir_raw_handler_list, list) { > > + /* use all protocol by default */ > > + if (raw->dev->allowed_protos == RC_TYPE_UNKNOWN || > > + raw->dev->allowed_protos & handler->protocols) > > + handler->decode(raw->dev, ev); > > + } > > Each IR protocol decoder already checks whether it is enabled or not; > should it not be so that only allowed protocols can be enabled rather > than checking both enabled_protocols and allowed_protocols? > > Just from reading store_protocols it looks like decoders which aren't > in allowed_protocols can be enabled, which makes no sense. Also > ir_raw_event_register all protocols are enabled rather than the > allowed ones. > > > Lastely I don't know why raw ir drivers should dictate which protocols > can be enabled. Would it not be better to remove it entirely? I agree with you. I just thought that the only thing a decoder should care is its decoding logic, but not including decoder management. My idaea is: 1) use enabled_protocols to select decoders in ir_raw.c, but not placed in decoders to do the judgement. 2) remove allowed_protocols or just use it to set the default decoder (also should rename allowed_protocols to default_protocol). I also have a question: Is there a requirement that one more decoders are enabled for a IR device at the same time? And if that will lead to a issue that each decoder may decode a same pulse sequence to different evnets since their protocol is different? [Du, Changbin] > > > > raw->prev_ev = ev; > > mutex_unlock(&ir_raw_handler_lock); > > } > > -- > > 1.7.9.5 > > > > -- > > 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 -- 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
On Tue, Sep 04, 2012 at 11:06:07AM +0800, Changbin Du wrote: > > > mutex_lock(&ir_raw_handler_lock); > > > - list_for_each_entry(handler, &ir_raw_handler_list, list) > > > - handler->decode(raw->dev, ev); > > > + list_for_each_entry(handler, &ir_raw_handler_list, list) { > > > + /* use all protocol by default */ > > > + if (raw->dev->allowed_protos == RC_TYPE_UNKNOWN || > > > + raw->dev->allowed_protos & handler->protocols) > > > + handler->decode(raw->dev, ev); > > > + } > > > > Each IR protocol decoder already checks whether it is enabled or not; > > should it not be so that only allowed protocols can be enabled rather > > than checking both enabled_protocols and allowed_protocols? > > > > Just from reading store_protocols it looks like decoders which aren't > > in allowed_protocols can be enabled, which makes no sense. Also > > ir_raw_event_register all protocols are enabled rather than the > > allowed ones. > > > > > > Lastely I don't know why raw ir drivers should dictate which protocols > > can be enabled. Would it not be better to remove it entirely? > > > I agree with you. I just thought that the only thing a decoder should care > is its decoding logic, but not including decoder management. My idaea is: > 1) use enabled_protocols to select decoders in ir_raw.c, but not > placed in decoders to do the judgement. > 2) remove allowed_protocols or just use it to set the default > decoder (also should rename allowed_protocols to default_protocol). The default decoder should be the one set by the rc keymap. > I also have a question: > Is there a requirement that one more decoders are enabled for a > IR device at the same time? Yes, you want to be able to multiple remotes on the IR device (which you can do as long as the scancodes don't overlap, I think), and the lirc device is implemented as a decoder, so you might want to see the raw IR as well as have it decoded. > And if that will lead to a issue that each decoder may decode a > same pulse sequence to different evnets since their protocol is > different? At the moment, no. David Hardeman has sent a patch for this: http://patchwork.linuxtv.org/patch/11388/ Sean -- 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
Sean , many thanks for your help. I know much more about IR framwork now. I'll try to work out a patch to remove "allowed_protocols". Thanks again! [Du, Changbin] 2012/9/4 Sean Young <sean@mess.org>: > On Tue, Sep 04, 2012 at 11:06:07AM +0800, Changbin Du wrote: >> > > mutex_lock(&ir_raw_handler_lock); >> > > - list_for_each_entry(handler, &ir_raw_handler_list, list) >> > > - handler->decode(raw->dev, ev); >> > > + list_for_each_entry(handler, &ir_raw_handler_list, list) { >> > > + /* use all protocol by default */ >> > > + if (raw->dev->allowed_protos == RC_TYPE_UNKNOWN || >> > > + raw->dev->allowed_protos & handler->protocols) >> > > + handler->decode(raw->dev, ev); >> > > + } >> > >> > Each IR protocol decoder already checks whether it is enabled or not; >> > should it not be so that only allowed protocols can be enabled rather >> > than checking both enabled_protocols and allowed_protocols? >> > >> > Just from reading store_protocols it looks like decoders which aren't >> > in allowed_protocols can be enabled, which makes no sense. Also >> > ir_raw_event_register all protocols are enabled rather than the >> > allowed ones. >> > >> > >> > Lastely I don't know why raw ir drivers should dictate which protocols >> > can be enabled. Would it not be better to remove it entirely? >> >> >> I agree with you. I just thought that the only thing a decoder should care >> is its decoding logic, but not including decoder management. My idaea is: >> 1) use enabled_protocols to select decoders in ir_raw.c, but not >> placed in decoders to do the judgement. >> 2) remove allowed_protocols or just use it to set the default >> decoder (also should rename allowed_protocols to default_protocol). > > The default decoder should be the one set by the rc keymap. > >> I also have a question: >> Is there a requirement that one more decoders are enabled for a >> IR device at the same time? > > Yes, you want to be able to multiple remotes on the IR device (which > you can do as long as the scancodes don't overlap, I think), and the > lirc device is implemented as a decoder, so you might want to see the > raw IR as well as have it decoded. > >> And if that will lead to a issue that each decoder may decode a >> same pulse sequence to different evnets since their protocol is >> different? > > At the moment, no. David Hardeman has sent a patch for this: > > http://patchwork.linuxtv.org/patch/11388/ > > > Sean -- 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 --git a/drivers/media/rc/ir-raw.c b/drivers/media/rc/ir-raw.c index a820251..198b6d8 100644 --- a/drivers/media/rc/ir-raw.c +++ b/drivers/media/rc/ir-raw.c @@ -63,8 +63,12 @@ static int ir_raw_event_thread(void *data) spin_unlock_irq(&raw->lock); mutex_lock(&ir_raw_handler_lock); - list_for_each_entry(handler, &ir_raw_handler_list, list) - handler->decode(raw->dev, ev); + list_for_each_entry(handler, &ir_raw_handler_list, list) { + /* use all protocol by default */ + if (raw->dev->allowed_protos == RC_TYPE_UNKNOWN || + raw->dev->allowed_protos & handler->protocols) + handler->decode(raw->dev, ev); + } raw->prev_ev = ev; mutex_unlock(&ir_raw_handler_lock); }