Am 15.12.2012 14:38, schrieb Antti Palosaari: > On 12/15/2012 03:11 PM, Frank Schäfer wrote: >> Am 15.12.2012 02:54, schrieb Mauro Carvalho Chehab: >>> Em Sat, 15 Dec 2012 02:56:25 +0200 >>> Antti Palosaari <crope@xxxxxx> escreveu: >>> >>>> On 12/15/2012 02:26 AM, Mauro Carvalho Chehab wrote: >>>>> Em Fri, 14 Dec 2012 17:39:50 -0200 >>>>> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> escreveu: >>>>> >>>>>>> Anyway, first we have to GET the bytes from the hardware. That's >>>>>>> our >>>>>>> current problem ! >>>>>>> And the hardware seems to need a different setup for reg 0x50 >>>>>>> for the >>>>>>> different NEC sub protocols. >>>>>>> Which means that the we need to know the sub protocol BEFORE we >>>>>>> get any >>>>>>> bytes from the device. >>>>>> No. All em28xx needs is to make sure that the NEC protocol will >>>>>> return >>>>>> the full 32 bits scancode. >>>>> It seems a way easier/quicker to just add the proper support there >>>>> at the >>>>> driver than keep answering to this thread ;) >>>>> >>>>> Tested here with a Terratec HTC stick, and using two different IR's: >>>>> - a Terratec IR (address code 0x14 - standard NEC); >>>>> - a Pixelview IR (address code 0x866b - 24 bits NEC). >>>>> >>>>> All tests were done with the very latest version of ir-keytable, >>>>> found at >>>>> v4l-utils.git tree (http://git.linuxtv.org/v4l-utils.git). >>>>> >>>>> See the results, with the Terratec table loaded (default when the >>>>> driver is loaded): >>>>> >>>>> # ir-keytable -t >>>>> Testing events. Please, press CTRL-C to abort. >>>>> # Terratec IR >>>>> 1355529698.198046: event type EV_MSC(0x04): scancode = 0x1402 >>>>> 1355529698.198046: event type EV_KEY(0x01) key_down: >>>>> KEY_1(0x0001) >>>>> 1355529698.198046: event type EV_SYN(0x00). >>>>> 11355529698.298170: event type EV_MSC(0x04): scancode = 0x1402 >>>>> 1355529698.298170: event type EV_SYN(0x00). >>>>> 1355529698.547998: event type EV_KEY(0x01) key_up: KEY_1(0x0001) >>>>> 1355529698.547998: event type EV_SYN(0x00). >>>>> # Pixelview IR >>>>> 1355530261.416415: event type EV_MSC(0x04): scancode = 0x866b01 >>>>> 1355530261.416415: event type EV_SYN(0x00). >>>>> 1355530262.216301: event type EV_MSC(0x04): scancode = 0x866b0b >>>>> 1355530262.216301: event type EV_SYN(0x00). >>>>> >>>>> Replacing the keytable to the Pixelview's one: >>>>> >>>>> # ir-keytable -w /etc/rc_keymaps/pixelview_002t -c >>>>> Read pixelview_002t table >>>>> Old keytable cleared >>>>> Wrote 26 keycode(s) to driver >>>>> Protocols changed to NEC >>>>> >>>>> # ir-keytable -t >>>>> Testing events. Please, press CTRL-C to abort. >>>>> 1355530569.420398: event type EV_MSC(0x04): scancode = 0x1402 >>>>> 1355530569.420398: event type EV_SYN(0x00). >>>>> 1355530588.120409: event type EV_MSC(0x04): scancode = 0x866b01 >>>>> 1355530588.120409: event type EV_KEY(0x01) key_down: >>>>> KEY_1(0x0001) >>>>> 1355530591.670077: event type EV_SYN(0x00). >>>>> >>>>> And, finally, keeping both keytables active at the same time: >>>>> >>>>> # ir-keytable -c -w /etc/rc_keymaps/pixelview_002t -w >>>>> /etc/rc_keymaps/nec_terratec_cinergy_xs >>>>> Read pixelview_002t table >>>>> Read nec_terratec_cinergy_xs table >>>>> Old keytable cleared >>>>> Wrote 74 keycode(s) to driver >>>>> Protocols changed to NEC >>>>> >>>>> # sudo ir-keytable -t >>>>> Testing events. Please, press CTRL-C to abort. >>>>> 1355530856.325201: event type EV_MSC(0x04): scancode = 0x866b01 >>>>> 1355530856.325201: event type EV_KEY(0x01) key_down: >>>>> KEY_1(0x0001) >>>>> 1355530856.325201: event type EV_SYN(0x00). >>>>> 11355530856.575070: event type EV_KEY(0x01) key_up: KEY_1(0x0001) >>>>> 1355530856.575070: event type EV_SYN(0x00). >>>>> 1355530869.125226: event type EV_MSC(0x04): scancode = 0x1402 >>>>> 1355530869.125226: event type EV_KEY(0x01) key_down: >>>>> KEY_1(0x0001) >>>>> 1355530869.125226: event type EV_SYN(0x00). >>>>> 11355530869.225216: event type EV_MSC(0x04): scancode = 0x1402 >>>>> 1355530869.225216: event type EV_SYN(0x00). >>>>> 1355530869.475075: event type EV_KEY(0x01) key_up: KEY_1(0x0001) >>>>> 1355530869.475075: event type EV_SYN(0x00). >>>>> >>>>> - >>>>> >>>>> em28xx: add support for 24bits/32 bits NEC variants on em2874 and >>>>> upper >>>>> >>>>> By disabling the NEC parity check, it is possible to handle all 3 NEC >>>>> protocol variants (32, 24 or 16 bits). >>>>> >>>>> Change the driver in order to handle all of them. >>>>> >>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> >>>> >>>> >>>> >>>> >>>> NACK. NEC variant selection logic is broken by design. >>>> >>>> >>>> >>>> >>>> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c >>>>> b/drivers/media/usb/em28xx/em28xx-input.c >>>>> index 97d36b4..c84e4c8 100644 >>>>> --- a/drivers/media/usb/em28xx/em28xx-input.c >>>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c >>>>> @@ -57,8 +57,8 @@ MODULE_PARM_DESC(ir_debug, "enable debug >>>>> messages [IR]"); >>>>> struct em28xx_ir_poll_result { >>>>> unsigned int toggle_bit:1; >>>>> unsigned int read_count:7; >>>>> - u8 rc_address; >>>>> - u8 rc_data[4]; /* 1 byte on em2860/2880, 4 on em2874 */ >>>>> + >>>>> + u32 scancode; >>>>> }; >>>>> >>>>> struct em28xx_IR { >>>>> @@ -72,6 +72,7 @@ struct em28xx_IR { >>>>> struct delayed_work work; >>>>> unsigned int full_code:1; >>>>> unsigned int last_readcount; >>>>> + u64 rc_type; >>>>> >>>>> int (*get_key)(struct em28xx_IR *, struct >>>>> em28xx_ir_poll_result *); >>>>> }; >>>>> @@ -236,11 +237,8 @@ static int default_polling_getkey(struct >>>>> em28xx_IR *ir, >>>>> /* Infrared read count (Reg 0x45[6:0] */ >>>>> poll_result->read_count = (msg[0] & 0x7f); >>>>> >>>>> - /* Remote Control Address (Reg 0x46) */ >>>>> - poll_result->rc_address = msg[1]; >>>>> - >>>>> - /* Remote Control Data (Reg 0x47) */ >>>>> - poll_result->rc_data[0] = msg[2]; >>>>> + /* Remote Control Address/Data (Regs 0x46/0x47) */ >>>>> + poll_result->scancode = msg[1] << 8 | msg[2]; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -266,13 +264,30 @@ static int em2874_polling_getkey(struct >>>>> em28xx_IR *ir, >>>>> /* Infrared read count (Reg 0x51[6:0] */ >>>>> poll_result->read_count = (msg[0] & 0x7f); >>>>> >>>>> - /* Remote Control Address (Reg 0x52) */ >>>>> - poll_result->rc_address = msg[1]; >>>>> - >>>>> - /* Remote Control Data (Reg 0x53-55) */ >>>>> - poll_result->rc_data[0] = msg[2]; >>>>> - poll_result->rc_data[1] = msg[3]; >>>>> - poll_result->rc_data[2] = msg[4]; >>>>> + /* Remote Control Address (Reg 0x52) */ >>>>> + /* Remote Control Data (Reg 0x53-0x55) */ >>>>> + switch (ir->rc_type) { >>>>> + case RC_TYPE_RC5: >>>>> + poll_result->scancode = msg[1] << 8 | msg[2]; >>>>> + break; >>>>> + case RC_TYPE_NEC: >>>>> + if ((msg[3] ^ msg[4]) != 0xff) /* 32 bits NEC */ >>>> See for example KEY_CYCLEWINDOWS from RC_MAP_TIVO. Do you think it >>>> works..... :-( >>> { 0xa10cfa05, KEY_CYCLEWINDOWS }, /* Window */ >>> >>> You're right: for it to work, this key would be needed to be defined >>> as: >>> >>> { 0xa10c05, KEY_CYCLEWINDOWS }, /* Window */ >>> >>> I agree, that's weird, but a vendor that uses a key definition like >>> that doesn't know what he's doing, as a remote control with address >>> = 0xa10c >>> will very likely produce the same code. >>> >>> Btw, the way it is currently declared won't work either with mceusb, as >>> the IR decoder also does the same thing. >>> >>> (c/c Jarod, as he added the Tivo IR). >>> >>> ... >>> >>>> OK, it is much better and I can even see that in Kernel than keeping >>>> old, very limited implementation. >>>> >>>> My aim was just to probe whole variant selection method is quite >>>> broken, >>>> and it is impossible to get working with 100% reliable. As I have said >>>> loudly :) , I want 32bit scancodes for all NEC remotes, no variants at >>>> all. I think you are about the only person who wants to keep current >>>> multiple NEC variant implementation... >>> I'm not bound to it, and no, I'm not the only one that voted for this >>> implementation. This were discussed in the past, when support for >>> "extended" >>> nec got added (24 bits). When the first 32 bits >>> NEC-yet-another-weird-variant >>> arrived, the choice was natural. >>> >>> The thing is: userspace can't be broken by whatever change we do. >>> The way >>> it got implemented were the one that wouldn't generate regressions. >>> It is as simple as that. >>> >>> Cheers, >>> Mauro >> >> Sorry.... we completely lost the focus ! >> Do you remeber the thread title ? ;) >> >> We have two separate issues here. >> 1) Making Matthews hardware / the scancode retrieval code work >> And _if_ it turns out that we can't make it work without knowing the sub >> protocol type in advance >> 2) how to handle this (which doesn't necessarily mean that we have to >> solve it in the RC core) >> >> So lets focus on 1) first: >> After reading the code again, it boils down to the following code lines >> in em28xx_ir_handle_key(): >> >> if (unlikely(poll_result.read_count != ir->last_readcount)) { >> dprintk("%s: toggle: %d, count: %d, key 0x%02x%02x\n", >> __func__, >> ... >> rc_keydown(...) >> >> With reg 0x50 set to EM2874_IR_NEC=0x00, Matthew doesn't get any >> debugging messages when he presses the RC buttons. >> With reg 0x50 set to 0x01, there are only few messages, with the same >> single scancode: >> >> Am 10.12.2012 17:00, schrieb Matthew Gyurgyik: >>>>> Here is the dmesg output: >>>>> >>>>>> [root@tux ~]# dmesg -t | sort | uniq | grep 'em28xx IR' | grep >>>>>> handle >>>>>> em28xx IR (em28xx #0)/ir: 6em28xx_ir_handle_key: toggle: 0, >>>>>> count: 1, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: 6em28xx_ir_handle_key: toggle: 0, >>>>>> count: 2, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: 6em28xx_ir_handle_key: toggle: 1, >>>>>> count: 1, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: em28xx_ir_handle_key: toggle: 0, count: 1, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: em28xx_ir_handle_key: toggle: 0, count: 2, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: em28xx_ir_handle_key: toggle: 1, count: 1, >>>>>> key 0x61d6 >>>>>> em28xx IR (em28xx #0)/ir: em28xx_ir_handle_key: toggle: 1, count: 2, >>>>>> key 0x61d6 >>>>> >>>>> I pressed all the buttons on the remote (40 buttons). >>>> >>>> Did you cut the dmesg output ? Or do you really get these messages for >>>> key 0x61d6 only ? >>> >>> Correct, I only got the messages for key 0x61d6 regardless of which >>> physical button I press. >> >> So if Matthew didn't make any mistakes, the problem seems to be the read >> count handling... > > That is what happened and it is correct behavior as Matthews remote is > 24 bit NEC (and driver does not know how to handle it). If you look > again those raw dumps which I send previous mail you could see the > issue. 61d6 seen here is remote controller address, two first bits. It > outputs that because debug does not output rest 2 remaining bytes > where is actual key code. If you set em28xx to 32bit NEC mode then key > code for that remote set 61d6 by driver mistakenly as it does not know > there is 2 bytes more to handle. Antti, the problem with Matthews RC isn't the number of bytes printed to the log. The problems is, that NO messages appear in the log (with one exception). > > I copy & pasted here relevant lines from the em28xx driver. Maybe you > could now see why code is wrong - why the extended address byte is set > as to scancode mistakenly. Look especially care following variables: > msg[1], msg[2], poll_result->rc_address and poll_result.rc_data[0] > > static int em2874_polling_getkey() > { > rc = dev->em28xx_read_reg_req_len(dev, 0, EM2874_R51_IR, msg, > sizeof(msg)); > > /* Remote Control Address (Reg 0x52) */ > poll_result->rc_address = msg[1]; > > /* Remote Control Data (Reg 0x53-55) */ > poll_result->rc_data[0] = msg[2]; > poll_result->rc_data[1] = msg[3]; > poll_result->rc_data[2] = msg[4]; > } > You missed the relevant part of the code: static int em2874_polling_getkey() { ... /* Infrared read count (Reg 0x51[6:0] */ poll_result->read_count = (msg[0] & 0x7f); ... } > > > static void em28xx_ir_handle_key() > { > dprintk("%s: toggle: %d, count: %d, key 0x%02x%02x\n", __func__, > poll_result.toggle_bit, poll_result.read_count, > poll_result.rc_address, poll_result.rc_data[0]); > } > Same here, you missed the if () statement: static void em28xx_ir_handle_key(struct em28xx_IR *ir) { ... if (unlikely(poll_result.read_count != ir->last_readcount)) { dprintk("%s: toggle: %d, count: %d, key 0x%02x%02x\n", __func__, poll_result.toggle_bit, poll_result.read_count, poll_result.rc_address, poll_result.rc_data[0]); ... } It doesn't matter which format the scancode (4 bytes) has, a message should be printed in any case. But according to Matthew, that doesn't happen. Hence, the poll_result.read_count != ir->last_readcount must be the problem. > > I am about 99% sure Mauro's patch works correctly for Matthews device. > If Matthew didn't make any mistakes, I can't see how these patches can make it work. Which doesn't mean that they don't make sense. Matthew, could you please validate your test results and try Mauros patches ? If it doesn't work, please create another USB-log. > Maybe you missed point hardware returns different format in case of > 32bit and 16bit values. If it is 16bit mode it return only 2 bytes and > if 32bit mode it returns 4 bytes? > No. Regards, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
