[PATCH] Fix cx88 remote control input

[PATCH] Fix cx88 remote control input

[Lawrence Rust]

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.

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.

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);
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;
 	}

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:

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

Nacked-by: Jarod Wilson <jarod@redhat.com>

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

This part is so very very wrong. We should NOT filter here. Filtering
can be achieved on the keymap side, and you *do* include the system
here. The fix for your issue is an update to the relevant keymap so
that its matching the system byte as well.

The divide fix looks sane though.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:

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

Just to clarify on this one, you're missing a step. We get the scancode,
and its passed to rc_keydown. rc_keydown then looks for a match in the
loaded keytable, then passes the *keycode* that matches the scancode
along to input_event. If you fix the keytable to contain system and
command, everything should work just fine again. Throwing away data is
a no-no though -- take a look at recent changes to ir-kdb-i2c, which
actually just recently made it start *including* system. :)

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Lawrence Rust]

On Fri, 2011-04-08 at 10:41 -0400, Jarod Wilson wrote:
> On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:
> 
> > 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:
> ...
> > 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.
> 
> Just to clarify on this one, you're missing a step. We get the scancode,
> and its passed to rc_keydown. rc_keydown then looks for a match in the
> loaded keytable, then passes the *keycode* that matches the scancode
> along to input_event. If you fix the keytable to contain system and
> command, everything should work just fine again. Throwing away data is
> a no-no though -- take a look at recent changes to ir-kdb-i2c, which
> actually just recently made it start *including* system. :)

Don't shoot the messenger.

I'm just reporting what I had to do to fix a clumsy hack by someone 6
months ago who didn't test their changes.  This patch _restores_ the
operation of a subsystem broken by those changes

Perhaps those responsible for commit
2997137be8eba5bf9c07a24d5fda1f4225f9ca7d:

    Signed-off-by: David Härdeman <david@hardeman.nu>
    Acked-by: Jarod Wilson <jarod@redhat.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

should fix the keytable.  In the meantime (next year) I'll be using this
patch.

-- 
Lawrence

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 11:22 AM, Lawrence Rust wrote:

> On Fri, 2011-04-08 at 10:41 -0400, Jarod Wilson wrote:
>> On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:
>> 
>>> 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:
>> ...
>>> 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.
>> 
>> Just to clarify on this one, you're missing a step. We get the scancode,
>> and its passed to rc_keydown. rc_keydown then looks for a match in the
>> loaded keytable, then passes the *keycode* that matches the scancode
>> along to input_event. If you fix the keytable to contain system and
>> command, everything should work just fine again. Throwing away data is
>> a no-no though -- take a look at recent changes to ir-kdb-i2c, which
>> actually just recently made it start *including* system. :)
> 
> Don't shoot the messenger.

Wasn't my intention. I was simply trying to explain why your "fix"
isn't correct.


> I'm just reporting what I had to do to fix a clumsy hack by someone 6
> months ago who didn't test their changes.  This patch _restores_ the
> operation of a subsystem broken by those changes
> 
> Perhaps those responsible for commit
> 2997137be8eba5bf9c07a24d5fda1f4225f9ca7d:
> 
>    Signed-off-by: David Härdeman <david@hardeman.nu>
>    Acked-by: Jarod Wilson <jarod@redhat.com>
>    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> should fix the keytable.  In the meantime (next year) I'll be using this
> patch.

The entire commit message:

    [media] ir-core: convert drivers/media/video/cx88 to ir-core
    
    This patch converts the cx88 driver (for sampling hw) to use the
    decoders provided by ir-core instead of the separate ones provided
    by ir-functions (and gets rid of those).
    
    The value for MO_DDS_IO had a comment saying it corresponded to
    a 4kHz samplerate. That comment was unfortunately misleading. The
    actual samplerate was something like 3250Hz.
    
    The current value has been derived by analyzing the elapsed time
    between interrupts for different values (knowing that each interrupt
    corresponds to 32 samples).
    
    Thanks to Mariusz Bialonczyk <manio@skyboo.net> for testing my patches
    (about one a day for two weeks!) on actual hardware.

Please note the part about how it *was* tested. And this certainly
was not a "clumsy hack", I'd actually call it a fairly skilled bit
of code de-duplication by David. Anyway...

The problem is that there isn't a "the keytable". There are many
many keytables. And a lot of different hardware. Testing all possible
combinations of hardware (both receiver side and remote side) is
next to impossible. We do what we can. Its unfortunate that your
hardware regressed in functionality. It happens, but it *can* be
fixed. The fix you provided just wasn't correct. The correct fix is
trivially updating drivers/media/rc/keymaps/<insert-your-keymap-here>.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Lawrence Rust]

On Fri, 2011-04-08 at 12:21 -0400, Jarod Wilson wrote:
> On Apr 8, 2011, at 11:22 AM, Lawrence Rust wrote:
> 
> > On Fri, 2011-04-08 at 10:41 -0400, Jarod Wilson wrote:
> >> On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:
> >> 
> >>> 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:
> >> ...
> >>> 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.
> >> 
> >> Just to clarify on this one, you're missing a step. We get the scancode,
> >> and its passed to rc_keydown. rc_keydown then looks for a match in the
> >> loaded keytable, then passes the *keycode* that matches the scancode
> >> along to input_event. If you fix the keytable to contain system and
> >> command, everything should work just fine again. Throwing away data is
> >> a no-no though -- take a look at recent changes to ir-kdb-i2c, which
> >> actually just recently made it start *including* system. :)
> > 
> > Don't shoot the messenger.
> 
> Wasn't my intention. I was simply trying to explain why your "fix"
> isn't correct.
> 
> 
> > I'm just reporting what I had to do to fix a clumsy hack by someone 6
> > months ago who didn't test their changes.  This patch _restores_ the
> > operation of a subsystem broken by those changes
> > 
> > Perhaps those responsible for commit
> > 2997137be8eba5bf9c07a24d5fda1f4225f9ca7d:
> > 
> >    Signed-off-by: David Härdeman <david@hardeman.nu>
> >    Acked-by: Jarod Wilson <jarod@redhat.com>
> >    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > 
> > should fix the keytable.  In the meantime (next year) I'll be using this
> > patch.
> 
> The entire commit message:
> 
>     [media] ir-core: convert drivers/media/video/cx88 to ir-core
>     
>     This patch converts the cx88 driver (for sampling hw) to use the
>     decoders provided by ir-core instead of the separate ones provided
>     by ir-functions (and gets rid of those).
>     
>     The value for MO_DDS_IO had a comment saying it corresponded to
>     a 4kHz samplerate. That comment was unfortunately misleading. The
>     actual samplerate was something like 3250Hz.
>     
>     The current value has been derived by analyzing the elapsed time
>     between interrupts for different values (knowing that each interrupt
>     corresponds to 32 samples).
>     
>     Thanks to Mariusz Bialonczyk <manio@skyboo.net> for testing my patches
>     (about one a day for two weeks!) on actual hardware.
> 
> Please note the part about how it *was* tested. And this certainly
> was not a "clumsy hack", I'd actually call it a fairly skilled bit
> of code de-duplication by David. Anyway...
> 
> The problem is that there isn't a "the keytable". There are many
> many keytables. And a lot of different hardware. Testing all possible
> combinations of hardware (both receiver side and remote side) is
> next to impossible.

Hauppauge have the lion's share of the DVB card market so this mod
should have been tested with a Haupauge RC, but clearly wasn't.  It also
clearly wasn't tested with a cx2388x card because the interrupt handler
has an obvious overflow.

>  We do what we can. Its unfortunate that your
> hardware regressed in functionality. It happens, but it *can* be
> fixed. The fix you provided just wasn't correct.

It is correct for 2.6.38, which is what was described.  I haven't
checked all the keymaps but a sample of 5 show that none include a
system ID.

>  The correct fix is
> trivially updating drivers/media/rc/keymaps/<insert-your-keymap-here>.

So, cherrypick the Hauppauge keytable from 2.6.39 and apply it to 2.6.38
together with the 2nd part of this patch.  It would be real nice if that
was in 2.6.38.3

-- 
Lawrence

Re: [PATCH] Fix cx88 remote control input

[Devin Heitmueller]

On Fri, Apr 8, 2011 at 12:21 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> The problem is that there isn't a "the keytable". There are many
> many keytables. And a lot of different hardware. Testing all possible
> combinations of hardware (both receiver side and remote side) is
> next to impossible. We do what we can. Its unfortunate that your
> hardware regressed in functionality. It happens, but it *can* be
> fixed. The fix you provided just wasn't correct. The correct fix is
> trivially updating drivers/media/rc/keymaps/<insert-your-keymap-here>.

I think the fundamental failure here was avoidable.  We introduced a
new requirement that keytables included system codes, knowing full
well that the vast majority of them did not meet the requirement.  In
fact, a quick scan through the first 20 or so keymaps show that even
today only *HALF* of them are populated today.  That means that half
of the remote keymaps are also completely broken.

This decision was doomed to fail.  It basically said, "Yes, I know
full well that I'm breaking most of the keymaps currently supported,
but maybe some of those users will eventually report the issue and
I'll make them provide an updated keymap which will eventually be
merged upstream for others so that their remotes are no longer
broken."

We should have introduced a RC profile property indicating how many
bits were "significant".  Then for those remotes which didn't have the
system code, we could have continued to match against only the key
code.  Then over time, as keymaps improved, those keymaps could be
updated and the number of significant bits could be adjusted to
indicate that the system code was present.

This was a crappy call, and it was completely foreseeable.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 1:07 PM, Devin Heitmueller wrote:

> On Fri, Apr 8, 2011 at 12:21 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
>> The problem is that there isn't a "the keytable". There are many
>> many keytables. And a lot of different hardware. Testing all possible
>> combinations of hardware (both receiver side and remote side) is
>> next to impossible. We do what we can. Its unfortunate that your
>> hardware regressed in functionality. It happens, but it *can* be
>> fixed. The fix you provided just wasn't correct. The correct fix is
>> trivially updating drivers/media/rc/keymaps/<insert-your-keymap-here>.
> 
> I think the fundamental failure here was avoidable.  We introduced a
> new requirement that keytables included system codes, knowing full
> well that the vast majority of them did not meet the requirement.  In
> fact, a quick scan through the first 20 or so keymaps show that even
> today only *HALF* of them are populated today.  That means that half
> of the remote keymaps are also completely broken.

Have to admit that I don't think it ever registered in my head that
we were going to break that many existing keymaps. But something
to consider: how many of those are *raw* rc-5 scancode keymaps, vs.
cooked scancodes from drivers that only provide command? It may
well be that we should have been more discriminating when building
those keymaps, to distinguish which were truly raw IR scancodes that
the in-kernel decoders ascertained, and which were just scancodes
handed to us directly from the IR hardware.


> This decision was doomed to fail.  It basically said, "Yes, I know
> full well that I'm breaking most of the keymaps currently supported,
> but maybe some of those users will eventually report the issue and
> I'll make them provide an updated keymap which will eventually be
> merged upstream for others so that their remotes are no longer
> broken."

Well, ir-keytable -w is also an option, though that does kill the
"Just Works"-ness when you first have to come up with the new map.


> We should have introduced a RC profile property indicating how many
> bits were "significant".  Then for those remotes which didn't have the
> system code, we could have continued to match against only the key
> code.

This was a change in the raw rc-5 IR decoder. There's *always* going
to be a system code (or at least, a resulting byte), isn't there?
Otherwise, its simply not an rc-5 signal. The "no system code" case
should really only apply to hardware decoders that strip it off
internally.

Speaking of which, something just occurred to me. Functionality of
Hauppauge receivers and remotes *was* tested. With ir-kbd-i2c. Which
was stripping off the system code at the time. It just wasn't tested
with Hauppauge hardware that actually passed along raw IR. In 2.6.39,
ir-kbd-i2c has been fixed so that it passes along system code and
the Hauppauge keymaps were updated accordingly, which also happens
to fix the cx88 raw IR case. 


> Then over time, as keymaps improved, those keymaps could be
> updated and the number of significant bits could be adjusted to
> indicate that the system code was present.
> 
> This was a crappy call, and it was completely foreseeable.

Possibly. I think too many of us are only hacking on this in their
limited free time though, so things like this may get overlooked.
I have quite a few pieces of Hauppauge hardware, several with IR
receivers and remotes, but all of which use ir-kbd-i2c (or
lirc_zilog), i.e., none of which pass along raw IR.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 12:50 PM, Lawrence Rust wrote:

> On Fri, 2011-04-08 at 12:21 -0400, Jarod Wilson wrote:
>> On Apr 8, 2011, at 11:22 AM, Lawrence Rust wrote:
>> 
>>> On Fri, 2011-04-08 at 10:41 -0400, Jarod Wilson wrote:
>>>> On Apr 8, 2011, at 8:50 AM, Lawrence Rust wrote:
>>>> 
>>>>> 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:
>>>> ...
>>>>> 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.
>>>> 
>>>> Just to clarify on this one, you're missing a step. We get the scancode,
>>>> and its passed to rc_keydown. rc_keydown then looks for a match in the
>>>> loaded keytable, then passes the *keycode* that matches the scancode
>>>> along to input_event. If you fix the keytable to contain system and
>>>> command, everything should work just fine again. Throwing away data is
>>>> a no-no though -- take a look at recent changes to ir-kdb-i2c, which
>>>> actually just recently made it start *including* system. :)
>>> 
>>> Don't shoot the messenger.
>> 
>> Wasn't my intention. I was simply trying to explain why your "fix"
>> isn't correct.
>> 
>> 
>>> I'm just reporting what I had to do to fix a clumsy hack by someone 6
>>> months ago who didn't test their changes.  This patch _restores_ the
>>> operation of a subsystem broken by those changes
>>> 
>>> Perhaps those responsible for commit
>>> 2997137be8eba5bf9c07a24d5fda1f4225f9ca7d:
>>> 
>>>   Signed-off-by: David Härdeman <david@hardeman.nu>
>>>   Acked-by: Jarod Wilson <jarod@redhat.com>
>>>   Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>> 
>>> should fix the keytable.  In the meantime (next year) I'll be using this
>>> patch.
>> 
>> The entire commit message:
>> 
>>    [media] ir-core: convert drivers/media/video/cx88 to ir-core
>> 
>>    This patch converts the cx88 driver (for sampling hw) to use the
>>    decoders provided by ir-core instead of the separate ones provided
>>    by ir-functions (and gets rid of those).
>> 
>>    The value for MO_DDS_IO had a comment saying it corresponded to
>>    a 4kHz samplerate. That comment was unfortunately misleading. The
>>    actual samplerate was something like 3250Hz.
>> 
>>    The current value has been derived by analyzing the elapsed time
>>    between interrupts for different values (knowing that each interrupt
>>    corresponds to 32 samples).
>> 
>>    Thanks to Mariusz Bialonczyk <manio@skyboo.net> for testing my patches
>>    (about one a day for two weeks!) on actual hardware.
>> 
>> Please note the part about how it *was* tested. And this certainly
>> was not a "clumsy hack", I'd actually call it a fairly skilled bit
>> of code de-duplication by David. Anyway...
>> 
>> The problem is that there isn't a "the keytable". There are many
>> many keytables. And a lot of different hardware. Testing all possible
>> combinations of hardware (both receiver side and remote side) is
>> next to impossible.
> 
> Hauppauge have the lion's share of the DVB card market so this mod
> should have been tested with a Haupauge RC, but clearly wasn't.

Its actually not that clear. It clearly wasn't tested with *your*
receiver hardware, but a Hauppauge RC *was* tested using other
receiver hardware, most notably, ir-kbd-i2c-driven IR hardware in
the PVR-250, HVR-1950 and HD-PVR. Umpteen different combinations
of bridge drivers, demod drivers, IR chips, etc., make it rather
hard to test every possible combination, even when looking solely
at products from a single company. :\


> It also
> clearly wasn't tested with a cx2388x card because the interrupt handler
> has an obvious overflow.

I'll give you that one. But look how many different devices are
listed in cx88_ir_init(), and you can see how it might be hard to
be able to test every single combination.


>> We do what we can. Its unfortunate that your
>> hardware regressed in functionality. It happens, but it *can* be
>> fixed. The fix you provided just wasn't correct.
> 
> It is correct for 2.6.38, which is what was described.  I haven't
> checked all the keymaps but a sample of 5 show that none include a
> system ID.

Likely because they're primarily used by non-raw IR drivers and/or
ir-kbd-i2c, which until 2.6.39, was stripping out system codes (which
made it impossible to use a universal remote programmed for rc-5
signals that aren't using Hauppauge's system codes of choice, despite
it being entirely possible for the hardware to handle it).


>> The correct fix is
>> trivially updating drivers/media/rc/keymaps/<insert-your-keymap-here>.
> 
> So, cherrypick the Hauppauge keytable from 2.6.39 and apply it to 2.6.38
> together with the 2nd part of this patch.  It would be real nice if that
> was in 2.6.38.3

See my reply to Devin's mail, re: ir-kbd-i2c. We're sort of in a crappy
situation here. Changing the keymaps to include system code in 2.6.38
means breaking every other Hauppauge card that uses ir-kbd-i2c, unless
that *also* gets updated in 2.6.38. I'm fairly certain that Hauppauge
hardware with ir-kbd-i2c-driven IR is actually more common in the field
than that which actually passes raw IR. However, I think by simply adding
a temporarily redundant keymap that gets used by raw IR hardware, or
by increasing the size of the single Hauppauge keymap by way of multiple
mappings for each key (both with and without system codes), we can
satisfy both cases in 2.6.38.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Devin Heitmueller]

On Fri, Apr 8, 2011 at 2:00 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> Have to admit that I don't think it ever registered in my head that
> we were going to break that many existing keymaps. But something
> to consider: how many of those are *raw* rc-5 scancode keymaps, vs.
> cooked scancodes from drivers that only provide command? It may
> well be that we should have been more discriminating when building
> those keymaps, to distinguish which were truly raw IR scancodes that
> the in-kernel decoders ascertained, and which were just scancodes
> handed to us directly from the IR hardware.

As far as I know, keymaps didn't even support more than eight bit
codes until recently.  Even if the hardware had supported system
codes, there was nothing to compare it against since the keymaps were
limited to eight entries.

>> This decision was doomed to fail.  It basically said, "Yes, I know
>> full well that I'm breaking most of the keymaps currently supported,
>> but maybe some of those users will eventually report the issue and
>> I'll make them provide an updated keymap which will eventually be
>> merged upstream for others so that their remotes are no longer
>> broken."
>
> Well, ir-keytable -w is also an option, though that does kill the
> "Just Works"-ness when you first have to come up with the new map.

Agreed.

>> We should have introduced a RC profile property indicating how many
>> bits were "significant".  Then for those remotes which didn't have the
>> system code, we could have continued to match against only the key
>> code.
>
> This was a change in the raw rc-5 IR decoder. There's *always* going
> to be a system code (or at least, a resulting byte), isn't there?
> Otherwise, its simply not an rc-5 signal. The "no system code" case
> should really only apply to hardware decoders that strip it off
> internally.

I realize that the actual remote controls do have system codes, but
our remote control keymaps are missing the info.  Wouldn't it have
been better if for those cases we only compared against the key code,
thereby not resulting in those keymaps being broken?  IMHO, it would
be better to have had the relatively low risk of the receiver
responding to keypresses from an incorrect remote because the keymap
wasn't explicit enough than have those remote controls stop working
entirely.

> Speaking of which, something just occurred to me. Functionality of
> Hauppauge receivers and remotes *was* tested. With ir-kbd-i2c. Which
> was stripping off the system code at the time. It just wasn't tested
> with Hauppauge hardware that actually passed along raw IR. In 2.6.39,
> ir-kbd-i2c has been fixed so that it passes along system code and
> the Hauppauge keymaps were updated accordingly, which also happens
> to fix the cx88 raw IR case.

I don't doubt that the Hauppauge remote worked against ir-kbd-i2c
because of the deficiency you described in the ir-kbd-i2c driver.  I
question the notion of introducing the requirement that all keymap
definitions must have system codes without having really thought
through the notion that it would result in breaking every existing
keymap which hadn't been updated.

>> Then over time, as keymaps improved, those keymaps could be
>> updated and the number of significant bits could be adjusted to
>> indicate that the system code was present.
>>
>> This was a crappy call, and it was completely foreseeable.
>
> Possibly. I think too many of us are only hacking on this in their
> limited free time though, so things like this may get overlooked.
> I have quite a few pieces of Hauppauge hardware, several with IR
> receivers and remotes, but all of which use ir-kbd-i2c (or
> lirc_zilog), i.e., none of which pass along raw IR.

You don't have an HVR-950 or some other stick which announces RC5
codes?  If not, let me know and I will send you something.  It's kind
of silly for someone doing that sort of work to not have at least one
product in each category of receiver.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 2:38 PM, Devin Heitmueller wrote:
...
> I question the notion of introducing the requirement that all keymap
> definitions must have system codes without having really thought
> through the notion that it would result in breaking every existing
> keymap which hadn't been updated.

Speaks the the "too many of us are only hacking on this in their
limited free time" point I raised. Hack, hack, hack, test with the
hardware available on hand (which is actually quite a bit in my case,
I think I have upwards of 35 receivers and even more remotes now),
see that it works, move on to the next issue. I'm certainly guilty of
not looking at the bigger picture and thinking about possible
ramifications more than once. :)

...
>> I have quite a few pieces of Hauppauge hardware, several with IR
>> receivers and remotes, but all of which use ir-kbd-i2c (or
>> lirc_zilog), i.e., none of which pass along raw IR.
> 
> You don't have an HVR-950 or some other stick which announces RC5
> codes?  If not, let me know and I will send you something.  It's kind
> of silly for someone doing that sort of work to not have at least one
> product in each category of receiver.

I don't think I even fully realized before today that there was
Hauppauge hardware shipping with the grey remotes and a raw IR
receiver. All the Hauppauge stuff I've got is either i2c IR
(PVR-250, PVR-350, HVR-1950, HD-PVR) or came with a bundled mceusb
transceiver (HVR-1500Q, HVR-1800, HVR-950Q -- model 72241, iirc),
and its all working these days (modulo some quirks with the HD-PVR
that still need sorting, but they're not regressions, its actually
better now than it used to be).

So yeah, I guess I have a gap in my IR hardware collection here,
and would be happy to have something to fill it.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Andy Walls]

Jarod Wilson <jarod@wilsonet.com> wrote:

>On Apr 8, 2011, at 2:38 PM, Devin Heitmueller wrote:
>...
>> I question the notion of introducing the requirement that all keymap
>> definitions must have system codes without having really thought
>> through the notion that it would result in breaking every existing
>> keymap which hadn't been updated.
>
>Speaks the the "too many of us are only hacking on this in their
>limited free time" point I raised. Hack, hack, hack, test with the
>hardware available on hand (which is actually quite a bit in my case,
>I think I have upwards of 35 receivers and even more remotes now),
>see that it works, move on to the next issue. I'm certainly guilty of
>not looking at the bigger picture and thinking about possible
>ramifications more than once. :)
>
>...
>>> I have quite a few pieces of Hauppauge hardware, several with IR
>>> receivers and remotes, but all of which use ir-kbd-i2c (or
>>> lirc_zilog), i.e., none of which pass along raw IR.
>> 
>> You don't have an HVR-950 or some other stick which announces RC5
>> codes?  If not, let me know and I will send you something.  It's kind
>> of silly for someone doing that sort of work to not have at least one
>> product in each category of receiver.
>
>I don't think I even fully realized before today that there was
>Hauppauge hardware shipping with the grey remotes and a raw IR
>receiver. All the Hauppauge stuff I've got is either i2c IR
>(PVR-250, PVR-350, HVR-1950, HD-PVR) or came with a bundled mceusb
>transceiver (HVR-1500Q, HVR-1800, HVR-950Q -- model 72241, iirc),
>and its all working these days (modulo some quirks with the HD-PVR
>that still need sorting, but they're not regressions, its actually
>better now than it used to be).
>
>So yeah, I guess I have a gap in my IR hardware collection here,
>and would be happy to have something to fill it.
>
>-- 
>Jarod Wilson
>jarod@wilsonet.com
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jarod,

The HVR-1850 uses a raw IR receiver in the CX23888 and older HVR-1250s use the raw IR receiver in the CX23885.  They both work for Rx (I need to tweak the Cx23885 rx watermark though), but I never found time to finish Tx (lack of kernel interface when I had time).

If you obtain one of these I can answer any driver questions.

Regards,
Andy
 

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

On Apr 8, 2011, at 4:50 PM, Andy Walls wrote:

> Jarod Wilson <jarod@wilsonet.com> wrote:
...
>>>> I have quite a few pieces of Hauppauge hardware, several with IR
>>>> receivers and remotes, but all of which use ir-kbd-i2c (or
>>>> lirc_zilog), i.e., none of which pass along raw IR.
>>> 
>>> You don't have an HVR-950 or some other stick which announces RC5
>>> codes?  If not, let me know and I will send you something.  It's kind
>>> of silly for someone doing that sort of work to not have at least one
>>> product in each category of receiver.
>> 
>> I don't think I even fully realized before today that there was
>> Hauppauge hardware shipping with the grey remotes and a raw IR
>> receiver. All the Hauppauge stuff I've got is either i2c IR
>> (PVR-250, PVR-350, HVR-1950, HD-PVR) or came with a bundled mceusb
>> transceiver (HVR-1500Q, HVR-1800, HVR-950Q -- model 72241, iirc),
>> and its all working these days (modulo some quirks with the HD-PVR
>> that still need sorting, but they're not regressions, its actually
>> better now than it used to be).
>> 
>> So yeah, I guess I have a gap in my IR hardware collection here,
>> and would be happy to have something to fill it.
>> 
> 
> Jarod,
> 
> The HVR-1850 uses a raw IR receiver in the CX23888 and older HVR-1250s use the raw IR receiver in the CX23885.  They both work for Rx (I need to tweak the Cx23885 rx watermark though), but I never found time to finish Tx (lack of kernel interface when I had time).
> 
> If you obtain one of these I can answer any driver questions.

Quite some time back, I bought an HVR-1800 and an HVR-1250. I know one of
them came with an mceusb transceiver and remote, as was pretty sure it was
the 1800. For some reason, I didn't recall the 1250 coming with anything at
all, but looking at dmesg output for it:

cx23885 driver version 0.0.2 loaded
cx23885 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
CORE cx23885[0]: subsystem: 0070:7911, board: Hauppauge WinTV-HVR1250 [card=3,autodetected]
tveeprom 0-0050: Hauppauge model 79001, rev E3D9, serial# 4904656
tveeprom 0-0050: MAC address is 00:0d:fe:4a:d6:d0
tveeprom 0-0050: tuner model is Microtune MT2131 (idx 139, type 4)
tveeprom 0-0050: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88)
tveeprom 0-0050: audio processor is CX23885 (idx 39)
tveeprom 0-0050: decoder processor is CX23885 (idx 33)
tveeprom 0-0050: has no radio, has IR receiver, has no IR transmitter

So it seems I do have hardware. However, its one of the two tuner cards in
my "production" mythtv backend right now, making it a bit hard to do any
experimenting with. If I can get it out of there, it looks like I just add
an enable_885_ir=1, and I should be able to poke at it...

-- 
Jarod Wilson
jarod@wilsonet.com

HVR-1250/CX23885 IR Rx (Re: [PATCH] Fix cx88 remote control input)

[Andy Walls]

On Sat, 2011-04-09 at 21:39 -0400, Jarod Wilson wrote:

> > Jarod,
> > 
> > The HVR-1850 uses a raw IR receiver in the CX23888 and older
> HVR-1250s use the raw IR receiver in the CX23885.  They both work for
> Rx (I need to tweak the Cx23885 rx watermark though), but I never
> found time to finish Tx (lack of kernel interface when I had time).
> > 
> > If you obtain one of these I can answer any driver questions.
> 
> Quite some time back, I bought an HVR-1800 and an HVR-1250. I know one of
> them came with an mceusb transceiver and remote, as was pretty sure it was
> the 1800. For some reason, I didn't recall the 1250 coming with anything at
> all, but looking at dmesg output for it:
> 
> cx23885 driver version 0.0.2 loaded
> cx23885 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
> CORE cx23885[0]: subsystem: 0070:7911, board: Hauppauge WinTV-HVR1250 [card=3,autodetected]
> tveeprom 0-0050: Hauppauge model 79001, rev E3D9, serial# 4904656
> tveeprom 0-0050: MAC address is 00:0d:fe:4a:d6:d0
> tveeprom 0-0050: tuner model is Microtune MT2131 (idx 139, type 4)
> tveeprom 0-0050: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88)
> tveeprom 0-0050: audio processor is CX23885 (idx 39)
> tveeprom 0-0050: decoder processor is CX23885 (idx 33)
> tveeprom 0-0050: has no radio, has IR receiver, has no IR transmitter
> 
> So it seems I do have hardware. However, its one of the two tuner cards in
> my "production" mythtv backend right now, making it a bit hard to do any
> experimenting with. If I can get it out of there, it looks like I just add
> an enable_885_ir=1, and I should be able to poke at it...

Yeah.  Igor's TeVii S470 CX23885 based card had interrupt storms when
enabled, so IR for '885 chips is disabled by default.  To investigate, I
tried to by an HVR-1250 with a CX23885, but instead got an HVR-1275 with
a CX23888.  dandel, on IRC, did a pretty decent job in testing HVR-1250
operation and finding it works, despite climbing kernel
build/development learning curve at the time.


You may also want to set ir_debug=2 for the *cx25840* module, if you
want to see the raw pulse/space measurements in the logs.  The cx25840
module handles the CX23885 IR controller.


When testing, you may want to add pci=nomsi to your kernel comandline.
Unless Igor has submitted his fix to reset some CX23885 hardware on
module unload or reload, CX23885 interrupts won't work right after
unloading and reloading the cx23885 module with MSI enabled. :(


In the cx25840 module you may also want to change the call in
drivers/media/video/cx25840/cx25840-ir.c:

	control_rx_irq_watermark(c, RX_FIFO_HALF_FULL);

to 

	control_rx_irq_watermark(c, RX_FIFO_NOT_EMPTY);

That's a design bug on my part.  The CX23885 IR controller is I2C
connected.  Waiting unitl the RX FIFO is half full risks losing some
pulse measurements.


Regards,
Andy

Re: [PATCH] Fix cx88 remote control input

[Mauro Carvalho Chehab]

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

Thanks!
Mauro

-
>From 4c0fb469bf88e0b1880c703ab27895d66eb940d9 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <mchehab@redhat.com>
Date: Fri, 8 Apr 2011 09:50:45 -0300
Subject: [PATCH] [media] Fix cx88 remote control input

As pointed by Lawrence Rust <lvr@softsystem.co.uk>:

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.

This is a regression caused by commit 2997137be8eba.

Reported-by: Lawrence Rust <lvr@softsystem.co.uk>
Cc: stable@kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c
index c820e2f..97fdf5a 100644
--- a/drivers/media/video/cx88/cx88-input.c
+++ b/drivers/media/video/cx88/cx88-input.c
@@ -27,6 +27,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <asm/div64.h>
 
 #include "cx88.h"
 #include <media/rc-core.h>
@@ -522,9 +523,16 @@ void cx88_ir_irq(struct cx88_core *core)
 
 	init_ir_raw_event(&ev);
 	for (todo = 32; todo > 0; todo -= bits) {
-		ev.pulse = samples & 0x80000000 ? false : true;
+		u64 duration = NSEC_PER_SEC;
+
 		bits = min(todo, 32U - fls(ev.pulse ? samples : ~samples));
-		ev.duration = (bits * NSEC_PER_SEC) / (1000 * ir_samplerate);
+
+		/* Avoid 32-bits overflow */
+		duration = bits * duration;
+		do_div(duration, 1000 * ir_samplerate);
+
+		ev.pulse = samples & 0x80000000 ? false : true;
+		ev.duration = duration;
 		ir_raw_event_store_with_filter(ir->dev, &ev);
 		samples <<= bits;
 	}

Re: [PATCH] Fix cx88 remote control input

[Lawrence Rust]

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.
> > 
> > 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.
> > 
> > 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;
 	}

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 >

> Thanks!
> Mauro
> 
> -
> >From 4c0fb469bf88e0b1880c703ab27895d66eb940d9 Mon Sep 17 00:00:00 2001
> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Date: Fri, 8 Apr 2011 09:50:45 -0300
> Subject: [PATCH] [media] Fix cx88 remote control input
> 
> As pointed by Lawrence Rust <lvr@softsystem.co.uk>:
> 
> 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.
> 
> This is a regression caused by commit 2997137be8eba.
> 
> Reported-by: Lawrence Rust <lvr@softsystem.co.uk>
> Cc: stable@kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c
> index c820e2f..97fdf5a 100644
> --- a/drivers/media/video/cx88/cx88-input.c
> +++ b/drivers/media/video/cx88/cx88-input.c
> @@ -27,6 +27,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <asm/div64.h>
>  
>  #include "cx88.h"
>  #include <media/rc-core.h>
> @@ -522,9 +523,16 @@ void cx88_ir_irq(struct cx88_core *core)
>  
>  	init_ir_raw_event(&ev);
>  	for (todo = 32; todo > 0; todo -= bits) {
> -		ev.pulse = samples & 0x80000000 ? false : true;
> +		u64 duration = NSEC_PER_SEC;
> +
>  		bits = min(todo, 32U - fls(ev.pulse ? samples : ~samples));
> -		ev.duration = (bits * NSEC_PER_SEC) / (1000 * ir_samplerate);
> +
> +		/* Avoid 32-bits overflow */
> +		duration = bits * duration;
> +		do_div(duration, 1000 * ir_samplerate);
> +
> +		ev.pulse = samples & 0x80000000 ? false : true;
> +		ev.duration = duration;
>  		ir_raw_event_store_with_filter(ir->dev, &ev);
>  		samples <<= bits;
>  	}
> 

-- 
Lawrence

Re: [PATCH] Fix cx88 remote control input

[Jarod Wilson]

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


-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] Fix cx88 remote control input

[Mauro Carvalho Chehab]

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

Re: [PATCH] Fix cx88 remote control input

[Greg KH]

On Wed, May 04, 2011 at 05:16:29PM -0300, Mauro Carvalho Chehab wrote:
> 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?

Yes, as long as .39 is working properly.  We take patches in -stable for
stuff like this at times, it just needs to be specified exactly like you
did above.  Want me to take this patch as-is for .38-stable?

thanks,

greg k-h

Re: [PATCH] Fix cx88 remote control input

[Mauro Carvalho Chehab]

Em 04-05-2011 17:36, Greg KH escreveu:
> Yes, as long as .39 is working properly.  We take patches in -stable for
> stuff like this at times, it just needs to be specified exactly like you
> did above.

OK.

> Want me to take this patch as-is for .38-stable?

Yes, please. I'm forwarding you bellow with the proper authorship/SOB/ack.

This patch fixes RC for 64 bits kernels. The extra fix for 32 bits kernels,
(solves a calculus overflow), were sent today to -next. I generally wait 
for a couple days before asking Linus to pull from it.

-

Subject: [media] cx88: Fix HVR4000 IR keymap
From: Lawrence Rust <lvr@softsystem.dot.uk>

Fixes the RC key input for Nova-S plus, HVR1100, HVR3000 and HVR4000 in 
the 2.6.38 kernel.

Signed-off-by: Lawrence Rust <lvr@softsystem.dot.uk>
Acked-by: Jarod Wilson <jarod@wilsonet.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

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:

Re: [PATCH] Fix cx88 remote control input

[Greg KH]

On Wed, May 04, 2011 at 11:25:10PM -0300, Mauro Carvalho Chehab wrote:
> Em 04-05-2011 17:36, Greg KH escreveu:
> > Yes, as long as .39 is working properly.  We take patches in -stable for
> > stuff like this at times, it just needs to be specified exactly like you
> > did above.
> 
> OK.
> 
> > Want me to take this patch as-is for .38-stable?
> 
> Yes, please. I'm forwarding you bellow with the proper authorship/SOB/ack.
> 
> This patch fixes RC for 64 bits kernels. The extra fix for 32 bits kernels,
> (solves a calculus overflow), were sent today to -next. I generally wait 
> for a couple days before asking Linus to pull from it.

Now queued up.

thanks,

greg k-h

Patch "[media] cx88: Fix HVR4000 IR keymap" has been added to the 2.6.38-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    [media] cx88: Fix HVR4000 IR keymap

to the 2.6.38-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     cx88-fix-hvr4000-ir-keymap.patch
and it can be found in the queue-2.6.38 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@kernel.org> know about it.


>From mchehab@redhat.com  Thu May  5 13:34:25 2011
From: Lawrence Rust <lvr@softsystem.dot.uk>
Date: Wed, 04 May 2011 23:25:10 -0300
Subject: [media] cx88: Fix HVR4000 IR keymap
To: Greg KH <greg@kroah.com>
Cc: Jarod Wilson <jarod@wilsonet.com>, Lawrence Rust <lawrence@softsystem.co.uk>, Linux Media Mailing List <linux-media@vger.kernel.org>
Message-ID: <4DC20A86.7010509@redhat.com>

From: Lawrence Rust <lvr@softsystem.dot.uk>

[fixed in .39 in a much different way that is too big to backport to
.38 - gregkh]

Fixes the RC key input for Nova-S plus, HVR1100, HVR3000 and HVR4000 in
the 2.6.38 kernel.

Signed-off-by: Lawrence Rust <lvr@softsystem.dot.uk>
Acked-by: Jarod Wilson <jarod@wilsonet.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

---
 drivers/media/video/cx88/cx88-input.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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


Patches currently in stable-queue which might be from lvr@softsystem.dot.uk are

queue-2.6.38/cx88-fix-hvr4000-ir-keymap.patch

Re: HVR-1250/CX23885 IR Rx

[Jarod Wilson]

On Apr 10, 2011, at 7:08 PM, Andy Walls wrote:

> On Sat, 2011-04-09 at 21:39 -0400, Jarod Wilson wrote:
> 
>>> Jarod,
>>> 
>>> The HVR-1850 uses a raw IR receiver in the CX23888 and older
>> HVR-1250s use the raw IR receiver in the CX23885.  They both work for
>> Rx (I need to tweak the Cx23885 rx watermark though), but I never
>> found time to finish Tx (lack of kernel interface when I had time).
>>> 
>>> If you obtain one of these I can answer any driver questions.
>> 
>> Quite some time back, I bought an HVR-1800 and an HVR-1250. I know one of
>> them came with an mceusb transceiver and remote, as was pretty sure it was
>> the 1800. For some reason, I didn't recall the 1250 coming with anything at
>> all, but looking at dmesg output for it:
>> 
>> cx23885 driver version 0.0.2 loaded
>> cx23885 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
>> CORE cx23885[0]: subsystem: 0070:7911, board: Hauppauge WinTV-HVR1250 [card=3,autodetected]
>> tveeprom 0-0050: Hauppauge model 79001, rev E3D9, serial# 4904656
>> tveeprom 0-0050: MAC address is 00:0d:fe:4a:d6:d0
>> tveeprom 0-0050: tuner model is Microtune MT2131 (idx 139, type 4)
>> tveeprom 0-0050: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88)
>> tveeprom 0-0050: audio processor is CX23885 (idx 39)
>> tveeprom 0-0050: decoder processor is CX23885 (idx 33)
>> tveeprom 0-0050: has no radio, has IR receiver, has no IR transmitter
>> 
>> So it seems I do have hardware. However, its one of the two tuner cards in
>> my "production" mythtv backend right now, making it a bit hard to do any
>> experimenting with. If I can get it out of there, it looks like I just add
>> an enable_885_ir=1, and I should be able to poke at it...
> 
> Yeah.  Igor's TeVii S470 CX23885 based card had interrupt storms when
> enabled, so IR for '885 chips is disabled by default.  To investigate, I
> tried to by an HVR-1250 with a CX23885, but instead got an HVR-1275 with
> a CX23888.  dandel, on IRC, did a pretty decent job in testing HVR-1250
> operation and finding it works, despite climbing kernel
> build/development learning curve at the time.
...

Finally got some time to play with my 1250, dug out its rx cable, turns out to
be the same one I special-ordered to work on the 1150 Devin sent me. Oops.
Anyway. First impressions, not so good. Got a panic on boot, somewhere in
cx23885_video_irq, iirc, when booting with enable_885_ir=1 set. However, dmesg
was somewhere in the middle of cx18 init of the HVR-1600 in the box. Dunno if
there's any way that's actually directly related, but I yanked the 1600. After
doing that, the box managed to boot fine, but then while testing w/ir-keytable
and an RC-6 remote, I got what I think was the same panic -- definitely the
cx23885_video_irq bit in the trace, something about sleeping while atomic, IP
at mwait_idle. (On the plus side, IR did seem to be working before that).

Note also that this is a 2.6.32-based kernel with a 2.6.38-era backport of the
driver code, because that's what was on this box. Was about to put it back into
"production" use, but hey, its summer, there's nothing really for it to record
for another few months, so I'll keep it out and throw it into another box with
a newer kernel and serial console, etc., so I can further debug...


-- 
Jarod Wilson
jarod@wilsonet.com

Re: HVR-1250/CX23885 IR Rx

[Andy Walls]

Jarod Wilson <jarod@wilsonet.com> wrote:

>On Apr 10, 2011, at 7:08 PM, Andy Walls wrote:
>
>> On Sat, 2011-04-09 at 21:39 -0400, Jarod Wilson wrote:
>> 
>>>> Jarod,
>>>> 
>>>> The HVR-1850 uses a raw IR receiver in the CX23888 and older
>>> HVR-1250s use the raw IR receiver in the CX23885.  They both work
>for
>>> Rx (I need to tweak the Cx23885 rx watermark though), but I never
>>> found time to finish Tx (lack of kernel interface when I had time).
>>>> 
>>>> If you obtain one of these I can answer any driver questions.
>>> 
>>> Quite some time back, I bought an HVR-1800 and an HVR-1250. I know
>one of
>>> them came with an mceusb transceiver and remote, as was pretty sure
>it was
>>> the 1800. For some reason, I didn't recall the 1250 coming with
>anything at
>>> all, but looking at dmesg output for it:
>>> 
>>> cx23885 driver version 0.0.2 loaded
>>> cx23885 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
>>> CORE cx23885[0]: subsystem: 0070:7911, board: Hauppauge
>WinTV-HVR1250 [card=3,autodetected]
>>> tveeprom 0-0050: Hauppauge model 79001, rev E3D9, serial# 4904656
>>> tveeprom 0-0050: MAC address is 00:0d:fe:4a:d6:d0
>>> tveeprom 0-0050: tuner model is Microtune MT2131 (idx 139, type 4)
>>> tveeprom 0-0050: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88)
>>> tveeprom 0-0050: audio processor is CX23885 (idx 39)
>>> tveeprom 0-0050: decoder processor is CX23885 (idx 33)
>>> tveeprom 0-0050: has no radio, has IR receiver, has no IR
>transmitter
>>> 
>>> So it seems I do have hardware. However, its one of the two tuner
>cards in
>>> my "production" mythtv backend right now, making it a bit hard to do
>any
>>> experimenting with. If I can get it out of there, it looks like I
>just add
>>> an enable_885_ir=1, and I should be able to poke at it...
>> 
>> Yeah.  Igor's TeVii S470 CX23885 based card had interrupt storms when
>> enabled, so IR for '885 chips is disabled by default.  To
>investigate, I
>> tried to by an HVR-1250 with a CX23885, but instead got an HVR-1275
>with
>> a CX23888.  dandel, on IRC, did a pretty decent job in testing
>HVR-1250
>> operation and finding it works, despite climbing kernel
>> build/development learning curve at the time.
>...
>
>Finally got some time to play with my 1250, dug out its rx cable, turns
>out to
>be the same one I special-ordered to work on the 1150 Devin sent me.
>Oops.
>Anyway. First impressions, not so good. Got a panic on boot, somewhere
>in
>cx23885_video_irq, iirc, when booting with enable_885_ir=1 set.
>However, dmesg
>was somewhere in the middle of cx18 init of the HVR-1600 in the box.
>Dunno if
>there's any way that's actually directly related, but I yanked the
>1600. After
>doing that, the box managed to boot fine, but then while testing
>w/ir-keytable
>and an RC-6 remote, I got what I think was the same panic -- definitely
>the
>cx23885_video_irq bit in the trace, something about sleeping while
>atomic, IP
>at mwait_idle. (On the plus side, IR did seem to be working before
>that).
>
>Note also that this is a 2.6.32-based kernel with a 2.6.38-era backport
>of the
>driver code, because that's what was on this box. Was about to put it
>back into
>"production" use, but hey, its summer, there's nothing really for it to
>record
>for another few months, so I'll keep it out and throw it into another
>box with
>a newer kernel and serial console, etc., so I can further debug...
>
>
>-- 
>Jarod Wilson
>jarod@wilsonet.com

In a very early version of the CX23885 IR changes I made the mistake of performing I2C transactions in an interrupt handler.  That has been fixed in cx23885 for quite some time though.  It also required some I2C fixes in cx25840-core.c IIRC, which again, has been fixed for some time.

Regards,
Andy

Re: HVR-1250/CX23885 IR Rx

[Jarod Wilson]

On Jun 28, 2011, at 6:30 AM, Andy Walls wrote:

> Jarod Wilson <jarod@wilsonet.com> wrote:
> 
>> On Apr 10, 2011, at 7:08 PM, Andy Walls wrote:
>> 
>>> On Sat, 2011-04-09 at 21:39 -0400, Jarod Wilson wrote:
>>> 
>>>>> Jarod,
>>>>> 
>>>>> The HVR-1850 uses a raw IR receiver in the CX23888 and older
>>>> HVR-1250s use the raw IR receiver in the CX23885.  They both work
>> for
>>>> Rx (I need to tweak the Cx23885 rx watermark though), but I never
>>>> found time to finish Tx (lack of kernel interface when I had time).
>>>>> 
>>>>> If you obtain one of these I can answer any driver questions.
>>>> 
>>>> Quite some time back, I bought an HVR-1800 and an HVR-1250. I know
>> one of
>>>> them came with an mceusb transceiver and remote, as was pretty sure
>> it was
>>>> the 1800. For some reason, I didn't recall the 1250 coming with
>> anything at
>>>> all, but looking at dmesg output for it:
>>>> 
>>>> cx23885 driver version 0.0.2 loaded
>>>> cx23885 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
>>>> CORE cx23885[0]: subsystem: 0070:7911, board: Hauppauge
>> WinTV-HVR1250 [card=3,autodetected]
>>>> tveeprom 0-0050: Hauppauge model 79001, rev E3D9, serial# 4904656
>>>> tveeprom 0-0050: MAC address is 00:0d:fe:4a:d6:d0
>>>> tveeprom 0-0050: tuner model is Microtune MT2131 (idx 139, type 4)
>>>> tveeprom 0-0050: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88)
>>>> tveeprom 0-0050: audio processor is CX23885 (idx 39)
>>>> tveeprom 0-0050: decoder processor is CX23885 (idx 33)
>>>> tveeprom 0-0050: has no radio, has IR receiver, has no IR
>> transmitter
>>>> 
>>>> So it seems I do have hardware. However, its one of the two tuner
>> cards in
>>>> my "production" mythtv backend right now, making it a bit hard to do
>> any
>>>> experimenting with. If I can get it out of there, it looks like I
>> just add
>>>> an enable_885_ir=1, and I should be able to poke at it...
>>> 
>>> Yeah.  Igor's TeVii S470 CX23885 based card had interrupt storms when
>>> enabled, so IR for '885 chips is disabled by default.  To
>> investigate, I
>>> tried to by an HVR-1250 with a CX23885, but instead got an HVR-1275
>> with
>>> a CX23888.  dandel, on IRC, did a pretty decent job in testing
>> HVR-1250
>>> operation and finding it works, despite climbing kernel
>>> build/development learning curve at the time.
>> ...
>> 
>> Finally got some time to play with my 1250, dug out its rx cable, turns
>> out to
>> be the same one I special-ordered to work on the 1150 Devin sent me.
>> Oops.
>> Anyway. First impressions, not so good. Got a panic on boot, somewhere
>> in
>> cx23885_video_irq, iirc, when booting with enable_885_ir=1 set.
>> However, dmesg
>> was somewhere in the middle of cx18 init of the HVR-1600 in the box.
>> Dunno if
>> there's any way that's actually directly related, but I yanked the
>> 1600. After
>> doing that, the box managed to boot fine, but then while testing
>> w/ir-keytable
>> and an RC-6 remote, I got what I think was the same panic -- definitely
>> the
>> cx23885_video_irq bit in the trace, something about sleeping while
>> atomic, IP
>> at mwait_idle. (On the plus side, IR did seem to be working before
>> that).
>> 
>> Note also that this is a 2.6.32-based kernel with a 2.6.38-era backport
>> of the
>> driver code, because that's what was on this box. Was about to put it
>> back into
>> "production" use, but hey, its summer, there's nothing really for it to
>> record
>> for another few months, so I'll keep it out and throw it into another
>> box with
>> a newer kernel and serial console, etc., so I can further debug...
> 
> In a very early version of the CX23885 IR changes I made the mistake of performing I2C transactions in an interrupt handler.  That has been fixed in cx23885 for quite some time though.  It also required some I2C fixes in cx25840-core.c IIRC, which again, has been fixed for some time.

Up and running on 3.0-rc5 now, and I'm not seeing the panic, but the
box keeps hard-locking after some number of keypresses. Can't get a
peep out of it with sysrq, nmi watchdog doesn't seem to fire, etc.

At the suggestion of "Dark Shadow", I've also tried booting the box
with pci=nomsi. Works a treat then. Since his HVR-1270 and my HVR-1250
both behave much better with pci=nomsi, I'm thinking that in the
short-term, we should probably make sure msi doesn't get enabled in
the cx23885 driver, and longer-term, we can look at fixing it.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: HVR-1250/CX23885 IR Rx

[Andy Walls]

On Tue, 2011-06-28 at 17:39 -0400, Jarod Wilson wrote:

> Up and running on 3.0-rc5 now, and I'm not seeing the panic, but the
> box keeps hard-locking after some number of keypresses. Can't get a
> peep out of it with sysrq, nmi watchdog doesn't seem to fire, etc.
> 
> At the suggestion of "Dark Shadow", I've also tried booting the box
> with pci=nomsi. Works a treat then. Since his HVR-1270 and my HVR-1250
> both behave much better with pci=nomsi, I'm thinking that in the
> short-term, we should probably make sure msi doesn't get enabled in
> the cx23885 driver, and longer-term, we can look at fixing it.

Sounds fine.  But fixcing the cx23885 driver to deal with both PCIe
emulation of legacy PCI INTx and PCIe MSI will likely be very involved.
(Maybe I'm wrong?)

Taking a trip down memory lane to 2 Dec 2010...
http://www.spinics.net/lists/linux-media/msg25956.html

On Wed, 2010-12-01 at 21:52 -0800, David Liontooth wrote:
> On 11/29/2010 04:38 AM, Andy Walls wrote:
> > On Sun, 2010-11-28 at 23:49 -0800, David Liontooth wrote:

> > For a quick band-aid, use "pci=nomsi" on your kernel command line, and
> > reboot to reset the CX23888 hardware.
> >
> > The problem is MSI.  The cx23885 driver isn't ready for it.  The patch
> > that enabled MSI for cx23885 probably needs to be reverted until some of
> > these issues are sorted out.

> -- what do we lose by removing the MSI support patch?

Problems mostly. The driver was written to work with emulated legacy PCI
INTx interrupts, which are to be treated as level triggered, and not
PCIe MSI, which are to be treated as edge triggered.  That's why I say
the cx23885 driver isn't ready for MSI, but I'm not sure how involved an
audit and conversion would be.  I know an audit will take time and
expertise.


Regards,
Andy

Re: HVR-1250/CX23885 IR Rx

[Jarod Wilson]

On Jun 28, 2011, at 6:32 PM, Andy Walls wrote:

> On Tue, 2011-06-28 at 17:39 -0400, Jarod Wilson wrote:
> 
>> Up and running on 3.0-rc5 now, and I'm not seeing the panic, but the
>> box keeps hard-locking after some number of keypresses. Can't get a
>> peep out of it with sysrq, nmi watchdog doesn't seem to fire, etc.
>> 
>> At the suggestion of "Dark Shadow", I've also tried booting the box
>> with pci=nomsi. Works a treat then. Since his HVR-1270 and my HVR-1250
>> both behave much better with pci=nomsi, I'm thinking that in the
>> short-term, we should probably make sure msi doesn't get enabled in
>> the cx23885 driver, and longer-term, we can look at fixing it.
> 
> Sounds fine.  But fixcing the cx23885 driver to deal with both PCIe
> emulation of legacy PCI INTx and PCIe MSI will likely be very involved.
> (Maybe I'm wrong?)

I'm not sure either, but I know a few PCI gurus at work who could
probably lend some insight.


> Taking a trip down memory lane to 2 Dec 2010...
> http://www.spinics.net/lists/linux-media/msg25956.html

Man. I really gotta learn to search the list archive (and bugzillas
assigned to me...) before sending mail to the list, eh? ;)


> On Wed, 2010-12-01 at 21:52 -0800, David Liontooth wrote:
>> On 11/29/2010 04:38 AM, Andy Walls wrote:
>>> On Sun, 2010-11-28 at 23:49 -0800, David Liontooth wrote:
> 
>>> For a quick band-aid, use "pci=nomsi" on your kernel command line, and
>>> reboot to reset the CX23888 hardware.
>>> 
>>> The problem is MSI.  The cx23885 driver isn't ready for it.  The patch
>>> that enabled MSI for cx23885 probably needs to be reverted until some of
>>> these issues are sorted out.
> 
>> -- what do we lose by removing the MSI support patch?
> 
> Problems mostly. The driver was written to work with emulated legacy PCI
> INTx interrupts, which are to be treated as level triggered, and not
> PCIe MSI, which are to be treated as edge triggered.  That's why I say
> the cx23885 driver isn't ready for MSI, but I'm not sure how involved an
> audit and conversion would be.  I know an audit will take time and
> expertise.

Dropping msi support looks to be quite trivial, I got the card behaving
after only a few lines of change in cx23885-core.c without having to pass
in pci=nomsi, but I *only* tried IR, I haven't tried video capture. I'll
see if I can give that a spin tomorrow, and if that behaves, I can send
along the diff. If we wanted to get really fancy, I could add a modparam
to let people turn msi back on. (Never had a single issue with this card
recording with msi enabled, only IR seems busted).

Just had another thought though... I have an HVR-1800 that *does* have
issues recording, and the way the video is corrupted, its possible that
its msi-related... Will have to keep that in mind next time I poke at it.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: HVR-1250/CX23885 IR Rx

[Andy Walls]

On Tue, 2011-06-28 at 22:17 -0400, Jarod Wilson wrote:
> On Jun 28, 2011, at 6:32 PM, Andy Walls wrote:
> 
> > On Tue, 2011-06-28 at 17:39 -0400, Jarod Wilson wrote:
> > 
> >> I'm thinking that in the
> >> short-term, we should probably make sure msi doesn't get enabled in
> >> the cx23885 driver, and longer-term, we can look at fixing it.
> > 
> > Sounds fine.  But fixcing the cx23885 driver to deal with both PCIe
> > emulation of legacy PCI INTx and PCIe MSI will likely be very involved.
> > (Maybe I'm wrong?)
> 
> I'm not sure either, but I know a few PCI gurus at work who could
> probably lend some insight.
> 
> 
> > Taking a trip down memory lane to 2 Dec 2010...
> > http://www.spinics.net/lists/linux-media/msg25956.html
> 
> Man. I really gotta learn to search the list archive (and bugzillas
> assigned to me...) before sending mail to the list, eh? ;)

You seem to stumble across things that I happened to run across about
6-7 months ago.  So use that as your starting search window. ;)


> > On Wed, 2010-12-01 at 21:52 -0800, David Liontooth wrote:
> >> On 11/29/2010 04:38 AM, Andy Walls wrote:
> >>> On Sun, 2010-11-28 at 23:49 -0800, David Liontooth wrote:
> > 
> >>> For a quick band-aid, use "pci=nomsi" on your kernel command line, and
> >>> reboot to reset the CX23888 hardware.
> >>> 
> >>> The problem is MSI.  The cx23885 driver isn't ready for it.  The patch
> >>> that enabled MSI for cx23885 probably needs to be reverted until some of
> >>> these issues are sorted out.
> > 
> >> -- what do we lose by removing the MSI support patch?
> > 
> > Problems mostly. The driver was written to work with emulated legacy PCI
> > INTx interrupts, which are to be treated as level triggered, and not
> > PCIe MSI, which are to be treated as edge triggered.  That's why I say
> > the cx23885 driver isn't ready for MSI, but I'm not sure how involved an
> > audit and conversion would be.  I know an audit will take time and
> > expertise.
> 
> Dropping msi support looks to be quite trivial,

It is trivial.

>  I got the card behaving
> after only a few lines of change in cx23885-core.c without having to pass
> in pci=nomsi, but I *only* tried IR, I haven't tried video capture. I'll
> see if I can give that a spin tomorrow, and if that behaves, I can send
> along the diff.

It should.  MSI was an ill tested addition to cx23885 IMHO.

BTW, IR interrupts with the CX23885 IR unit are tricky.  The CX23885
(CX25843) A/V core is I2C connected.  Getting the A/V core to clear it's
INT_N line (wired to the main core of the CX23885) requires clearing all
the audio, video, and IR interrupts in the A/V core unit.  The video and
audio interrupts from the A/V core are currently masked.  The IR
interrupts have to be cleared by actually servicing the IR unit.

To deal with that headache, when an A/V core interrupt comes in, I
masked the PCI_AVCORE_INT (or whatever) interuupt on the CX23885 bridge
until the IR unit can be serviced.  Since the A/V core is I2C connected,
the IR unit is serviced in a workqueue work handler thread, where
sleeping is allowed.

I'm not sure how MSI can get a storming or stuck interrupt from the
CX23885 with me masking the PCI_AVCORE_INT.

IIRC the cx23885_irq handler calls some functions that might take a long
time to execute.  Maybe that matters with MSI enabled.  With MSI
disabled, you might want to set up ftrace on the cx23885 driver
functions and look to see if there are any slow paths being encountered
by the cx23885_irq handler.  I suspect the cx23885_irq handler should be
deferring some work other than just IR handling.

http://www.spinics.net/lists/linux-media/msg15762.html

Or, of course, my IR handling could be just so f---ed up that it fails
to clear the IR interrupt.  ;) ftrace might let you see that too.
(I don't have a CX23885 chip, only CX23888's, so I can't experiment.)

>  If we wanted to get really fancy, I could add a modparam
> to let people turn msi back on. (Never had a single issue with this card
> recording with msi enabled, only IR seems busted).

Getting the driver to properly support both MSI and INTx emulation might
take a few changes.  (a bunch of if statements maybe?)  I don't think
letting users choose without the driver being inspected and/or corrected
to handle both is a good idea.


> Just had another thought though... I have an HVR-1800 that *does* have
> issues recording, and the way the video is corrupted, its possible that
> its msi-related... Will have to keep that in mind next time I poke at it.

Uh, huh.

Regards,
Andy