[PATCH 1/3] [media] winbond-cir: Fix txandrx module info

[PATCH 1/3] [media] winbond-cir: Fix txandrx module info

[Anton Blanchard]

We aren't getting any module info for the txandx option because
of a typo:

parm:           txandrx:bool

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/drivers/media/rc/winbond-cir.c
===================================================================
--- linux-2.6.orig/drivers/media/rc/winbond-cir.c	2011-11-20 20:30:57.831906589 +1100
+++ linux-2.6/drivers/media/rc/winbond-cir.c	2011-11-20 20:32:13.472362123 +1100
@@ -232,7 +232,7 @@ MODULE_PARM_DESC(invert, "Invert the sig
 
 static int txandrx; /* default = 0 */
 module_param(txandrx, bool, 0444);
-MODULE_PARM_DESC(invert, "Allow simultaneous TX and RX");
+MODULE_PARM_DESC(txandrx, "Allow simultaneous TX and RX");
 
 static unsigned int wake_sc = 0x800F040C;
 module_param(wake_sc, uint, 0644);

[PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos

[Anton Blanchard]

We need to set a timeout so we can go idle on no activity.

We weren't setting driver_type and allowed_protos, so fix that
up too.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux/drivers/media/rc/winbond-cir.c
===================================================================
--- linux.orig/drivers/media/rc/winbond-cir.c	2012-06-18 10:32:54.436717423 +1000
+++ linux/drivers/media/rc/winbond-cir.c	2012-06-18 10:33:00.192754858 +1000
@@ -1032,6 +1032,9 @@ wbcir_probe(struct pnp_dev *device, cons
 	data->dev->tx_ir = wbcir_tx;
 	data->dev->priv = data;
 	data->dev->dev.parent = &device->dev;
+	data->dev->timeout = MS_TO_NS(100);
+	data->dev->driver_type = RC_DRIVER_IR_RAW;
+	data->dev->allowed_protos = RC_TYPE_ALL;
 
 	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
 		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",

Re: [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos

[David Härdeman]

On Mon, Jul 02, 2012 at 11:58:52AM +1000, Anton Blanchard wrote:
>
>We need to set a timeout so we can go idle on no activity.

This change Acked-by: David Härdeman <david@hardeman.nu>

>We weren't setting driver_type and allowed_protos, so fix that
>up too.

driver_type is set in the upstream tree.

allowed_protos isn't used for RC_DRIVER_IR_RAW type drivers (IIRC).

>
>Signed-off-by: Anton Blanchard <anton@samba.org>
>---
>
>Index: linux/drivers/media/rc/winbond-cir.c
>===================================================================
>--- linux.orig/drivers/media/rc/winbond-cir.c	2012-06-18 10:32:54.436717423 +1000
>+++ linux/drivers/media/rc/winbond-cir.c	2012-06-18 10:33:00.192754858 +1000
>@@ -1032,6 +1032,9 @@ wbcir_probe(struct pnp_dev *device, cons
> 	data->dev->tx_ir = wbcir_tx;
> 	data->dev->priv = data;
> 	data->dev->dev.parent = &device->dev;
>+	data->dev->timeout = MS_TO_NS(100);
>+	data->dev->driver_type = RC_DRIVER_IR_RAW;
>+	data->dev->allowed_protos = RC_TYPE_ALL;
> 
> 	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
> 		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
>

[PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Anton Blanchard]

When using my Logitech Harmony remote I get regular dropped events
(about 1 in every 3). Adjusting the sample frequency to 6us so we
sample at a multiple of an RC-6 pulse (444us) fixes it.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux/drivers/media/rc/winbond-cir.c
===================================================================
--- linux.orig/drivers/media/rc/winbond-cir.c	2012-07-01 14:54:28.993475033 +1000
+++ linux/drivers/media/rc/winbond-cir.c	2012-07-01 14:55:50.500939910 +1000
@@ -358,7 +358,7 @@ wbcir_irq_rx(struct wbcir_data *data, st
 		if (data->rxstate == WBCIR_RXSTATE_ERROR)
 			continue;
 		rawir.pulse = irdata & 0x80 ? false : true;
-		rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
+		rawir.duration = US_TO_NS((irdata & 0x7F) * 6);
 		ir_raw_event_store_with_filter(data->dev, &rawir);
 	}
 
@@ -874,8 +874,8 @@ wbcir_init_hw(struct wbcir_data *data)
 	/* prescaler 1.0, tx/rx fifo lvl 16 */
 	outb(0x30, data->sbase + WBCIR_REG_SP3_EXCR2);
 
-	/* Set baud divisor to sample every 10 us */
-	outb(0x0F, data->sbase + WBCIR_REG_SP3_BGDL);
+	/* Set baud divisor to sample every 6 us */
+	outb(0x09, data->sbase + WBCIR_REG_SP3_BGDL);
 	outb(0x00, data->sbase + WBCIR_REG_SP3_BGDH);
 
 	/* Set CEIR mode */

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[David Härdeman]

On Mon, Jul 02, 2012 at 11:59:37AM +1000, Anton Blanchard wrote:
>
>When using my Logitech Harmony remote I get regular dropped events
>(about 1 in every 3). Adjusting the sample frequency to 6us so we
>sample at a multiple of an RC-6 pulse (444us) fixes it.

Sounds weird.

The in-kernel RC6 decoder already has margins of around 50% for most
pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution from
10 to 6 us should have little to no effect on the RC6 decoding (also,
the Windows driver uses a 50us resolution IIRC).

Do you have a log of a successful and unsuccesful event (the timings
that is)?

>Signed-off-by: Anton Blanchard <anton@samba.org>
>---
>
>Index: linux/drivers/media/rc/winbond-cir.c
>===================================================================
>--- linux.orig/drivers/media/rc/winbond-cir.c	2012-07-01 14:54:28.993475033 +1000
>+++ linux/drivers/media/rc/winbond-cir.c	2012-07-01 14:55:50.500939910 +1000
>@@ -358,7 +358,7 @@ wbcir_irq_rx(struct wbcir_data *data, st
> 		if (data->rxstate == WBCIR_RXSTATE_ERROR)
> 			continue;
> 		rawir.pulse = irdata & 0x80 ? false : true;
>-		rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
>+		rawir.duration = US_TO_NS((irdata & 0x7F) * 6);
> 		ir_raw_event_store_with_filter(data->dev, &rawir);
> 	}
> 
>@@ -874,8 +874,8 @@ wbcir_init_hw(struct wbcir_data *data)
> 	/* prescaler 1.0, tx/rx fifo lvl 16 */
> 	outb(0x30, data->sbase + WBCIR_REG_SP3_EXCR2);
> 
>-	/* Set baud divisor to sample every 10 us */
>-	outb(0x0F, data->sbase + WBCIR_REG_SP3_BGDL);
>+	/* Set baud divisor to sample every 6 us */
>+	outb(0x09, data->sbase + WBCIR_REG_SP3_BGDL);
> 	outb(0x00, data->sbase + WBCIR_REG_SP3_BGDH);
> 
> 	/* Set CEIR mode */
>

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Anton Blanchard]

Hi David,

> The in-kernel RC6 decoder already has margins of around 50% for most
> pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution
> from 10 to 6 us should have little to no effect on the RC6 decoding
> (also, the Windows driver uses a 50us resolution IIRC).
> 
> Do you have a log of a successful and unsuccesful event (the timings
> that is)?

I had a closer look. I dumped the RC6 debug, but I also printed the raw
data in the interrupt handler:

    printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration);

A successful event begins with:

7f 1 1270000
7f 1 1270000
 8 1 80000
db 0 910000
27 1 390000
b3 0 510000
26 1 380000
b0 0 480000
ir_rc6_decode: RC6 decode started at state 0 (2620us pulse)
ir_rc6_decode: RC6 decode started at state 1 (910us space)
ir_rc6_decode: RC6 decode started at state 2 (390us pulse)
ir_rc6_decode: RC6 decode started at state 3 (510us space)
ir_rc6_decode: RC6 decode started at state 2 (66us space)
ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
26 1 380000
db 0 910000
26 1 380000
dd 0 930000
7d 1 1250000    <---------------
dd 0 930000
25 1 370000
b4 0 520000
ir_rc6_decode: RC6 decode started at state 3 (480us space)
ir_rc6_decode: RC6 decode started at state 2 (36us space)
ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
ir_rc6_decode: RC6 decode started at state 3 (910us space)
ir_rc6_decode: RC6 decode started at state 2 (466us space)
ir_rc6_decode: RC6 decode started at state 3 (380us pulse)
ir_rc6_decode: RC6 decode started at state 4 (0us pulse)
ir_rc6_decode: RC6 decode started at state 4 (930us space)
ir_rc6_decode: RC6 decode started at state 5 (1250us pulse)
ir_rc6_decode: RC6 decode started at state 6 (361us pulse)
ir_rc6_decode: RC6 decode started at state 7 (930us space)

Now compare to an unsuccesful event, in particular the byte
I have tagged in both traces:

7f 1 1270000
7f 1 1270000
 2 1 20000
df 0 950000
26 1 380000
b0 0 480000
26 1 380000
b0 0 480000
26 1 380000
dc 0 920000
26 1 380000
ir_rc6_decode: RC6 decode started at state 0 (2560us pulse)
ir_rc6_decode: RC6 decode started at state 1 (950us space)
ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
ir_rc6_decode: RC6 decode started at state 3 (480us space)
ir_rc6_decode: RC6 decode started at state 2 (36us space)
ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
ir_rc6_decode: RC6 decode started at state 3 (480us space)
ir_rc6_decode: RC6 decode started at state 2 (36us space)
ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
ir_rc6_decode: RC6 decode started at state 3 (920us space)
ir_rc6_decode: RC6 decode started at state 2 (476us space)
dc 0 920000
ff 0 1270000 <----------------
de 0 940000
25 1 370000
b1 0 490000
26 1 380000
b0 0 480000
26 1 380000
ir_rc6_decode: RC6 decode started at state 3 (380us pulse)
ir_rc6_decode: RC6 decode started at state 4 (0us pulse)
ir_rc6_decode: RC6 decode started at state 4 (3130us space)
ir_rc6_decode: RC6 decode failed at state 4 (3130us space)

That should have been a pulse but it came out as a space. This makes me
wonder if there is an issue with the run length encoding, perhaps when
a pulse is the right size to just saturate it. It does seem like we
set the top bit even though we should not have.

If true we could choose any sample rate that avoids it.

Anton

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Konstantin Dimitrov]

hi Anton,

it's very debatable that is fault of the Linux driver(s) and let me
elaborate why i think so: i have Logitech Harmony 890 remote and MCE
USB IR receiver that is supported by 'mceusb.c' in Linux, i.e. it's
different than your IR receiver, but yet the work is very unstable
together with my Harmony no matter what type of pulses are used, e.g.
RC5, RC6 or NEC. however, that is true only if default setting for the
Harmony are set in the Logitech software for Windows that i used to
initially setup the remote. so, if in "Logitech Harmony Remote
Software (7.7.0)" for Windows go to:

Devices -> Settings -> select 'Troubleshoot' -> click 'Next' -> select
'...responds to some commands either too many times or only
occasionally' -> click 'Next'

then you're given the option to choose number from 0 to 5 with the
following explanation by Logitech:

"If your device responds too slowly, or not at all when you press a
button on the remote, increase the value to 4 or 5.
If your device responds too quickly, lower the value to 2, 1 or 0."

there are no any details about what actually that setting is doing to
the Harmony firmware and in fact i found that neither the default
value, nor Logitech suggestion what value to choose in which case are
good, but using value of "2" fixed all the issues with my MCE USB IR
receiver in Linux - totally stable and proper work.

in short i think the problem is created from Harmony firmware and the
proper solution is to just play with those numbers in Logitech
software until you find the setting with which you get stable work. of
course, someone more interested than i'm in this could do further
investigation and understand the root cause of the issue and what
changes in how Harmony firmware behaves based on those settings
Logitech offered in their software, but my point is that i have
similar issue with RC5 pulses, Harmony 890 and MCE USB IR receiver
that is supported by 'mceusb.c' or quite different setup than yours
and the fix i found is from Logitech side and the fact Logitech
offered such setting suggests that the proper way to fix it.

best regards,
konstantin

On Thu, Jul 5, 2012 at 1:30 PM, Anton Blanchard <anton@samba.org> wrote:
>
> Hi David,
>
>> The in-kernel RC6 decoder already has margins of around 50% for most
>> pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution
>> from 10 to 6 us should have little to no effect on the RC6 decoding
>> (also, the Windows driver uses a 50us resolution IIRC).
>>
>> Do you have a log of a successful and unsuccesful event (the timings
>> that is)?
>
> I had a closer look. I dumped the RC6 debug, but I also printed the raw
> data in the interrupt handler:
>
>     printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration);
>
> A successful event begins with:
>
> 7f 1 1270000
> 7f 1 1270000
>  8 1 80000
> db 0 910000
> 27 1 390000
> b3 0 510000
> 26 1 380000
> b0 0 480000
> ir_rc6_decode: RC6 decode started at state 0 (2620us pulse)
> ir_rc6_decode: RC6 decode started at state 1 (910us space)
> ir_rc6_decode: RC6 decode started at state 2 (390us pulse)
> ir_rc6_decode: RC6 decode started at state 3 (510us space)
> ir_rc6_decode: RC6 decode started at state 2 (66us space)
> ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
> 26 1 380000
> db 0 910000
> 26 1 380000
> dd 0 930000
> 7d 1 1250000    <---------------
> dd 0 930000
> 25 1 370000
> b4 0 520000
> ir_rc6_decode: RC6 decode started at state 3 (480us space)
> ir_rc6_decode: RC6 decode started at state 2 (36us space)
> ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 3 (910us space)
> ir_rc6_decode: RC6 decode started at state 2 (466us space)
> ir_rc6_decode: RC6 decode started at state 3 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 4 (0us pulse)
> ir_rc6_decode: RC6 decode started at state 4 (930us space)
> ir_rc6_decode: RC6 decode started at state 5 (1250us pulse)
> ir_rc6_decode: RC6 decode started at state 6 (361us pulse)
> ir_rc6_decode: RC6 decode started at state 7 (930us space)
>
> Now compare to an unsuccesful event, in particular the byte
> I have tagged in both traces:
>
> 7f 1 1270000
> 7f 1 1270000
>  2 1 20000
> df 0 950000
> 26 1 380000
> b0 0 480000
> 26 1 380000
> b0 0 480000
> 26 1 380000
> dc 0 920000
> 26 1 380000
> ir_rc6_decode: RC6 decode started at state 0 (2560us pulse)
> ir_rc6_decode: RC6 decode started at state 1 (950us space)
> ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 3 (480us space)
> ir_rc6_decode: RC6 decode started at state 2 (36us space)
> ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 3 (480us space)
> ir_rc6_decode: RC6 decode started at state 2 (36us space)
> ir_rc6_decode: RC6 decode started at state 2 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 3 (920us space)
> ir_rc6_decode: RC6 decode started at state 2 (476us space)
> dc 0 920000
> ff 0 1270000 <----------------
> de 0 940000
> 25 1 370000
> b1 0 490000
> 26 1 380000
> b0 0 480000
> 26 1 380000
> ir_rc6_decode: RC6 decode started at state 3 (380us pulse)
> ir_rc6_decode: RC6 decode started at state 4 (0us pulse)
> ir_rc6_decode: RC6 decode started at state 4 (3130us space)
> ir_rc6_decode: RC6 decode failed at state 4 (3130us space)
>
> That should have been a pulse but it came out as a space. This makes me
> wonder if there is an issue with the run length encoding, perhaps when
> a pulse is the right size to just saturate it. It does seem like we
> set the top bit even though we should not have.
>
> If true we could choose any sample rate that avoids it.
>
> Anton
> --
> 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

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[David Härdeman]

On Thu, 5 Jul 2012 20:30:35 +1000, Anton Blanchard <anton@samba.org>
wrote:
> I had a closer look. I dumped the RC6 debug, but I also printed the raw
> data in the interrupt handler:
> 
>     printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration);
> 
...
> That should have been a pulse but it came out as a space. This makes me
> wonder if there is an issue with the run length encoding, perhaps when
> a pulse is the right size to just saturate it. It does seem like we
> set the top bit even though we should not have.

It's quite weird to see a "short" space followed by a max space followed
by a "short" space (0xdc 0xff 0xde). Almost like there's one or more
(pulse) bytes missing in between.

I've tested long pulses/spaces before and they've worked as expected (e.g.
"max", "max", "short" events....the leading 0x7f 0x7f 0x08 sequence in your
log is a good example).

Now, there is a minor bug in the RLE decoding in that the duration should
have 1 added to it (meaning that 0x00 or 0x80 are valid values).

Just to make sure something like that isn't happening, could you correct
the line in wbcir_irq_rx() which currently reads:

rawir.duration = US_TO_NS((irdata & 0x7F) * 10);

so that it reads

rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10);

However, I'm guessing you inserted the extra debug printk inside
wbcir_irq_rx() so any 0x00 or 0x80 bytes would have been printed?

Another possibility is that the printk in the interrupt handler causes
overhead...could you do a debug run without the printk in the interrupt
handler?

Also, could you provide me with the full versions of both logs? (i.e. all
the way to idle....it might help spot a missed pulse/space)

Thanks,
David

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Konstantin Dimitrov]

hi David,

excuse me for my ignorance, but don't you think adjusting the IR
receiver to universal remote control is fundamentally wrong, while the
whole point of universal remote control like Logitech Harmony is to be
adjusted to the IR receiver and be able to be adjusted to any IR
receiver and not the other way around. so, that being said, my point
is maybe the whole discussion here is just wild goose chase until
those settings i mentioned in Logitech control software are not tried
and there is no evidence that has already being done based on the
information provided by Anton. we don't know what exactly those
settings applied to Logitech Harmony firmware via Logitech control
software do and it could be default pulse timings that are set trough
them are just out of specification for RC6 and need to be manually
refined using the Harmony firmware settings in question - once again
after all universal remote control is supposed to be able to fit any
IR receiver and any type of pulses and that's why provides series of
different settings in order to do that - the issue seems more like
misconfiguration of the universal remote control than Linux drivers
problem. i'm just trying to save you time chasing not existing
problems and don't mean anything else - i didn't even look at the
source code you're discussing - i just have practical experience with
Logitech Harmony 890 and thus i know keymaps and protocols are
independently set from the proper pulse timings with Logitech control
software.

best regards,
konstantin

On Thu, Jul 5, 2012 at 5:13 PM, David Härdeman <david@hardeman.nu> wrote:
> On Thu, 5 Jul 2012 20:30:35 +1000, Anton Blanchard <anton@samba.org>
> wrote:
>> I had a closer look. I dumped the RC6 debug, but I also printed the raw
>> data in the interrupt handler:
>>
>>     printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration);
>>
> ...
>> That should have been a pulse but it came out as a space. This makes me
>> wonder if there is an issue with the run length encoding, perhaps when
>> a pulse is the right size to just saturate it. It does seem like we
>> set the top bit even though we should not have.
>
> It's quite weird to see a "short" space followed by a max space followed
> by a "short" space (0xdc 0xff 0xde). Almost like there's one or more
> (pulse) bytes missing in between.
>
> I've tested long pulses/spaces before and they've worked as expected (e.g.
> "max", "max", "short" events....the leading 0x7f 0x7f 0x08 sequence in your
> log is a good example).
>
> Now, there is a minor bug in the RLE decoding in that the duration should
> have 1 added to it (meaning that 0x00 or 0x80 are valid values).
>
> Just to make sure something like that isn't happening, could you correct
> the line in wbcir_irq_rx() which currently reads:
>
> rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
>
> so that it reads
>
> rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10);
>
> However, I'm guessing you inserted the extra debug printk inside
> wbcir_irq_rx() so any 0x00 or 0x80 bytes would have been printed?
>
> Another possibility is that the printk in the interrupt handler causes
> overhead...could you do a debug run without the printk in the interrupt
> handler?
>
> Also, could you provide me with the full versions of both logs? (i.e. all
> the way to idle....it might help spot a missed pulse/space)
>
> Thanks,
> David
>
> --
> 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

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[David Härdeman]

On Thu, 5 Jul 2012 17:39:18 +0300, Konstantin Dimitrov
<kosio.dimitrov@gmail.com> wrote:
> excuse me for my ignorance, but don't you think adjusting the IR
> receiver to universal remote control is fundamentally wrong, while the
> whole point of universal remote control like Logitech Harmony is to be
> adjusted to the IR receiver and be able to be adjusted to any IR
> receiver and not the other way around. so, that being said, my point
> is maybe the whole discussion here is just wild goose chase until
> those settings i mentioned in Logitech control software are not tried
> and there is no evidence that has already being done based on the
> information provided by Anton. we don't know what exactly those
> settings applied to Logitech Harmony firmware via Logitech control
> software do and it could be default pulse timings that are set trough
> them are just out of specification for RC6 and need to be manually
> refined using the Harmony firmware settings in question - once again
> after all universal remote control is supposed to be able to fit any
> IR receiver and any type of pulses and that's why provides series of
> different settings in order to do that - the issue seems more like
> misconfiguration of the universal remote control than Linux drivers
> problem. i'm just trying to save you time chasing not existing
> problems and don't mean anything else - i didn't even look at the
> source code you're discussing - i just have practical experience with
> Logitech Harmony 890 and thus i know keymaps and protocols are
> independently set from the proper pulse timings with Logitech control
> software.

Konstantin,

thanks for your concern, but some of the byte sequences that Anton showed
are "incorrect" in the sense that the receiver hardware should never
generate them no matter what the (Logitech) remote is sending (unless I've
misunderstood something of course).

"0xdc 0xff 0xde" is a sequence which shouldn't happen.

So even if the Logitech can be tweaked (and I'm sure Anton is grateful for
the information), there is something wrong here and I'd like to get to the
bottom of what it is.

Kind regards,
David

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Konstantin Dimitrov]

David, i see your point - as i mentioned i have no any knowledge of IR
receiver part you're discussing and/or its Linux drivers and don't
want just to spam, but my simple thinking is that if the Logitech
Harmony universal remote control is with wrongly configured firmware
it can emit something that is so messed up that makes the IR receiver
part behaves in completely unpredictable way. anyway, at least Anton
have some ideas to try...

On Thu, Jul 5, 2012 at 5:45 PM, David Härdeman <david@hardeman.nu> wrote:
> On Thu, 5 Jul 2012 17:39:18 +0300, Konstantin Dimitrov
> <kosio.dimitrov@gmail.com> wrote:
>> excuse me for my ignorance, but don't you think adjusting the IR
>> receiver to universal remote control is fundamentally wrong, while the
>> whole point of universal remote control like Logitech Harmony is to be
>> adjusted to the IR receiver and be able to be adjusted to any IR
>> receiver and not the other way around. so, that being said, my point
>> is maybe the whole discussion here is just wild goose chase until
>> those settings i mentioned in Logitech control software are not tried
>> and there is no evidence that has already being done based on the
>> information provided by Anton. we don't know what exactly those
>> settings applied to Logitech Harmony firmware via Logitech control
>> software do and it could be default pulse timings that are set trough
>> them are just out of specification for RC6 and need to be manually
>> refined using the Harmony firmware settings in question - once again
>> after all universal remote control is supposed to be able to fit any
>> IR receiver and any type of pulses and that's why provides series of
>> different settings in order to do that - the issue seems more like
>> misconfiguration of the universal remote control than Linux drivers
>> problem. i'm just trying to save you time chasing not existing
>> problems and don't mean anything else - i didn't even look at the
>> source code you're discussing - i just have practical experience with
>> Logitech Harmony 890 and thus i know keymaps and protocols are
>> independently set from the proper pulse timings with Logitech control
>> software.
>
> Konstantin,
>
> thanks for your concern, but some of the byte sequences that Anton showed
> are "incorrect" in the sense that the receiver hardware should never
> generate them no matter what the (Logitech) remote is sending (unless I've
> misunderstood something of course).
>
> "0xdc 0xff 0xde" is a sequence which shouldn't happen.
>
> So even if the Logitech can be tweaked (and I'm sure Anton is grateful for
> the information), there is something wrong here and I'd like to get to the
> bottom of what it is.
>
> Kind regards,
> David
>

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[Anton Blanchard]

Hi David,

> Just to make sure something like that isn't happening, could you
> correct the line in wbcir_irq_rx() which currently reads:
> 
> rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
> 
> so that it reads
> 
> rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10);

Sure, I have added the change. This is what my diff to mainline looks
like right now:

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 342c2c8..6381c11 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -232,7 +232,7 @@ MODULE_PARM_DESC(invert, "Invert the signal from the IR receiver");
 
 static bool txandrx; /* default = 0 */
 module_param(txandrx, bool, 0444);
-MODULE_PARM_DESC(invert, "Allow simultaneous TX and RX");
+MODULE_PARM_DESC(txandrx, "Allow simultaneous TX and RX");
 
 static unsigned int wake_sc = 0x800F040C;
 module_param(wake_sc, uint, 0644);
@@ -358,7 +358,8 @@ wbcir_irq_rx(struct wbcir_data *data, struct pnp_dev *device)
 		if (data->rxstate == WBCIR_RXSTATE_ERROR)
 			continue;
 		rawir.pulse = irdata & 0x80 ? false : true;
-		rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
+		rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10);
+		printk(KERN_DEBUG "%x %d %d\n", irdata, rawir.pulse, rawir.duration);
 		ir_raw_event_store_with_filter(data->dev, &rawir);
 	}
 
@@ -1026,6 +1027,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 	data->dev->input_id.product = WBCIR_ID_FAMILY;
 	data->dev->input_id.version = WBCIR_ID_CHIP;
 	data->dev->map_name = RC_MAP_RC6_MCE;
+	data->dev->timeout = MS_TO_NS(100);
 	data->dev->s_idle = wbcir_idle_rx;
 	data->dev->s_tx_mask = wbcir_txmask;
 	data->dev->s_tx_carrier = wbcir_txcarrier;

Here is the debug output:

http://ozlabs.org/~anton/winbond.log1.gz

> Another possibility is that the printk in the interrupt handler causes
> overhead...could you do a debug run without the printk in the
> interrupt handler?

Here is the output without the printk in the interrupt handler:

http://ozlabs.org/~anton/winbond.log2.gz

Anton

Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability

[David Härdeman]

On Mon, Jul 09, 2012 at 12:02:08PM +1000, Anton Blanchard wrote:
>
>Hi David,
>
>> Just to make sure something like that isn't happening, could you
>> correct the line in wbcir_irq_rx() which currently reads:
>> 
>> rawir.duration = US_TO_NS((irdata & 0x7F) * 10);
>> 
>> so that it reads
>> 
>> rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10);
>
>Sure, I have added the change. This is what my diff to mainline looks
>like right now:

Hmm, still no good theory here.

Could you try the suggestions Konstantin Dimitrov proposed (i.e. the
Harmony fixups) and see if that makes any difference? Perhaps the
harmony actually does manage to send some bogus data (of sufficiently
short duration not to show up in the captured RX data)...


-- 
David Härdeman

Re: [PATCH 1/3] [media] winbond-cir: Fix txandrx module info

[David Härdeman]

On Mon, Jul 02, 2012 at 11:58:00AM +1000, Anton Blanchard wrote:
>
>We aren't getting any module info for the txandx option because
>of a typo:
>
>parm:           txandrx:bool
>
>Signed-off-by: Anton Blanchard <anton@samba.org>

Acked-by: David Härdeman <david@hardeman.nu>

>---
>
>Index: linux-2.6/drivers/media/rc/winbond-cir.c
>===================================================================
>--- linux-2.6.orig/drivers/media/rc/winbond-cir.c	2011-11-20 20:30:57.831906589 +1100
>+++ linux-2.6/drivers/media/rc/winbond-cir.c	2011-11-20 20:32:13.472362123 +1100
>@@ -232,7 +232,7 @@ MODULE_PARM_DESC(invert, "Invert the sig
> 
> static int txandrx; /* default = 0 */
> module_param(txandrx, bool, 0444);
>-MODULE_PARM_DESC(invert, "Allow simultaneous TX and RX");
>+MODULE_PARM_DESC(txandrx, "Allow simultaneous TX and RX");
> 
> static unsigned int wake_sc = 0x800F040C;
> module_param(wake_sc, uint, 0644);
>