| Message ID | 7983411.lVWEDlBWc6@radagast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 27, 2014 at 11:21:23PM +0000, James Hogan wrote: >Hi David, > >On Thursday 27 March 2014 22:00:37 David Härdeman wrote: >> This reverts 18bc17448147e93f31cc9b1a83be49f1224657b2 >> >> The patch ignores the fact that NEC32 scancodes are generated not only in >> the NEC raw decoder but also directly in some drivers. Whichever approach >> is chosen it should be consistent across drivers and this patch needs more >> discussion. > >Fair enough. For reference which drivers are you referring to? The ones I'm aware of right now are: drivers/media/usb/dvb-usb/dib0700_core.c drivers/media/usb/dvb-usb-v2/az6007.c drivers/media/usb/dvb-usb-v2/af9035.c drivers/media/usb/dvb-usb-v2/rtl28xxu.c drivers/media/usb/dvb-usb-v2/af9015.c drivers/media/usb/em28xx/em28xx-input.c >> Furthermore, I'm convinced that we have to stop playing games trying to >> decipher the "meaning" of NEC scancodes (what's the customer/vendor/address, >> which byte is the MSB, etc). > >Well when all the buttons on a remote have the same address, and the numeric >buttons are sequential commands only in a certain bit/byte order, then I think >the word "decipher" is probably a bit of a stretch. I think you misunderstood me. "decipher" is a bit of a stretch when talking of one remote control (I'm guessing you're referring to the Tivo remote). It's not that much of a stretch if we're referring to trying to derive a common meaning from the encoding used for *all* remote controls out there. The discussion about the 24-bit version of NEC and whether the address bytes were in MSB or LSB order was a good example. Andy Walls cited a NEC manual which stated one thing and people also referred to http://www.sbprojects.com/knowledge/ir/nec.php which stated the opposite (while referring to an unnamed VCR service manual). As a third example...I've read a Samsung service manual which happily stated that the remote (which used the NEC protocol) sent IR commands starting with the address x 2 (and looking at the raw NEC command, it did start with something like 0x07 0x07). So don't get me wrong, I wasn't referring to your analysis of the Tivo remote but more the general approach that has been taken until now wrt. the NEC protocol in the kernel drivers. >Nevertheless I don't have any attachment to 32-bit NEC. If it's likely to >change again I'd prefer img-ir-nec just not support it for now, so please >could you add the following hunks to your patch (or if the original patch is >to be dropped this could be squashed into the img-ir-nec patch): I'd rather show you my complete proposal first before doing something radical with your driver. But it was a good reminder that I need to keep the NEC32 parsing in your driver in mind as well. >> I'll post separate proposals to that effect later. > >Great, please do Cc me > >(I have a work in progress branch to unify NEC scancodes, but I'm not sure >I'd have time to complete it any time soon anyway) That is what I'm working on as well at the moment. It's actually to solve two problems...both to unify NEC scancodes (by simply using 32 bit scancodes everywhere and some fallback code...I'm not 100% sure it's doable but I hope so since it's the only sane solution I can think of in the long run)...and to make sure that protocol information actually gets used in keymaps, etc. I hope to post patches soon that'll make it clearer. Regards, David -- 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 Friday 28 March 2014 01:08:56 David Härdeman wrote: > On Thu, Mar 27, 2014 at 11:21:23PM +0000, James Hogan wrote: > >Hi David, > > > >On Thursday 27 March 2014 22:00:37 David Härdeman wrote: > >> This reverts 18bc17448147e93f31cc9b1a83be49f1224657b2 > >> > >> The patch ignores the fact that NEC32 scancodes are generated not only in > >> the NEC raw decoder but also directly in some drivers. Whichever approach > >> is chosen it should be consistent across drivers and this patch needs > >> more > >> discussion. > > > >Fair enough. For reference which drivers are you referring to? > > The ones I'm aware of right now are: Thanks, I hadn't looked properly outside of drivers/media/rc/ :( > drivers/media/usb/dvb-usb/dib0700_core.c AFAICT this only seems to support 16bit and 24bit NEC, so NEC-32 doesn't affect it. I may have missed something subtle. > drivers/media/usb/dvb-usb-v2/az6007.c > drivers/media/usb/dvb-usb-v2/af9035.c > drivers/media/usb/dvb-usb-v2/rtl28xxu.c > drivers/media/usb/dvb-usb-v2/af9015.c > drivers/media/usb/em28xx/em28xx-input.c Note, it appears none of these do any bit reversing for the 32bit case compared to 16/24 bit, so they're already different to the NEC32 scancode encoding that the raw nec decoder and tivo keymap were using, which used a different bitorder (!!) between the 32-bit and the 24/16-bit cases. > > >> Furthermore, I'm convinced that we have to stop playing games trying to > >> decipher the "meaning" of NEC scancodes (what's the > >> customer/vendor/address, which byte is the MSB, etc). > > > >Well when all the buttons on a remote have the same address, and the > >numeric buttons are sequential commands only in a certain bit/byte order, > >then I think the word "decipher" is probably a bit of a stretch. > > I think you misunderstood me. "decipher" is a bit of a stretch when > talking of one remote control (I'm guessing you're referring to the Tivo > remote). It's not that much of a stretch if we're referring to trying to > derive a common meaning from the encoding used for *all* remote controls > out there. > > The discussion about the 24-bit version of NEC and whether the address > bytes were in MSB or LSB order was a good example. Andy Walls cited a > NEC manual which stated one thing and people also referred to > http://www.sbprojects.com/knowledge/ir/nec.php which stated the opposite > (while referring to an unnamed VCR service manual). > > As a third example...I've read a Samsung service manual which happily > stated that the remote (which used the NEC protocol) sent IR commands > starting with the address x 2 (and looking at the raw NEC command, it > did start with something like 0x07 0x07). > > So don't get me wrong, I wasn't referring to your analysis of the Tivo > remote but more the general approach that has been taken until now wrt. > the NEC protocol in the kernel drivers. Okay, thanks for the clarification. > > >Nevertheless I don't have any attachment to 32-bit NEC. If it's likely to > >change again I'd prefer img-ir-nec just not support it for now, so please > >could you add the following hunks to your patch (or if the original patch > >is > >to be dropped this could be squashed into the img-ir-nec patch): > I'd rather show you my complete proposal first before doing something > radical with your driver. But it was a good reminder that I need to keep > the NEC32 parsing in your driver in mind as well. Okay no problem. I had assumed you were aiming for a short term fix to prevent the encoding change hitting mainline or an actual release (v3.15). Cheers James > > >> I'll post separate proposals to that effect later. > > > >Great, please do Cc me > > > >(I have a work in progress branch to unify NEC scancodes, but I'm not sure > >I'd have time to complete it any time soon anyway) > > That is what I'm working on as well at the moment. It's actually to > solve two problems...both to unify NEC scancodes (by simply using 32 bit > scancodes everywhere and some fallback code...I'm not 100% sure it's > doable but I hope so since it's the only sane solution I can think of in > the long run)...and to make sure that protocol information actually gets > used in keymaps, etc. > > I hope to post patches soon that'll make it clearer. > > Regards, > David > > -- > 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 Fri, Mar 28, 2014 at 11:17:09PM +0000, James Hogan wrote: >On Friday 28 March 2014 01:08:56 David Härdeman wrote: >> drivers/media/usb/dvb-usb-v2/az6007.c >> drivers/media/usb/dvb-usb-v2/af9035.c >> drivers/media/usb/dvb-usb-v2/rtl28xxu.c >> drivers/media/usb/dvb-usb-v2/af9015.c >> drivers/media/usb/em28xx/em28xx-input.c > >Note, it appears none of these do any bit reversing for the 32bit case >compared to 16/24 bit, so they're already different to the NEC32 scancode >encoding that the raw nec decoder and tivo keymap were using, which used a >different bitorder (!!) between the 32-bit and the 24/16-bit cases. I know, and none of those drivers have an in-kernel NEC32 keymap, so if anyone is using them in that manner...it's with a homebrew keymap. >> I'd rather show you my complete proposal first before doing something >> radical with your driver. But it was a good reminder that I need to keep >> the NEC32 parsing in your driver in mind as well. > >Okay no problem. I had assumed you were aiming for a short term fix to prevent >the encoding change hitting mainline or an actual release (v3.15). I am aiming for a fix within that time frame...but I hope that it can be more than a short term one :) Patches are on their way right now... Regards, David -- 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 Fri, Mar 28, 2014 at 11:17:09PM +0000, James Hogan wrote: >On Friday 28 March 2014 01:08:56 David Härdeman wrote: >> On Thu, Mar 27, 2014 at 11:21:23PM +0000, James Hogan wrote: >>>On Thursday 27 March 2014 22:00:37 David Härdeman wrote: >>>> This reverts 18bc17448147e93f31cc9b1a83be49f1224657b2 >>>> >>>> The patch ignores the fact that NEC32 scancodes are generated not only in >>>> the NEC raw decoder but also directly in some drivers. Whichever approach >>>> is chosen it should be consistent across drivers and this patch needs >>>> more >>>> discussion. >>> >>>Fair enough. For reference which drivers are you referring to? >> >> The ones I'm aware of right now are: > >Thanks, I hadn't looked properly outside of drivers/media/rc/ :( > >> drivers/media/usb/dvb-usb/dib0700_core.c > >AFAICT this only seems to support 16bit and 24bit NEC, so NEC-32 doesn't affect >it. I may have missed something subtle. Nah. You're right, it can be converted to NEC32 by simply removing one check, but it isn't NEC32 capable yet. >> drivers/media/usb/dvb-usb-v2/az6007.c >> drivers/media/usb/dvb-usb-v2/af9035.c >> drivers/media/usb/dvb-usb-v2/rtl28xxu.c >> drivers/media/usb/dvb-usb-v2/af9015.c >> drivers/media/usb/em28xx/em28xx-input.c > >Note, it appears none of these do any bit reversing for the 32bit case >compared to 16/24 bit, so they're already different to the NEC32 scancode >encoding that the raw nec decoder and tivo keymap were using, which used a >different bitorder (!!) between the 32-bit and the 24/16-bit cases. I think that might also be a reason to generate the NEC32 scancode in the order that I've proposed (i.e. it only requires change to the sw NEC decoder). On the other hand I'm still dithering on whether your proposed NEC32 scancode (which is what the sw decoder uses) or my proposal (which is what the other hw decoders use) should be canonical... :)
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c b/drivers/media/rc/img-ir/img-ir-nec.c index e7a731b..419d087 100644 --- a/drivers/media/rc/img-ir/img-ir-nec.c +++ b/drivers/media/rc/img-ir/img-ir-nec.c @@ -21,12 +21,7 @@ static int img_ir_nec_scancode(int len, u64 raw, int *scancode, u64 protocols) data = (raw >> 16) & 0xff; data_inv = (raw >> 24) & 0xff; if ((data_inv ^ data) != 0xff) { - /* 32-bit NEC (used by Apple and TiVo remotes) */ - /* scan encoding: aaAAddDD */ - *scancode = addr_inv << 24 | - addr << 16 | - data_inv << 8 | - data; + return -EINVAL; } else if ((addr_inv ^ addr) != 0xff) { /* Extended NEC */ /* scan encoding: AAaaDD */ @@ -53,14 +48,7 @@ static int img_ir_nec_filter(const struct rc_scancode_filter *in, data_m = in->mask & 0xff; if ((in->data | in->mask) & 0xff000000) { - /* 32-bit NEC (used by Apple and TiVo remotes) */ - /* scan encoding: aaAAddDD */ - addr_inv = (in->data >> 24) & 0xff; - addr_inv_m = (in->mask >> 24) & 0xff; - addr = (in->data >> 16) & 0xff; - addr_m = (in->mask >> 16) & 0xff; - data_inv = (in->data >> 8) & 0xff; - data_inv_m = (in->mask >> 8) & 0xff; + return -EINVAL; } else if ((in->data | in->mask) & 0x00ff0000) { /* Extended NEC */ /* scan encoding AAaaDD */