| Message ID | 20140329161136.13234.733.stgit@zeus.muc.hardeman.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/03/14 16:11, David Härdeman wrote: > Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core > and the nec decoder without any loss of functionality. > > In order to maintain backwards compatibility, some heuristics are added > in rc-main.c to convert scancodes to NEC32 as necessary. > > I plan to introduce a different ioctl later which makes the protocol > explicit (and which expects all NEC scancodes to be 32 bit, thereby > removing the need for guesswork). > > Signed-off-by: David Härdeman <david@hardeman.nu> > --- > diff --git a/drivers/media/rc/img-ir/img-ir-nec.c b/drivers/media/rc/img-ir/img-ir-nec.c > index 40ee844..133ea45 100644 > --- a/drivers/media/rc/img-ir/img-ir-nec.c > +++ b/drivers/media/rc/img-ir/img-ir-nec.c > @@ -5,42 +5,20 @@ > */ > > #include "img-ir-hw.h" > -#include <linux/bitrev.h> > > /* Convert NEC data to a scancode */ > static int img_ir_nec_scancode(int len, u64 raw, enum rc_type *protocol, > u32 *scancode, u64 enabled_protocols) > { > - unsigned int addr, addr_inv, data, data_inv; > /* a repeat code has no data */ > if (!len) > return IMG_IR_REPEATCODE; > + > if (len != 32) > return -EINVAL; > - /* raw encoding: ddDDaaAA */ > - addr = (raw >> 0) & 0xff; > - addr_inv = (raw >> 8) & 0xff; > - 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 (LSBit first) */ > - *scancode = bitrev8(addr) << 24 | > - bitrev8(addr_inv) << 16 | > - bitrev8(data) << 8 | > - bitrev8(data_inv); > - } else if ((addr_inv ^ addr) != 0xff) { > - /* Extended NEC */ > - /* scan encoding: AAaaDD */ > - *scancode = addr << 16 | > - addr_inv << 8 | > - data; > - } else { > - /* Normal NEC */ > - /* scan encoding: AADD */ > - *scancode = addr << 8 | > - data; > - } > + > + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ > + *scancode = swab32((u32)raw); What's the point of the byte swapping? Surely the most natural NEC encoding would just treat it as a single 32-bit (LSBit first) field rather than 4 8-bit fields that needs swapping. Cheers James
On 2014-03-31 11:44, James Hogan wrote: > On 29/03/14 16:11, David Härdeman wrote: >> Using the full 32 bits for all kinds of NEC scancodes simplifies >> rc-core >> and the nec decoder without any loss of functionality. >> >> In order to maintain backwards compatibility, some heuristics are >> added >> in rc-main.c to convert scancodes to NEC32 as necessary. >> >> I plan to introduce a different ioctl later which makes the protocol >> explicit (and which expects all NEC scancodes to be 32 bit, thereby >> removing the need for guesswork). >> >> Signed-off-by: David Härdeman <david@hardeman.nu> >> --- >> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c >> b/drivers/media/rc/img-ir/img-ir-nec.c >> index 40ee844..133ea45 100644 >> --- a/drivers/media/rc/img-ir/img-ir-nec.c >> +++ b/drivers/media/rc/img-ir/img-ir-nec.c >> @@ -5,42 +5,20 @@ >> */ >> >> #include "img-ir-hw.h" >> -#include <linux/bitrev.h> >> >> /* Convert NEC data to a scancode */ >> static int img_ir_nec_scancode(int len, u64 raw, enum rc_type >> *protocol, >> u32 *scancode, u64 enabled_protocols) >> { >> - unsigned int addr, addr_inv, data, data_inv; >> /* a repeat code has no data */ >> if (!len) >> return IMG_IR_REPEATCODE; >> + >> if (len != 32) >> return -EINVAL; >> - /* raw encoding: ddDDaaAA */ >> - addr = (raw >> 0) & 0xff; >> - addr_inv = (raw >> 8) & 0xff; >> - 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 (LSBit first) */ >> - *scancode = bitrev8(addr) << 24 | >> - bitrev8(addr_inv) << 16 | >> - bitrev8(data) << 8 | >> - bitrev8(data_inv); >> - } else if ((addr_inv ^ addr) != 0xff) { >> - /* Extended NEC */ >> - /* scan encoding: AAaaDD */ >> - *scancode = addr << 16 | >> - addr_inv << 8 | >> - data; >> - } else { >> - /* Normal NEC */ >> - /* scan encoding: AADD */ >> - *scancode = addr << 8 | >> - data; >> - } >> + >> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ >> + *scancode = swab32((u32)raw); > > What's the point of the byte swapping? > > Surely the most natural NEC encoding would just treat it as a single > 32-bit (LSBit first) field rather than 4 8-bit fields that needs > swapping. Thanks for having a look at the patches, I agree with your comments on the other patches (and I have to respin some of them because I missed two drivers), but the comments to this patch confuses me a bit. That the NEC data is transmitted as 32 bits encoded with LSB bit order within each byte is AFAIK just about the only thing that all sources/documentation of the protocal can agree on (so bitrev:ing the bits within each byte makes sense, unless the hardware has done it already). As for the byte order, AAaaDDdd corresponds to the transmission order and seems to be what most drivers expect/use for their RX data. Are you suggesting that rc-core should standardize on ddDDaaAA order? -- 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 31/03/14 11:19, David Härdeman wrote: > On 2014-03-31 11:44, James Hogan wrote: >> On 29/03/14 16:11, David Härdeman wrote: >>> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core >>> and the nec decoder without any loss of functionality. >>> >>> In order to maintain backwards compatibility, some heuristics are added >>> in rc-main.c to convert scancodes to NEC32 as necessary. >>> >>> I plan to introduce a different ioctl later which makes the protocol >>> explicit (and which expects all NEC scancodes to be 32 bit, thereby >>> removing the need for guesswork). >>> >>> Signed-off-by: David Härdeman <david@hardeman.nu> >>> --- >>> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c >>> b/drivers/media/rc/img-ir/img-ir-nec.c >>> index 40ee844..133ea45 100644 >>> --- a/drivers/media/rc/img-ir/img-ir-nec.c >>> +++ b/drivers/media/rc/img-ir/img-ir-nec.c >>> @@ -5,42 +5,20 @@ >>> */ >>> >>> #include "img-ir-hw.h" >>> -#include <linux/bitrev.h> >>> >>> /* Convert NEC data to a scancode */ >>> static int img_ir_nec_scancode(int len, u64 raw, enum rc_type >>> *protocol, >>> u32 *scancode, u64 enabled_protocols) >>> { >>> - unsigned int addr, addr_inv, data, data_inv; >>> /* a repeat code has no data */ >>> if (!len) >>> return IMG_IR_REPEATCODE; >>> + >>> if (len != 32) >>> return -EINVAL; >>> - /* raw encoding: ddDDaaAA */ >>> - addr = (raw >> 0) & 0xff; >>> - addr_inv = (raw >> 8) & 0xff; >>> - 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 (LSBit first) */ >>> - *scancode = bitrev8(addr) << 24 | >>> - bitrev8(addr_inv) << 16 | >>> - bitrev8(data) << 8 | >>> - bitrev8(data_inv); >>> - } else if ((addr_inv ^ addr) != 0xff) { >>> - /* Extended NEC */ >>> - /* scan encoding: AAaaDD */ >>> - *scancode = addr << 16 | >>> - addr_inv << 8 | >>> - data; >>> - } else { >>> - /* Normal NEC */ >>> - /* scan encoding: AADD */ >>> - *scancode = addr << 8 | >>> - data; >>> - } >>> + >>> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ >>> + *scancode = swab32((u32)raw); >> >> What's the point of the byte swapping? >> >> Surely the most natural NEC encoding would just treat it as a single >> 32-bit (LSBit first) field rather than 4 8-bit fields that needs >> swapping. > > Thanks for having a look at the patches, I agree with your comments on > the other patches (and I have to respin some of them because I missed > two drivers), but the comments to this patch confuses me a bit. > > That the NEC data is transmitted as 32 bits encoded with LSB bit order > within each byte is AFAIK just about the only thing that all > sources/documentation of the protocal can agree on (so bitrev:ing the > bits within each byte makes sense, unless the hardware has done it > already). Agreed (in the case of img-ir there's a bit orientation setting which ensures that the u64 raw has the correct bit order, in the case of NEC the first bit received goes in the lowest order bit of the raw data). > As for the byte order, AAaaDDdd corresponds to the transmission order > and seems to be what most drivers expect/use for their RX data. AAaaDDdd is big endian rendering, no? (like "%08x") If it should be interpreted as LSBit first, then the first bits received should go in the low bits of the scancode, and by extension the first bytes received in the low bytes of the scancode, i.e. at the end of the inherently big-endian hexadecimal rendering of the scancode. > Are you suggesting that rc-core should standardize on ddDDaaAA order? Yes (where ddDDaaAA means something like scancode "0x(~cmd)(cmd)(~addr)(addr)") This would mean that if the data is put in the right bit order (first bit received in BIT(0), last bit received in BIT(31)), then the scancode = raw, and if the data is received in the reverse bit order (like the raw decoder, shifting the data left and inserting the last bit in BIT(0)) then the scancode = bitrev32(raw). Have I missed something? Cheers James
Hi David, Em Mon, 31 Mar 2014 12:19:10 +0200 David Härdeman <david@hardeman.nu> escreveu: > On 2014-03-31 11:44, James Hogan wrote: > > On 29/03/14 16:11, David Härdeman wrote: > >> Using the full 32 bits for all kinds of NEC scancodes simplifies > >> rc-core > >> and the nec decoder without any loss of functionality. > >> > >> In order to maintain backwards compatibility, some heuristics are > >> added > >> in rc-main.c to convert scancodes to NEC32 as necessary. > >> > >> I plan to introduce a different ioctl later which makes the protocol > >> explicit (and which expects all NEC scancodes to be 32 bit, thereby > >> removing the need for guesswork). > >> > >> Signed-off-by: David Härdeman <david@hardeman.nu> > >> --- > >> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c > >> b/drivers/media/rc/img-ir/img-ir-nec.c > >> index 40ee844..133ea45 100644 > >> --- a/drivers/media/rc/img-ir/img-ir-nec.c > >> +++ b/drivers/media/rc/img-ir/img-ir-nec.c > >> @@ -5,42 +5,20 @@ > >> */ > >> > >> #include "img-ir-hw.h" > >> -#include <linux/bitrev.h> > >> > >> /* Convert NEC data to a scancode */ > >> static int img_ir_nec_scancode(int len, u64 raw, enum rc_type > >> *protocol, > >> u32 *scancode, u64 enabled_protocols) > >> { > >> - unsigned int addr, addr_inv, data, data_inv; > >> /* a repeat code has no data */ > >> if (!len) > >> return IMG_IR_REPEATCODE; > >> + > >> if (len != 32) > >> return -EINVAL; > >> - /* raw encoding: ddDDaaAA */ > >> - addr = (raw >> 0) & 0xff; > >> - addr_inv = (raw >> 8) & 0xff; > >> - 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 (LSBit first) */ > >> - *scancode = bitrev8(addr) << 24 | > >> - bitrev8(addr_inv) << 16 | > >> - bitrev8(data) << 8 | > >> - bitrev8(data_inv); > >> - } else if ((addr_inv ^ addr) != 0xff) { > >> - /* Extended NEC */ > >> - /* scan encoding: AAaaDD */ > >> - *scancode = addr << 16 | > >> - addr_inv << 8 | > >> - data; > >> - } else { > >> - /* Normal NEC */ > >> - /* scan encoding: AADD */ > >> - *scancode = addr << 8 | > >> - data; > >> - } > >> + > >> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ > >> + *scancode = swab32((u32)raw); > > > > What's the point of the byte swapping? > > > > Surely the most natural NEC encoding would just treat it as a single > > 32-bit (LSBit first) field rather than 4 8-bit fields that needs > > swapping. > > Thanks for having a look at the patches, I agree with your comments on > the other patches (and I have to respin some of them because I missed > two drivers), but the comments to this patch confuses me a bit. > > That the NEC data is transmitted as 32 bits encoded with LSB bit order > within each byte is AFAIK just about the only thing that all > sources/documentation of the protocal can agree on (so bitrev:ing the > bits within each byte makes sense, unless the hardware has done it > already). > > As for the byte order, AAaaDDdd corresponds to the transmission order > and seems to be what most drivers expect/use for their RX data. > > Are you suggesting that rc-core should standardize on ddDDaaAA order? Let's better name this, as AAaaDDdd implies that: aa = ~AA dd = ~DD As described at the NEC protocol. The 24 or 32 bits variation is actually a violation of the NEC protocol. What some IRs actually provide is: xxyyADDdd (24 bits NEC) where: Address = yyxx Data = DD As described as "Extended NEC protocol" at: http://www.sbprojects.com/knowledge/ir/nec.php or: xxyyADDzz (32 bits NEC) where: Address = zzxxyy Data = DD Also, currently, there's just one IR table with 32 bits nec: rc-tivo.c, used by the mceusb driver. Well, changing the NEC decoders to always send a 32 bits code has several issues: 1) It makes the normal NEC protocol as an exception, and not as a rule; 2) It breaks all in-kernel tables for 16 bits and 24 bits NEC. As already said, currently, there's just one driver using 32 bits NEC, and just for one IR type (RC_MAP_TIVO); 3) It causes regressions to userspace, as userspace tables won't work anymore; 4) Your to_nec32() macro will break support for 24-bits IRs shipped with devices that can only provide 16 bits. In order to explain (4), let's see what happens when a 24-bits NEC code is received by a in-hardware decoder. There are a wide range of Chinese IR devices shipped with widely used media hardware that produce a 24-bit NEC code. One of the most popular of such manufacturers use the address = 0x866b (btw, the get_key_beholdm6xx() function at saa7134 driver seems to be wrong, as the keytables for behold device has the address of this vendor mapped as 0x6b86). The way those codes are handled inside each in-hardware NEC decoder are different. I've seen all those alternatives: a) the full 24-bits code is received by the driver; b) some hardware will simply discard the MSB of the address; c) a few hardware will discard the entire keycode, as the checksum bytes won't match. The devices from the 0x866b manufacturer is used by a wide range of devices that can do either (a) or (b). Well, as the to_nec32() doesn't know the original keycode, it would map an address like 0x866b as 0x946b, with is wrong, and won't match the corresponding NEC table. Due to (3) (it causes userspace regressions), we can't apply such changes. > > -- > 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 2014-03-31 14:14, Mauro Carvalho Chehab wrote: > Em Mon, 31 Mar 2014 12:19:10 +0200 > David Härdeman <david@hardeman.nu> escreveu: >> On 2014-03-31 11:44, James Hogan wrote: >> > On 29/03/14 16:11, David Härdeman wrote: >> >> Using the full 32 bits for all kinds of NEC scancodes simplifies >> >> rc-core >> >> and the nec decoder without any loss of functionality. >> >> >> >> In order to maintain backwards compatibility, some heuristics are >> >> added >> >> in rc-main.c to convert scancodes to NEC32 as necessary. >> >> >> >> I plan to introduce a different ioctl later which makes the protocol >> >> explicit (and which expects all NEC scancodes to be 32 bit, thereby >> >> removing the need for guesswork). >> >> >> >> Signed-off-by: David Härdeman <david@hardeman.nu> >> >> --- >> >> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c >> >> b/drivers/media/rc/img-ir/img-ir-nec.c >> >> index 40ee844..133ea45 100644 >> >> --- a/drivers/media/rc/img-ir/img-ir-nec.c >> >> +++ b/drivers/media/rc/img-ir/img-ir-nec.c >> >> @@ -5,42 +5,20 @@ >> >> */ >> >> >> >> #include "img-ir-hw.h" >> >> -#include <linux/bitrev.h> >> >> >> >> /* Convert NEC data to a scancode */ >> >> static int img_ir_nec_scancode(int len, u64 raw, enum rc_type >> >> *protocol, >> >> u32 *scancode, u64 enabled_protocols) >> >> { >> >> - unsigned int addr, addr_inv, data, data_inv; >> >> /* a repeat code has no data */ >> >> if (!len) >> >> return IMG_IR_REPEATCODE; >> >> + >> >> if (len != 32) >> >> return -EINVAL; >> >> - /* raw encoding: ddDDaaAA */ >> >> - addr = (raw >> 0) & 0xff; >> >> - addr_inv = (raw >> 8) & 0xff; >> >> - 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 (LSBit first) */ >> >> - *scancode = bitrev8(addr) << 24 | >> >> - bitrev8(addr_inv) << 16 | >> >> - bitrev8(data) << 8 | >> >> - bitrev8(data_inv); >> >> - } else if ((addr_inv ^ addr) != 0xff) { >> >> - /* Extended NEC */ >> >> - /* scan encoding: AAaaDD */ >> >> - *scancode = addr << 16 | >> >> - addr_inv << 8 | >> >> - data; >> >> - } else { >> >> - /* Normal NEC */ >> >> - /* scan encoding: AADD */ >> >> - *scancode = addr << 8 | >> >> - data; >> >> - } >> >> + >> >> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ >> >> + *scancode = swab32((u32)raw); >> > >> > What's the point of the byte swapping? >> > >> > Surely the most natural NEC encoding would just treat it as a single >> > 32-bit (LSBit first) field rather than 4 8-bit fields that needs >> > swapping. >> >> Thanks for having a look at the patches, I agree with your comments on >> the other patches (and I have to respin some of them because I missed >> two drivers), but the comments to this patch confuses me a bit. >> >> That the NEC data is transmitted as 32 bits encoded with LSB bit order >> within each byte is AFAIK just about the only thing that all >> sources/documentation of the protocal can agree on (so bitrev:ing the >> bits within each byte makes sense, unless the hardware has done it >> already). >> >> As for the byte order, AAaaDDdd corresponds to the transmission order >> and seems to be what most drivers expect/use for their RX data. >> >> Are you suggesting that rc-core should standardize on ddDDaaAA order? > > > Let's better name this, as AAaaDDdd implies that: > aa = ~AA > dd = ~DD > As described at the NEC protocol. I really don't think James and I had any trouble understanding each other :) > The 24 or 32 bits variation is actually a violation of the NEC > protocol. Violation is a misnomer. NEC created the 24 bit version, it's an extension. Many companies (such as your employer :)) have created further variations. > What some IRs actually provide is: > xxyyADDdd (24 bits NEC) > where: > Address = yyxx > Data = DD > > As described as "Extended NEC protocol" at: > http://www.sbprojects.com/knowledge/ir/nec.php > > or: > xxyyADDzz (32 bits NEC) > where: > Address = zzxxyy > Data = DD No need to explain the protocol to me. > Also, currently, there's just one IR table with 32 bits nec: > rc-tivo.c, used by the mceusb driver. Yes, I know. > Well, changing the NEC decoders to always send a 32 bits code has > several issues: > > 1) It makes the normal NEC protocol as an exception, and not as a > rule; It's not an exception. I just makes all 32 bits explicit. And the lack of that explicit information currently makes the scancode ambiguous. Right now if I give you a NEC scancode of 0xff00 (like we give to userspace with the EV_SCAN event), you can't tell what it means...it could, for example, be a 32 bit code of 0x0000ff00... > 2) It breaks all in-kernel tables for 16 bits and 24 bits NEC. > As already said, currently, there's just one driver using 32 > bits NEC, and just for one IR type (RC_MAP_TIVO); No, the proposed patch doesn't break all in-kernel tables. The in-kernel tables are converted on the fly to NEC32 when loaded. > 3) It causes regressions to userspace, as userspace tables won't > work anymore; I know it may cause troubles for userspace, however: a) You've already accepted patches that change the scancode format of the NEC decoder within the last few weeks so you've already set the stage for the same kind of trouble (even if I agree with James on parts of that patch) b) The current code is broken as well...using the same remote will generate different scancodes depending on the driver (even if the old and new hardware *can* receive the full scancode), meaning that your keytable will suddenly stop working if you change HW. That's bad. > 4) Your to_nec32() macro will break support for 24-bits IRs > shipped with devices that can only provide 16 bits. > > In order to explain (4), let's see what happens when a 24-bits > NEC code is received by a in-hardware decoder. > > There are a wide range of Chinese IR devices shipped with widely > used media hardware that produce a 24-bit NEC code. One of the > most popular of such manufacturers use the address = 0x866b > (btw, the get_key_beholdm6xx() function at saa7134 driver seems > to be wrong, as the keytables for behold device has the address of > this vendor mapped as 0x6b86). I know, I've already identified and fixed that problem in a separate patch that's posted to the list. And it will also break out-of-kernel user-defined keymaps. Any inconsistency is a no-win situation. And we *do* have inconsistencies right now. > The way those codes are handled inside each in-hardware NEC > decoder are different. I've seen all those alternatives: > > a) the full 24-bits code is received by the driver; > b) some hardware will simply discard the MSB of the address; > c) a few hardware will discard the entire keycode, as the > checksum bytes won't match. I know there's a lot of variety, another example is drivers that discard (possibly after matching address) everything but the "command" part of the scancode. That should not be used as an excuse not to try to make the behavior as consistent as possible. After all...that's the point of a common API. > The devices from the 0x866b manufacturer is used by a wide range > of devices that can do either (a) or (b). > > Well, as the to_nec32() doesn't know the original keycode, it > would map an address like 0x866b as 0x946b, with is wrong, and > won't match the corresponding NEC table. Yes, if the hardware throws away information, rc-core will sometime generate a scancode which does not match the real one. As you say: if the actual remote control transmits: 0x866b01fe and the hardware truncates it to: 0x..6b01fe then rc-core would convert back to: 0x946b01fe And that could be fixed with a scanmask for that driver (0xffffff)? (We could also expose the scanmask to userspace so it knows which part of the scancode it can trust...) > Due to (3) (it causes userspace regressions), we can't apply > such changes. I know Linus' policy with regard to userspace regressions, but see above. 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
Em Mon, 31 Mar 2014 14:58:10 +0200 David Härdeman <david@hardeman.nu> escreveu: > On 2014-03-31 14:14, Mauro Carvalho Chehab wrote: > > Em Mon, 31 Mar 2014 12:19:10 +0200 > > David Härdeman <david@hardeman.nu> escreveu: > >> On 2014-03-31 11:44, James Hogan wrote: > >> > On 29/03/14 16:11, David Härdeman wrote: > >> >> Using the full 32 bits for all kinds of NEC scancodes simplifies > >> >> rc-core > >> >> and the nec decoder without any loss of functionality. > >> >> > >> >> In order to maintain backwards compatibility, some heuristics are > >> >> added > >> >> in rc-main.c to convert scancodes to NEC32 as necessary. > >> >> > >> >> I plan to introduce a different ioctl later which makes the protocol > >> >> explicit (and which expects all NEC scancodes to be 32 bit, thereby > >> >> removing the need for guesswork). > >> >> > >> >> Signed-off-by: David Härdeman <david@hardeman.nu> > >> >> --- > >> >> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c > >> >> b/drivers/media/rc/img-ir/img-ir-nec.c > >> >> index 40ee844..133ea45 100644 > >> >> --- a/drivers/media/rc/img-ir/img-ir-nec.c > >> >> +++ b/drivers/media/rc/img-ir/img-ir-nec.c > >> >> @@ -5,42 +5,20 @@ > >> >> */ > >> >> > >> >> #include "img-ir-hw.h" > >> >> -#include <linux/bitrev.h> > >> >> > >> >> /* Convert NEC data to a scancode */ > >> >> static int img_ir_nec_scancode(int len, u64 raw, enum rc_type > >> >> *protocol, > >> >> u32 *scancode, u64 enabled_protocols) > >> >> { > >> >> - unsigned int addr, addr_inv, data, data_inv; > >> >> /* a repeat code has no data */ > >> >> if (!len) > >> >> return IMG_IR_REPEATCODE; > >> >> + > >> >> if (len != 32) > >> >> return -EINVAL; > >> >> - /* raw encoding: ddDDaaAA */ > >> >> - addr = (raw >> 0) & 0xff; > >> >> - addr_inv = (raw >> 8) & 0xff; > >> >> - 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 (LSBit first) */ > >> >> - *scancode = bitrev8(addr) << 24 | > >> >> - bitrev8(addr_inv) << 16 | > >> >> - bitrev8(data) << 8 | > >> >> - bitrev8(data_inv); > >> >> - } else if ((addr_inv ^ addr) != 0xff) { > >> >> - /* Extended NEC */ > >> >> - /* scan encoding: AAaaDD */ > >> >> - *scancode = addr << 16 | > >> >> - addr_inv << 8 | > >> >> - data; > >> >> - } else { > >> >> - /* Normal NEC */ > >> >> - /* scan encoding: AADD */ > >> >> - *scancode = addr << 8 | > >> >> - data; > >> >> - } > >> >> + > >> >> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ > >> >> + *scancode = swab32((u32)raw); > >> > > >> > What's the point of the byte swapping? > >> > > >> > Surely the most natural NEC encoding would just treat it as a single > >> > 32-bit (LSBit first) field rather than 4 8-bit fields that needs > >> > swapping. > >> > >> Thanks for having a look at the patches, I agree with your comments on > >> the other patches (and I have to respin some of them because I missed > >> two drivers), but the comments to this patch confuses me a bit. > >> > >> That the NEC data is transmitted as 32 bits encoded with LSB bit order > >> within each byte is AFAIK just about the only thing that all > >> sources/documentation of the protocal can agree on (so bitrev:ing the > >> bits within each byte makes sense, unless the hardware has done it > >> already). > >> > >> As for the byte order, AAaaDDdd corresponds to the transmission order > >> and seems to be what most drivers expect/use for their RX data. > >> > >> Are you suggesting that rc-core should standardize on ddDDaaAA order? > > > > > > Let's better name this, as AAaaDDdd implies that: > > aa = ~AA > > dd = ~DD > > As described at the NEC protocol. > > I really don't think James and I had any trouble understanding each > other :) Ok, but others on reading this thread may misunderstand the meanings. > > > The 24 or 32 bits variation is actually a violation of the NEC > > protocol. > > Violation is a misnomer. NEC created the 24 bit version, it's an > extension. Many companies (such as your employer :)) have created > further variations. I'm fine if you call it as an extension, but the original NEC _is_ 16 bits, and most drivers are compliant with it. We should not break what's working. > > What some IRs actually provide is: > > xxyyADDdd (24 bits NEC) > > where: > > Address = yyxx > > Data = DD > > > > As described as "Extended NEC protocol" at: > > http://www.sbprojects.com/knowledge/ir/nec.php > > > > or: > > xxyyADDzz (32 bits NEC) > > where: > > Address = zzxxyy > > Data = DD > > No need to explain the protocol to me. > > > Also, currently, there's just one IR table with 32 bits nec: > > rc-tivo.c, used by the mceusb driver. > > Yes, I know. > > > Well, changing the NEC decoders to always send a 32 bits code has > > several issues: > > > > 1) It makes the normal NEC protocol as an exception, and not as a > > rule; > > It's not an exception. I just makes all 32 bits explicit. Well, if all drivers but one only have 16 or 24 bits tables, this is an exception. > And the lack of that explicit information currently makes the scancode > ambiguous. Right now if I give you a NEC scancode of 0xff00 (like we > give to userspace with the EV_SCAN event), you can't tell what it > means...it could, for example, be a 32 bit code of 0x0000ff00... > > > 2) It breaks all in-kernel tables for 16 bits and 24 bits NEC. > > As already said, currently, there's just one driver using 32 > > bits NEC, and just for one IR type (RC_MAP_TIVO); > > No, the proposed patch doesn't break all in-kernel tables. The in-kernel > tables are converted on the fly to NEC32 when loaded. That's messy. We should either change everything in Kernelspace to 32 bits or keep as is. If such emulation is needed, it should be only for userspace tables. > > 3) It causes regressions to userspace, as userspace tables won't > > work anymore; > > I know it may cause troubles for userspace, however: > > a) You've already accepted patches that change the scancode format of > the NEC decoder within the last few weeks so you've already set the > stage for the same kind of trouble (even if I agree with James on parts > of that patch) If I let this pass, we should revert it before it reaches upstream. What patch caused regressions? > b) The current code is broken as well...using the same remote will > generate different scancodes depending on the driver (even if the old > and new hardware *can* receive the full scancode), meaning that your > keytable will suddenly stop working if you change HW. That's bad. On the devices I have here, it is not broken. Let's fix it where this is broken, and not use it as an excuse to break even more things. > > 4) Your to_nec32() macro will break support for 24-bits IRs > > shipped with devices that can only provide 16 bits. > > > > In order to explain (4), let's see what happens when a 24-bits > > NEC code is received by a in-hardware decoder. > > > > There are a wide range of Chinese IR devices shipped with widely > > used media hardware that produce a 24-bit NEC code. One of the > > most popular of such manufacturers use the address = 0x866b > > (btw, the get_key_beholdm6xx() function at saa7134 driver seems > > to be wrong, as the keytables for behold device has the address of > > this vendor mapped as 0x6b86). > > I know, I've already identified and fixed that problem in a separate > patch that's posted to the list. And it will also break out-of-kernel > user-defined keymaps. Any inconsistency is a no-win situation. And we > *do* have inconsistencies right now. Yes. That's one of the reasons why this was not fixed yet (and the other one is that I don't have any of such device in hands, in order to be sure that this is not another vendor that, by coincidence, has address 0x6b86). > > The way those codes are handled inside each in-hardware NEC > > decoder are different. I've seen all those alternatives: > > > > a) the full 24-bits code is received by the driver; > > b) some hardware will simply discard the MSB of the address; > > c) a few hardware will discard the entire keycode, as the > > checksum bytes won't match. > > I know there's a lot of variety, another example is drivers that discard > (possibly after matching address) everything but the "command" part of > the scancode. That should not be used as an excuse not to try to make > the behavior as consistent as possible. After all...that's the point of > a common API. It should be consistent, and it should be able to support the existing hardware. > > The devices from the 0x866b manufacturer is used by a wide range > > of devices that can do either (a) or (b). > > > > Well, as the to_nec32() doesn't know the original keycode, it > > would map an address like 0x866b as 0x946b, with is wrong, and > > won't match the corresponding NEC table. > > Yes, if the hardware throws away information, rc-core will sometime > generate a scancode which does not match the real one. > > As you say: > > if the actual remote control transmits: 0x866b01fe > and the hardware truncates it to: 0x..6b01fe > then rc-core would convert back to: 0x946b01fe > > And that could be fixed with a scanmask for that driver (0xffffff)? I think you're meaning 0x0000ffff, right? > (We could also expose the scanmask to userspace so it knows which part > of the scancode it can trust...) Yes, we could do it, but the current userspace should keep working. Eventually, that means to add some backward compat code there, in order to preserve the behavior with current tables. > > Due to (3) (it causes userspace regressions), we can't apply > > such changes. > > I know Linus' policy with regard to userspace regressions, but see > above. > > 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 2014-03-31 12:56, James Hogan wrote: > On 31/03/14 11:19, David Härdeman wrote: >> On 2014-03-31 11:44, James Hogan wrote: >>> On 29/03/14 16:11, David Härdeman wrote: >>>> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ >>>> + *scancode = swab32((u32)raw); >>> >>> What's the point of the byte swapping? >>> >>> Surely the most natural NEC encoding would just treat it as a single >>> 32-bit (LSBit first) field rather than 4 8-bit fields that needs >>> swapping. >> >> Thanks for having a look at the patches, I agree with your comments on >> the other patches (and I have to respin some of them because I missed >> two drivers), but the comments to this patch confuses me a bit. >> >> That the NEC data is transmitted as 32 bits encoded with LSB bit order >> within each byte is AFAIK just about the only thing that all >> sources/documentation of the protocal can agree on (so bitrev:ing the >> bits within each byte makes sense, unless the hardware has done it >> already). > > Agreed (in the case of img-ir there's a bit orientation setting which > ensures that the u64 raw has the correct bit order, in the case of NEC > the first bit received goes in the lowest order bit of the raw data). > >> As for the byte order, AAaaDDdd corresponds to the transmission order >> and seems to be what most drivers expect/use for their RX data. > > AAaaDDdd is big endian rendering, no? (like "%08x") Yeah, you could call it that. > If it should be interpreted as LSBit first, then the first bits > received > should go in the low bits of the scancode, and by extension the first > bytes received in the low bytes of the scancode, i.e. at the end of the > inherently big-endian hexadecimal rendering of the scancode. I'm not saying the whole scancode should be interpreted as one 32 bit LSBit integer, just that the endianness within each byte should be respected. >> Are you suggesting that rc-core should standardize on ddDDaaAA order? > > Yes (where ddDDaaAA means something like scancode > "0x(~cmd)(cmd)(~addr)(addr)") Yes, that's what I meant. > This would mean that if the data is put in the right bit order (first > bit received in BIT(0), last bit received in BIT(31)), then the > scancode > = raw, and if the data is received in the reverse bit order (like the > raw decoder, shifting the data left and inserting the last bit in > BIT(0)) then the scancode = bitrev32(raw). > > Have I missed something? I just think we have to agree to disagree :) For me, storing/presenting the scancode as 0xAAaaDDdd is "obviously" the clearest and least confusing interpretation. But I might have spent too long time using that notation in code and mentally to be able to find anything else intuitive :) 0xAAaaDDdd means that you read/parse/print it left to right, just as you would if you drew a pulse-space chart showing the received IR pulse (time normally progresses to the right...modulo the per-byte bitrev). It kind of matches the other protocol scancodes as well (the "address" bits high, cmd bits low, the high bits tend to remain constant for one given remote, the low bits change, although it's not a hard rule) and it matches most software I've ever seen (AFAIK, LIRC represents NEC32 scancodes this way, as does e.g. the Pronto software and protocol). That said...I think we at least agree that we need *a* representation and that it should be used consistently in all drivers, right? //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 2014-03-31 15:15, Mauro Carvalho Chehab wrote: > Em Mon, 31 Mar 2014 14:58:10 +0200 > David Härdeman <david@hardeman.nu> escreveu: >> On 2014-03-31 14:14, Mauro Carvalho Chehab wrote: >>> The 24 or 32 bits variation is actually a violation of the NEC >>> protocol. >> >> Violation is a misnomer. NEC created the 24 bit version, it's an >> extension. Many companies (such as your employer :)) have created >> further variations. > > I'm fine if you call it as an extension, but the original NEC _is_ > 16 bits, and most drivers are compliant with it. > > We should not break what's working. You're misrepresenting the proposed changes now. I'm trying to fixup the scancode handling in the best way possible, I'm not willfully breaking anything. Some things are inconsistent right now between the drivers, in such a situation when driver A says "first X then Y" and driver B says "first Y then X" the situation is: a) already "broken"; and b) can't be fixed without introducing "breakage" of a different kind to either A or B >>> Well, changing the NEC decoders to always send a 32 bits code has >>> several issues: >>> >>> 1) It makes the normal NEC protocol as an exception, and not as a >>> rule; >> >> It's not an exception. I just makes all 32 bits explicit. > > Well, if all drivers but one only have 16 or 24 bits tables, this is > an exception. Not really. 32 bits are transmitted no matter what you call the protocol. I'm proposing storing those 32 bits in the scancode<->keycode table. Not what I'd call an exception (this particular point starts to feel a bit off-topic though so I think we can drop it). >> And the lack of that explicit information currently makes the scancode >> ambiguous. Right now if I give you a NEC scancode of 0xff00 (like we >> give to userspace with the EV_SCAN event), you can't tell what it >> means...it could, for example, be a 32 bit code of 0x0000ff00... You didn't answer this part. It's actually one of the biggest reasons for introducing the full scancode everywhere. >> > 2) It breaks all in-kernel tables for 16 bits and 24 bits NEC. >> > As already said, currently, there's just one driver using 32 >> > bits NEC, and just for one IR type (RC_MAP_TIVO); >> >> No, the proposed patch doesn't break all in-kernel tables. The >> in-kernel >> tables are converted on the fly to NEC32 when loaded. > > That's messy. We should either change everything in Kernelspace to > 32 bits or keep as is. No problem, I could respin the patch to also patch the keytables (which is what I did first), but I'll wait until we've agreed on something). > If such emulation is needed, it should be only for userspace tables. > >> > 3) It causes regressions to userspace, as userspace tables won't >> > work anymore; >> >> I know it may cause troubles for userspace, however: >> >> a) You've already accepted patches that change the scancode format of >> the NEC decoder within the last few weeks so you've already set the >> stage for the same kind of trouble (even if I agree with James on >> parts >> of that patch) > > If I let this pass, we should revert it before it reaches upstream. > > What patch caused regressions? 18bc17448147e93f31cc9b1a83be49f1224657b2, since it changes the scancode it'll break userspace keytables, it's mentioned in patch 4/11 in my patchset. >> b) The current code is broken as well...using the same remote will >> generate different scancodes depending on the driver (even if the old >> and new hardware *can* receive the full scancode), meaning that your >> keytable will suddenly stop working if you change HW. That's bad. > > On the devices I have here, it is not broken. Let's fix it where this > is broken, and not use it as an excuse to break even more things. Whether the hardware you happen to have agrees is beside the point? >>> (btw, the get_key_beholdm6xx() function at saa7134 driver seems >>> to be wrong, as the keytables for behold device has the address of >>> this vendor mapped as 0x6b86). >> >> I know, I've already identified and fixed that problem in a separate >> patch that's posted to the list. And it will also break out-of-kernel >> user-defined keymaps. Any inconsistency is a no-win situation. And we >> *do* have inconsistencies right now. > > Yes. That's one of the reasons why this was not fixed yet (and the > other > one is that I don't have any of such device in hands, in order to be > sure that this is not another vendor that, by coincidence, has address > 0x6b86). I know we can't be 100% sure, but the byte order in the driver itself also supports the notion that the address bytes have been reversed. >>> The way those codes are handled inside each in-hardware NEC >>> decoder are different. I've seen all those alternatives: >>> >>> a) the full 24-bits code is received by the driver; >>> b) some hardware will simply discard the MSB of the address; >>> c) a few hardware will discard the entire keycode, as the >>> checksum bytes won't match. >> >> I know there's a lot of variety, another example is drivers that >> discard >> (possibly after matching address) everything but the "command" part of >> the scancode. That should not be used as an excuse not to try to make >> the behavior as consistent as possible. After all...that's the point >> of >> a common API. > > It should be consistent, and it should be able to support the existing > hardware. Yes, I agree, but I'm not sure what your point is? Existing hardware doesn't lose support with my patches? >>> The devices from the 0x866b manufacturer is used by a wide range >>> of devices that can do either (a) or (b). >>> >>> Well, as the to_nec32() doesn't know the original keycode, it >>> would map an address like 0x866b as 0x946b, with is wrong, and >>> won't match the corresponding NEC table. >> >> Yes, if the hardware throws away information, rc-core will sometime >> generate a scancode which does not match the real one. >> >> As you say: >> >> if the actual remote control transmits: 0x866b01fe >> and the hardware truncates it to: 0x..6b01fe >> then rc-core would convert back to: 0x946b01fe >> >> And that could be fixed with a scanmask for that driver (0xffffff)? > > I think you're meaning 0x0000ffff, right? Why 0x0000ffff?....the example I gave suggested HW which throws away one byte, meaning the last three bytes (0x6b01fe) remain valid? >> (We could also expose the scanmask to userspace so it knows which part >> of the scancode it can trust...) > > Yes, we could do it, but the current userspace should keep working. > Eventually, that means to add some backward compat code there, in order > to preserve the behavior with current tables. Not sure what you're suggesting? 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 31/03/14 14:22, David Härdeman wrote: > On 2014-03-31 12:56, James Hogan wrote: >> This would mean that if the data is put in the right bit order (first >> bit received in BIT(0), last bit received in BIT(31)), then the scancode >> = raw, and if the data is received in the reverse bit order (like the >> raw decoder, shifting the data left and inserting the last bit in >> BIT(0)) then the scancode = bitrev32(raw). >> >> Have I missed something? > > I just think we have to agree to disagree :) > > For me, storing/presenting the scancode as 0xAAaaDDdd is "obviously" the > clearest and least confusing interpretation. But I might have spent too > long time using that notation in code and mentally to be able to find > anything else intuitive :) > > 0xAAaaDDdd means that you read/parse/print it left to right, just as you > would if you drew a pulse-space chart showing the received IR pulse > (time normally progresses to the right...modulo the per-byte bitrev). Sure, but the NEC bit order is little endian, and the scancode is a 32bit value not an array of 4 bytes, so it's artificial to expect it to make any sense when read as big endian. E.g. if you extended the transmission to 48 bits you'd expect the hex printed scancode to extend to the left not the right. The bits in the 32-bit word also become discontinuous for no good reason, especially considering the cases we're trying to take into account (NEC-32 and NEC-24) both effectively have 16-bit fields. > It kind of matches the other protocol scancodes as well (the "address" > bits high, cmd bits low, the high bits tend to remain constant for one > given remote, the low bits change, although it's not a hard rule) and it Very true, but you still have the low byte of the command in the 2nd lowest byte, which is why my original suggestion was: 0xaaAAddDD I.e. swap 16bit halves, each 16bit field intact. > matches most software I've ever seen (AFAIK, LIRC represents NEC32 > scancodes this way, as does e.g. the Pronto software and protocol). > > That said...I think we at least agree that we need *a* representation > and that it should be used consistently in all drivers, right? Yes, that would be nice. Cheers James
Em Mon, 31 Mar 2014 15:22:47 +0200 David Härdeman <david@hardeman.nu> escreveu: > On 2014-03-31 12:56, James Hogan wrote: > > On 31/03/14 11:19, David Härdeman wrote: > >> On 2014-03-31 11:44, James Hogan wrote: > >>> On 29/03/14 16:11, David Härdeman wrote: > >>>> + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ > >>>> + *scancode = swab32((u32)raw); > >>> > >>> What's the point of the byte swapping? > >>> > >>> Surely the most natural NEC encoding would just treat it as a single > >>> 32-bit (LSBit first) field rather than 4 8-bit fields that needs > >>> swapping. > >> > >> Thanks for having a look at the patches, I agree with your comments on > >> the other patches (and I have to respin some of them because I missed > >> two drivers), but the comments to this patch confuses me a bit. > >> > >> That the NEC data is transmitted as 32 bits encoded with LSB bit order > >> within each byte is AFAIK just about the only thing that all > >> sources/documentation of the protocal can agree on (so bitrev:ing the > >> bits within each byte makes sense, unless the hardware has done it > >> already). > > > > Agreed (in the case of img-ir there's a bit orientation setting which > > ensures that the u64 raw has the correct bit order, in the case of NEC > > the first bit received goes in the lowest order bit of the raw data). > > > >> As for the byte order, AAaaDDdd corresponds to the transmission order > >> and seems to be what most drivers expect/use for their RX data. > > > > AAaaDDdd is big endian rendering, no? (like "%08x") > > Yeah, you could call it that. > > > If it should be interpreted as LSBit first, then the first bits > > received > > should go in the low bits of the scancode, and by extension the first > > bytes received in the low bytes of the scancode, i.e. at the end of the > > inherently big-endian hexadecimal rendering of the scancode. > > I'm not saying the whole scancode should be interpreted as one 32 bit > LSBit integer, just that the endianness within each byte should be > respected. > > >> Are you suggesting that rc-core should standardize on ddDDaaAA order? > > > > Yes (where ddDDaaAA means something like scancode > > "0x(~cmd)(cmd)(~addr)(addr)") > > Yes, that's what I meant. > > > This would mean that if the data is put in the right bit order (first > > bit received in BIT(0), last bit received in BIT(31)), then the > > scancode > > = raw, and if the data is received in the reverse bit order (like the > > raw decoder, shifting the data left and inserting the last bit in > > BIT(0)) then the scancode = bitrev32(raw). > > > > Have I missed something? > > I just think we have to agree to disagree :) > > For me, storing/presenting the scancode as 0xAAaaDDdd is "obviously" the > clearest and least confusing interpretation. But I might have spent too > long time using that notation in code and mentally to be able to find > anything else intuitive :) Inside the RC core, for all other protocols, the order always ADDRESS + COMMAND. Up to NEC-24 bits, this is preserved, as the command is always 0xDD and the address is either 0xaaAA or 0xAA. The 32-bits NEC is a little ackward, if we consider the command as also being 8 bits, and the address having 24 bits. The Tivo keytable is weird: { 0x3085f009, KEY_MEDIA }, /* TiVo Button */ { 0x3085e010, KEY_POWER2 }, /* TV Power */ { 0x3085e011, KEY_TV }, /* Live TV/Swap */ { 0x3085c034, KEY_VIDEO_NEXT }, /* TV Input */ { 0x3085e013, KEY_INFO }, { 0x3085a05f, KEY_CYCLEWINDOWS }, /* Window */ { 0x0085305f, KEY_CYCLEWINDOWS }, { 0x3085c036, KEY_EPG }, /* Guide */ ... There, the only part of the scancode that doesn't change is 0x85. It seems that they're using 8 bits for address (0xaa) and 24 bits for command (0xAADDdd). So, it seems that they're actually sending address/command as: [command >> 24><Address][(command >>8) & 0xff][command & 0xff] With seems too awkward. IMHO, it would make more sense to store those data as: <address><command> So, KEY_MEDIA, for example, would be: + { 0x8530f009, KEY_MEDIA }, /* TiVo Button */ However, I'm not sure how other 32 bits NEC scancodes might be. So, I think we should keep the internal representation as-is, for now, while we're not sure about how other vendors handle it, as, for now, there's just one IR table with 32 bits nec. That's said, I don't mind much how this is internally stored at the Kernel level, as we can always change it, but we should provide backward compatibility for userspace, when userspace sends to Kernel a 16 bit or a 24 bit keytable. So, I think we should first focus on how to properly get/set the bitsize at the API in a way that this is backward compatible. Ok, the API actually sends the bit size of each keycode, as the size length is variable, but I'm not sure if this is reliable enough, as I think that the current userspace just sets it to 32 bits, even when passing a 16 bits key. In any case, it doesn't make any sense to require userspace to convert a 16 bits normal NEC table (or a 24 bits "extended" NEC table) into a 32 bits data+checksum bitpack on userspace. 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
On Mon, Mar 31, 2014 at 12:26:56PM -0300, Mauro Carvalho Chehab wrote: >Inside the RC core, for all other protocols, the order always >ADDRESS + COMMAND. > >Up to NEC-24 bits, this is preserved, as the command is always 0xDD >and the address is either 0xaaAA or 0xAA. > >The 32-bits NEC is a little ackward, if we consider the command as >also being 8 bits, and the address having 24 bits. > >The Tivo keytable is weird: > > { 0x3085f009, KEY_MEDIA }, /* TiVo Button */ > { 0x3085e010, KEY_POWER2 }, /* TV Power */ > { 0x3085e011, KEY_TV }, /* Live TV/Swap */ > { 0x3085c034, KEY_VIDEO_NEXT }, /* TV Input */ > { 0x3085e013, KEY_INFO }, > { 0x3085a05f, KEY_CYCLEWINDOWS }, /* Window */ > { 0x0085305f, KEY_CYCLEWINDOWS }, > { 0x3085c036, KEY_EPG }, /* Guide */ > ... > >There, the only part of the scancode that doesn't change is 0x85. >It seems that they're using 8 bits for address (0xaa) and 24 >bits for command (0xAADDdd). > >So, it seems that they're actually sending address/command as: > > [command >> 24><Address][(command >>8) & 0xff][command & 0xff] > >With seems too awkward. > >IMHO, it would make more sense to store those data as: > <address><command> > >So, KEY_MEDIA, for example, would be: >+ { 0x8530f009, KEY_MEDIA }, /* TiVo Button */ > >However, I'm not sure how other 32 bits NEC scancodes might be. And it's completely irrelevant. There's little to no value in trying to determine what's a "command" and what's an "address". We have to standardize on one in-memory representation of the 32 bits, and then we should just treat it as that...as a u32 lookup key for the scancode<->keycode table which lacks any further "meaning". >So, I think we should keep the internal representation as-is, >for now, while we're not sure about how other vendors handle >it, as, for now, there's just one IR table with 32 bits nec. It doesn't matter how other vendors handle (i.e. interpret) the different bits, that's what we want to get away from, it's the whole point of this discussion. >That's said, I don't mind much how this is internally stored at >the Kernel level, as we can always change it, but we should provide >backward compatibility for userspace, when userspace sends >to Kernel a 16 bit or a 24 bit keytable. Yes, which is part of what I've proposed. It's not a coicidence that I've proposed a new ioctl and the NEC32 standardization at the same time. A new ioctl is the perfect time and place to get this right once and for all. So with the new ioctl, the "protocol" is made explicit, and the definition of a scancode follows from the "protocol" (protocol as in RC_TYPE_*). For RC_TYPE_NEC, that scancode would be a 32 bit int (exact byte and bit order to be determined, but not terribly important for this discussion). That removes *all* ambiguity and makes RC_TYPE_NEC behave *exactly* like all other protocols. At the same time it removes pointless policy from the kernel and causes a reduction in code (mostly thinking of the pointless NEC16/24/32 parsing code that gets duplicated across drivers). >So, I think we should first focus on how to properly get/set the >bitsize at the API in a way that this is backward compatible. No, adding bitsizes adds complexity and additional layers of abstraction for no good reason. And it is not needed for *any other protocol*. Why? Because the protocol already defines the bitsize. And so would NEC if we would just use all 32 bits throughout. With that change, the bitsize is implicit in *each protocol* and the new ioctl I proposed makes the protocol explicit (while providing at least a best-effort guess for NEC scancodes when the legacy ioctl is used). (and no, please, don't suggest we add RC_TYPE_NEC, RC_TYPE_NEC24, RC_TYPE_NEC32...) >Ok, the API actually sends the bit size of each keycode, as the >size length is variable, but I'm not sure if this is reliable enough, >as I think that the current userspace just sets it to 32 bits, even >when passing a 16 bits key. That won't work as you've noted yourself. >In any case, it doesn't make any sense to require userspace to >convert a 16 bits normal NEC table (or a 24 bits "extended" NEC >table) into a 32 bits data+checksum bitpack on userspace. I disagree. Strongly. It makes perfect sense. Policy doesn't belong in the kernel and all that. Asking userspace to provide a full description of the 32 bits that are transmitted removes all ambiguity and makes any "bitsize" irrelevant. For all the other protocols we support, the "bitsize" is known on a per-protocol basis. The same can be true for RC_TYPE_NEC. And userspace can still write nice user-friendly 16 bit keymaps if it likes and convert to kernel scancode notation on the fly. That's something userspace anyways has to do today. Consider the 32 bit scancode as simply being the communication protocol between userspace <-> kernel if you like. There's no reason to complicate that with bitsizes and/or multiple protocols when a single 32 bit scancode describes exactly everything that the kernel and userspace needs to know.
diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c index 887429b..4ba61ff 100644 --- a/drivers/media/pci/saa7134/saa7134-input.c +++ b/drivers/media/pci/saa7134/saa7134-input.c @@ -342,11 +342,9 @@ static int get_key_beholdm6xx(struct IR_i2c *ir, enum rc_type *protocol, return -EIO; } - if (data[9] != (unsigned char)(~data[8])) - return 0; - *protocol = RC_TYPE_NEC; - *scancode = RC_SCANCODE_NECX(data[11] << 8 | data[10], data[9]); + *scancode = RC_SCANCODE_NEC32(data[11] << 24 | data[10] << 16 | + data[9] << 8 | data[8]); *toggle = 0; return 1; } diff --git a/drivers/media/rc/img-ir/img-ir-nec.c b/drivers/media/rc/img-ir/img-ir-nec.c index 40ee844..133ea45 100644 --- a/drivers/media/rc/img-ir/img-ir-nec.c +++ b/drivers/media/rc/img-ir/img-ir-nec.c @@ -5,42 +5,20 @@ */ #include "img-ir-hw.h" -#include <linux/bitrev.h> /* Convert NEC data to a scancode */ static int img_ir_nec_scancode(int len, u64 raw, enum rc_type *protocol, u32 *scancode, u64 enabled_protocols) { - unsigned int addr, addr_inv, data, data_inv; /* a repeat code has no data */ if (!len) return IMG_IR_REPEATCODE; + if (len != 32) return -EINVAL; - /* raw encoding: ddDDaaAA */ - addr = (raw >> 0) & 0xff; - addr_inv = (raw >> 8) & 0xff; - 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 (LSBit first) */ - *scancode = bitrev8(addr) << 24 | - bitrev8(addr_inv) << 16 | - bitrev8(data) << 8 | - bitrev8(data_inv); - } else if ((addr_inv ^ addr) != 0xff) { - /* Extended NEC */ - /* scan encoding: AAaaDD */ - *scancode = addr << 16 | - addr_inv << 8 | - data; - } else { - /* Normal NEC */ - /* scan encoding: AADD */ - *scancode = addr << 8 | - data; - } + + /* raw encoding : ddDDaaAA -> scan encoding: AAaaDDdd */ + *scancode = swab32((u32)raw); *protocol = RC_TYPE_NEC; return IMG_IR_SCANCODE; } @@ -49,52 +27,9 @@ static int img_ir_nec_scancode(int len, u64 raw, enum rc_type *protocol, static int img_ir_nec_filter(const struct rc_scancode_filter *in, struct img_ir_filter *out, u64 protocols) { - unsigned int addr, addr_inv, data, data_inv; - unsigned int addr_m, addr_inv_m, data_m, data_inv_m; - - data = in->data & 0xff; - data_m = in->mask & 0xff; - - if ((in->data | in->mask) & 0xff000000) { - /* 32-bit NEC (used by Apple and TiVo remotes) */ - /* scan encoding: AAaaDDdd (LSBit first) */ - addr = bitrev8((in->data >> 24) & 0xff); - addr_m = (in->mask >> 24) & 0xff; - addr_inv = bitrev8((in->data >> 16) & 0xff); - addr_inv_m = (in->mask >> 16) & 0xff; - data = bitrev8((in->data >> 8) & 0xff); - data_m = (in->mask >> 8) & 0xff; - data_inv = bitrev8((in->data >> 0) & 0xff); - data_inv_m = (in->mask >> 0) & 0xff; - } else if ((in->data | in->mask) & 0x00ff0000) { - /* Extended NEC */ - /* scan encoding AAaaDD */ - addr = (in->data >> 16) & 0xff; - addr_m = (in->mask >> 16) & 0xff; - addr_inv = (in->data >> 8) & 0xff; - addr_inv_m = (in->mask >> 8) & 0xff; - data_inv = data ^ 0xff; - data_inv_m = data_m; - } else { - /* Normal NEC */ - /* scan encoding: AADD */ - addr = (in->data >> 8) & 0xff; - addr_m = (in->mask >> 8) & 0xff; - addr_inv = addr ^ 0xff; - addr_inv_m = addr_m; - data_inv = data ^ 0xff; - data_inv_m = data_m; - } - - /* raw encoding: ddDDaaAA */ - out->data = data_inv << 24 | - data << 16 | - addr_inv << 8 | - addr; - out->mask = data_inv_m << 24 | - data_m << 16 | - addr_inv_m << 8 | - addr_m; + /* scan encoding: AAaaDDdd -> raw encoding: ddDDaaAA */ + out->data = swab32((u32)in->data); + out->mask = swab32((u32)in->mask); return 0; } diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c index c4333d5..798e32b 100644 --- a/drivers/media/rc/ir-nec-decoder.c +++ b/drivers/media/rc/ir-nec-decoder.c @@ -50,7 +50,6 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev) struct nec_dec *data = &dev->raw->nec; u32 scancode; u8 address, not_address, command, not_command; - bool send_32bits = false; if (!rc_protocols_enabled(dev, RC_BIT_NEC)) return 0; @@ -163,28 +162,11 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev) command = bitrev8((data->bits >> 8) & 0xff); not_command = bitrev8((data->bits >> 0) & 0xff); - if ((command ^ not_command) != 0xff) { - IR_dprintk(1, "NEC checksum error: received 0x%08x\n", - data->bits); - send_32bits = true; - } - - if (send_32bits) { - /* NEC transport, but modified protocol, used by at - * least Apple and TiVo remotes */ - scancode = data->bits; - IR_dprintk(1, "NEC (modified) scancode 0x%08x\n", scancode); - } else if ((address ^ not_address) != 0xff) { - /* Extended NEC */ - scancode = address << 16 | - not_address << 8 | - command; - IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode); - } else { - /* Normal NEC */ - scancode = address << 8 | command; - IR_dprintk(1, "NEC scancode 0x%04x\n", scancode); - } + scancode = RC_SCANCODE_NEC32(address << 24 | + not_address << 16 | + command << 8 | + not_command); + IR_dprintk(1, "NEC scancode 0x%08x\n", scancode); if (data->is_nec_x) data->necx_repeat = true; diff --git a/drivers/media/rc/keymaps/rc-tivo.c b/drivers/media/rc/keymaps/rc-tivo.c index 454e062..63ed7ad 100644 --- a/drivers/media/rc/keymaps/rc-tivo.c +++ b/drivers/media/rc/keymaps/rc-tivo.c @@ -15,62 +15,61 @@ * Initial mapping is for the TiVo remote included in the Nero LiquidTV bundle, * which also ships with a TiVo-branded IR transceiver, supported by the mceusb * driver. Note that the remote uses an NEC-ish protocol, but instead of having - * a command/not_command pair, it has a vendor ID of 0xa10c, but some keys, the - * NEC extended checksums do pass, so the table presently has the intended - * values and the checksum-passed versions for those keys. + * a command/not_command pair, it has a vendor ID of 0x8530, but some keys, the + * NEC extended checksums do pass, so the table has the full code for all keys. */ static struct rc_map_table tivo[] = { - { 0xa10c900f, KEY_MEDIA }, /* TiVo Button */ - { 0xa10c0807, KEY_POWER2 }, /* TV Power */ - { 0xa10c8807, KEY_TV }, /* Live TV/Swap */ - { 0xa10c2c03, KEY_VIDEO_NEXT }, /* TV Input */ - { 0xa10cc807, KEY_INFO }, - { 0xa10cfa05, KEY_CYCLEWINDOWS }, /* Window */ - { 0x0085305f, KEY_CYCLEWINDOWS }, - { 0xa10c6c03, KEY_EPG }, /* Guide */ + { 0x853009f0, KEY_MEDIA }, /* TiVo Button */ + { 0x853010e0, KEY_POWER2 }, /* TV Power */ + { 0x853011e0, KEY_TV }, /* Live TV/Swap */ + { 0x853034c0, KEY_VIDEO_NEXT }, /* TV Input */ + { 0x853013e0, KEY_INFO }, + { 0x85305fa0, KEY_CYCLEWINDOWS }, /* Window */ + { 0x85005f30, KEY_CYCLEWINDOWS }, + { 0x853036c0, KEY_EPG }, /* Guide */ - { 0xa10c2807, KEY_UP }, - { 0xa10c6807, KEY_DOWN }, - { 0xa10ce807, KEY_LEFT }, - { 0xa10ca807, KEY_RIGHT }, + { 0x853014e0, KEY_UP }, + { 0x853016e0, KEY_DOWN }, + { 0x853017e0, KEY_LEFT }, + { 0x853015e0, KEY_RIGHT }, - { 0xa10c1807, KEY_SCROLLDOWN }, /* Red Thumbs Down */ - { 0xa10c9807, KEY_SELECT }, - { 0xa10c5807, KEY_SCROLLUP }, /* Green Thumbs Up */ + { 0x853018e0, KEY_SCROLLDOWN }, /* Red Thumbs Down */ + { 0x853019e0, KEY_SELECT }, + { 0x85301ae0, KEY_SCROLLUP }, /* Green Thumbs Up */ - { 0xa10c3807, KEY_VOLUMEUP }, - { 0xa10cb807, KEY_VOLUMEDOWN }, - { 0xa10cd807, KEY_MUTE }, - { 0xa10c040b, KEY_RECORD }, - { 0xa10c7807, KEY_CHANNELUP }, - { 0xa10cf807, KEY_CHANNELDOWN }, - { 0x0085301f, KEY_CHANNELDOWN }, + { 0x85301ce0, KEY_VOLUMEUP }, + { 0x85301de0, KEY_VOLUMEDOWN }, + { 0x85301be0, KEY_MUTE }, + { 0x853020d0, KEY_RECORD }, + { 0x85301ee0, KEY_CHANNELUP }, + { 0x85301fe0, KEY_CHANNELDOWN }, + { 0x85001f30, KEY_CHANNELDOWN }, - { 0xa10c840b, KEY_PLAY }, - { 0xa10cc40b, KEY_PAUSE }, - { 0xa10ca40b, KEY_SLOW }, - { 0xa10c440b, KEY_REWIND }, - { 0xa10c240b, KEY_FASTFORWARD }, - { 0xa10c640b, KEY_PREVIOUS }, - { 0xa10ce40b, KEY_NEXT }, /* ->| */ + { 0x853021d0, KEY_PLAY }, + { 0x853023d0, KEY_PAUSE }, + { 0x853025d0, KEY_SLOW }, + { 0x853022d0, KEY_REWIND }, + { 0x853024d0, KEY_FASTFORWARD }, + { 0x853026d0, KEY_PREVIOUS }, + { 0x853027d0, KEY_NEXT }, /* ->| */ - { 0xa10c220d, KEY_ZOOM }, /* Aspect */ - { 0xa10c120d, KEY_STOP }, - { 0xa10c520d, KEY_DVD }, /* DVD Menu */ + { 0x853044b0, KEY_ZOOM }, /* Aspect */ + { 0x853048b0, KEY_STOP }, + { 0x85304ab0, KEY_DVD }, /* DVD Menu */ - { 0xa10c140b, KEY_NUMERIC_1 }, - { 0xa10c940b, KEY_NUMERIC_2 }, - { 0xa10c540b, KEY_NUMERIC_3 }, - { 0xa10cd40b, KEY_NUMERIC_4 }, - { 0xa10c340b, KEY_NUMERIC_5 }, - { 0xa10cb40b, KEY_NUMERIC_6 }, - { 0xa10c740b, KEY_NUMERIC_7 }, - { 0xa10cf40b, KEY_NUMERIC_8 }, - { 0x0085302f, KEY_NUMERIC_8 }, - { 0xa10c0c03, KEY_NUMERIC_9 }, - { 0xa10c8c03, KEY_NUMERIC_0 }, - { 0xa10ccc03, KEY_ENTER }, - { 0xa10c4c03, KEY_CLEAR }, + { 0x853028d0, KEY_NUMERIC_1 }, + { 0x853029d0, KEY_NUMERIC_2 }, + { 0x85302ad0, KEY_NUMERIC_3 }, + { 0x85302bd0, KEY_NUMERIC_4 }, + { 0x85302cd0, KEY_NUMERIC_5 }, + { 0x85302dd0, KEY_NUMERIC_6 }, + { 0x85302ed0, KEY_NUMERIC_7 }, + { 0x85302fd0, KEY_NUMERIC_8 }, + { 0x85002f30, KEY_NUMERIC_8 }, + { 0x853030c0, KEY_NUMERIC_9 }, + { 0x853031c0, KEY_NUMERIC_0 }, + { 0x853033c0, KEY_ENTER }, + { 0x853032c0, KEY_CLEAR }, }; static struct rc_map_list tivo_map = { diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 8675e07..59bc44f 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -316,6 +316,49 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev, } /** + * guess_protocol() - heuristics to guess the protocol for a scancode + * @rdev: the struct rc_dev device descriptor + * @return: the guessed RC_TYPE_* protocol + * + * Internal routine to guess the current IR protocol for legacy ioctls. + */ +static inline enum rc_type guess_protocol(struct rc_dev *rdev) +{ + struct rc_map *rc_map = &rdev->rc_map; + + if (hweight64(rdev->enabled_protocols[RC_FILTER_NORMAL]) == 1) + return rc_bitmap_to_type(rdev->enabled_protocols[RC_FILTER_NORMAL]); + else if (hweight64(rdev->allowed_protocols[RC_FILTER_NORMAL]) == 1) + return rc_bitmap_to_type(rdev->allowed_protocols[RC_FILTER_NORMAL]); + else + return rc_map->rc_type; +} + +/** + * to_nec32() - helper function to try to convert misc NEC scancodes to NEC32 + * @orig: original scancode + * @return: NEC32 scancode + * + * This helper routine is used to provide backwards compatibility. + */ +static u32 to_nec32(u32 orig) +{ + u8 b3 = (u8)(orig >> 16); + u8 b2 = (u8)(orig >> 8); + u8 b1 = (u8)(orig >> 0); + + if (orig <= 0xffff) + /* Plain old NEC */ + return b2 << 24 | ((u8)~b2) << 16 | b1 << 8 | ((u8)~b1); + else if (orig <= 0xffffff) + /* NEC extended */ + return b3 << 24 | b2 << 16 | b1 << 8 | ((u8)~b1); + else + /* NEC32 */ + return orig; +} + +/** * ir_setkeycode() - set a keycode in the scancode->keycode table * @idev: the struct input_dev device descriptor * @scancode: the desired scancode @@ -348,6 +391,9 @@ static int ir_setkeycode(struct input_dev *idev, if (retval) goto out; + if (guess_protocol(rdev) == RC_TYPE_NEC) + scancode = to_nec32(scancode); + index = ir_establish_scancode(rdev, rc_map, scancode, true); if (index >= rc_map->len) { retval = -ENOMEM; @@ -388,7 +434,10 @@ static int ir_setkeytable(struct rc_dev *dev, for (i = 0; i < from->size; i++) { index = ir_establish_scancode(dev, rc_map, - from->scan[i].scancode, false); + from->rc_type == RC_TYPE_NEC ? + to_nec32(from->scan[i].scancode) : + from->scan[i].scancode, + false); if (index >= rc_map->len) { rc = -ENOMEM; break; @@ -462,6 +511,8 @@ static int ir_getkeycode(struct input_dev *idev, if (retval) goto out; + if (guess_protocol(rdev) == RC_TYPE_NEC) + scancode = to_nec32(scancode); index = ir_lookup_by_scancode(rc_map, scancode); } diff --git a/drivers/media/usb/dvb-usb-v2/af9015.c b/drivers/media/usb/dvb-usb-v2/af9015.c index 5ca738a..8776eaf 100644 --- a/drivers/media/usb/dvb-usb-v2/af9015.c +++ b/drivers/media/usb/dvb-usb-v2/af9015.c @@ -1230,24 +1230,10 @@ static int af9015_rc_query(struct dvb_usb_device *d) /* Remember this key */ memcpy(state->rc_last, &buf[12], 4); - if (buf[14] == (u8) ~buf[15]) { - if (buf[12] == (u8) ~buf[13]) { - /* NEC */ - state->rc_keycode = RC_SCANCODE_NEC(buf[12], - buf[14]); - } else { - /* NEC extended*/ - state->rc_keycode = RC_SCANCODE_NECX(buf[12] << 8 | - buf[13], - buf[14]); - } - } else { - /* 32 bit NEC */ - state->rc_keycode = RC_SCANCODE_NEC32(buf[12] << 24 | - buf[13] << 16 | - buf[14] << 8 | - buf[15]); - } + state->rc_keycode = RC_SCANCODE_NEC32(buf[12] << 24 | + buf[13] << 16 | + buf[14] << 8 | + buf[15]); rc_keydown(d->rc_dev, RC_TYPE_NEC, state->rc_keycode, 0); } else { dev_dbg(&d->udev->dev, "%s: no key press\n", __func__); diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c index 3bfba13..6bc9693 100644 --- a/drivers/media/usb/dvb-usb-v2/af9035.c +++ b/drivers/media/usb/dvb-usb-v2/af9035.c @@ -1284,19 +1284,8 @@ static int af9035_rc_query(struct dvb_usb_device *d) else if (ret < 0) goto err; - if ((buf[2] + buf[3]) == 0xff) { - if ((buf[0] + buf[1]) == 0xff) { - /* NEC standard 16bit */ - key = RC_SCANCODE_NEC(buf[0], buf[2]); - } else { - /* NEC extended 24bit */ - key = RC_SCANCODE_NECX(buf[0] << 8 | buf[1], buf[2]); - } - } else { - /* NEC full code 32bit */ - key = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 | - buf[2] << 8 | buf[3]); - } + key = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 | + buf[2] << 8 | buf[3]); dev_dbg(&d->udev->dev, "%s: %*ph\n", __func__, 4, buf); diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c index 935dbaa..7e38278 100644 --- a/drivers/media/usb/dvb-usb-v2/az6007.c +++ b/drivers/media/usb/dvb-usb-v2/az6007.c @@ -214,18 +214,10 @@ static int az6007_rc_query(struct dvb_usb_device *d) if (st->data[1] == 0x44) return 0; - if ((st->data[3] ^ st->data[4]) == 0xff) { - if ((st->data[1] ^ st->data[2]) == 0xff) - code = RC_SCANCODE_NEC(st->data[1], st->data[3]); - else - code = RC_SCANCODE_NECX(st->data[1] << 8 | st->data[2], - st->data[3]); - } else { - code = RC_SCANCODE_NEC32(st->data[1] << 24 | - st->data[2] << 16 | - st->data[3] << 8 | - st->data[4]); - } + code = RC_SCANCODE_NEC32(st->data[1] << 24 | + st->data[2] << 16 | + st->data[3] << 8 | + st->data[4]); rc_keydown(d->rc_dev, RC_TYPE_NEC, code, st->data[5]); diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c b/drivers/media/usb/dvb-usb-v2/lmedm04.c index 6e3ca72..c78833e 100644 --- a/drivers/media/usb/dvb-usb-v2/lmedm04.c +++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c @@ -294,14 +294,10 @@ static void lme2510_int_response(struct urb *lme_urb) ibuf[4] ^= 0xff; ibuf[5] ^= 0xff; - if (ibuf[4] ^ ibuf[5] == 0xff) - key = RC_SCANCODE_NECX(ibuf[2] << 8 | ibuf[3], - ibuf[4]); - else - key = RC_SCANCODE_NEC32(ibuf[2] << 24 | - ibuf[3] << 16 | - ibuf[4] << 8 | - ibuf[5]); + key = RC_SCANCODE_NEC32(ibuf[2] << 24 | + ibuf[3] << 16 | + ibuf[4] << 8 | + ibuf[5]); deb_info(1, "INT Key =%08x", key); rc_keydown(adap_to_d(adap)->rc_dev, RC_TYPE_NEC, key, 0); diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index 574f4ee..45c77b1 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1246,20 +1246,8 @@ static int rtl2831u_rc_query(struct dvb_usb_device *d) goto err; if (buf[4] & 0x01) { - if (buf[2] == (u8) ~buf[3]) { - if (buf[0] == (u8) ~buf[1]) { - /* NEC standard (16 bit) */ - rc_code = RC_SCANCODE_NEC(buf[0], buf[2]); - } else { - /* NEC extended (24 bit) */ - rc_code = RC_SCANCODE_NECX(buf[0] << 8 | buf[1], - buf[2]); - } - } else { - /* NEC full (32 bit) */ - rc_code = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 | - buf[2] << 8 | buf[3]); - } + rc_code = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 | + buf[2] << 8 | buf[3]); rc_keydown(d->rc_dev, RC_TYPE_NEC, rc_code, 0); diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c index 4f5caf5..d7b7932 100644 --- a/drivers/media/usb/dvb-usb/dib0700_core.c +++ b/drivers/media/usb/dvb-usb/dib0700_core.c @@ -715,22 +715,11 @@ static void dib0700_rc_urb_completion(struct urb *purb) break; } - if (poll_reply->data ^ poll_reply->not_data != 0xff) { - deb_data("NEC32 protocol\n"); - scancode = RC_SCANCODE_NEC32(poll_reply->system << 24 | - poll_reply->not_system << 16 | - poll_reply->data << 8 | - poll_reply->not_data); - } else if (poll_reply->system ^ poll_reply->not_system != 0xff) { - deb_data("NEC extended protocol\n"); - scancode = RC_SCANCODE_NECX(poll_reply->system << 8 | - poll_reply->not_system, - poll_reply->data); - } else { - deb_data("NEC normal protocol\n"); - scancode = RC_SCANCODE_NEC(poll_reply->system, - poll_reply->data); - } + deb_data("NEC protocol\n"); + scancode = RC_SCANCODE_NEC32(poll_reply->system << 24 | + poll_reply->not_system << 16 | + poll_reply->data << 8 | + poll_reply->not_data); break; default: diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c index 014888f..4bbd8e4 100644 --- a/drivers/media/usb/em28xx/em28xx-input.c +++ b/drivers/media/usb/em28xx/em28xx-input.c @@ -261,16 +261,10 @@ static int em2874_polling_getkey(struct em28xx_IR *ir, case RC_BIT_NEC: poll_result->protocol = RC_TYPE_RC5; poll_result->scancode = msg[1] << 8 | msg[2]; - if ((msg[3] ^ msg[4]) != 0xff) /* 32 bits NEC */ - poll_result->scancode = RC_SCANCODE_NEC32((msg[1] << 24) | - (msg[2] << 16) | - (msg[3] << 8) | - (msg[4])); - else if ((msg[1] ^ msg[2]) != 0xff) /* 24 bits NEC */ - poll_result->scancode = RC_SCANCODE_NECX(msg[1] << 8 | - msg[2], msg[3]); - else /* Normal NEC */ - poll_result->scancode = RC_SCANCODE_NEC(msg[1], msg[3]); + poll_result->scancode = RC_SCANCODE_NEC32((msg[1] << 24) | + (msg[2] << 16) | + (msg[3] << 8) | + (msg[4])); break; case RC_BIT_RC6_0: diff --git a/include/media/rc-map.h b/include/media/rc-map.h index 894c7e4..2e6c659 100644 --- a/include/media/rc-map.h +++ b/include/media/rc-map.h @@ -33,6 +33,8 @@ enum rc_type { RC_TYPE_SHARP = 18, /* Sharp protocol */ }; +#define rc_bitmap_to_type(x) fls64(x) + #define RC_BIT_NONE 0 #define RC_BIT_UNKNOWN (1 << RC_TYPE_UNKNOWN) #define RC_BIT_OTHER (1 << RC_TYPE_OTHER) @@ -64,13 +66,21 @@ enum rc_type { #define RC_SCANCODE_UNKNOWN(x) (x) #define RC_SCANCODE_OTHER(x) (x) -#define RC_SCANCODE_NEC(addr, cmd) (((addr) << 8) | (cmd)) -#define RC_SCANCODE_NECX(addr, cmd) (((addr) << 8) | (cmd)) -#define RC_SCANCODE_NEC32(data) ((data) & 0xffffffff) #define RC_SCANCODE_RC5(sys, cmd) (((sys) << 8) | (cmd)) #define RC_SCANCODE_RC5_SZ(sys, cmd) (((sys) << 8) | (cmd)) #define RC_SCANCODE_RC6_0(sys, cmd) (((sys) << 8) | (cmd)) #define RC_SCANCODE_RC6_6A(vendor, sys, cmd) (((vendor) << 16) | ((sys) << 8) | (cmd)) +#define RC_SCANCODE_NEC(addr, cmd) \ + ((( (addr) & 0xff) << 24) | \ + ((~(addr) & 0xff) << 16) | \ + (( (cmd) & 0xff) << 8 ) | \ + ((~(cmd) & 0xff) << 0 )) +#define RC_SCANCODE_NECX(addr, cmd) \ + ((( (addr) & 0xffff) << 16) | \ + (( (cmd) & 0x00ff) << 8) | \ + ((~(cmd) & 0x00ff) << 0)) +#define RC_SCANCODE_NEC32(data) ((data) & 0xffffffff) + struct rc_map_table { u32 scancode;
Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core and the nec decoder without any loss of functionality. In order to maintain backwards compatibility, some heuristics are added in rc-main.c to convert scancodes to NEC32 as necessary. I plan to introduce a different ioctl later which makes the protocol explicit (and which expects all NEC scancodes to be 32 bit, thereby removing the need for guesswork). Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/pci/saa7134/saa7134-input.c | 6 +- drivers/media/rc/img-ir/img-ir-nec.c | 79 ++---------------------- drivers/media/rc/ir-nec-decoder.c | 28 ++------- drivers/media/rc/keymaps/rc-tivo.c | 95 ++++++++++++++--------------- drivers/media/rc/rc-main.c | 53 ++++++++++++++++ drivers/media/usb/dvb-usb-v2/af9015.c | 22 +------ drivers/media/usb/dvb-usb-v2/af9035.c | 15 +---- drivers/media/usb/dvb-usb-v2/az6007.c | 16 +---- drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 +--- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 16 +---- drivers/media/usb/dvb-usb/dib0700_core.c | 21 ++---- drivers/media/usb/em28xx/em28xx-input.c | 14 +--- include/media/rc-map.h | 16 ++++- 13 files changed, 151 insertions(+), 242 deletions(-) -- 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