Re: em28xx: msi Digivox ATSC board id [0db0:8810]

[Antti Palosaari <crope@iki.fi>]

On 12/15/2012 06:21 PM, Frank Schäfer wrote:
> 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@iki.fi> 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@redhat.com> escreveu:

>>> 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).

What is that exception?

In that case there should be around 40 similar debug lines - as address 
byte is same for all buttons and debug prints only address byte, toggle 
and count. As toggle and count still outputs some numbers we will see 
that many line. Without toggle and count there is likely only one debug 
line seen as output is piped through uniq which drops similar lines.

>> 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 don't see which messages are missing.

I think read_count is not incremented in case NEC 16bit checksum fails - 
hw just discards whole code => read_count not increase => no debug. For 
my tests there was always 81/01 when key was pressed and 00 when no key 
pressed (as seen also logs I posted yesterday).

>> 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

regards
Antti

-- 
http://palosaari.fi/