Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]