2010-11-26 13:12:48

by James Hogan

[permalink] [raw]
Subject: [PATCH] ir-nec-decoder: fix extended NEC scancodes

Could somebody check this as I'm unable to test it.

I'm also not entirely certain it isn't winbond-cir that is in error
instead of ir-nec-decoder.

Cheers
James
--
After comparing the extended NEC scancode construction of the software
decoder and winbond-cir it appears the software decoder is putting the
two address bytes the wrong way around.

Here's how the decoders currently generate scancodes:
winbond-cir normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
soft normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
winbond-cir extended NEC: msb [ 0x0, not_addr, addr, cmd ] lsb
soft extended NEC: msb [ 0x0, addr, not_addr, cmd ] lsb

The soft decider is not consistent with [1], assuming the "Address high"
byte (not_addr) should be more significant than the "Address low" byte
(addr) in the scancode.

[1] http://www.sbprojects.com/knowledge/ir/nec.htm

Signed-off-by: James Hogan <[email protected]>
---
drivers/media/IR/ir-nec-decoder.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index 70993f7..11d3e78 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -166,8 +166,8 @@ static int ir_nec_decode(struct input_dev
*input_dev, struct ir_raw_event ev)

if ((address ^ not_address) != 0xff) {
/* Extended NEC */
- scancode = address << 16 |
- not_address << 8 |
+ scancode = not_address << 16 |
+ address << 8 |
command;
IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
} else {
--
1.7.2.3


2010-11-26 13:39:57

by Andy Walls

[permalink] [raw]
Subject: Re: [PATCH] ir-nec-decoder: fix extended NEC scancodes

You might want to check the handling against this NEC datasheet

http://www.datasheetcatalog.org/datasheet/nec/UPD6122G-002.pdf

The datasheet calls the address bytes "custom code" (high byte apparently) and "custom code'" (low byte apparently) with both bytes sent lsb first. It appears the high byte is sent first when using 16 bit codes.

I'm away from my computer so I can't check much more.

Regards,
Andy


James Hogan <[email protected]> wrote:

>Could somebody check this as I'm unable to test it.
>
>I'm also not entirely certain it isn't winbond-cir that is in error
>instead of ir-nec-decoder.
>
>Cheers
>James
>--
>After comparing the extended NEC scancode construction of the software
>decoder and winbond-cir it appears the software decoder is putting the
>two address bytes the wrong way around.
>
>Here's how the decoders currently generate scancodes:
>winbond-cir normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
>soft normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
>winbond-cir extended NEC: msb [ 0x0, not_addr, addr, cmd ] lsb
>soft extended NEC: msb [ 0x0, addr, not_addr, cmd ] lsb
>
>The soft decider is not consistent with [1], assuming the "Address high"
>byte (not_addr) should be more significant than the "Address low" byte
>(addr) in the scancode.
>
>[1] http://www.sbprojects.com/knowledge/ir/nec.htm
>
>Signed-off-by: James Hogan <[email protected]>
>---
> drivers/media/IR/ir-nec-decoder.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/media/IR/ir-nec-decoder.c
>b/drivers/media/IR/ir-nec-decoder.c
>index 70993f7..11d3e78 100644
>--- a/drivers/media/IR/ir-nec-decoder.c
>+++ b/drivers/media/IR/ir-nec-decoder.c
>@@ -166,8 +166,8 @@ static int ir_nec_decode(struct input_dev
>*input_dev, struct ir_raw_event ev)
>
> if ((address ^ not_address) != 0xff) {
> /* Extended NEC */
>- scancode = address << 16 |
>- not_address << 8 |
>+ scancode = not_address << 16 |
>+ address << 8 |
> command;
> IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
> } else {
>--
>1.7.2.3
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2010-11-26 21:12:25

by James Hogan

[permalink] [raw]
Subject: Re: [PATCH] ir-nec-decoder: fix extended NEC scancodes

(expanded cc list)

On Fri, Nov 26, 2010 at 08:39:25AM -0500, Andy Walls wrote:
> You might want to check the handling against this NEC datasheet
>
> http://www.datasheetcatalog.org/datasheet/nec/UPD6122G-002.pdf
>
> The datasheet calls the address bytes "custom code" (high byte apparently) and "custom code'" (low byte apparently) with both bytes sent lsb first. It appears the high byte is sent first when using 16 bit codes.

Thanks for the link Andy. You appear to be correct, which suggests that
winbond-cir is the "wrong" one. Curiously there is a comment in
winbond-cir.c which explicitly mentions which byte is which:

854 * With NEC extended, Address1 is the LSB of the Address and
855 * Address2 is the MSB, Command parsing remains unchanged.

but then it also says:

25 * To do:
26 * o Test NEC and RC5

I suppose its all a matter of convention, but I think they should at
least be consistent, so I'll go by the NEC datasheet and submit a patch
to winbond-cir instead.

Cheers
James

> James Hogan <[email protected]> wrote:
>
> >Could somebody check this as I'm unable to test it.
> >
> >I'm also not entirely certain it isn't winbond-cir that is in error
> >instead of ir-nec-decoder.
> >
> >Cheers
> >James
> >--
> >After comparing the extended NEC scancode construction of the software
> >decoder and winbond-cir it appears the software decoder is putting the
> >two address bytes the wrong way around.
> >
> >Here's how the decoders currently generate scancodes:
> >winbond-cir normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
> >soft normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
> >winbond-cir extended NEC: msb [ 0x0, not_addr, addr, cmd ] lsb
> >soft extended NEC: msb [ 0x0, addr, not_addr, cmd ] lsb
> >
> >The soft decider is not consistent with [1], assuming the "Address high"
> >byte (not_addr) should be more significant than the "Address low" byte
> >(addr) in the scancode.
> >
> >[1] http://www.sbprojects.com/knowledge/ir/nec.htm
> >
> >Signed-off-by: James Hogan <[email protected]>
> >---
> > drivers/media/IR/ir-nec-decoder.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/media/IR/ir-nec-decoder.c
> >b/drivers/media/IR/ir-nec-decoder.c
> >index 70993f7..11d3e78 100644
> >--- a/drivers/media/IR/ir-nec-decoder.c
> >+++ b/drivers/media/IR/ir-nec-decoder.c
> >@@ -166,8 +166,8 @@ static int ir_nec_decode(struct input_dev
> >*input_dev, struct ir_raw_event ev)
> >
> > if ((address ^ not_address) != 0xff) {
> > /* Extended NEC */
> >- scancode = address << 16 |
> >- not_address << 8 |
> >+ scancode = not_address << 16 |
> >+ address << 8 |
> > command;
> > IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
> > } else {

2010-12-02 17:30:30

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH] ir-nec-decoder: fix extended NEC scancodes

Em 26-11-2010 19:12, James Hogan escreveu:
> (expanded cc list)
>
> On Fri, Nov 26, 2010 at 08:39:25AM -0500, Andy Walls wrote:
>> You might want to check the handling against this NEC datasheet
>>
>> http://www.datasheetcatalog.org/datasheet/nec/UPD6122G-002.pdf
>>
>> The datasheet calls the address bytes "custom code" (high byte apparently) and "custom code'" (low byte apparently) with both bytes sent lsb first. It appears the high byte is sent first when using 16 bit codes.
>
> Thanks for the link Andy. You appear to be correct, which suggests that
> winbond-cir is the "wrong" one. Curiously there is a comment in
> winbond-cir.c which explicitly mentions which byte is which:
>
> 854 * With NEC extended, Address1 is the LSB of the Address and
> 855 * Address2 is the MSB, Command parsing remains unchanged.
>
> but then it also says:
>
> 25 * To do:
> 26 * o Test NEC and RC5
>
> I suppose its all a matter of convention, but I think they should at
> least be consistent, so I'll go by the NEC datasheet and submit a patch
> to winbond-cir instead.

Yes, they should be consistent. I also think that RC core is right. So, the
better is to change it at winbond-cir.

>
> Cheers
> James
>
>> James Hogan <[email protected]> wrote:
>>
>>> Could somebody check this as I'm unable to test it.
>>>
>>> I'm also not entirely certain it isn't winbond-cir that is in error
>>> instead of ir-nec-decoder.
>>>
>>> Cheers
>>> James
>>> --
>>> After comparing the extended NEC scancode construction of the software
>>> decoder and winbond-cir it appears the software decoder is putting the
>>> two address bytes the wrong way around.
>>>
>>> Here's how the decoders currently generate scancodes:
>>> winbond-cir normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
>>> soft normal NEC: msb [ 0x0, 0x0, addr, cmd ] lsb
>>> winbond-cir extended NEC: msb [ 0x0, not_addr, addr, cmd ] lsb
>>> soft extended NEC: msb [ 0x0, addr, not_addr, cmd ] lsb
>>>
>>> The soft decider is not consistent with [1], assuming the "Address high"
>>> byte (not_addr) should be more significant than the "Address low" byte
>>> (addr) in the scancode.
>>>
>>> [1] http://www.sbprojects.com/knowledge/ir/nec.htm
>>>
>>> Signed-off-by: James Hogan <[email protected]>
>>> ---
>>> drivers/media/IR/ir-nec-decoder.c | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/IR/ir-nec-decoder.c
>>> b/drivers/media/IR/ir-nec-decoder.c
>>> index 70993f7..11d3e78 100644
>>> --- a/drivers/media/IR/ir-nec-decoder.c
>>> +++ b/drivers/media/IR/ir-nec-decoder.c
>>> @@ -166,8 +166,8 @@ static int ir_nec_decode(struct input_dev
>>> *input_dev, struct ir_raw_event ev)
>>>
>>> if ((address ^ not_address) != 0xff) {
>>> /* Extended NEC */
>>> - scancode = address << 16 |
>>> - not_address << 8 |
>>> + scancode = not_address << 16 |
>>> + address << 8 |
>>> command;
>>> IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
>>> } else {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html