Re: [PATCH 04/11] rc-core: do not change 32bit NEC scancode format for now

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

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

On 29/03/14 16:11, 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.
> 
> 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).
> 
> This patch is in preparation for the next few patches in this series.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/img-ir/img-ir-nec.c |   27 ++++++-----
>  drivers/media/rc/ir-nec-decoder.c    |    5 --
>  drivers/media/rc/keymaps/rc-tivo.c   |   86 +++++++++++++++++-----------------
>  3 files changed, 59 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/media/rc/img-ir/img-ir-nec.c b/drivers/media/rc/img-ir/img-ir-nec.c
> index c0111d6..40ee844 100644
> --- a/drivers/media/rc/img-ir/img-ir-nec.c
> +++ b/drivers/media/rc/img-ir/img-ir-nec.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #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,
> @@ -23,11 +24,11 @@ static int img_ir_nec_scancode(int len, u64 raw, enum rc_type *protocol,
>  	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;
> +		/* 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 */
> @@ -56,13 +57,15 @@ static int img_ir_nec_filter(const struct rc_scancode_filter *in,
>  
>  	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;
> +		/* 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;

I think the masks need bit reversing too, otherwise the mask bits won't
line up with the data as intended.

Otherwise this patch looks okay to me.

Cheers
James


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