* [PATCH 1/3] [media] winbond-cir: Fix txandrx module info
@ 2012-07-02 1:58 Anton Blanchard
2012-07-02 1:58 ` [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos Anton Blanchard
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Anton Blanchard @ 2012-07-02 1:58 UTC (permalink / raw)
To: mchehab, david; +Cc: linux-media
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);
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos 2012-07-02 1:58 [PATCH 1/3] [media] winbond-cir: Fix txandrx module info Anton Blanchard @ 2012-07-02 1:58 ` Anton Blanchard 2012-07-03 20:22 ` David Härdeman 2012-07-02 1:59 ` [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability Anton Blanchard 2012-07-03 20:18 ` [PATCH 1/3] [media] winbond-cir: Fix txandrx module info David Härdeman 2 siblings, 1 reply; 14+ messages in thread From: Anton Blanchard @ 2012-07-02 1:58 UTC (permalink / raw) To: mchehab, david; +Cc: linux-media 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", ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos 2012-07-02 1:58 ` [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos Anton Blanchard @ 2012-07-03 20:22 ` David Härdeman 0 siblings, 0 replies; 14+ messages in thread From: David Härdeman @ 2012-07-03 20:22 UTC (permalink / raw) To: Anton Blanchard; +Cc: mchehab, linux-media 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", > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-02 1:58 [PATCH 1/3] [media] winbond-cir: Fix txandrx module info Anton Blanchard 2012-07-02 1:58 ` [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos Anton Blanchard @ 2012-07-02 1:59 ` Anton Blanchard 2012-07-03 20:28 ` David Härdeman 2012-07-03 20:18 ` [PATCH 1/3] [media] winbond-cir: Fix txandrx module info David Härdeman 2 siblings, 1 reply; 14+ messages in thread From: Anton Blanchard @ 2012-07-02 1:59 UTC (permalink / raw) To: mchehab, david; +Cc: linux-media 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 */ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-02 1:59 ` [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability Anton Blanchard @ 2012-07-03 20:28 ` David Härdeman 2012-07-05 10:30 ` Anton Blanchard 0 siblings, 1 reply; 14+ messages in thread From: David Härdeman @ 2012-07-03 20:28 UTC (permalink / raw) To: Anton Blanchard; +Cc: mchehab, linux-media 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 */ > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-03 20:28 ` David Härdeman @ 2012-07-05 10:30 ` Anton Blanchard 2012-07-05 11:04 ` Konstantin Dimitrov 2012-07-05 14:13 ` David Härdeman 0 siblings, 2 replies; 14+ messages in thread From: Anton Blanchard @ 2012-07-05 10:30 UTC (permalink / raw) To: David Härdeman; +Cc: mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 10:30 ` Anton Blanchard @ 2012-07-05 11:04 ` Konstantin Dimitrov 2012-07-05 14:13 ` David Härdeman 1 sibling, 0 replies; 14+ messages in thread From: Konstantin Dimitrov @ 2012-07-05 11:04 UTC (permalink / raw) To: Anton Blanchard; +Cc: David Härdeman, mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 10:30 ` Anton Blanchard 2012-07-05 11:04 ` Konstantin Dimitrov @ 2012-07-05 14:13 ` David Härdeman 2012-07-05 14:39 ` Konstantin Dimitrov 2012-07-09 2:02 ` Anton Blanchard 1 sibling, 2 replies; 14+ messages in thread From: David Härdeman @ 2012-07-05 14:13 UTC (permalink / raw) To: Anton Blanchard; +Cc: mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 14:13 ` David Härdeman @ 2012-07-05 14:39 ` Konstantin Dimitrov 2012-07-05 14:45 ` David Härdeman 2012-07-09 2:02 ` Anton Blanchard 1 sibling, 1 reply; 14+ messages in thread From: Konstantin Dimitrov @ 2012-07-05 14:39 UTC (permalink / raw) To: David Härdeman; +Cc: Anton Blanchard, mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 14:39 ` Konstantin Dimitrov @ 2012-07-05 14:45 ` David Härdeman 2012-07-05 14:56 ` Konstantin Dimitrov 0 siblings, 1 reply; 14+ messages in thread From: David Härdeman @ 2012-07-05 14:45 UTC (permalink / raw) To: Konstantin Dimitrov; +Cc: Anton Blanchard, mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 14:45 ` David Härdeman @ 2012-07-05 14:56 ` Konstantin Dimitrov 0 siblings, 0 replies; 14+ messages in thread From: Konstantin Dimitrov @ 2012-07-05 14:56 UTC (permalink / raw) To: David Härdeman; +Cc: Anton Blanchard, mchehab, linux-media 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-05 14:13 ` David Härdeman 2012-07-05 14:39 ` Konstantin Dimitrov @ 2012-07-09 2:02 ` Anton Blanchard 2012-08-20 21:40 ` David Härdeman 1 sibling, 1 reply; 14+ messages in thread From: Anton Blanchard @ 2012-07-09 2:02 UTC (permalink / raw) To: David Härdeman; +Cc: mchehab, linux-media 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability 2012-07-09 2:02 ` Anton Blanchard @ 2012-08-20 21:40 ` David Härdeman 0 siblings, 0 replies; 14+ messages in thread From: David Härdeman @ 2012-08-20 21:40 UTC (permalink / raw) To: Anton Blanchard; +Cc: mchehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] [media] winbond-cir: Fix txandrx module info 2012-07-02 1:58 [PATCH 1/3] [media] winbond-cir: Fix txandrx module info Anton Blanchard 2012-07-02 1:58 ` [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos Anton Blanchard 2012-07-02 1:59 ` [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability Anton Blanchard @ 2012-07-03 20:18 ` David Härdeman 2 siblings, 0 replies; 14+ messages in thread From: David Härdeman @ 2012-07-03 20:18 UTC (permalink / raw) To: Anton Blanchard; +Cc: mchehab, linux-media 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); > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-08-20 21:40 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-02 1:58 [PATCH 1/3] [media] winbond-cir: Fix txandrx module info Anton Blanchard 2012-07-02 1:58 ` [PATCH 2/3] [media] winbond-cir: Initialise timeout, driver_type and allowed_protos Anton Blanchard 2012-07-03 20:22 ` David Härdeman 2012-07-02 1:59 ` [PATCH 3/3] [media] winbond-cir: Adjust sample frequency to improve reliability Anton Blanchard 2012-07-03 20:28 ` David Härdeman 2012-07-05 10:30 ` Anton Blanchard 2012-07-05 11:04 ` Konstantin Dimitrov 2012-07-05 14:13 ` David Härdeman 2012-07-05 14:39 ` Konstantin Dimitrov 2012-07-05 14:45 ` David Härdeman 2012-07-05 14:56 ` Konstantin Dimitrov 2012-07-09 2:02 ` Anton Blanchard 2012-08-20 21:40 ` David Härdeman 2012-07-03 20:18 ` [PATCH 1/3] [media] winbond-cir: Fix txandrx module info David Härdeman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.