Re: [PATCH] Fix cx88 remote control input

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Hi Lawerence,

Em 03-05-2011 14:19, Jarod Wilson escreveu:
> On May 3, 2011, at 3:25 AM, Lawrence Rust wrote:
> 
>> On Mon, 2011-05-02 at 15:50 -0300, Mauro Carvalho Chehab wrote:
>>> Em 08-04-2011 09:50, Lawrence Rust escreveu:
>>>> This patch restores remote control input for cx2388x based boards on
>>>> Linux kernels >= 2.6.38.
>>>>
>>>> After upgrading from Linux 2.6.37 to 2.6.38 I found that the remote
>>>> control input of my Hauppauge Nova-S plus was no longer functioning.  
>>>> I posted a question on this newsgroup and Mauro Carvalho Chehab gave
>>>> some helpful pointers as to the likely cause.
>>>>
>>>> Turns out that there are 2 problems:
>>>>
>>>> 1. In the IR interrupt handler of cx88-input.c there's a 32-bit multiply
>>>> overflow which causes IR pulse durations to be incorrectly calculated.

I'm adding the patch for it today on my linux-next tree. I'll probably send
upstream on the next couple days.

>>>>
>>>> 2. The RC5 decoder appends the system code to the scancode and passes
>>>> the combination to rc_keydown().  Unfortunately, the combined value is
>>>> then forwarded to input_event() which then fails to recognise a valid
>>>> scancode and hence no input events are generated.

In this case, a patch should be sent to -stable in separate.

Greg,

On 2.6.38, there are two RC5 keytables for Hauppauge devices, one with incomplete
scancodes (just 8 bits for each key) and the other one with 14 bits. One patch
changed the IR handling for cx88 to accept 14-bits for scancodes, but the change
didn't switch to the complete table.

For 2.6.39, all keytables for Hauppauge (4 different tables) were unified into
just one keytable. So, on 2.6.39-rc, the cx88 code already works fine for 64-bits
kernels, and the fix for 32-bits is undergoing.

In the case of 2.6.38 kernel, the Remote Controller is broken for both kernels.
The fix is as simple as:

--- a/drivers/media/video/cx88/cx88-input.c
+++ b/drivers/media/video/cx88/cx88-input.c
@@ -283,7 +283,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
	case CX88_BOARD_PCHDTV_HD3000:
	case CX88_BOARD_PCHDTV_HD5500:
	case CX88_BOARD_HAUPPAUGE_IRONLY:
-		ir_codes = RC_MAP_HAUPPAUGE_NEW;
+		ir_codes = RC_MAP_RC5_HAUPPAUGE_NEW;
		ir->sampling = 1;
		break;
	case CX88_BOARD_WINFAST_DTV2000H:


But this change diverges from upstream, due to the table unify. Would such patch
be acceptable for stable, even not having a corresponding upstream commit?

Thanks!
Mauro.

>>>>
>>>> I note that in commit 2997137be8eba5bf9c07a24d5fda1f4225f9ca7d, which
>>>> introduced these changes, David Härdeman changed the IR sample frequency
>>>> to a supposed 4kHz.  However, the registers dealing with IR input are
>>>> undocumented in the cx2388x datasheets and there's no publicly available
>>>> information on them.  I have to ask the question why this change was
>>>> made as it is of no apparent benefit and could have unanticipated
>>>> consequences.  IMHO that change should also be reverted unless there is
>>>> evidence to substantiate it.
>>>>
>>>> Signed off by: Lawrence Rust <lvr at softsystem dot co dot uk>
>>>>
>>>> diff --git a/drivers/media/rc/ir-rc5-decoder.c b/drivers/media/rc/ir-rc5-decoder.c
>>>> index ebdba55..c4052da 100644
>>>> --- a/drivers/media/rc/ir-rc5-decoder.c
>>>> +++ b/drivers/media/rc/ir-rc5-decoder.c
>>>> @@ -144,10 +144,15 @@ again:
>>>> 			system   = (data->bits & 0x007C0) >> 6;
>>>> 			toggle   = (data->bits & 0x00800) ? 1 : 0;
>>>> 			command += (data->bits & 0x01000) ? 0 : 0x40;
>>>> -			scancode = system << 8 | command;
>>>> -
>>>> -			IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
>>>> -				   scancode, toggle);
>>>> +            /* Notes
>>>> +             * 1. Should filter unknown systems e.g Hauppauge use 0x1e or 0x1f
>>>> +             * 2. Don't include system in the scancode otherwise input_event()
>>>> +             *    doesn't recognise the scancode
>>>> +             */
>>>> +			scancode = command;
>>>> +
>>>> +			IR_dprintk(1, "RC5 scancode 0x%02x (system: 0x%02x toggle: %u)\n",
>>>> +				   scancode, system, toggle);
>>>> 		}
>>>>
>>>> 		rc_keydown(dev, scancode, toggle);
>>>
>>> I agree with Jarod: The above hunk shouldn't go upstream, or else it would break _lots_ of
>>> remotes.
>>>
>>>> diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c
>>>> index c820e2f..7281db4 100644
>>>> --- a/drivers/media/video/cx88/cx88-input.c
>>>> +++ b/drivers/media/video/cx88/cx88-input.c
>>>> @@ -524,7 +524,7 @@ void cx88_ir_irq(struct cx88_core *core)
>>>> 	for (todo = 32; todo > 0; todo -= bits) {
>>>> 		ev.pulse = samples & 0x80000000 ? false : true;
>>>> 		bits = min(todo, 32U - fls(ev.pulse ? samples : ~samples));
>>>> -		ev.duration = (bits * NSEC_PER_SEC) / (1000 * ir_samplerate);
>>>> +		ev.duration = bits * (NSEC_PER_SEC / (1000 * ir_samplerate)); /* NB avoid 32-bit overflow */
>>>> 		ir_raw_event_store_with_filter(ir->dev, &ev);
>>>> 		samples <<= bits;
>>>> 	}
>>>
>>> This change is OK, though. Yet. due to precision issues, it is better to do a 64-bit
>>> multiplication and use do_div for the division. This is compatible with 32 bits and 64
>>> bits systems, and will reduce error noise at the duration.
>>>
>>> I've reworked that part of the patch, as follows.
>>
>> The following is a much simpler change that maintains precision and
>> avoids 64-bit arithmetic.  Moving the division by 1000 and grouping it
>> with (NSEC_PER_SEC / 1000), an exact integer, avoids the 32-bit overflow
>> and allows the compiler to optimise the division without losing any
>> precision.
>>
>> diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c
>> index 06f7d1d..67a2b08 100644
>> --- a/drivers/media/video/cx88/cx88-input.c
>> +++ b/drivers/media/video/cx88/cx88-input.c
>> @@ -523,7 +523,7 @@ void cx88_ir_irq(struct cx88_core *core)
>> 	for (todo = 32; todo > 0; todo -= bits) {
>> 		ev.pulse = samples & 0x80000000 ? false : true;
>> 		bits = min(todo, 32U - fls(ev.pulse ? samples : ~samples));
>> -		ev.duration = (bits * NSEC_PER_SEC) / (1000 * ir_samplerate);
>> +		ev.duration = (bits * (NSEC_PER_SEC / 1000)) / ir_samplerate;
>> 		ir_raw_event_store_with_filter(ir->dev, &ev);
>> 		samples <<= bits;
>> 	}
> 
> ACK on this part for the devel tree.
> 
> 
>> And, FWIW the following patch fixes RC key input for Nova-S plus,
>> HVR1100, HVR3000 and HVR4000 in the 2.6.38 kernel.  Apparently a
>> Hauppauge keymap with system ID code was added in this release but the
>> cx88 code was not updated when the RC5 decoder changes were made:
>>
>> diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c
>> index 06f7d1d..67a2b08 100644
>> --- a/drivers/media/video/cx88/cx88-input.c
>> +++ b/drivers/media/video/cx88/cx88-input.c
>> @@ -283,7 +283,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
>> 	case CX88_BOARD_PCHDTV_HD3000:
>> 	case CX88_BOARD_PCHDTV_HD5500:
>> 	case CX88_BOARD_HAUPPAUGE_IRONLY:
>> -		ir_codes = RC_MAP_HAUPPAUGE_NEW;
>> +		ir_codes = RC_MAP_RC5_HAUPPAUGE_NEW;
>> 		ir->sampling = 1;
>> 		break;
>> 	case CX88_BOARD_WINFAST_DTV2000H:
>>
>> Signed off by: Lawrence Rust < lvr at softsystem dot co dot uk >
> 
> ACK on this part for the 2.6.38.y stable tree.
> 
>